fix: input should filter away disallowed characters from value prop #4338
Conversation
🦋 Changeset detectedLatest commit: eb594e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
macci001
requested review from
wingkwong
and removed request for
jrgarciadev
WalkthroughThe changes introduce a patch for the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.changeset/silent-jars-grow.md (1)
1-5: Enhance the changeset description for better clarity.Consider expanding the description to better explain the fix and its impact:
-Change ensures that the input value does not accept any disallowed characters when the value prop contains them(#4329) +fix: InputOtp now correctly filters disallowed characters from value prop, ensuring consistency with allowedKeys regex pattern. This fixes the validation behavior when used with React Hook Form (#4329)packages/components/input-otp/__tests__/input-otp.test.tsx (1)
187-194: Enhance test coverage with additional edge cases.While the current test verifies basic filtering functionality, consider adding more test cases:
it("should ensure the value prop avoids any disallowed keys.", () => { const value = "4abc12uiond2"; render(<InputOtp length={4} value={value} />); const input = screen.getByRole("textbox"); expect(input).toHaveAttribute("value", "4122"); }); + +it("should handle empty value prop correctly", () => { + render(<InputOtp length={4} value="" />); + const input = screen.getByRole("textbox"); + expect(input).toHaveAttribute("value", ""); +}); + +it("should handle custom allowedKeys with value prop", () => { + const value = "123abc"; + render(<InputOtp length={4} value={value} allowedKeys="^[a-z]*$" />); + const input = screen.getByRole("textbox"); + expect(input).toHaveAttribute("value", "abc"); +});
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
.changeset/silent-jars-grow.md(1 hunks)packages/components/input-otp/__tests__/input-otp.test.tsx(1 hunks)packages/components/input-otp/src/use-input-otp.ts(2 hunks)
🔇 Additional comments (1)
packages/components/input-otp/src/use-input-otp.ts (1)
151-151: Verify value prop handling in useControlledState.
The value prop is now filtered before being passed to useControlledState. Let's verify this doesn't cause any issues with React's controlled component pattern.
✅ Verification successful
Value prop filtering is correctly implemented and tested
The filtering of the value prop before passing it to useControlledState is intentional and properly handled:
- The code filters out any characters that don't match the allowed regex pattern before passing to useControlledState
- This is thoroughly tested as shown in the test file:
- Test case "should ensure the value prop avoids any disallowed keys" verifies this exact behavior
- The test confirms that "4abc12uiond2" is correctly filtered to "4122" when only numbers are allowed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of useControlledState with filtered values
ast-grep --pattern 'useControlledState($filtered, $default, $onChange)'
# Search for related tests or documentation
rg -A 5 'useControlledState.*filtered'
Length of output: 114
Script:
#!/bin/bash
# Let's first look at the useControlledState implementation
ast-grep --pattern 'function useControlledState'
# Check the actual usage in the input-otp component
ast-grep --pattern 'useControlledState($value, $_, $_)'
# Look at the full context of the file
cat packages/components/input-otp/src/use-input-otp.ts
# Check for tests
fd "input-otp.test" --exec cat {}
Length of output: 17204
macci001
force-pushed
the
macci001/4239
branch
from
ef4fba0 to
eb594e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/components/input-otp/stories/input-otp.stories.tsx (1)
180-221: Consider using optional chaining for error handlingThe implementation looks good, but we can improve the error handling using optional chaining.
- errorMessage={errors.otp && errors.otp.message} + errorMessage={errors.otp?.message}🧰 Tools
🪛 Biome (1.9.4)
[error] 203-203: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/silent-jars-grow.md(1 hunks)packages/components/input-otp/__tests__/input-otp.test.tsx(2 hunks)packages/components/input-otp/src/use-input-otp.ts(2 hunks)packages/components/input-otp/stories/input-otp.stories.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .changeset/silent-jars-grow.md
- packages/components/input-otp/tests/input-otp.test.tsx
- packages/components/input-otp/src/use-input-otp.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/input-otp/stories/input-otp.stories.tsx
[error] 203-203: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
packages/components/input-otp/stories/input-otp.stories.tsx (3)
4-4: LGTM: Clean import organization
Good practice combining related imports from react-hook-form.
383-389: LGTM: Story export follows established patterns
The story export is well-structured and maintains consistency with other stories in the file.
201-206: Verify disallowed characters filtering
Since this story is crucial for testing the fix for disallowed characters, we should verify that the InputOtp component correctly filters them out.
✅ Verification successful
Let me gather more information about the validation logic implementation.
Character filtering is properly implemented
The code shows that character filtering is correctly implemented through multiple mechanisms:
- The
allowedKeysprop is used to create a RegExp pattern that validates input - Input validation occurs in the
onChangehandler withingetBaseProps - The pattern is also applied via the HTML
patternattribute ingetInputOtpProps
The story in question correctly tests this functionality by providing different allowedKeys patterns, including the lowercase alphabets test case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for allowedKeys prop usage and related validation logic
rg -A 5 "allowedKeys.*=.*"
ast-grep --pattern 'const $_ = $$$; if (!$_.test($_)) { $$$ }'
Length of output: 3803
Script:
#!/bin/bash
# Search for the input validation logic in the InputOtp component
ast-grep --pattern 'const $pattern = new RegExp($allowedKeys);
$$$
if (!$pattern.test($_)) {
$$$
}'
# Search for related test cases
rg -A 10 "test.*disallowed.*characters"
rg -A 10 "test.*allowed.*keys"
Length of output: 186
Script:
#!/bin/bash
# Let's check the implementation details of the input validation
rg -A 10 "handleKeyDown|handleChange" packages/components/input-otp/
# Also search for the validation logic with a different pattern
ast-grep --pattern 'function $_(event) {
$$$
if ($pattern.test($_)) {
$$$
}
}'
Length of output: 161
Script:
#!/bin/bash
# Let's examine the content of the input-otp component and its hook
cat packages/components/input-otp/src/use-input-otp.ts
cat packages/components/input-otp/src/input-otp.tsx
Length of output: 11395
🧰 Tools
🪛 Biome (1.9.4)
[error] 203-203: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Closes #4329
📝 Description
Change ensures that the input value does not accept any disallowed characters when the value prop contains them.
⛳️ Current behavior (updates)
Screen.Recording.2024-12-11.at.11.13.13.PM.mov
🚀 New behavior
Screen.Recording.2024-12-11.at.11.10.10.PM.mov
💣 Is this a breaking change (Yes/No): No
Summary by CodeRabbit
New Features
react-hook-formfunctionality for improved form handling of the OTP input.Bug Fixes
Tests