feat: add OTP field component#802
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request introduces a complete OTPField component to the Raystack UI library. The implementation wraps Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
apps/www/src/content/docs/components/otp-field/index.mdx (1)
25-25: 💤 Low valueConsider clarifying the
lengthrequirement explanation.The phrase "before all slots hydrate" is slightly confusing. The
lengthprop is required so the component can:
- Size the input correctly
- Validate the complete value
- Detect when all slots are filled
Consider rewording to: "
lengthis required so the field can manage state, validate input, and detect completion."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/www/src/content/docs/components/otp-field/index.mdx` at line 25, Update the explanatory sentence about the length prop in the OTP field docs: replace the confusing phrase that mentions "before all slots hydrate" with a clearer description that references the length prop's responsibilities (e.g., "`length` is required so the field can manage state, validate input, and detect completion"). Locate the text in apps/www/src/content/docs/components/otp-field/index.mdx where the `length` prop is explained and swap the sentence while keeping the surrounding import/assembly guidance intact and still mentioning sizing, validation, and completion detection for the `length` prop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/www/src/components/playground/otp-field-examples.tsx`:
- Around line 7-13: renderSlots currently computes aria-label totals using
length + offset, which produces incorrect "of" counts for grouped/separated
examples; change the function signature to accept a separate totalLength (e.g.,
renderSlots(length: number, offset = 0, totalLength = length)) and use
totalLength in the aria-label for OTPField.Input (use `Character ${i + 1 +
offset} of ${totalLength}`). Update all calls to renderSlots (including the
separator example and the other instance around lines 58-66) to pass the full
field length as the third argument so screen readers see the correct total.
---
Nitpick comments:
In `@apps/www/src/content/docs/components/otp-field/index.mdx`:
- Line 25: Update the explanatory sentence about the length prop in the OTP
field docs: replace the confusing phrase that mentions "before all slots
hydrate" with a clearer description that references the length prop's
responsibilities (e.g., "`length` is required so the field can manage state,
validate input, and detect completion"). Locate the text in
apps/www/src/content/docs/components/otp-field/index.mdx where the `length` prop
is explained and swap the sentence while keeping the surrounding import/assembly
guidance intact and still mentioning sizing, validation, and completion
detection for the `length` prop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90a47a3a-70de-49f4-81d4-bbf6a32a4950
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
apps/www/src/components/playground/index.tsapps/www/src/components/playground/otp-field-examples.tsxapps/www/src/content/docs/components/otp-field/demo.tsapps/www/src/content/docs/components/otp-field/index.mdxapps/www/src/content/docs/components/otp-field/props.tspackages/raystack/components/otp-field/__tests__/otp-field.test.tsxpackages/raystack/components/otp-field/index.tsxpackages/raystack/components/otp-field/otp-field.module.csspackages/raystack/components/otp-field/otp-field.tsxpackages/raystack/index.tsxpackages/raystack/package.json
| const renderSlots = (length: number, offset = 0) => | ||
| Array.from({ length }, (_, i) => ( | ||
| <OTPField.Input | ||
| key={i + offset} | ||
| aria-label={`Character ${i + 1 + offset} of ${length + offset}`} | ||
| /> | ||
| )); |
There was a problem hiding this comment.
Fix incorrect slot aria-label totals in separator example.
Line 11 generates labels like “Character 1 of 3” for the first group in a 6-character field, which gives incorrect context to screen readers. Pass the total field length separately.
Suggested fix
-const renderSlots = (length: number, offset = 0) =>
+const renderSlots = (length: number, offset = 0, totalLength = length + offset) =>
Array.from({ length }, (_, i) => (
<OTPField.Input
key={i + offset}
- aria-label={`Character ${i + 1 + offset} of ${length + offset}`}
+ aria-label={`Character ${i + 1 + offset} of ${totalLength}`}
/>
));
@@
- {renderSlots(3)}
+ {renderSlots(3, 0, 6)}Also applies to: 58-66
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/www/src/components/playground/otp-field-examples.tsx` around lines 7 -
13, renderSlots currently computes aria-label totals using length + offset,
which produces incorrect "of" counts for grouped/separated examples; change the
function signature to accept a separate totalLength (e.g., renderSlots(length:
number, offset = 0, totalLength = length)) and use totalLength in the aria-label
for OTPField.Input (use `Character ${i + 1 + offset} of ${totalLength}`). Update
all calls to renderSlots (including the separator example and the other instance
around lines 58-66) to pass the full field length as the third argument so
screen readers see the correct total.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/raystack/components/slider/__tests__/slider.test.tsx`:
- Around line 140-146: The test "sets aria-valuetext" currently only checks
existence of the input but not the attribute; update the assertion for the
Slider render (the test using render(<Slider value={50} aria-valuetext='50
percent' />), container.querySelector('input[type="range"]') and the slider
variable) to assert that the input has aria-valuetext equal to '50 percent'
(e.g., use expect(slider).toHaveAttribute('aria-valuetext', '50 percent') or
equivalent).
- Around line 79-83: The tests for Slider are inconsistent: "handles single
value" uses synchronous assertions but "handles range values" uses waitFor
unnecessarily; since both are controlled props their attributes should be set on
initial render, so remove the async waitFor from the "handles range values" test
and use the same synchronous pattern as in "handles single value" (query the
input[type=\"range\"] and assert aria-valuenow, aria-valuemin and aria-valuemax
directly). Locate the tests by their descriptions ("handles single value" and
"handles range values") in slider.test.tsx and update the latter to match the
synchronous assertions used in the former.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d82d8cc1-6039-4595-8587-7c0804e306f2
📒 Files selected for processing (2)
packages/raystack/components/slider/__tests__/slider.test.tsxpackages/raystack/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/raystack/index.tsx
| it('handles single value', () => { | ||
| const { container } = render(<Slider value={50} />); | ||
| const slider = container.querySelector('input[type="range"]'); | ||
| expect(slider).toHaveAttribute('aria-valuenow', '50'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if other similar tests in the codebase use sync or async assertions for controlled component values
rg -n -C3 -g '*.test.tsx' -g '*.test.ts' 'value=\{.*\}.*aria-valuenow'Repository: raystack/apsara
Length of output: 41
🏁 Script executed:
cat -n packages/raystack/components/slider/__tests__/slider.test.tsx | head -150Repository: raystack/apsara
Length of output: 6813
🏁 Script executed:
# Search for other controlled component tests to understand patterns
rg -n 'aria-valuenow' -A2 -B2 packages/raystack/components/slider/__tests__/Repository: raystack/apsara
Length of output: 2457
🏁 Script executed:
# Check the Slider component implementation to see if it has async behavior
fd -g 'slider.tsx' -o -g 'slider.ts' packages/raystack/components/slider/ | grep -v test | grep -v '.test'Repository: raystack/apsara
Length of output: 176
🏁 Script executed:
fd -type f -name 'slider.tsx' -o -name 'slider.ts' | grep -v test | head -5Repository: raystack/apsara
Length of output: 230
🏁 Script executed:
find packages/raystack/components/slider -type f \( -name '*.tsx' -o -name '*.ts' \) ! -path '*test*' ! -path '*\.test\.*'Repository: raystack/apsara
Length of output: 152
🏁 Script executed:
cat -n packages/raystack/components/slider/slider.tsx | head -100Repository: raystack/apsara
Length of output: 3524
Synchronous and asynchronous assertions are inconsistently applied for controlled values.
The "handles single value" test (lines 79-83) uses synchronous assertions, while the "handles range values" test (lines 85-93) uses waitFor. Both tests use controlled props that should set attributes on render immediately. Verify whether the range test actually requires async assertions, or if both should use the same pattern for consistency and to avoid unnecessary delays in test execution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/slider/__tests__/slider.test.tsx` around lines
79 - 83, The tests for Slider are inconsistent: "handles single value" uses
synchronous assertions but "handles range values" uses waitFor unnecessarily;
since both are controlled props their attributes should be set on initial
render, so remove the async waitFor from the "handles range values" test and use
the same synchronous pattern as in "handles single value" (query the
input[type=\"range\"] and assert aria-valuenow, aria-valuemin and aria-valuemax
directly). Locate the tests by their descriptions ("handles single value" and
"handles range values") in slider.test.tsx and update the latter to match the
synchronous assertions used in the former.
| it('sets aria-valuetext', () => { | ||
| const { container } = render( | ||
| <Slider value={50} aria-valuetext='50 percent' /> | ||
| ); | ||
| const slider = container.querySelector('input[type="range"]'); | ||
| expect(slider).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
Critical test coverage regression: aria-valuetext attribute not verified.
The test is named "sets aria-valuetext" but only checks that the slider element exists. It no longer verifies that the aria-valuetext attribute is actually set to the expected value ('50 percent').
🐛 Restore the missing assertion
it('sets aria-valuetext', () => {
const { container } = render(
<Slider value={50} aria-valuetext='50 percent' />
);
const slider = container.querySelector('input[type="range"]');
- expect(slider).toBeInTheDocument();
+ expect(slider).toHaveAttribute('aria-valuetext', '50 percent');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('sets aria-valuetext', () => { | |
| const { container } = render( | |
| <Slider value={50} aria-valuetext='50 percent' /> | |
| ); | |
| const slider = container.querySelector('input[type="range"]'); | |
| expect(slider).toBeInTheDocument(); | |
| }); | |
| it('sets aria-valuetext', () => { | |
| const { container } = render( | |
| <Slider value={50} aria-valuetext='50 percent' /> | |
| ); | |
| const slider = container.querySelector('input[type="range"]'); | |
| expect(slider).toHaveAttribute('aria-valuetext', '50 percent'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/slider/__tests__/slider.test.tsx` around lines
140 - 146, The test "sets aria-valuetext" currently only checks existence of the
input but not the attribute; update the assertion for the Slider render (the
test using render(<Slider value={50} aria-valuetext='50 percent' />),
container.querySelector('input[type="range"]') and the slider variable) to
assert that the input has aria-valuetext equal to '50 percent' (e.g., use
expect(slider).toHaveAttribute('aria-valuetext', '50 percent') or equivalent).
1 participant
Summary
OTPFieldcomponent built on Base UI's OTP field with composableOTPField.InputandOTPField.Separatorslotslength,validationType,mask,disabled,readOnly,autoSubmit