[number field] Respect rounding mode on blur#4804
Conversation
commit: |
Bundle size
PerformanceTotal duration: 1,151.52 ms -20.95 ms(-1.8%) | Renders: 50 (+0) | Paint: 1,766.02 ms -39.04 ms(-2.2%)
8 tests within noise — details Check out the code infra dashboard for more information about this PR. |
bc5b17c to
11213c4
Compare
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
mj12albert
left a comment
There was a problem hiding this comment.
Codex finding:
style: 'percent'still rounds the raw fraction, not the displayed percent.
Invalidate.ts:37, the new Intl path builds a plain decimal formatter. But percent inputs are parsed as fractions inparse.ts:211, and blur commits viaNumberFieldInput.tsx:191. Forformat={{ style: 'percent', maximumFractionDigits: 2, roundingMode: 'floor' }},1.239%parses to0.01239,commits as0.01,and displays as1%; Intl would display the parsed value as1.23%. Consider scaling percent values before applying the helper’s fraction rounding.
bfa245c to
b798d64
Compare
b798d64 to
ca1af48
Compare
flaviendelangle
left a comment
There was a problem hiding this comment.
PR Review Summary — mui/base-ui #4804
[number field] Respect rounding mode on blur — 4 files, +191/-21, fixes #4803
The PR correctly fixes the core bug (toFixed ignoring Intl roundingMode) and the percent-scale issue. Tests are well-structured. However, the refactor introduces two critical silent failures and the type-design / behavior of getMaximumFractionDigits has real correctness concerns.
Critical Issues (must fix before merge)
-
NaN leak via
Number(Intl.NumberFormat.format(Infinity))—packages/react/src/number-field/utils/validate.ts:49
Number.isFinite(value)guards the input, butvalueToRound = value * 100can overflow toInfinity; en-US formats that as"∞",Number(...)returnsNaN, whichonValueChangesilently emits. The pre-PR code returned a finite-or-Infinity number — this PR turns that into a NaN leak. -
Uncaught
RangeErroron invalidroundingMode/roundingIncrement—packages/react/src/number-field/utils/validate.ts:40-49
A user passing e.g.roundingIncrement: 3(not in Intl's allowed set) throws on every blur. The oldtoFixedpath silently ignored those fields. No try/catch, noBase UI Plus:warning per AGENTS.md guidelines. -
getMaximumFractionDigitsreturns wrong digit count when onlyminimumFractionDigitsis set —packages/react/src/number-field/utils/validate.ts:13-22
{ minimumFractionDigits: 2 }resolves to max=3 (Intl picksmax(min, 3)), so user typing1.2399rounds to1.240, not1.24. Worse: pre-PR code skipped rounding entirely in this branch (it requiredtypeof maxFrac === 'number'), so the PR introduces a behavior change beyond what the description claims. The existing percent min-only test only passes becausefloor+ that input gives the same result at 2 or 3 digits.
Important Issues (should fix)
-
getMaximumFractionDigitsconditional is misleading —packages/react/src/number-field/utils/validate.ts:14-19
Theformat?.minimumFractionDigits == null ? undefined : formatbranch partially fixes currency/percent defaults but only for one input shape. Either always passformator always passundefined— the middle ground silently introduces inconsistency. -
roundingMode: stringandroundingIncrement: numberdiscard standard-library invariants —packages/react/src/number-field/utils/validate.ts:6-9
TS 6.0.3'slib.es2023.intl.d.tsalready encodes precise literal unions (9 mode values, 15 increments). The local type alias is strictly weaker than what the standard provides. The real fix: bumptsconfig.base.jsonlibfrom["es2022", "dom"]to["es2023", "dom"]and delete the alias entirely. Node 22+ and all evergreen browsers support these at runtime. -
Rounding-mode test coverage only exercises
'floor'
At minimum add'halfEven'(1.235 → 1.24,1.245 → 1.24, the canonical proof the path goes throughIntl.NumberFormat) and a negative-value'trunc'/'floor'divergence test (e.g.-1.239). Without these, a future regression totoFixedsemantics could pass every new test. -
roundingIncrementblur integration test asserts display only, not committed value —packages/react/src/number-field/input/NumberFieldInput.test.tsx:643-667
Add one assertion ononValueChange.mock.lastCall?.[0]. -
style: 'currency'blur path is untested
Currency does not get the*100scale; one blur test would lock that contract down.
Suggestions
-
Comment wording invites rot —
packages/react/src/number-field/utils/validate.ts:6
"configured Intl types … yet" obscures thattsconfig.base.jsonlib: ["es2022"]is the real knob. Either bumplib(recommended) or reword to name the gating setting so it's clear when to delete the alias. -
Missing comments on the two trickiest lines
isPercentWithExplicitPrecision+ scale=100 trick, and themin-only ? undefined : formatconditional both deserve a one-line WHY. -
Untested edge cases
Negative numbers, NaN early-return, currency-style, and step+roundingMode interaction. -
Redundant tests
packages/react/src/number-field/utils/validate.test.ts:37-51packs two cases in oneit(); the percent min-only path is covered twice (unit and integration).
Strengths
- Correct percent-scale fix;
style:'unit', unit:'percent'correctly excluded from*100scaling and tested. useGrouping: false+'en-US'formatter guarantees safeNumber(...)parse (no scientific notation instandardnotation).renderControlledNumberFieldhelper andit.eachmatrix for fr-FR / ar-EG locales are high-signal, DAMP-readable tests.- Commit messages follow
[number field] …scope convention. - Import path
NumberFieldInput.tsx → ../utils/validateis appropriate; no layering concern.
Recommended Action
- Fix the critical issues first — guard the
Number(formatter.format(...))result withNumber.isFinite, wrap formatter construction in try/catch with aBase UI Plus:dev warning, and decide themin-only → resolved-maxsemantics deliberately (probably: useformat.minimumFractionDigitsdirectly). - Bump tsconfig
libtoes2023and delete the type alias. - Add
halfEvenand negative-value rounding tests, plus anonValueChangeassertion for theroundingIncrementblur test. - Reword or remove the misleading "configured Intl types" comment.
- Re-run
/pr-review-toolkit:review-prafter changes.
Some points are invalid, addressed the valid ones:
|
flaviendelangle
left a comment
There was a problem hiding this comment.
PR #4804 Re-Review — Second Pass
Repo: mui/base-ui
PR: #4804 — [number field] Respect rounding mode on blur
HEAD: db7a23065 ([number field] Harden rounding edge cases)
Fixes: #4803
The author addressed most prior findings well, but the overflow "fix" introduces a new high-severity divergence between onValueChange and onValueCommitted. I'm also retracting one of my previous critical findings — the author's rebuttal was correct.
What's resolved (acknowledged)
- Overflow → NaN leak (prior C1): the
Number.isFinite(valueToRound)guard atvalidate.ts:36-38does what it says. - RangeError rebuttal (prior C2) — sound:
parse.ts:74,NumberFieldRoot.tsx:147,269,314,699already callgetFormatterwith the user'sformaton render/keystroke. An invalidroundingIncrement: 3crashes on mount, not on blur. No new surface added. - Test gaps:
halfEven(both directions), negative-value mode divergence, currency blur, andonValueChangeassertion forroundingIncrementare all properly closed. - Comment: factually accurate now (
tsconfig.base.json:5confirmslib: ["es2022", "dom"]).
Retracted from prior review
- Min-only-precision (prior C3) — withdraw. With
{minimumFractionDigits: 2},getMaximumFractionDigitsresolvesdigits = 3(Intl decimal default), but Intl display also usesmax=3for the same options. Committed numeric value matches displayed text. Author was correct.
NEW high-severity finding
Infinity leaks past the overflow guard into onValueCommitted and the visible input — NumberFieldInput.tsx:185-218
On blur with style: 'percent' + a finite-but-huge value (Number.MAX_VALUE), removeFloatingPointErrors now returns Infinity. Downstream:
setValue(Infinity, …)→toValidatedNumber→clamp(Infinity, MIN_SAFE_INTEGER, MAX_SAFE_INTEGER)=MAX_SAFE_INTEGER→onValueChangefires withMAX_SAFE_INTEGER.onValueCommitted(committed, …)is NOT re-validated (forwarded raw atNumberFieldRoot.tsx:129-134) → fires withInfinity. Two listeners observe different values.formatNumber(Infinity, locale, {style:'percent'})returns"∞%". The input visibly displays"∞%"while the form state holdsMAX_SAFE_INTEGER.parseNumber("∞%", …)returnsnull→ next blur is a no-op. User is stuck.
The previous overflow fix moved the silent failure one level up. Recommendation: in the overflow branch at validate.ts:36-38, return a sentinel (null or the pre-blur value) that the blur path treats as "don't commit", rather than letting Infinity propagate.
Medium
- Subnormal underflow to 0 —
validate.ts:41.(Number.MIN_VALUE * 100).toFixed(2) === "0.00"→ a non-zero input silently becomes 0. Same class as the original NaN bug; exotic in practice. - Currency +
minimumFractionDigitsis now an undocumented behavior shift. Pre-PR,getFormatter('en-US')resolved max=3 (decimal default). Post-PR, the conditional atvalidate.ts:17forwards the format, so currency resolves max=2. More correct — but a1.239input with{style:'currency', currency:'USD', minimumFractionDigits:1}now rounds to1.24instead of1.239. Worth a one-line changelog note.
Test gaps (low–medium)
step×roundingModeinteraction untested (validate.ts:115,125,128).clamp×roundingModeinteraction untested.- Negative-value integration test (keyboard type
-1.239, assertonValueChange) untested. - No
ceil/halfExpandcoverage despite the PR title claiming general rounding-mode support.
Comments (nice-to-have)
validate.ts:6: tie alias lifetime to a deletion trigger — "Delete oncetsconfig.base.jsonlibincludes es2023."validate.ts:30-34: theisPercentWithExplicitPrecision+ scale=100 trick needs a one-liner pointing atparse.ts:211-215(percent stored as fraction; rounding must happen at display scale).validate.ts:17: the conditionalformat?.minimumFractionDigits == null ? undefined : formatreads like a typo; a one-line WHY (currency/percent default-max would otherwise clamp precision unexpectedly) would prevent a "fix" that breaks it.
Recommended action
- Address the
Infinityleak in the blur path (high). The cleanest fix is at the helper: don't returnInfinity— propagate a non-commit signal. - Add a step + roundingMode test to lock the composition.
- Add the three one-line comments at
validate.ts:6, :17, :30. - Note the currency-default behavior change in the changelog.
- Items 2–4 can ride; item 1 is a real bug.
flaviendelangle
left a comment
There was a problem hiding this comment.
PR Review Summary — #4804 [number field] Respect rounding mode on blur
Author: atomiks • Base: master → Head: codex/number-field-rounding-mode • Diff: +448 / −26 across 6 files • Fixes: #4803
The PR replaces toFixed-based blur rounding with Intl.NumberFormat-driven rounding that honors roundingMode, roundingIncrement, roundingPriority, and significant-digit options, plus a new percent display-scale rounding step. Tests, typecheck, and lint pass.
Critical Issues
None blocking. All five agents converged on this — no correctness, security, or merge-blocker concerns.
Important Issues
-
[silent-failure] No finite-guard on
committedbeforesetValue/onValueCommitted—packages/react/src/number-field/input/NumberFieldInput.tsx:187-208. On master,Number(parsedValue.toFixed(...))could only produce finite values. After the PR,removeFloatingPointErrorsreaches intoIntl.NumberFormatand the call paths broaden — but the blur handler still treats the result as definitely-finite. Defense-in-depth:if (!Number.isFinite(committed)) committed = parsedValue;immediately after the helper call. -
[silent-failure] Runtime feature gap for
roundingMode/roundingIncrementsilently no-ops —packages/react/src/number-field/utils/validate.ts:62-76. Older Safari/Node silently drop these option keys instead of throwing, so a user on Safari 15.3 sees rounding silently not applied while Chrome works. Consider one-time feature-detection at module load and a dev-only Base UI warning. -
[type-design]
roundingMode: stringandroundingPriority: stringthrow away TC39 unions —packages/react/src/number-field/utils/validate.ts:7-12. The standard ES2023 lib types these as"ceil" | "floor" | "expand" | "trunc" | "halfCeil" | ...etc. Modeling the real unions costs ~3 lines and makes the shim a drop-in replacement when the lib bump lands. Today, the local type is strictly weaker than the eventual stdlib version. -
[comments] Rot-risk: "Delete this once tsconfig.base.json includes es2023" —
packages/react/src/number-field/utils/validate.ts:6-7. Action-item-as-comment that nothing forces to be deleted on a future lib bump. Keep the WHY (Local augmentation for ES2023 Intl rounding fields; the repo's tsconfig lib is "es2022"), drop the action sentence (or put it in a tracking issue). -
[tests] No end-to-end test for increment/decrement +
roundingMode—removeFloatingPointErrorsis now also called fromtoValidatedNumber, which is hit by every Arrow/scrub increment. Only a single direct unit test atpackages/react/src/number-field/utils/validate.test.ts:327-339covers this path. A regression that bypassedremoveFloatingPointErrorsinside the step path would slip through. -
[tests]
parseNumberInfinity change is thinly covered — only one new case (packages/react/src/number-field/utils/parse.test.ts:111-119). Add a permille overflow case to harden the!Number.isFiniteswap; the pre-existing test at lines 105-109 would have passed on master too.
Suggestions
-
[type-design] Move the type into
formatNumber.tsand updategetFormatter'soptionsparameter — eliminates theas NumberFormatOptionsWithRoundingcast atpackages/react/src/number-field/utils/validate.ts:74, and centralizes the shim so the eventuallib: es2023deletion is one diff. Even better: bumptsconfig.base.jsonlibto["es2023", "dom"]now — it would also let users actually typeroundingModeon the publicformatprop atpackages/react/src/number-field/root/NumberFieldRoot.tsx:596(currently they can pass it, but TS errors at their call site). -
[code-review] Magic
3default formaximumFractionDigits—packages/react/src/number-field/utils/validate.ts:32. Matches en-US Intl default but loses style-sensitive defaults (e.g. JPY currency = 0). Pre-existing on master, so not a regression — call out as follow-up. Either name the constant or derive it fromgetFormatter('en-US', format).resolvedOptions()unconditionally. -
[silent-failure] Extract the percent branch explicitly —
packages/react/src/number-field/utils/validate.ts:44-47. The inner!Number.isFinite(valueToRound)check is only reachable whenscale === 100(the outer check at line 26 handles everything else). Pulling the percent path into its own branch makes the recovery path obvious and removes the need for the explanatory comment. -
[comments] Update the stale "round the committed numeric value" comment —
packages/react/src/number-field/input/NumberFieldInput.tsx:185. The helper now handles significant-digits, rounding increments, modes, and percent scaling — not just precision. Drop, or rewrite to capture the non-obvious WHY (why gated byhasExplicitPrecision). -
[comments] Tighten "preserve the old finite value" —
packages/react/src/number-field/utils/validate.ts:45. There's no "new value yet" to contrast with "old"; "fall back to the unscaled value" reads cleaner. -
[tests] Missing direct unit test for
hasExplicitNumberFormatPrecision, especiallymaximumSignificantDigits/minimumSignificantDigits-only cases. Indirect coverage exists, but the predicate's contract is worth pinning. -
[tests] Locale-robust display assertions —
expect(input).toHaveValue('1.23%')and'0.46%'atpackages/react/src/number-field/input/NumberFieldInput.test.tsx:715-744are hardcoded. Other tests in the file construct expectations vianew Intl.NumberFormat(...)— consistency would protect against CLDR drift across Node/Chromium versions. -
[tests] Add NaN/Infinity input tests for
removeFloatingPointErrors— one-liners covering the early-return guard at line 24-26.
Strengths
- The fix is correct and well-scoped. The blur path was using
toFixed, which ignored every Intl v3 rounding option. The Intl-routed path is gated only when an option that actually needs it is present, preserving the cheaptoFixedfor the simple case. - The
0.46%boundary handling is the standout. Thevalue * 100pre-scrub attoFixed(digits + 6)(packages/react/src/number-field/utils/validate.ts:50-52) is exactly the right fix for binary noise breaking directional rounding modes, parameterized acrossfloor | ceil | trunc | expandatpackages/react/src/number-field/utils/validate.test.ts:81-92. - Two best comments (
validate.ts:50-51andvalidate.ts:65) are model examples of "explain WHY" — one names the underlying mechanic and gives a concrete example, the other justifies an absence (omittingstyle/unit/notation). - Splitting
hasExplicitNumberFormatPrecisionis the right call — reused at both the call site (gate for whether to round at all) and inside the helper (gate for percent scaling), co-located so they can't drift. - No CLAUDE.md / AGENTS.md violations: no
as any, no new hooks (souseTimeout/useStableCallback/useIsoLayoutEffectN/A), no DOM traversal, no thrown public errors. - Tests use real
Intl.NumberFormatend-to-end — exactly right because the entire fix is about delegating to platform rounding semantics.unit/percentvspercent,onValueChangevsonValueCommitted, and locale variants (en-US/fr-FR/ar-EG) are all distinguished. - Overflow safety at
validate.ts:44-47andparse.ts:217(Infinity now returns null) is tested atvalidate.test.ts:169-176and the percent-MAX_VALUE case inparse.test.ts.
Recommended Action
-
Address before merge (low-effort hardening):
- Add the
Number.isFinite(committed)defense-in-depth guard inNumberFieldInput.tsx. - Tighten the
roundingMode/roundingPrioritytypes to their TC39 unions. - Rewrite the rot-risk comment at
validate.ts:6-7(drop the "Delete this once..." sentence). - Update the stale comment at
NumberFieldInput.tsx:185.
- Add the
-
Consider before merge (broader, but high value):
- Bump
tsconfig.base.jsonlibto["es2023", "dom"]and deleteNumberFormatOptionsWithRoundingentirely. This also unlocks the rounding fields on the publicformatprop for library consumers — without it, this PR's behavior is only triggerable via type-unsafe object literals.
- Bump
-
Follow-up (separate PR or changelog note):
- Add increment/decrement +
roundingModeend-to-end test. - Add direct unit tests for
hasExplicitNumberFormatPrecisionand NaN/Infinity inputs toremoveFloatingPointErrors. - Mention the
parseNumberInfinity behavior change (overflow →null) in the changelog for downstream consumers.
- Add increment/decrement +
Fixes #4803
Keep the committed value in sync with Intl rounding options when a number field rounds on blur.
Root cause
The blur commit path used
toFixed, so it rounded with the default mode even when the display formatter used options likeroundingMode: floor. Percent values also need to round at their displayed scale before converting back to the stored fraction.Changes
roundingModeorroundingIncrementis present, preserving the existingtoFixedpath otherwise.