feat(number-input): add isRealTimeFormat prop #5926
Conversation
|
|
@hasegawa-101 is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis PR adds real-time number formatting to the NumberInput component by introducing a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Input as Input Element
participant Handler as handleBeforeInput
participant Parser as NumberParser
participant Formatter as Intl.NumberFormat
participant State as Component State
User->>Input: Types character
Input->>Handler: Fires beforeinput event<br/>(isRealTimeFormat enabled)
Handler->>Parser: Validate partial input
alt Valid partial number
Parser-->>Handler: Parsing succeeds
Handler->>Formatter: Format validated number
Formatter-->>Handler: Formatted display value
Handler->>State: Update internal & numeric value
Handler->>Input: preventDefault + set formatted value
State->>Input: Emit onChange with formatted value
else Invalid input
Parser-->>Handler: Parsing fails
Handler->>Input: preventDefault (block invalid char)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly Related PRs
Suggested Reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-aria-overlay
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-form-reset
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/use-viewport-size
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/components/number-input/package.json(1 hunks)packages/components/number-input/src/use-number-input.ts(5 hunks)packages/components/number-input/stories/number-input.stories.tsx(1 hunks)
🔇 Additional comments (8)
packages/components/number-input/stories/number-input.stories.tsx (1)
534-547: LGTM! Good demonstration of the new feature.The story effectively demonstrates real-time formatting with appropriate format options and uses a controlled component to showcase the value updates.
packages/components/number-input/src/use-number-input.ts (6)
17-17: LGTM! Appropriate import for number parsing.The
NumberParserfrom@internationalized/numberis correctly imported to support locale-aware number parsing.
83-88: LGTM! Clear prop definition with good documentation.The
isRealTimeFormatprop is well-documented and defaults tofalse, maintaining backward compatibility with existing behavior.
314-320: LGTM! Proper memoization of formatters.The
numberFormatterandnumberParserare correctly memoized with appropriate dependencies to avoid unnecessary recreations while ensuring they update when locale or format options change.
461-462: Good conditional attachment pattern.The conditional attachment of
onBeforeInputonly whenshouldFormatis true is a good practice. However, this depends on fixing theshouldFormatlogic flagged earlier.
469-484: LGTM! Dependency array correctly updated.The dependency array properly includes
shouldFormatandhandleBeforeInput, ensuring the props getter updates when these values change.
332-376: Review comment is factually incorrect on both concerns.
Deletion handling is implemented:
handleKeyDownexplicitly handles Backspace (lines 271–303) withe.preventDefault()and custom logic for grouping separators. Deletion operations are not ignored.Validation input type is correct: React Aria's
useNumberFieldState.validateexpects a string input, so callingstate.validate(formattedValue)with the formatted string is the correct approach, not an error.The code follows React Aria's design patterns appropriately.
Likely an incorrect or invalid review comment.
packages/components/number-input/package.json (1)
51-51: No issues found — dependency version is current and secure.Verification confirms that
@internationalized/[email protected]is the latest stable version with no known security vulnerabilities.
| const shouldFormat = useMemo(() => { | ||
| // Return false if isRealTimeFormat is not enabled (React Aria default) | ||
| if (!originalProps.isRealTimeFormat) return false; | ||
|
|
||
| // Only check useGrouping if isRealTimeFormat is true | ||
| const resolved = numberFormatter.resolvedOptions(); | ||
|
|
||
| return resolved.useGrouping !== false; | ||
| }, [originalProps.isRealTimeFormat, numberFormatter]); |
There was a problem hiding this comment.
Overly restrictive condition for enabling real-time formatting.
The current logic only enables real-time formatting when useGrouping !== false. However, users may want real-time formatting for other scenarios like currency (style: "currency"), percentages (style: "percent"), or custom decimal places, even without grouping separators.
Consider simplifying to enable formatting whenever isRealTimeFormat is true:
const shouldFormat = useMemo(() => {
- // Return false if isRealTimeFormat is not enabled (React Aria default)
- if (!originalProps.isRealTimeFormat) return false;
-
- // Only check useGrouping if isRealTimeFormat is true
- const resolved = numberFormatter.resolvedOptions();
-
- return resolved.useGrouping !== false;
+ return !!originalProps.isRealTimeFormat;
}, [originalProps.isRealTimeFormat, numberFormatter]);📝 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.
| const shouldFormat = useMemo(() => { | |
| // Return false if isRealTimeFormat is not enabled (React Aria default) | |
| if (!originalProps.isRealTimeFormat) return false; | |
| // Only check useGrouping if isRealTimeFormat is true | |
| const resolved = numberFormatter.resolvedOptions(); | |
| return resolved.useGrouping !== false; | |
| }, [originalProps.isRealTimeFormat, numberFormatter]); | |
| const shouldFormat = useMemo(() => { | |
| return !!originalProps.isRealTimeFormat; | |
| }, [originalProps.isRealTimeFormat, numberFormatter]); |
| const shouldFormat = useMemo(() => { | |
| // Return false if isRealTimeFormat is not enabled (React Aria default) | |
| if (!originalProps.isRealTimeFormat) return false; | |
| // Only check useGrouping if isRealTimeFormat is true | |
| const resolved = numberFormatter.resolvedOptions(); | |
| return resolved.useGrouping !== false; | |
| }, [originalProps.isRealTimeFormat, numberFormatter]); | |
| const shouldFormat = useMemo(() => { | |
| return !!originalProps.isRealTimeFormat; | |
| }, [originalProps.isRealTimeFormat]); |
🤖 Prompt for AI Agents
In packages/components/number-input/src/use-number-input.ts around lines 322 to
330, the memoized shouldFormat is too restrictive because it only enables
real-time formatting when resolved.useGrouping !== false; instead, change it to
enable formatting whenever originalProps.isRealTimeFormat is truthy. Concretely:
remove the numberFormatter.resolvedOptions() check and have the memo return
Boolean(originalProps.isRealTimeFormat). Update the hook dependency array to
only include originalProps.isRealTimeFormat (remove numberFormatter) since it is
no longer used in this memo.
| const handleBeforeInput = useCallback( | ||
| (e: React.FormEvent<HTMLInputElement> & {data: string | null}) => { | ||
| if (!e.data) return; | ||
|
|
||
| const input = domRef.current; | ||
|
|
||
| if (!input) return; | ||
|
|
||
| const {value, selectionStart, selectionEnd} = input; | ||
| const nextValue = | ||
| value.slice(0, selectionStart ?? 0) + e.data + value.slice(selectionEnd ?? 0); | ||
|
|
||
| // Use React Aria's NumberParser for validation and parsing | ||
| // This handles full-width numbers and locale-specific symbols | ||
| if (!numberParser.isValidPartialNumber(nextValue)) { | ||
| e.preventDefault(); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const parsedValue = numberParser.parse(nextValue); | ||
|
|
||
| if (isNaN(parsedValue)) return; | ||
|
|
||
| e.preventDefault(); | ||
|
|
||
| const formattedValue = numberFormatter.format(parsedValue); | ||
|
|
||
| // Call validate like React Aria does | ||
| if (!state.validate(formattedValue)) { | ||
| return; | ||
| } | ||
|
|
||
| state.setInputValue(formattedValue); | ||
| state.setNumberValue(parsedValue); | ||
|
|
||
| if (onChange) { | ||
| onChange({ | ||
| target: {value: formattedValue}, | ||
| currentTarget: {value: formattedValue}, | ||
| } as React.ChangeEvent<HTMLInputElement>); | ||
| } | ||
| }, | ||
| [numberParser, numberFormatter, state, domRef, onChange], | ||
| ); |
There was a problem hiding this comment.
Multiple concerns with the handleBeforeInput implementation.
-
Line 354 - Missing preventDefault for NaN case: When
parsedValueis NaN, the function returns without callinge.preventDefault(), allowing the default input behavior to proceed. This can result in invalid characters being entered. -
No cursor position management: After formatting, the cursor position is not adjusted. For example, if a user types "1234" and the formatter adds a separator to make "1,234", the cursor position will be incorrect after each keystroke.
-
Browser compatibility: The
onBeforeInputevent is not supported in older versions of Firefox and some other browsers. This could cause the feature to silently fail for some users. -
Limited input method coverage:
onBeforeInputdoesn't handle paste operations, cut operations, or IME (Input Method Editor) input used for languages like Chinese and Japanese.
Recommendations:
- Add
e.preventDefault()when parsedValue is NaN:
const parsedValue = numberParser.parse(nextValue);
if (isNaN(parsedValue)) {
+ e.preventDefault();
return;
}-
Manage cursor position after formatting (this is complex and may require tracking the position relative to digits rather than characters).
-
Consider using
onInputoronChangeinstead, or add fallback handling for browsers withoutonBeforeInputsupport. -
Handle paste events separately with
onPastehandler.
📝 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.
| const handleBeforeInput = useCallback( | |
| (e: React.FormEvent<HTMLInputElement> & {data: string | null}) => { | |
| if (!e.data) return; | |
| const input = domRef.current; | |
| if (!input) return; | |
| const {value, selectionStart, selectionEnd} = input; | |
| const nextValue = | |
| value.slice(0, selectionStart ?? 0) + e.data + value.slice(selectionEnd ?? 0); | |
| // Use React Aria's NumberParser for validation and parsing | |
| // This handles full-width numbers and locale-specific symbols | |
| if (!numberParser.isValidPartialNumber(nextValue)) { | |
| e.preventDefault(); | |
| return; | |
| } | |
| const parsedValue = numberParser.parse(nextValue); | |
| if (isNaN(parsedValue)) return; | |
| e.preventDefault(); | |
| const formattedValue = numberFormatter.format(parsedValue); | |
| // Call validate like React Aria does | |
| if (!state.validate(formattedValue)) { | |
| return; | |
| } | |
| state.setInputValue(formattedValue); | |
| state.setNumberValue(parsedValue); | |
| if (onChange) { | |
| onChange({ | |
| target: {value: formattedValue}, | |
| currentTarget: {value: formattedValue}, | |
| } as React.ChangeEvent<HTMLInputElement>); | |
| } | |
| }, | |
| [numberParser, numberFormatter, state, domRef, onChange], | |
| ); | |
| const handleBeforeInput = useCallback( | |
| (e: React.FormEvent<HTMLInputElement> & {data: string | null}) => { | |
| if (!e.data) return; | |
| const input = domRef.current; | |
| if (!input) return; | |
| const {value, selectionStart, selectionEnd} = input; | |
| const nextValue = | |
| value.slice(0, selectionStart ?? 0) + e.data + value.slice(selectionEnd ?? 0); | |
| // Use React Aria's NumberParser for validation and parsing | |
| // This handles full-width numbers and locale-specific symbols | |
| if (!numberParser.isValidPartialNumber(nextValue)) { | |
| e.preventDefault(); | |
| return; | |
| } | |
| const parsedValue = numberParser.parse(nextValue); | |
| if (isNaN(parsedValue)) { | |
| e.preventDefault(); | |
| return; | |
| } | |
| e.preventDefault(); | |
| const formattedValue = numberFormatter.format(parsedValue); | |
| // Call validate like React Aria does | |
| if (!state.validate(formattedValue)) { | |
| return; | |
| } | |
| state.setInputValue(formattedValue); | |
| state.setNumberValue(parsedValue); | |
| if (onChange) { | |
| onChange({ | |
| target: {value: formattedValue}, | |
| currentTarget: {value: formattedValue}, | |
| } as React.ChangeEvent<HTMLInputElement>); | |
| } | |
| }, | |
| [numberParser, numberFormatter, state, domRef, onChange], | |
| ); |
🤖 Prompt for AI Agents
In packages/components/number-input/src/use-number-input.ts around lines 332 to
376, the beforeInput handler fails to prevent default on NaN (allowing invalid
characters), does not restore cursor position after formatter changes, and lacks
fallbacks for browsers/IMEs/paste/cut; fix by (1) calling e.preventDefault()
immediately when parsedValue is NaN so the input is blocked; (2) compute and
restore the caret based on digit-relative position (map cursor to index among
numeric characters before formatting, apply formatter, then set
selectionStart/End on the input to the equivalent digit-relative position in the
formatted string) after state.setInputValue so typing keeps expected cursor
placement; (3) add fallbacks/augmentation: handle onPaste and onCut to sanitize
pasted/cut content and call the same parse/format logic, and add an
onInput/onChange fallback branch for browsers lacking onBeforeInput or IME
composition (also respect compositionstart/compositionend to avoid interfering
with IME); ensure domRef is used to set selection and that all new handlers call
e.preventDefault where appropriate.
🧩 Analysis chain
Consider IME composition and accessibility implications.
The onBeforeInput handler intercepts and prevents default input behavior, which can interfere with:
-
IME (Input Method Editor) composition: Users typing in languages like Japanese, Chinese, or Korean use composition events. Intercepting
onBeforeInputmay break the composition process. -
Accessibility tools: Screen readers and other assistive technologies may rely on standard input events.
Consider checking for composition state before intercepting:
const handleBeforeInput = useCallback(
(e: React.FormEvent<HTMLInputElement> & {data: string | null}) => {
if (!e.data) return;
// Skip if IME composition is in progress
const input = domRef.current;
if (!input) return;
// Check if composition is active (for IME input)
if ((e as any).isComposing) {
return;
}
// ... rest of the logic
},
[/* dependencies */]
);Additionally, search for how React Aria handles IME in their number field:
🌐 Web query:
How does React Aria's useNumberField handle IME composition events?
💡 Result:
Short answer: React Aria's useNumberField supports IME by treating composition as composed input — it listens for compositionstart/compositionend and defers parsing/validation until composition finishes so the IME intermediate text isn't interpreted as a final numeric value. The public docs explicitly say it "Handles composed input from input method editors (e.g. Pinyin)". [1]
Practical consequence: because useNumberField defers committing/parsing partial input, the component often only updates the numeric value (and calls onChange for the parsed value) on blur or compositionend rather than on every keystroke; this is discussed in related GitHub issues about onChange firing only on blur. [1][2]
Sources:
- React Aria useNumberField docs. [1]
- react-spectrum / react-aria GitHub issue about useNumberField onChange behavior. [2]
🏁 Script executed:
#!/bin/bash
# Check for composition event handling in the use-number-input.ts file
rg -n "composit|isComposing" packages/components/number-input/src/use-number-input.tsLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for all event handlers in the file to understand the full implementation
rg -n "handle[A-Z]|useCallback|useRef|useState" packages/components/number-input/src/use-number-input.ts -A 2 | head -100Length of output: 2613
🏁 Script executed:
#!/bin/bash
# Check if there are any composition event handlers defined
ast-grep --pattern 'onComposition$_'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the complete use-number-input.ts file to understand the full context
wc -l packages/components/number-input/src/use-number-input.tsLength of output: 60
🏁 Script executed:
#!/bin/bash
# Get full context of handleBeforeInput and surrounding code
sed -n '320,390p' packages/components/number-input/src/use-number-input.tsLength of output: 2218
🏁 Script executed:
#!/bin/bash
# Search for composition event listeners in the entire number-input directory
fd . packages/components/number-input/src --type f -name "*.ts" -o -name "*.tsx" | xargs rg -l "compositionstart|compositionend|composing" 2>/dev/null || echo "No composition handlers found"Length of output: 260
🏁 Script executed:
#!/bin/bash
# Check if useNumberFieldState is imported and what it provides
rg -n "useNumberFieldState|from.*numberfield" packages/components/number-input/src/use-number-input.ts -B 2 -A 5Length of output: 1298
🏁 Script executed:
#!/bin/bash
# Check where inputProps from useAriaNumberInput is used and if composition handlers come from there
sed -n '440,475p' packages/components/number-input/src/use-number-input.tsLength of output: 1202
🏁 Script executed:
#!/bin/bash
# Look for where useAriaNumberInput is called to see what inputProps it provides
rg -n "useAriaNumberInput|inputProps" packages/components/number-input/src/use-number-input.ts -B 2 -A 2Length of output: 1467
Add IME composition check to handleBeforeInput in packages/components/number-input/src/use-number-input.ts.
The custom handleBeforeInput handler at lines 332–376 overrides React Aria's composition-aware event handler when shouldFormat is true. React Aria's useNumberField handles composed input from input method editors (e.g. Pinyin) by deferring parsing/validation until composition finishes, but the custom implementation intercepts and prevents default without checking composition state. This breaks input for users typing in languages like Japanese, Chinese, and Korean.
Add a composition check before the preventDefault calls:
const handleBeforeInput = useCallback(
(e: React.FormEvent<HTMLInputElement> & {data: string | null}) => {
if (!e.data) return;
// Skip if IME composition is active
if ((e.nativeEvent as any).isComposing) {
return;
}
const input = domRef.current;
if (!input) return;
// ... rest of the logic
},
[numberParser, numberFormatter, state, domRef, onChange],
);🤖 Prompt for AI Agents
In packages/components/number-input/src/use-number-input.ts around lines 332 to
376, the custom handleBeforeInput unconditionally prevents default and parses
input even when an IME composition is active, breaking CJK input; add an IME
composition guard immediately after the existing "if (!e.data) return" check
(e.g. if ((e.nativeEvent as any).isComposing) return;) so the handler bails out
during composition and only proceeds with parsing/validation/ preventDefault
when composition is not active.
|
@wingkwong I would appreciate it if you could review the implementation and let me know if the direction is correct 🙏 |
1 participant
Closes #5136
📝 Description
Add
isRealTimeFormatprop to enable real-time number formatting while typing.⛳️ Current behavior (updates)
NumberInput only formats numbers on blur. Users cannot type more than 4 digits with
useGrouping: truebecause the formatted comma interferes with input.🚀 New behavior
isRealTimeFormatprop (default:false)💣 Is this a breaking change (Yes/No):
No - Default behavior unchanged. Opt-in only.
📝 Additional Information
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.