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. |
WalkthroughAdds an opt-in real-time number-formatting feature to NumberInput: new Changes
Sequence DiagramsequenceDiagram
participant User
participant Input as Input Element
participant Hook as useRealTimeInputFormatting
participant Parser as NumberParser (`@internationalized/number`)
participant Formatter as Intl.NumberFormat
participant State as NumberFieldState
User->>Input: type / paste / cut / composition
Input->>Hook: onBeforeInput / onInput / onPaste / onCut / composition events
Hook->>Parser: validate / parse partial input
alt valid partial or number
Parser-->>Hook: parsed value
Hook->>Formatter: format using locale & formatOptions
Formatter-->>Hook: formatted string
Hook->>Input: set formatted display & restore cursor
Hook->>State: update numeric value and emit onChange/onValueChange
else invalid
Hook->>Input: preventDefault / ignore
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-27T21:48:35.308ZApplied to files:
🧬 Code graph analysis (2)packages/components/number-input/__tests__/number-input.test.tsx (3)
packages/components/number-input/src/use-number-input.ts (1)
🔇 Additional comments (13)
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/number@3.6.5is the latest stable version with no known security vulnerabilities.
|
@wingkwong I would appreciate it if you could review the implementation and let me know if the direction is correct 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/components/number-input/__tests__/number-input.test.tsx (1)
751-768: Consider making locale explicit in currency formatting test.The test at line 766 uses a regex
/\$1234(\.00)?/that assumes en-US locale formatting. While the test environment likely defaults to en-US, making the locale explicit would improve test reliability across different environments.Consider wrapping the test with an explicit locale or making the assertion more flexible:
it("should format even if useGrouping is false when isRealTimeFormat is true", async () => { const {container} = render( + <HeroUIProvider locale="en-US"> <NumberInput isRealTimeFormat formatOptions={{style: "currency", currency: "USD", useGrouping: false}} label="Price" /> + </HeroUIProvider>, ); const input = container.querySelector("input") as HTMLInputElement; - // Type 1234. Should be formatted as $1234 (no comma) - // Note: Currency symbol depends on locale. Default en-US -> $ await user.type(input, "1234"); - // Standard currency formatting often adds decimals and currency symbol - // We check that it has some currency formatting but NO commas for thousands expect(input.value).toMatch(/\$1234(\.00)?/); expect(input.value).not.toContain(","); });packages/components/number-input/src/use-number-input.ts (1)
352-361: Remove unnecessarynumberParserfromhandleKeyDowndependency array.The
handleKeyDowncallback includesnumberParserin its dependency array (line 359), butnumberParseris never used within the callback. OnlynumberFormatter.formatToPartsis used at line 318 to extract the grouping character. The backspace logic manually parses withparseFloat(line 329) rather than using the parser.Remove the unused dependency:
}, [ inputValue, state, onClear, isClearable, originalProps.isReadOnly, originalProps.isRealTimeFormat, - numberParser, numberFormatter, ], );packages/components/number-input/src/use-real-time-formatting.ts (1)
68-73: Consider safer typing for synthetic onChange events.The handlers cast plain objects to
React.ChangeEvent<HTMLInputElement>at lines 68-73, 132-137, 194-199, and 262-267. This type assertion is not fully safe—realChangeEventobjects include properties likenativeEvent,preventDefault,stopPropagation, etc., which are not present in the synthetic objects. Consumer code accessing these properties may encounter runtime errors.Consider one of these approaches:
Option 1: Define a minimal event type:
type MinimalChangeEvent = { target: { value: string }; currentTarget: { value: string }; }; // Then pass without casting: if (onChange) { onChange({ target: { value: formattedValue }, currentTarget: { value: formattedValue }, } as any); }Option 2: Document the limitation in the prop type:
export interface UseRealTimeInputFormattingProps { // ... + /** + * Optional change handler. Note: receives a synthetic event with only + * target.value and currentTarget.value populated. + */ onChange?: React.ChangeEventHandler<HTMLInputElement>; }Also applies to: 132-137, 194-199, 262-267
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/number-input/__tests__/number-input.test.tsx(1 hunks)packages/components/number-input/src/use-number-input.ts(6 hunks)packages/components/number-input/src/use-real-time-formatting.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T21:48:35.308Z
Learnt from: adbjo
Repo: heroui-inc/heroui PR: 5846
File: packages/components/tabs/src/tabs.tsx:76-101
Timestamp: 2025-10-27T21:48:35.308Z
Learning: In packages/components/tabs/src/tabs.tsx, the updateCursorPosition useCallback dependency array intentionally includes `cursorRef.current` to handle the case where the cursor span element is unmounted and remounted (e.g., when `disableAnimation` or `disableCursorAnimation` toggles). This ensures the callback is recreated when the ref points to a new element, triggering a dependency chain that re-establishes the ResizeObserver and initializes the new cursor element with the data-initialized attribute.
</learning]
Applied to files:
packages/components/number-input/src/use-number-input.ts
🧬 Code graph analysis (2)
packages/components/number-input/src/use-number-input.ts (1)
packages/components/number-input/src/use-real-time-formatting.ts (1)
useRealTimeInputFormatting(15-281)
packages/components/number-input/__tests__/number-input.test.tsx (3)
packages/core/theme/src/components/user.ts (1)
user(33-33)packages/components/number-input/src/index.ts (1)
NumberInput(10-10)packages/core/theme/src/components/input.ts (1)
input(926-926)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: ESLint
- GitHub Check: Tests
- GitHub Check: Build
- GitHub Check: Continuous Release
- GitHub Check: TypeScript
🔇 Additional comments (2)
packages/components/number-input/src/use-number-input.ts (1)
265-289: LGTM: Clean integration of real-time formatting.The memoization of
numberFormatterandnumberParseris appropriate to avoid unnecessary recreations. The integration withuseRealTimeInputFormattingcorrectly passes all required dependencies, and the conditional handler attachment at lines 447-457 ensures the feature is truly opt-in.packages/components/number-input/src/use-real-time-formatting.ts (1)
204-270: LGTM: Comprehensive beforeInput handling with IME support.The
handleBeforeInputimplementation correctly addresses previous review concerns:
- Checks for IME composition at line 206 (both via ref and native event)
- Calls
preventDefault()for invalid input (lines 218, 226)- Always prevents default at line 231 to take full control of formatting
- Manages cursor position at lines 239-260
The double-check for composition state provides robust IME support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/number-input/src/use-real-time-formatting.ts (3)
19-21: Remove unnecessary memoization.The
shouldFormatcomputation is justBoolean(isRealTimeFormat), which is trivial. TheuseMemooverhead exceeds any benefit here.Apply this diff:
- const shouldFormat = useMemo(() => { - return Boolean(isRealTimeFormat); - }, [isRealTimeFormat]); + const shouldFormat = Boolean(isRealTimeFormat);
185-188: Use proper type assertion instead ofany.Accessing
isComposingthroughas anybypasses type safety. ThenativeEventis anInputEventwhich has theisComposingproperty.Apply this diff:
const handleBeforeInput = useCallback( (e: React.FormEvent<HTMLInputElement> & {data: string | null}) => { - if (isComposingRef.current || (e.nativeEvent as any).isComposing) return; + if (isComposingRef.current || (e.nativeEvent as InputEvent)?.isComposing) return; if (!e.data) return;
76-80: Consider using a more complete synthetic event or documenting the limitation.The synthetic
ChangeEventcreated at lines 76–80 includes onlytargetandcurrentTargetwithvalueproperties. While mostonChangehandlers access onlye.target.value, consumers could theoretically rely on other event properties (e.g.,type,timeStamp,nativeEvent). The current type assertion masks this incompleteness. If you want to improve robustness, either construct a more complete event object or document that only thevalueproperty is guaranteed to be available.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/number-input/__tests__/number-input.test.tsx(1 hunks)packages/components/number-input/src/use-real-time-formatting.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T21:48:35.308Z
Learnt from: adbjo
Repo: heroui-inc/heroui PR: 5846
File: packages/components/tabs/src/tabs.tsx:76-101
Timestamp: 2025-10-27T21:48:35.308Z
Learning: In packages/components/tabs/src/tabs.tsx, the updateCursorPosition useCallback dependency array intentionally includes `cursorRef.current` to handle the case where the cursor span element is unmounted and remounted (e.g., when `disableAnimation` or `disableCursorAnimation` toggles). This ensures the callback is recreated when the ref points to a new element, triggering a dependency chain that re-establishes the ResizeObserver and initializes the new cursor element with the data-initialized attribute.
</learning]
Applied to files:
packages/components/number-input/__tests__/number-input.test.tsxpackages/components/number-input/src/use-real-time-formatting.ts
🧬 Code graph analysis (1)
packages/components/number-input/__tests__/number-input.test.tsx (3)
packages/core/theme/src/components/user.ts (1)
user(33-33)packages/components/number-input/src/index.ts (1)
NumberInput(10-10)packages/core/theme/src/components/input.ts (1)
input(926-926)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: TypeScript
- GitHub Check: Tests
- GitHub Check: Prettier
- GitHub Check: Build
- GitHub Check: Continuous Release
- GitHub Check: ESLint
🔇 Additional comments (6)
packages/components/number-input/__tests__/number-input.test.tsx (1)
815-822: Timer mocking correctly implemented.The fake timer setup and teardown have been properly added to support the cursor restoration tests that call
jest.runAllTimers(). This addresses the previous review feedback.packages/components/number-input/src/use-real-time-formatting.ts (5)
27-45: Well-structured cursor restoration helper.The extraction of this helper function addresses the previous review feedback about code duplication. The logic correctly counts digits to restore cursor position after formatting.
47-83: LGTM - well-structured input handler.The handler correctly:
- Skips formatting during composition
- Validates partial numbers before parsing
- Checks state constraints before applying formatted values
- Preserves cursor position
- Emits onChange events
93-134: LGTM - paste handler correctly implemented.The paste handler properly:
- Prevents default behavior to maintain control
- Validates the resulting value before applying
- Counts digits accurately for cursor restoration
- Updates both input and numeric state
136-183: LGTM - cut handler with appropriate NaN handling.The cut handler correctly:
- Manages clipboard data manually for control
- Handles the edge case where cutting results in NaN by clearing the input
- Validates and formats the remaining value
- Restores cursor position appropriately
185-237: LGTM - beforeInput handler with proper validation.The beforeInput handler effectively:
- Blocks invalid input via
preventDefaultbefore it reaches the DOM- Validates the resulting value would be parseable
- Formats immediately for real-time feedback
- Correctly calculates cursor position including the new input
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/number-input/src/use-real-time-formatting.ts (1)
91-132: AddshouldFormatguard for consistency withhandleInput.
handleInput(line 47) returns early when!shouldFormat, buthandlePaste,handleCut, andhandleBeforeInputlack this guard. If these handlers are ever attached when formatting is disabled, they would still execute formatting logic.For defensive consistency:
const handlePaste = useCallback( (e: React.ClipboardEvent<HTMLInputElement>) => { + if (!shouldFormat) return; e.preventDefault(); const clipboardData = e.clipboardData.getData("text/plain");Apply the same pattern to
handleCut(line 134) andhandleBeforeInput(line 183).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/number-input/__tests__/number-input.test.tsx(1 hunks)packages/components/number-input/src/use-real-time-formatting.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T21:48:35.308Z
Learnt from: adbjo
Repo: heroui-inc/heroui PR: 5846
File: packages/components/tabs/src/tabs.tsx:76-101
Timestamp: 2025-10-27T21:48:35.308Z
Learning: In packages/components/tabs/src/tabs.tsx, the updateCursorPosition useCallback dependency array intentionally includes `cursorRef.current` to handle the case where the cursor span element is unmounted and remounted (e.g., when `disableAnimation` or `disableCursorAnimation` toggles). This ensures the callback is recreated when the ref points to a new element, triggering a dependency chain that re-establishes the ResizeObserver and initializes the new cursor element with the data-initialized attribute.
</learning]
Applied to files:
packages/components/number-input/src/use-real-time-formatting.tspackages/components/number-input/__tests__/number-input.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Tests
- GitHub Check: Continuous Release
- GitHub Check: ESLint
- GitHub Check: TypeScript
- GitHub Check: Build
- GitHub Check: Prettier
🔇 Additional comments (7)
packages/components/number-input/__tests__/number-input.test.tsx (3)
811-821: Timer mocking no longer required with current approach.The cursor restoration tests now use real timers with
await new Promise((resolve) => setTimeout(resolve, 0))wrapped inact()instead ofjest.runAllTimers(). This approach works correctly since it allows the production code'ssetTimeout(0)to execute before assertions.This is a valid alternative to the fake timer approach mentioned in the past review.
737-749: LGTM!The core real-time formatting test correctly validates that typing "1234" results in "1,234" when
isRealTimeFormatis enabled with grouping.
770-781: VerifybeforeInputevent behavior in JSDOM.This test assumes
userEvent.typefiresbeforeInputevents that can be prevented. JSDOM's support forbeforeInputevents may vary. If this test passes in CI, it confirms the implementation works correctly.packages/components/number-input/src/use-real-time-formatting.ts (4)
25-43: Cursor restoration helper correctly extracted.The
restoreCursorPositionhelper addresses the previous review feedback about duplicated cursor restoration logic. The implementation correctly counts digits and positions the cursor after formatting.
183-214:handleBeforeInputcorrectly intercepts and formats valid input.The design of always calling
e.preventDefault()at line 210 for valid input is intentional and correct. This allows the hook to take full control of formatting by:
- Preventing default input behavior
- Manually formatting and setting the value
- Restoring cursor position
This approach ensures consistent formatting without race conditions between default behavior and state updates.
155-160: Good edge case handling for cutting all content.Correctly clears the input and sets
numberValuetoNaNwhen the cut operation would result in an invalid/empty number.
73-78: Synthetic event is a minimal mock.The synthetic
ChangeEventonly includestarget.valueandcurrentTarget.value. This is sufficient for the common use case of accessing the formatted value, but consumers relying on other event properties (e.g.,nativeEvent,bubbles) would receiveundefined. This is an acceptable trade-off for this feature scope.
✅ Actions performedFull review triggered. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.