Skip to content

fix: allow external onChange to override PhoneNumberInput's internal handler#137

Closed
jaruesink wants to merge 1 commit into
mainfrom
fix/phone-input-onChange-override
Closed

fix: allow external onChange to override PhoneNumberInput's internal handler#137
jaruesink wants to merge 1 commit into
mainfrom
fix/phone-input-onChange-override

Conversation

@jaruesink
Copy link
Copy Markdown
Contributor

@jaruesink jaruesink commented Sep 12, 2025

Summary by CodeRabbit

  • Bug Fixes
    • Phone Input now respects parent-provided value and event handlers, ensuring controlled usage works as expected.
    • Custom onChange/onKeyDown from consumers reliably override internal behavior, improving compatibility with form libraries and key handling.

…handler

The PhoneNumberInput component was spreading props before its internal
onChange and onKeyDown handlers, which prevented external handlers from
overriding the internal ones. This caused issues in forms where the
parent component needed to manage state updates.

By moving the props spread to the end, external onChange handlers can
now properly override the internal handler when needed, matching the
behavior of other form components like TextField.

This fixes the issue where phone number fields would immediately restore
their original values when users tried to edit them in forms that manage
their own state.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@bolt-new-by-stackblitz
Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Sep 12, 2025

Walkthrough

Reordered prop spread on the input element in phone-input.tsx so that {...props} is applied later, allowing parent-provided value and handlers to override internal ones. No other logic or public API changes.

Changes

Cohort / File(s) Summary of Changes
UI Input Prop Order
packages/components/src/ui/phone-input.tsx
Moved ...props after internal handlers, changing prop precedence so external value, onChange, onKeyDown override internal defaults. No signature or logic changes otherwise.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant P as Parent
  participant C as PhoneInput
  participant I as Native Input

  P->>C: Render with props (value, onChange, onKeyDown, ...)
  C->>C: Prepare internal value/display and handlers
  Note over C: Apply props in order<br/>Internal props then {...props}
  C->>I: Render input with merged props (parent overrides)
  I-->>P: Events (onChange/onKeyDown) handled by parent if provided
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title accurately and concisely summarizes the primary change: reordering the props spread so a parent-supplied onChange can override the component's internal handler, which matches the diff in packages/components/src/ui/phone-input.tsx. It is specific, clear, and focused on the behavioral fix rather than noisy or vague. Mentioning that value and onKeyDown are also affected is optional and not required for the title to be useful.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Poem

I twitch my ears at props that flow,
Late to the party, they steal the show.
Parent whispers: “I’ll drive tonight.”
Internal plans yield to that right.
Hop-hop—override achieved,
Carrots aligned, behavior reprieved.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0abcba2 and c0b8ab1.

📒 Files selected for processing (1)
  • packages/components/src/ui/phone-input.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
packages/components/src/ui/**/*.tsx

📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)

packages/components/src/ui/**/*.tsx: Build on @radix-ui components as the foundation for base UI components
Follow the component composition pattern for UI components that accept form integration

Files:

  • packages/components/src/ui/phone-input.tsx
packages/components/src/ui/*.{tsx,ts}

📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)

Base UI components should be named as ComponentName in ui/ directory

Files:

  • packages/components/src/ui/phone-input.tsx
**/*.{tsx,ts}

📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)

**/*.{tsx,ts}: Props interfaces should be named as ComponentNameProps
Form schemas should be named formSchema or componentNameSchema

Files:

  • packages/components/src/ui/phone-input.tsx
packages/components/src/{remix-hook-form,ui}/*.{tsx,ts}

📄 CodeRabbit inference engine (.cursor/rules/form-component-patterns.mdc)

Always export both the component and its props type

Files:

  • packages/components/src/ui/phone-input.tsx
{apps,packages}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)

{apps,packages}/**/*.{ts,tsx}: Use package name imports for published packages (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form')
Import from specific entry points (e.g., import { TextField } from '@lambdacurry/forms/remix-hook-form/text-field')
Do not use relative imports across packages (e.g., avoid import { TextField } from '../../packages/components/src/remix-hook-form/text-field')
Order imports: 1) external libraries, 2) internal package imports, 3) cross-package imports, 4) type-only imports (grouped separately)

Files:

  • packages/components/src/ui/phone-input.tsx
{apps,packages}/**/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)

{apps,packages}/**/src/**/*.{ts,tsx}: Use relative imports within the same package (e.g., import { FormControl } from './form')
Use relative imports for sibling directories (e.g., import { Button } from '../ui/button')

Files:

  • packages/components/src/ui/phone-input.tsx
packages/components/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)

packages/components/src/**/*.{ts,tsx}: Always export both component and props type (e.g., export { ComponentName }; export type { ComponentNameProps };)
Use named exports for components for better tree-shaking (e.g., export const ComponentName = ...; avoid default exports)
Avoid default exports for components
Use tree-shaking friendly exports

Files:

  • packages/components/src/ui/phone-input.tsx
{apps,packages}/**/src/**/*.{tsx,ts}

📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)

{apps,packages}/**/src/**/*.{tsx,ts}: Use kebab-case for component file names (e.g., text-field.tsx, data-table.tsx)
Match component name in PascalCase to the exported component from the file (e.g., text-field.tsx exports TextField)

Files:

  • packages/components/src/ui/phone-input.tsx
packages/components/src/ui/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)

Don't import from remix-hook-form package in ui components

Files:

  • packages/components/src/ui/phone-input.tsx
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/monorepo-organization.mdc)

Avoid circular dependencies

Files:

  • packages/components/src/ui/phone-input.tsx
🔇 Additional comments (1)
packages/components/src/ui/phone-input.tsx (1)

137-141: Keep {...props} before invariants; compose handlers instead.

Moving {...props} last lets consumers override type/inputMode/onKeyDown (value/onChange are destructured and won't be replaced); keep the spread before invariants and compose onKeyDown to call internal then consumer handler.

File: packages/components/src/ui/phone-input.tsx — around lines 137–141

-      data-slot="input"
-      aria-label={props['aria-label']}
-      value={display}
-      onChange={handleInputChange}
-      onKeyDown={handleKeyDown}
-      {...props}
+      data-slot="input"
+      {...rest}
+      value={display}
+      onChange={handleInputChange}
+      onKeyDown={(e) => {
+        handleKeyDown(e);
+        onKeyDownProp?.(e);
+      }}
-export const PhoneNumberInput = ({
-  value,
-  onChange,
-  isInternational = false,
-  className,
-  inputClassName,
-  ...props
-}: PhoneInputProps & { ref?: Ref<HTMLInputElement> }) => {
+export const PhoneNumberInput = ({
+  value,
+  onChange,
+  isInternational = false,
+  className,
+  inputClassName,
+  onKeyDown: onKeyDownProp,
+  ...rest
+}: PhoneInputProps & { ref?: Ref<HTMLInputElement> }) => {
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/phone-input-onChange-override

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

📝 Storybook Preview: View Storybook

This preview will be updated automatically when you push new changes to this PR.

Note: The preview will be available after the workflow completes and the PR is approved for deployment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants