Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
d335946 to
d913c3c
Compare
Codex Review (GPT-5.4)This remains a 1. Remaining Bugs / Issues (None)I do not currently see any remaining actionable correctness or API issues on the branch head. 2. Public API & Composition AssessmentThe public shape is now much cleaner than the original branch state. Making 3. Accessibility and Interaction AssessmentThe interaction model looks solid at this point: canceled changes no longer move focus, forward delete reselects the shifted value, invalid rejected input preserves selection, readonly slots still participate in focus tracking and keyboard navigation, and the root/label wiring is internally consistent. For a preview component, the keyboard and focus behavior now looks well covered. 4. Previous Review Issues Status
Merge Confidence ScoreOverall merge confidence is 5/5. I do not see remaining blockers, and the branch now has strong coverage for the keyboard, focus, validation, docs, and form-integration paths that carry most of the risk for this component. |
832d3fc to
d5b66da
Compare
flaviendelangle
left a comment
There was a problem hiding this comment.
I'll look at the DX more in depth later
Code Review: [otp field] Add OTP Field component (#4365)
Author: atomiks | +4,109 / -1 | 44 files
Overview
This PR adds a new OTPField component (marked as Preview) with Root, Group, and Input parts. It includes full documentation (hero, alphanumeric, password, custom-sanitize demos), comprehensive tests (~1,240 lines), type-safety spec tests, SSR support, hidden validation input for native form integration, and Field/Form compatibility. The length prop on Root is the source of truth, while CompositeList handles slot ordering.
Strengths
- Excellent test coverage - 912 lines of root tests + 328 lines of input tests covering value handling, controlled/uncontrolled modes, async state reconciliation, cancel semantics, keyboard interactions, pasting, SSR, Field integration, form submission, and data attributes.
- Well-designed API - Discriminated union event details (
ChangeEventDetails,InvalidEventDetails,CompleteEventDetails) with proper TypeScript narrowing, verified by spec tests. - Solid controlled mode handling - The
pendingFocusRef/pendingCompleteValueRefpattern correctly handles async controlled updates (e.g.,onValueChangewithsetTimeout), with tests for stale vs. accepted completions. - Good accessibility -
role="group",aria-labelledby/aria-describedbypropagation through Field and native labels, per-slotaria-label,enterKeyHintbased on slot position. - Follows project conventions - Uses existing utilities (
useStableCallback,useControlled,CompositeList,useField,useRenderElement), proper data attributes, standard file structure, error code registered.
Issues & Suggestions
1. ReadOnly fields skip focus tracking (bug)
In OTPFieldInput.tsx:221-223, onFocus returns early when readOnly:
onFocus(event) {
if (event.defaultPrevented || disabled || readOnly) {
return;
}This means data-focused is never set on readonly fields, and handleInputFocus never runs. Readonly fields should still be focusable and trackable - only editing should be blocked. The disabled guard is correct (disabled inputs can't focus natively), but readOnly should be removed here. Same for onMouseDown - the event.preventDefault() and focusInput call should still work for readonly (they just select the text).
2. Native contains instead of shadow DOM-safe utility
In OTPFieldRoot.tsx:283-284:
if (nextTarget instanceof Node && rootRef.current?.contains(nextTarget)) {Per CLAUDE.md, the shadow DOM-safe contains utility should be used instead of the native .contains() method.
3. Double clipboard read in onPaste
In OTPFieldInput.tsx:259-264, event.clipboardData.getData('text/plain') is called twice. Store it in a variable:
const rawValue = event.clipboardData.getData('text/plain');
const nextDigits = normalizeOTPValue(rawValue, length, validationType, sanitizeValue);
const didSanitize = stripOTPWhitespace(rawValue).length > nextDigits.length;4. Demo useInvalidFeedback uses raw setTimeout
useInvalidFeedback.ts:42-47 uses setTimeout / clearTimeout directly. CLAUDE.md recommends useTimeout from @base-ui/utils/useTimeout. This is demo code so it's less critical, but it's shown to users as a reference.
5. Missing test coverage for alpha validation on typing
The alpha validationType is tested via defaultValue splitting but there's no test for typing/pasting into an alpha-mode field.
Minor Observations
- Significant CSS duplication across demo CSS modules (hero, alphanumeric, password share ~80% identical styles) - fine for demo independence.
{1}quantifier in slot patterns (e.g.\\d{1}) is technically redundant but keeps consistency with the root pattern.- The
onValueChangeJSDoc intypes.mdis missing line breaks between the bullet points for reason descriptions (renders as a run-on sentence in the generated table).
Rating: 4/5
This is a high-quality, well-architected component addition with excellent test coverage and thoughtful API design. The readonly focus tracking bug (#1) and missing shadow DOM-safe contains (#2) are the only issues that should be addressed before merge. Everything else is polish.
aarongarciah
left a comment
There was a problem hiding this comment.
I did a quick pass, leaving some feedback/questions before going deeper:
-
I'm not quite sure I follow the logic that decides when the text is selected or not. Can you explain the reasoning behind when it should be selected or not? Is it useful to auto-select text on inputs? I see other OTP fields in the wild that don't do this and maybe they feel more natural. But I need to play more with them before having a strong opinion.
Kapture.2026-03-26.at.16.39.52.mp4
-
In the video above, there's a moment when I press numbers inside an input but nothing happens. Does it happen because the text inside the input is not selected? It felt surprising.
-
What's the use case for
OTPField.Group? Is there a scenario where several of them would be used? If that's the case, a demo would be nice.
docs/src/app/(docs)/react/components/otp-field/demos/hero/css-modules/index.module.css
Show resolved
Hide resolved
It should always be selected if you navigate via keyboard or click into an already-filled input. I actually can't get into the state you got in in the recording when pressing Backspace - it's always selected in that case 🤔
I can't get into the state where this happens, do you have the exact steps/keys?
It's mainly to match |
So is it expected that the last input gets selected when filling it? Kapture.2026-03-26.at.22.57.41.mp4
You can see the exact keys being pressed in the video, but I can reproduce it consistently with this sequence: 1, 2, 3, 4, 5, 6, ⬅️, Del: at this point the selection is lost and pressing any number doesn't have an effect.
Sounds like it's worth considering removing it. |
Fixed
This one too noisy, if you want
Fixed
Fixed |
Closes #75
Preview: https://deploy-preview-4365--base-ui.netlify.app/react/components/otp-field
Summary
This adds a preview OTP Field component with Root and Input parts, along with docs, hero demos, tests, and generated API reference.
API note:
lengthis required onOTPField.Rootas the source of truth for the intended OTP size. The component uses it for validation, hidden-input form integration, completion state, and deterministic SSR or initial render behavior, while CompositeList is used only for slot order and indexing.