[MI-1311] Fix currency input issue (rounding, clipping, ...) - #12
Merged
lcmohsen merged 10 commits intoJul 29, 2026
Merged
Conversation
…ng input - Introduced a draft display value mechanism in ControlledCurrencyInput to maintain intermediate decimal inputs (e.g., "19.") while typing. - Updated the input handling to ensure valid decimal values are not truncated or blocked during editing. - Added new stories to demonstrate and test the preservation of decimal values in various scenarios, including typing and editing existing values. - Implemented normalization of currency input to improve user experience and input accuracy.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesDecimal currency input preservation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ControlledCurrencyInputField
participant FormController
participant CurrencyFieldShell
User->>ControlledCurrencyInputField: enter decimal text
ControlledCurrencyInputField->>ControlledCurrencyInputField: normalize and preserve draft
ControlledCurrencyInputField->>FormController: store number or exact string
ControlledCurrencyInputField->>CurrencyFieldShell: render draft or grouped value
User->>ControlledCurrencyInputField: blur input
ControlledCurrencyInputField->>FormController: reconcile display value
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
📝 Storybook Preview: View Storybook This preview will be updated automatically when you push new changes to this PR.
|
![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-1311 — accept, store, and preserve decimal currency values during entry and editing without truncation, unexpected rounding, or blocked valid input; maintain compatibility for shared-library consumers.
- Related: Consulted
medusa-forms-patterns,medusa-stories-patterns,react-typescript-patterns, andstorybook-testing. No relevant meeting-note results were found. - DevAgent plan: N/A
- Scope: ✅ Matches the ticket. The change is limited to the controlled currency input, focused behavioral stories, and a patch changeset.
- Checks: ✅ GitHub build, test, and deploy checks passed at
3564f44; CodeRabbit was still pending.
🔍 Gap Analysis
- ✅ AC1 is covered:
valueAsNumberandsetValueAsretain a numeric form value while the focused input preserves decimal text such as19.and19.99. - ✅ AC2 is covered in the focused editing path, including replacing an existing decimal value.
⚠️ Every new decimal story stops while the input is focused. Add atab()/blur assertion so the new field-to-display reconciliation path and final post-edit value are exercised too.- No scope creep, missing plan items, or public contract changes were found.
🧪 Code Quality
- The focused display draft cleanly separates transient text from the transformed React Hook Form value, which fixes the reported coercion bug without changing the component API.
- Consumer
onFocus,onBlur, andonChangecallbacks are preserved alongside the React Hook Form handlers. - No TypeScript, security, migration, or material performance issues were found.
- The blurred synchronization is unnecessarily effect-driven; see the simplicity finding below.
🔧 Simplicity Audit
- [VIOLATION]
ControlledCurrencyInputFieldmirrorsfield.valueintodraftthroughuseEffectwhenever the input is blurred. This is derived state, makes resets/blur display updates one render late, and reruns when an inlinerulesobject changes identity. RenderisFocused ? draft : toDisplayValue(field.value, rules, hasTransform)and seeddrafton focus instead. That keeps the justified focused draft without duplicating the blurred form state. -1.0
🌟 Highlights
- The implementation targets the actual failure mode: numeric coercion can no longer erase an intermediate trailing decimal.
- Story coverage includes
valueAsNumber, nullable-numbersetValueAs, direct decimal entry, and editing an existing value. - The patch changeset clearly communicates the consumer-visible behavior change.
📊 Merge Confidence: 8.7/10
- ✅ Ticket behavior is implemented, the public API remains stable, and build/test/deploy checks are green.
⚠️ +1.0: remove the effect-based blurred-state mirror and derive the blurred display directly from the field value.⚠️ +0.3: cover blur/post-edit reconciliation in a Storybook interaction test.- 🔧 Simplicity deductions applied: -1.0 total
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/docs/src/medusa-forms/ControlledCurrencyInput.stories.tsx (1)
144-145: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRegex neither anchors nor generalizes the "trailing decimal" check it's named for.
TRAILING_DECIMAL_DISPLAYmatches"19."anywhere in the string (no$anchor), so it doesn't actually assert the decimal point is trailing, and the literal"19"is baked into a generically-named constant — reusing this name for a different value in a future story would silently test the wrong thing.♻️ Suggested generalization
-const TRAILING_DECIMAL_DISPLAY = /19\./; +const TRAILING_DECIMAL_DISPLAY = /^\d+\.$/;🤖 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 `@apps/docs/src/medusa-forms/ControlledCurrencyInput.stories.tsx` around lines 144 - 145, Update TRAILING_DECIMAL_DISPLAY to match any numeric value ending with a decimal point, anchoring the pattern at the end of the string and removing the hard-coded “19” literal. Keep the existing trailing-decimal validation behavior while making the constant reusable for other values.
🤖 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/medusa-forms/src/controlled/ControlledCurrencyInput.tsx`:
- Around line 14-21: Update normalizeCurrencyInputValue to remove invalid extra
characters without truncating the remainder of the input: allow only a leading
minus, retain at most one decimal point, and preserve all valid digits after
stray mid-string signs or decimals. Replace the prefix-only NUMERIC_VALUE_REGEX
approach while keeping the existing normalization behavior for valid currency
inputs.
---
Nitpick comments:
In `@apps/docs/src/medusa-forms/ControlledCurrencyInput.stories.tsx`:
- Around line 144-145: Update TRAILING_DECIMAL_DISPLAY to match any numeric
value ending with a decimal point, anchoring the pattern at the end of the
string and removing the hard-coded “19” literal. Keep the existing
trailing-decimal validation behavior while making the constant reusable for
other values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bae62ad-0ef2-4630-8e94-c659b2365fcb
📒 Files selected for processing (3)
.changeset/decimal-currency-input.mdapps/docs/src/medusa-forms/ControlledCurrencyInput.stories.tsxpackages/medusa-forms/src/controlled/ControlledCurrencyInput.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Updated regex for trailing decimal display to ensure proper validation of decimal inputs. - Enhanced test scenarios in stories to validate the preservation of decimal values during user interactions. - Adjusted input handling to maintain intermediate values while typing, ensuring a smoother user experience.
…alues' of https://github.com/lambda-curry/medusa-forms into MI-1311-enhance-medusa-forms-input-to-support-decimal-values
Contributor
Author
|
@currybot-lc re-review please |
There was a problem hiding this comment.
🍛 Currybot Review
📋 Context
- Ticket: MI-1311 — accept, store, and preserve decimal currency values during entry and editing without truncation, unexpected rounding, or blocked valid input; maintain compatibility for shared-library consumers.
- Related: Re-checked
medusa-forms-patterns,medusa-stories-patterns,react-typescript-patterns, andstorybook-testing. - DevAgent plan: N/A
- Scope: ✅ Matches the ticket. This re-review covers the three commits since Currybot's review at
3564f44; the changes remain limited to the currency-input fix, focused behavioral stories, and the unchanged patch changeset. - Checks: ✅ Build, test, deploy, and CodeRabbit are green at
1336371.
🔍 Gap Analysis
- ✅ Resolved — blurred-state mirroring:
useEffectwas removed. The focused draft now preserves intermediate text, while the blurred display derives directly from the React Hook Form field value; focus seeds the draft for the next edit. - ✅ Resolved — blur/post-edit coverage: all four new decimal interaction stories now tab away and assert the reconciled display and form value. The trailing-decimal story explicitly verifies
19.reconciles to19on blur before completing a19.99entry. - ✅ AC1 and AC2 remain covered for
valueAsNumber, nullable-numbersetValueAs, direct decimal entry, and editing an existing decimal value. - No unmet ticket requirements, scope creep, plan gaps, or public contract changes were found.
🧪 Code Quality
- The normalization update preserves one leading minus and one decimal separator while stripping currency display characters and stray punctuation.
- Consumer
onFocus,onBlur, andonChangecallbacks remain composed with the React Hook Form handlers. - The added interactions follow the repository's click-before-clear and real keyboard-navigation patterns.
- No new correctness, TypeScript, security, performance, or test-coverage concerns were found.
🔧 Simplicity Audit
- ✅ [RESOLVED] The prior effect-derived-state violation is gone. Blurred display state is derived directly, and the remaining focused draft has a clear, documented reason to exist.
- No new
[VIOLATION]or[MISSING JUSTIFICATION]findings. - 🔧 Simplicity deductions applied: 0.0 total
🌟 Highlights
- The fix now has explicit regression coverage across both transient typing and blur reconciliation.
- Removing the synchronization effect makes resets/default updates immediate while avoiding dependency-identity churn.
- The component API and consumer callback behavior remain backward-compatible.
📊 Merge Confidence: 9.8/10
- ✅ Both prior findings are resolved, ticket behavior is covered, the implementation is simpler, and all relevant checks are green.
⚠️ +0.2: downstream Medusa Admin preview/soak validation would provide the final consumer-level signal; no additional code change is requested.- 🔧 Simplicity deductions applied: 0.0 total
There was a problem hiding this comment.
🍛 Currybot Review
📋 Context
- Ticket: MI-1311 — accept, store, and preserve decimal currency values during entry and editing without truncation, unexpected rounding, or blocked valid input; maintain compatibility for shared-library consumers.
- Related: Re-checked
medusa-forms-patterns,medusa-stories-patterns,react-typescript-patterns, andstorybook-testing. The current RAG lookup returned no additional snippets. - DevAgent plan: N/A
- Scope: ✅ Matches the ticket; no public contract or unrelated scope changes.
- Checks: ✅ Build, test, deploy, and CodeRabbit pass at
13363719; cleanup is intentionally skipped.
🔍 Gap Analysis
- ✅ Resolved — prior simplicity finding: the
useEffectmirror is gone. While focused, the component uses the justified text draft; while blurred, it derives display directly fromfield.value, so resets/defaults stay current without duplicated state. - ✅ Resolved — prior coverage finding: every new decimal interaction now crosses blur with
tab()and verifies the reconciled display/form value. The trailing-decimal story specifically checks"19." → 19on blur, then completes and re-checks19.99. - ✅ Ticket requirements remain covered. No new requirement, scope, or plan gaps were introduced since the last review.
🧪 Code Quality
- The new normalization keeps one decimal point and an optional leading minus without restoring the original truncation behavior.
- Focus/change/blur handlers preserve both React Hook Form behavior and consumer callbacks.
- No new TypeScript, security, migration, performance, or correctness findings since the last review.
🔧 Simplicity Audit
- ✅ The previous
[VIOLATION]is resolved: blurred state is derived during render and the focused draft is seeded at the lifecycle boundary where it is needed. - No new
[VIOLATION]or[MISSING JUSTIFICATION]findings.
🌟 Highlights
- The fix now has a clean ownership boundary: form state owns the blurred value; the local draft exists only for intermediate focused text.
- The interaction tests cover both stable decimals and the tricky trailing-decimal transition.
- The revision removed complexity while retaining the component API and all green CI signals.
📊 Merge Confidence: 9.7/10
- ✅ Both prior findings are resolved, ticket behavior remains intact, and all relevant checks pass.
⚠️ +0.3: focused table tests for malformed pasted strings (multiple separators/minus signs) would fully lock down the newly refined normalizer; this is additional edge-case assurance, not a merge blocker.- 🔧 Simplicity deductions applied: 0 total
Bfunk54
requested changes
Jul 29, 2026
Bfunk54
left a comment
Bfunk54
left a comment
There was a problem hiding this comment.
I tested this locally and there seems to be some oddity here with the cursor moving to the wrong position when I add decimals and delete parts of the number.
- Improved decimal input preservation during typing and editing, eliminating caret jumps and float rounding issues. - Implemented a precision-safe currency field that maintains high-magnitude values as strings to prevent IEEE-754 precision loss. - Added new stories to validate the behavior of the currency input under various scenarios, including handling multiple decimal points and large values. - Introduced helper functions for better currency formatting and validation without coercing through Number().
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/docs/src/medusa-forms/ControlledCurrencyInput.stories.tsx (1)
598-602: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLet
userEventestablish the selection.Remove the preceding
setSelectionRangecalls; each interaction already suppliesinitialSelectionStartandinitialSelectionEnd. This keeps the stories user-focused and avoids redundant low-level DOM manipulation.As per coding guidelines, “Simulate real user interactions with
userEventrather than low-level DOM manipulation.” <coding_guidelines>Also applies to: 665-670, 715-719
🤖 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 `@apps/docs/src/medusa-forms/ControlledCurrencyInput.stories.tsx` around lines 598 - 602, Remove the preceding input.setSelectionRange calls from the interaction examples in ControlledCurrencyInput stories, including the cases around the shown interaction and the referenced locations. Rely on each userEvent.type call’s initialSelectionStart and initialSelectionEnd options to establish the selection, preserving the existing typing behavior.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/medusa-forms/src/controlled/currencyPrecision.ts`:
- Around line 24-27: Update the precision-loss check in the currency precision
logic to compare the normalized decimal value against its float64 round-trip,
rather than treating every value with more than 15 significant digits as lossy.
Preserve exactly representable values such as 9007199254740991 as numbers, and
only return true when the normalized round-trip differs.
In `@packages/medusa-forms/src/controlled/valueTransforms.ts`:
- Around line 36-41: Update the setValueAs handling in valueTransforms to always
return its caller-defined result without applying the lossy numeric conversion
fallback. Move or retain that precision-preserving fallback exclusively within
the valueAsNumber path, leaving custom setValueAs numeric transforms unchanged.
In `@packages/medusa-forms/src/ui/CurrencyInput.tsx`:
- Around line 19-27: Remove onValueChange and disableGroupSeparators from the
base MedusaCurrencyInputProps API and CurrencyInput destructuring in
packages/medusa-forms/src/ui/CurrencyInput.tsx (lines 19-27 and 62-70), while
retaining them only for ControlledCurrencyInput. Update the corresponding
exported type definitions in packages/medusa-forms/src/ui/types.d.ts (lines
28-43) so the native CurrencyInput no longer promises unsupported behavior.
---
Nitpick comments:
In `@apps/docs/src/medusa-forms/ControlledCurrencyInput.stories.tsx`:
- Around line 598-602: Remove the preceding input.setSelectionRange calls from
the interaction examples in ControlledCurrencyInput stories, including the cases
around the shown interaction and the referenced locations. Rely on each
userEvent.type call’s initialSelectionStart and initialSelectionEnd options to
establish the selection, preserving the existing typing behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 547d95bb-f811-43d1-a45d-20afa125597e
📒 Files selected for processing (7)
.changeset/decimal-currency-input.mdapps/docs/src/medusa-forms/ControlledCurrencyInput.stories.tsxpackages/medusa-forms/src/controlled/ControlledCurrencyInput.tsxpackages/medusa-forms/src/controlled/currencyPrecision.tspackages/medusa-forms/src/controlled/valueTransforms.tspackages/medusa-forms/src/ui/CurrencyInput.tsxpackages/medusa-forms/src/ui/types.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/medusa-forms/src/controlled/ControlledCurrencyInput.tsx
- Introduced constants for leading and trailing zero regex patterns to enhance readability and maintainability. - Updated normalization logic to handle leading and trailing zeros more effectively, ensuring accurate representation of decimal values. - Adjusted round-trip validation to account for negative values, improving precision checks during input processing.
- Streamlined the value transformation process by directly returning the result of the setValueAs function, enhancing code clarity and maintainability. - Removed unnecessary checks for number conversion loss, focusing on the primary transformation logic.
…nitions - Updated normalization logic to improve currency input handling by replacing `replaceAll` with `split` and `join` for better performance. - Enhanced the `toDisplayValue` function to handle serialized values more robustly, ensuring proper formatting for arrays. - Introduced a new type definition for `CurrencyInputValueChangeValues` to improve type safety and clarity. - Added a new prop `disableGroupSeparators` to `ControlledCurrencyInputProps` for better control over formatting behavior. - Cleaned up unused code and improved input event handling for better performance and readability.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/medusa-forms/src/controlled/valueTransforms.ts (1)
51-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve empty values during serialization.
This now calls
String(value)for every non-NaNvalue, so anundefinedornullfield value can render as literal"undefined"or"null"instead of an empty input. Keep a nullish guard before stringification.Proposed fix
+ if (value == null) { + return ''; + } if (typeof value === 'number' && Number.isNaN(value)) { return ''; }🤖 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/medusa-forms/src/controlled/valueTransforms.ts` around lines 51 - 55, Update the value serialization logic in valueTransforms to return an empty string for nullish values before reaching String(value). Preserve the existing NaN handling and full-digit string behavior for all non-nullish values.
🤖 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.
Outside diff comments:
In `@packages/medusa-forms/src/controlled/valueTransforms.ts`:
- Around line 51-55: Update the value serialization logic in valueTransforms to
return an empty string for nullish values before reaching String(value).
Preserve the existing NaN handling and full-digit string behavior for all
non-nullish values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 081f4825-5abf-44ad-947c-0eda579c58c4
📒 Files selected for processing (3)
packages/medusa-forms/src/controlled/ControlledCurrencyInput.tsxpackages/medusa-forms/src/controlled/currencyPrecision.tspackages/medusa-forms/src/controlled/valueTransforms.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/medusa-forms/src/controlled/currencyPrecision.ts
- packages/medusa-forms/src/controlled/ControlledCurrencyInput.tsx
…validation - Added waitFor checks in stories to validate input behavior while focused, ensuring caret position and value representation are correct. - Improved user interaction scenarios by verifying state updates for both string and number types during input and tabbing events. - Enhanced test coverage for currency input formatting and state representation after user interactions.
…ut and CurrencyInput - Enhanced the normalization logic in ControlledCurrencyInput for better clarity and performance by adjusting string manipulation methods. - Simplified the props destructuring in CurrencyFieldShell for improved readability. - Cleaned up className definitions to streamline the component's styling logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
19.remain visible during editing.New Features