-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5510
jiunshinn · open, draft only — no public action · reviewed at f9a07aae004d1bd5384e036bb01fb5391e4a984d · view on GitHub
Versions: Review Loop 1.4.0 · Component Audit Rubric 1.12
Lane: full · Time: 36 minutes · Verdict: approve
A user on a decimal-bounded NumberInput can press ArrowUp, scroll the focused wheel, or use the stepper at value={99} / max={99.99} and see the field commit 100, outside its own maximum. The same rounded-after-clamp shape exists at fractional lower bounds, so the visible value and aria-valuenow can contradict the declared range.
Verdict: clear
(1 decision · ~7 runtime lines of 124 changed)
- Round the snapped step value before clamping, then use that clamped value for the integer check and return. This fixes #5506.
The stepping path still chooses the next step slot from the current value, step, and step base. It now rounds that snapped value first and applies min / max last, so a fractional bound can be returned even when the step precision is coarser.
Verdict: clear
Owner: NumberInput's stepping helper. Keyboard, wheel, and stepper routes all flow through stepValue → getSteppedValue; controlled rerender updates visible value, aria-valuenow, and stepper disabled state. The behavior unit remains one pure deterministic utility; no new system or tier is introduced.
Verdict: clear
Every app using NumberInput stepping with a fractional bound and a coarser step gets the fix. Screen-visible and assistive-technology values stay inside the declared range; ordinary integral ranges and decimal-step arithmetic remain unchanged. PowerSearch can pass consumer-supplied bounds through, so money-like max={99.99} cases are reachable even though the repo did not hard-code one.
Verdict: clear
No API change. Existing min?: number | null, max?: number | null, step?: number | null, isIntegerOnly?: boolean, keyboard stepping, wheel stepping, and number-stepper behavior keep their signatures and intended meaning.
The unchanged caller shape is the issue's example:
<NumberInput label="Price" max={99.99} value={99} onChange={setValue} />Its behavior changes from committing 100 to committing 99.99.
Verdict: clear
No new theme targets or styling change. No prop, target, variant, export, accepted value, or public vocabulary is added; this restores the existing range contract.
Verdict: clear
- API — no existing call site stops compiling; the same props now honor their stated range.
-
Visual — intentional value-only change in the bug state:
100→99.99, and0→4e-13. Geometry and styling are unchanged. - Theme — no target, token, variable, or override changes.
- Behavior — intentionally restores the declared range contract on keyboard, wheel, and stepper paths.
Verdict: clear
No new effects, state, subscriptions, memo dependencies, listeners, layout reads, observers, dependencies, or imports. The existing wheel listener is unchanged and remains focus-gated.
Verdict: clear
Manual frames were required because the displayed endpoint is the acceptance behavior and exact-head visual CI does not drive it. Sensor-matched 900×400 light/LTR captures show main at value=100, aria-valuenow=100; the PR shows value=99.99, aria-valuenow=99.99, with the increment button disabled and unchanged input geometry.
Explicit visual judgement: only the intended input text changes from 100 to 99.99; no layout, color, or state-treatment difference appears.
Chromium route matrix:
| route | main | PR head |
|---|---|---|
| max — keyboard |
100 / aria-valuenow=100
|
99.99 / aria-valuenow=99.99
|
| max — wheel |
100 / aria-valuenow=100
|
99.99 / aria-valuenow=99.99
|
| max — stepper |
100 / aria-valuenow=100
|
99.99 / aria-valuenow=99.99
|
| min — keyboard |
0 / aria-valuenow=0
|
4e-13 / aria-valuenow=4e-13
|
| min — wheel |
0 / aria-valuenow=0
|
4e-13 / aria-valuenow=4e-13
|
| min — stepper |
0 / aria-valuenow=0
|
4e-13 / aria-valuenow=4e-13
|
Verdict: clear
pr-a11y is green with no baseline change. Chromium verifies that the exposed spinbutton value follows the visible value at both fractional bounds; the regression also asserts visible value, aria-valuenow, valid state, and disabled stepper state. No user-visible or AT-facing string is added.
Verdict: clear
Low-risk: no new surface, behavior regression, performance regression, or growth found. Full lane was still correct because this changes core input behavior across keyboard, wheel, and stepper routes.
Verdict: clear
Approve. No negative finding and no human API or design decision. Real Chromium shows main committing 100 / 0 and this head committing 99.99 / 4e-13 across all three input routes.
origin/main advanced to 90bf6c9cd10c77e296cd061b9b3954e323ca61e6; a clean no-commit merge produced tree 403fd000119a0585e82f663c0fc3e35e0873730c, and the focused suite passed there too.
AUTHOR CAN PROCEED: yes. The review can be posted as an approval.
Thanks, this is good. The stepped paths now keep fractional min/max in range, including the visible value and stepper state.
[Reviewed by Robohands]
No inline comments.
- Exact head:
npx --yes pnpm@11.10.0 exec vitest run packages/core/src/NumberInput/NumberInput.test.tsx→ 1 file, 139/139 passed, 4.43s. - No-commit merge with current main: same command → 1 file, 150/150 passed, 50.30s.
- Browser build:
npx --yes pnpm@11.10.0 -F @astryxdesign/build build→ Vite JS and declarations built. - Storybook: port 6251, ready in 1.06s manager / 194ms preview; stopped after the probe.
- GitHub exact-head CI: build, test, lint, Storybook, sandbox,
pr-a11y,pr-visual, andpr-rtlgreen. Vercel deployment status was red but did not invalidate those checks.
Two critic passes. Gate 1 failed the visual-evidence receipt wording; gate 2 added the inspected pixel/text judgement and passed cleanly. No review finding changed.
Not posted. Proposed public text is the clean gate-2 draft; public actions were explicitly forbidden.