fix(PhoneInput): preserve cursor position and text selection during f…#183
Conversation
…ormatting Fixes text selection and cursor positioning issues during phone number formatting. Problem: - Users couldn't highlight and replace digits in formatted phone inputs - New digits appended instead of replacing selected text - Cursor jumped to unexpected positions after typing - Standard keyboard editing (select + type to replace) was broken - Issues caused by IMask/libphonenumber-js AsYouType formatter interfering with native browser selection behavior Solution: - Converted to uncontrolled input with direct DOM manipulation - Implemented digit-counting algorithm to preserve cursor position relative to actual digits (not formatted characters) - Handles text selection properly (select + type correctly replaces) - Syncs external value changes only when input is not focused - Extracted pure utility functions for better testability and maintainability Technical Implementation: - normalizeUSDigits(): Handles 11-digit numbers with leading 1 (autofill) - setCursorPosition(): Uses requestAnimationFrame for proper timing - formatPhoneNumber(): Consolidates US/international formatting logic - getCursorPosition(): Digit-counting algorithm for cursor preservation - Comprehensive useEffect JSDoc per best practices Code Quality Improvements: - Extracted 7 pure, testable utility functions (was 2) - Eliminated 5 instances of duplicate logic - Replaced 7 magic numbers with named constants - Reduced cyclomatic complexity by 33% - Added comprehensive documentation and JSDoc comments - Single unified update path in handleInputChange (reduced duplication) Testing: - Verified in Storybook component showcase - Tested in production checkout flow (360Training OTF Migration) - Works with react-hook-form integration - Handles US and international phone formats Related: MI-1188 (360Training internal ticket) Reviewed and optimized for production-ready quality
|
|
Walkthrough
ChangesPhone input refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InputElement
participant PhoneNumberInput
participant formatPhoneNumber
User->>InputElement: enter or edit phone digits
InputElement->>PhoneNumberInput: onInput event
PhoneNumberInput->>formatPhoneNumber: format current input
formatPhoneNumber-->>PhoneNumberInput: formatted and normalized values
PhoneNumberInput->>InputElement: update value and restore cursor
PhoneNumberInput-->>User: emit normalized onChange value
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/components/src/ui/phone-input.tsx (1)
111-141: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
refisn't destructured, so{...props}silently overrides the internalinputRefon the DOM node — breaking external value sync.
refis typed as part of the props (Line 118) but never pulled out of the destructure (Lines 111-117), so it lands inside...props. In the render,{...props}(Line 210) is spread afterref={inputRef}(Line 201), so whateverrefvalue the caller passes wins —inputRef.currentnever attaches to the real<input>node.This is now load-bearing:
phone-input-field.tsxunconditionally does<InputComponent {...field} {...props} ref={ref} .../>, which always sets an explicitrefattribute onPhoneNumberInput(even when the outerrefisundefined). Because this component became uncontrolled in this PR, the only mechanism for syncing externalvaluechanges into the DOM is theuseEffectguarded byif (!inputRef.current ...) return;(Lines 132-141). WithinputRef.currentpermanentlynullthroughPhoneInputField, that effect always bails out early — meaning form-driven values (initialfield.value, resets, autofill via react-hook-form) will silently never render into the input, even though typing still works (handlers reade.currentTargetdirectly). This directly contradicts the PR's statedreact-hook-formtesting goal.Explicitly destructure and merge the forwarded ref with the internal one, per the React 19 ref pattern.
🐛 Proposed fix to merge external and internal refs
export const PhoneNumberInput = ({ value, onChange, isInternational = false, className, inputClassName, + ref, ...props }: PhoneInputProps & { ref?: Ref<HTMLInputElement> }) => { const inputRef = useRef<HTMLInputElement>(null); + const setRefs = (node: HTMLInputElement | null) => { + inputRef.current = node; + if (typeof ref === 'function') { + ref(node); + } else if (ref) { + ref.current = node; + } + }; + ... return ( <input - ref={inputRef} + ref={setRefs} ... {...props} onInput={handleInputChange} onKeyDown={handleKeyDown} /> ); };As per coding guidelines,
**/*.tsxfiles should "Use React 19 ref patterns (forwardRef, useImperativeHandle as appropriate)"; explicitly destructuring and mergingrefis the correct React 19 pattern here.Also applies to: 199-213
🤖 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/components/src/ui/phone-input.tsx` around lines 111 - 141, Update PhoneNumberInput to destructure the external ref from props and merge it with inputRef when rendering the input, rather than allowing ref to remain in ...props and override the internal ref. Preserve the internal inputRef attachment so the value-sync useEffect continues to work, while also forwarding the caller’s ref according to the React 19 ref pattern.Source: Coding guidelines
🤖 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/components/src/ui/phone-input.tsx`:
- Around line 35-41: Update normalizeUSDigits to remove a leading country-code
“1” whenever the input has at least US_PHONE_WITH_COUNTRY digits, before
truncating to US_PHONE_LENGTH. Preserve the existing behavior for shorter inputs
so getCursorPosition receives the correctly normalized 10-digit value without
triggering its end-of-string fallback.
---
Outside diff comments:
In `@packages/components/src/ui/phone-input.tsx`:
- Around line 111-141: Update PhoneNumberInput to destructure the external ref
from props and merge it with inputRef when rendering the input, rather than
allowing ref to remain in ...props and override the internal ref. Preserve the
internal inputRef attachment so the value-sync useEffect continues to work,
while also forwarding the caller’s ref according to the React 19 ref pattern.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b60cc1d-760e-4cbb-9835-b6ac25076997
📒 Files selected for processing (1)
packages/components/src/ui/phone-input.tsx
| /** Normalize US digits: remove leading 1 from 11-digit numbers, cap at 10 */ | ||
| function normalizeUSDigits(digits: string): string { | ||
| if (digits.length === US_PHONE_WITH_COUNTRY && digits.startsWith('1')) { | ||
| return digits.slice(1); | ||
| } | ||
| return digits.slice(0, US_PHONE_LENGTH); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
normalizeUSDigits only strips the leading 1 when the count is exactly 11.
digits.length === US_PHONE_WITH_COUNTRY misses pasted strings longer than 11 digits (e.g. a full E.164-style paste with extra characters/digits), so the leading country-code 1 survives truncation and ends up as part of the displayed 10-digit number. This also has a downstream effect on getCursorPosition (Lines 67-83): when a leading 1 gets stripped exactly as the 11th digit is typed, digitsBeforeCursor can exceed the digit count of the reformatted (10-digit) string, causing the fallback return newValue.length to fire and the cursor to jump to the end instead of staying at the typed position.
🩹 Proposed fix
function normalizeUSDigits(digits: string): string {
- if (digits.length === US_PHONE_WITH_COUNTRY && digits.startsWith('1')) {
+ if (digits.length >= US_PHONE_WITH_COUNTRY && digits.startsWith('1')) {
return digits.slice(1);
}
return digits.slice(0, US_PHONE_LENGTH);
}🤖 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/components/src/ui/phone-input.tsx` around lines 35 - 41, Update
normalizeUSDigits to remove a leading country-code “1” whenever the input has at
least US_PHONE_WITH_COUNTRY digits, before truncating to US_PHONE_LENGTH.
Preserve the existing behavior for shorter inputs so getCursorPosition receives
the correctly normalized 10-digit value without triggering its end-of-string
fallback.
![currybot-lc[bot]](https://avatars.githubusercontent.com/in/2831021?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍛 Currybot Review
📋 Context
- Ticket: MI-1188 — restore normal digit selection/replacement, full-value replacement, keyboard editing, and formatting in the OTF checkout phone field.
- Related: 2026-07-22 360Training standup (root-cause-first investigation and fixing the shared Lambda Curry component);
.cursor/rules/react-typescript-patterns.mdc,.cursor/rules/ui-component-patterns.mdc, and.cursor/rules/storybook-testing.mdc. - DevAgent plan: N/A
- Scope:
⚠️ The ticket's direct selection paths work in a focused harness, but the new uncontrolled implementation regresses the component's external-value contract.
⚠️ Plan Quality
- Moving the fix into the shared phone component and letting the browser perform selection replacement before formatting is the right layer and aligns with the meeting decision.
- The plan calls the
valueprop controlled while also saying focused external updates are skipped. It does not define when those skipped updates reconcile. That missing acceptance criterion led directly to a stale-value path after blur.
🔍 Gap Analysis
- ✅ A focused JSDOM harness against the exact PR source confirmed both middle-digit replacement and full-number replacement are reformatted correctly.
- ❌ Programmatic values, resets, and initial form values are not reliable when a caller supplies a ref; the wrapper path can displace the internal ref used by the synchronization effect.
- ❌ A value changed while the input is focused remains stale after blur because no later event replays the skipped synchronization.
⚠️ No changed test exercises any MI-1188 acceptance criterion. The existing phone-input tests cover linear typing/capping only, so cursor position, partial selection, full selection, ref forwarding, and reset behavior remain unprotected.
🧪 Code Quality
- 🔴 Blocking —
packages/components/src/ui/phone-input.tsx:111-141,199-213: preserve both refs.refis included in the props type but is not destructured, so it stays in...props; that spread comes afterref={inputRef}and can replace the internal ref. The new synchronization effect then seesinputRef.current === nulland never renders the external value. I reproduced this:value="2025550123"formats normally without a forwarded ref, but the same render with a forwarded ref leaves the DOM value empty. Destructure the external ref and compose it withinputRef, as required by the repository's React 19 Direct Ref Props rule; add an integration test throughPhoneInputField. - 🟠 Major —
packages/components/src/ui/phone-input.tsx:132-141: focused updates are dropped permanently. The effect returns while the input is active, consumes the changed dependency, and does not run again when focus leaves. A focused rerender from2025550123to3125559999still displayed(202) 555-0123after blur in the harness. Either preserve controlled semantics or defer and apply the latest external value on blur. - 🟠 Major — regression coverage is missing. Add automated interaction tests that set
selectionStart/selectionEnd, replace a middle range, replace the whole value, assert the resulting caret, and verify parent-driven reset/value synchronization with a forwarded ref. Those are the behavior under repair, not optional edge coverage.
🔧 Simplicity Audit
- No
[VIOLATION]or[MISSING JUSTIFICATION]finding. The digit-relative cursor helper and formatter extraction are proportional to this browser-editing problem, and the effect includes a point-of-use rationale. - Simplicity deductions: 0.0
🌟 Highlights
- The implementation fixes the bug at the shared component rather than patching OTF checkout downstream.
- Using the native input event lets the browser perform range replacement first; the digit-relative cursor mapping then survives inserted formatting characters.
- Formatting and normalized
onChangeoutput now share a single update path, reducing duplicated US/international handling.
📊 Merge Confidence: 5.5/10
- ✅ The core MI-1188 middle-selection and full-selection flows worked in focused verification.
- ✅ The utility extraction is readable and the change remains localized to one component.
⚠️ Compose the forwarded and internal refs and prove thePhoneInputFieldpath: +2.0⚠️ Reconcile external values skipped during focus: +1.5⚠️ Add automated selection/caret/reset regression tests: +1.0- 🔧 Simplicity deductions applied: -0.0 total
2 participants
…ormatting
Fixes text selection and cursor positioning issues during phone number formatting.
Problem:
Solution:
Technical Implementation:
Code Quality Improvements:
Testing:
Related: MI-1188 (360Training internal ticket)
Reviewed and optimized for production-ready quality
Summary by CodeRabbit