-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5152
Merged as 60cea48a97ad — squashed to main 25 Aug 2026, all 18 checks green.
Two loops: the first (gate-1, gate-2) returned approve-with-nits and
MERGE: hold; the author closed all three findings and rebased, and the second
loop (round2-gate-1, round2-gate-2) returned approve, no findings. The
remaining hold reason — min/max changing meaning for every caller with
nobody opted in — was the maintainer's own stated intent for the change, so it
was discharged rather than escalated.
One live defect was found and handed off rather than posted:
#5506, the stepper rounding
for precision after clamping, so ArrowUp in a max={99.99} field commits
100. Pre-existing, proved on both sides in Chromium.
Gates on the Mac: ~/astryx/review-artifacts/pr-5152/.
#5152 fix(NumberInput): commit an out-of-range entry at the nearest bound by cixzhang (bucket: the maintainer — also in .github/ENGOWNERS)
5972101402ea7871d64afa34f8014c16c6ff6490 <- every claim below was verified at this commit
Still no posted review and no review comments — pulls/5152/reviews and
pulls/5152/comments are both empty (R1e checked; nothing to extend or
contradict, and nothing gate-2 wrote ever reached the PR). The only prior round
is the loop's own private gate-2.md, reconciled below.
The head moved: gate-2 reviewed 0675b808, which the rebase rewrote to
7e4b7fe ("show the clamped value in the field after Enter"). The new work is
one commit on top, 5972101 — "keep the clamped bound inside the field's own
rules". git rev-list --count 5972101..origin/main is 1, and that one
commit is cb20a01 ci: build Core before inspecting downloaded themes (#5482),
which touches no file this PR touches (R31c: main has not moved under it).
Someone typing a page number past the last page lands on the first digit they
typed instead of the last page. Re-measured on main at this head's base,
core-numberinput--with-min-max (1–5): click, type 100, blur — the field
reads 1 (/tmp/pr5152r2-shots/before__blur-100.png, opened). Every
keystroke that parses in range commits, so 1 commits, 10 and 100 are
rejected outright, and the stale 1 survives the blur.
Unchanged from gate-2, and re-verified rather than carried. VERDICT: clear
1. an out-of-range entry commits at the nearest bound on blur and Enter,
instead of being rejected [the fix]
- parseNumberInput splits into shape rules (parseNumericInput) and
shape+range, so the commit path reuses the shape half [how, not a decision]
- Enter clears the pending text when the commit clamped [how, not a decision]
- NEW this round: the bound is rounded inwards under isIntegerOnly, and
an empty range reverts instead of committing [how, not a decision]
The count did not grow. The delta could not ship apart from the clamp: a clamp that emits a value the field's own rules reject is not a state we would ship, which is exactly why gate-2 raised it. One decision, tracing to the one stated problem.
Worth naming, because it is the part that was easy to get wrong: the delta takes
two different remedies from two different precedents in the same file, and
picks the right one in each place. Where rounding can save the entry it rounds,
the way stepping from empty does (NumberInput.tsx:597-600); where no rounding
can (min above max), it refuses, the way the stepper's own shape guard does
(:627). I read both and the comment's claim — "the same way stepping from
empty does" — is true, not decoration.
7d hypothesis, stated early: everything I find will be a child of the new
rounding will have a hole of its own, since it was written under review
pressure. It was refuted, and that is the more useful result: the one hole
I went looking for (the stepper clamping to a raw bound) is already guarded at
:627, and I killed the finding before writing it. What survived is a different
defect in the neighbouring function, and it is not this PR's — see IMPACT.
VERDICT: clear
Everyone on the next release using a NumberInput with min or max, and
every Pagination page box. You type 100 into a 1–5 field and it settles on 5 —
the number you asked for — instead of dropping back to 1. For the builder the
call site is byte-identical and the contract is not: onChange now fires with a
value the user did not type, where before it did not fire at all.
What the delta adds to that: nothing anyone experiences. getCommittableBounds
is the identity function unless isIntegerOnly is set and a bound is
fractional, and the empty-range revert needs min above max. Counted rather
than reasoned about — 24 <NumberInput> occurrences across 18 files outside
tests and stories, and the bounds they pass are 1/Math.ceil(...) (Pagination :728), 100, 50, 10, 99,
200, 41, or consumer-supplied. Zero can reach either branch. Pagination
cannot even in principle: it returns null at Pagination.tsx:458-463 when the
page count is ≤ 0, so min={1} max={0} never renders.
What landing this does NOT close, and it is still-broken rather than newly visible. Three places in this component decide what value may land in the field, and they do not agree:
| path | on max={99.99}, from 99 |
measured |
|---|---|---|
| typing + commit | refuses 100, settles at 99.99 | this PR made this right |
| the stepper | ArrowUp commits 100, aria-invalid unset |
unchanged, and wrong |
getSteppedValue clamps to [min, max] and then rounds to the precision of
step (NumberInput.tsx:611-626), so min(99.99, 100) → (99.99).toFixed(0)
→ 100. Driven in Chromium on both sides — identical on main and on this
head — so it is pre-existing and not this PR's (R13/R3i), and it is off the
posted comment for that reason. It is in JUDGEMENT because the reader should
know the family is only half fixed.
VERDICT: note — min/max change meaning for every existing caller; nobody opted in and no codemod can carry it.
No API change — no prop added, removed or retyped.
// the call site is byte-identical; the behaviour is not
<NumberInput label="Rating" min={1} max={5} value={v} onChange={setV} />
// type 100, blur: before → onChange never fires, field falls back to 1
// after → onChange(5), field shows 5| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
~ |
min/max meaning: reject-out-of-range → clamp-at-commit |
yes — every caller, no opt-in | — |
en+zh, and the TS JSDoc (doc.mjs:131 + :137 en, :415 + :420 zh; NumberInput.tsx:354,:359) |
ok |
+ |
parseNumericInput, parseNumberInputForCommit, getCommittableBounds
|
no — module-local, barrel untouched | — | n/a | ok |
gate-2's API finding is CLOSED. It said the docsite still described the old contract in both locales. The delta rewrites both: "Minimum value allowed. A smaller entry commits at this value on blur or Enter." and "允许的最小值。更小的输入会在失焦或按 Enter 时提交为该值。" — English and Chinese moved in the same commit, which is the failure mode that usually leaks.
The changeset (patch, credited @cixzhang) describes the clamp and not the
rounding, which is right: the rounding reaches nobody, and a changeset that
lists every private helper is noise to a consumer reading a changelog.
Ossification: nothing new freezes. All three helpers are module-local.
getCommittableBounds is a third private helper beside two that already
existed; it earns its name because it answers a question neither of the others
does ("what bounds may this field actually commit to").
R1j — is every change argued? Yes, quotably, and the delta was added to the
body in the same push: "The bound is put through the field's own shape rule
before it is committed, so an isIntegerOnly field handed a fractional bound
rounds inwards (max={9.5} commits 9, not 9.5), and a range no number can
satisfy (min above max) reverts rather than committing outside both ends."
No unrecorded change in the diff.
VERDICT: clear
n/a — structural only, verified on the whole diff's added lines:
grep -cE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(|stylex\.|xstyle" → 0
grep -cE "astryx-[a-z-]+" → 0
No new theme targets, none removed, no value pinned on xstyle.
VERDICT: clear
@astryxdesign/core is published and private is false, so there are
consumers who can be broken.
- API — no. No signature change; every existing call site compiles.
-
Visual — no at rest. Nothing about geometry, colour or DOM changes. The
before/after frames are pixel-identical apart from the digit in the field
(
before__blur-100.png/after__blur-100.png, both opened). Nothing grew, so R15d does not apply. - Theme — no.
-
Behaviour — yes, and it is the point. Driven in Chromium, three revisions
of
NumberInput.tsxswapped in one worktree against one browser session (probe-kit/numberinput-stepper-bounds.cjs,SB_PORT=6270):
| state | typed | main | prev head 7e4b7fe
|
this head |
|---|---|---|---|---|
[1,5], blur |
100 |
1 | 5 | 5 |
[1,5], Enter |
100 |
field 100 / value 1 |
5 | 5 |
isIntegerOnly max={9.5} |
20 |
2 | 9.5 | 9 |
isIntegerOnly min={0.5} |
0 |
1 | 0.5 | 1 |
isIntegerOnly [0.2,0.8] |
5 |
(empty) | 0.8 | (empty) |
isIntegerOnly max={9} (control) |
20 |
2 | 9 | 9 |
shape-invalid 4abc
|
— | 4 | 4 | 4 — unchanged |
| empty / loading / error / disabled / readonly | not reachable — the diff adds no state, no default, no prop |
R14f — driven past the bound, not through it, and past the rounded bound
too: rows 3–5 are the delta's own territory and every one of them is now inside
the field's rules. The [0.2,0.8] row is the sharpest — after rounding there is
no integer in the range at all, and the field correctly refuses rather than
inventing one.
gate-2's BREAKING note is CLOSED. It read "the clamp bypasses
isIntegerOnly; measured both sides, reaches nobody today, one line to close."
It is closed, and it took ~17 lines rather than one, because the honest fix had
to handle the range going empty as a consequence of the rounding.
The one behaviour that is not "no" and is not this diff's is the stepper overshoot in IMPACT; it reaches JUDGEMENT there (R16d). VERDICT: note — the fix changes what a bounded field commits, for every caller, with no opt-in.
Effects — zero. grep -cE "^\+.*use(Layout)?Effect" on the whole diff → 0.
None added, moved or deleted; no eslint-disable.
+ useCallback [isIntegerOnly, locale, max, min] NumberInput.tsx:745 parseInputForCommit
+ useCallback [isIntegerOnly, locale] NumberInput.tsx:751 parseNumeric
Both dep lists name exactly what the closure reads, one hop, no callback deps, no proxies (R18d). Resolved by reading, not assumed.
-
Render — no new state, no new subscription, nothing moved into render.
The delta adds one object allocation (
getCommittableBounds) per commit — per blur or Enter, not per keystroke. - Listeners / observers — none added.
-
Layout — no
getComputedStyle,offsetWidthorgetBoundingClientRectanywhere in the diff. - Bundle — three module-local functions, ~79 runtime lines. No dependency.
- Perf test — not owed. O(1) per commit, on blur/Enter, not on typing.
VERDICT: clear
Frames for whoever posts. They publish to assets/pr-5152 on
cixzhang/astryx — the fork, never facebook/astryx. All captured in real
Chromium. Eleven of the twelve were opened with the read tool; the twelfth
(before__typed-100.png) is settled by checksum instead, below.
- 100 into a 1–5 field, blurred before/after EMBED /tmp/pr5152r2-shots/before__blur-100.png · after__blur-100.png
- the same entry, still typing before/after /tmp/pr5152r2-shots/before__typed-100.png · after__typed-100.png
- the same entry, Enter before/after EMBED /tmp/pr5152r2-shots/before__enter-100.png · after__enter-100.png
- THE DELTA: isIntegerOnly max 9.5 prev-head/after EMBED /tmp/pr5152r2-shots/prevhead__intmax95.png · newhead__intmax95.png
- THE DELTA: isIntegerOnly [0.2,0.8] prev-head/after /tmp/pr5152r2-shots/prevhead__emptyrange.png · newhead__emptyrange.png
- pre-existing, NOT this PR: max 99.99, at 99 then ArrowUp /tmp/pr5152r2-shots/overshoot__at-99.png · overshoot__after-arrowup.png
"Typing is deliberately untouched" is not taken on trust — it is a checksum.
before__typed-100.png and after__typed-100.png are byte-identical
(sha256 636d1047452fddc7…), so nothing about the mid-typing state moved. The
same hash also covers before__enter-100.png, which is the finding underneath
it: on main, pressing Enter on 100 changes nothing on screen at all.
Every difference, labelled:
| difference | verdict | source |
|---|---|---|
the field settles on 5 instead of 1
|
intentional | the body: "the previous treatment (clamp to max) is more intuitive, and that's what people expect from a bounded field" |
on Enter, the invalid border treatment clears and the field reads 5
|
intentional | the body: "Enter left the field reading 100 on a value of 1, with aria-invalid still set"
|
9.5 → 9 in an integer-only field |
intentional | the body: "an isIntegerOnly field handed a fractional bound rounds inwards (max={9.5} commits 9, not 9.5)"
|
0.8 → empty on [0.2,0.8]
|
intentional | the body: "a range no number can satisfy … reverts rather than committing outside both ends" |
| mid-typing, nothing at all | intentional | the body: "Typing is deliberately untouched" — proved by the identical hash above |
100 in a field whose max is 99.99
|
unintentional, and pre-existing — byte-for-byte the same on main, so not a difference this PR makes |
gate-2's fractional-bound table was measured with isIntegerOnly OFF.
probe-kit/numberinput-clamp-bounds.cjs set it through the storybook args=
URL as isIntegerOnly:!true, and that arg never reaches this component:
typing 2.5 into a field configured that way still commits 2.5, on both
ports. Its rows labelled "int-only" were a plain fractional-max field, which
happens to return the same number, so the table read correct and proved nothing.
I only caught it because the new head returned 9.5 where the code plainly
says Math.floor — the result surprised me, so I suspected the harness first,
which is the rule that saved this run.
Every number above is instead taken from stories that set the prop in code
(RevIntMaxFrac, RevIntMinFrac, RevIntEmptyAfterRounding, RevIntControl,
added locally to apps/storybook/stories/NumberInput.stories.tsx in my
worktree, never committed). The trap is now written into the head of
probe-kit/numberinput-stepper-bounds.cjs so the next run does not repeat it.
gate-2's VISUAL note is UNRESOLVED, not closed. It flagged three shots CI
attributed to the old head that it could not reproduce. CI has not finished on
this head — six runs at the time of writing, CI still in_progress — so
pr-visual has said nothing yet. Carried to WHAT I COULD NOT VERIFY.
VERDICT: clear — the diff's own visual differences all have a source in the body.
§9 strings — n/a, verified. grep -cE "aria-|role=|useTranslator|t\('@astryx"
on the diff's added lines → 0; packages/core/locales/ → 0 lines
changed. The two doc strings the delta adds are .doc.mjs prop descriptions,
which are already authored per-locale (en and zh both updated) and are not
catalog keys.
§9 direction — n/a. No CSS, no logical or physical properties, nothing
rendered moves.
§1 automated — cannot be read yet. pr-a11y has not reported on this head.
.github/a11y-baseline.json is not in the diff, so nothing was bought with
silence, but the run itself is pending.
Checked by me, driven in Chromium (/tmp/a11y-blur.cjs, 6100 vs 6270 — a
real two-sided measurement, not a source read; R34c):
| main | this head | |
|---|---|---|
typed 100, focused |
aria-invalid=true, assertive region "Invalid number"
|
same |
| after blur | field 1, aria-valuenow=1, invalid cleared, region cleared |
field 5, aria-valuenow=5, invalid cleared, region cleared |
| after Enter | field 100, aria-valuenow=1, invalid still true, region still asserting "Invalid number"
|
field 5, aria-valuenow=5, invalid cleared, region cleared |
Nothing here is not looked at. The Enter row is a real a11y fix: on main a
screen-reader user who presses Enter is left in a field that reads 100, whose
value is 1, with an assertive error still announced and no way out but
retyping. gate-2 filed this as an unclaimed improvement; the body now claims
it — "That is more than cosmetic: before, Enter left the field reading 100
on a value of 1, with aria-invalid still set and an assertive 'Invalid number'
still announced, and no way out but retyping." Finding closed.
I also checked the one thing that could have made this worse: after a clamped
commit the value changes to a number the user did not type, and nothing
announces it. On Enter focus stays in the field and aria-valuenow moves 1 → 5,
which a focused spinbutton announces; on blur nothing announces it, exactly as
nothing announced the old fall-back to 1. Not a regression, and not a
BLOCKS — the user is not stuck, cannot lose their place, and has a visible,
correct number in front of them.
VERDICT: clear — driven in Chromium on both sides; the diff adds no aria, no role and no string (grep 0), and pr-a11y has not run on this head yet.
PROBLEM clear
SOLUTION clear
IMPACT note — min/max change meaning for every caller, no opt-in
API clear — docs now current in en + zh; nothing ossifies
THEMING clear
BREAKING note — behaviour changes for every bounded field, no opt-in
PERFORMANCE clear
VISUAL clear — every difference sourced to the body
A11Y & I18N clear — and the Enter fix is a real improvement, now claimed
Reading the bodies, not the labels (R34d). Two things only this level sees.
First, IMPACT and BREAKING are one finding seen twice — a contract that changed meaning with nobody opting in. Gate-2 paired IMPACT with a stale-docs finding in API; the delta closed the docs half, so what is left is the meaning change alone. It is not severe and it does not block. It is the whole reason this is not an unattended merge, which is a lower bar (R1g exception 2: a changed meaning on existing surface is judged at the same bar as new surface, because nobody opted in).
Second, IMPACT's body contains a sentence a user would call a bug — "ArrowUp
commits 100 in a field whose max is 99.99, aria-invalid unset" — and IMPACT's
verdict is note. That is exactly the shape R34a says to check, so I checked
it, twice, and the verdict stands: the anchor is NumberInput.tsx:626, which
this diff does not touch, and reverting the whole PR does not change the
behaviour by one pixel (measured on both sides in Chromium). It is not this
PR's, R13 and R3i both say it does not belong in the verdict on this PR, and
R24 says a finding raised on a PR must be filed in the same turn — which this
read-only round cannot do. So it is handed to you here rather than posted, and
it wants its own PR. The one-line shape: clamp after the precision rounding,
not before.
Is this THE fix or A fix? (R3c/R3d) Still a fix, and a better one than
last round. Root cause is the per-keystroke commit at NumberInput.tsx:796-798; an
unevadable fix lives there — let the pending/commit split the component already
has own the commit outright. This is a step toward it, not a detour: it ships no
permanent surface, all three helpers are module-local, and un-shipping it costs
nothing, so R3e does not apply. R3d's same-shape probe now returns two neighbours
rather than one — the keystroke commit and the stepper's post-clamp rounding —
and both are pre-existing, both are recorded, and neither is this diff's to
carry.
Worth saying because it is the thing that changed: Pagination.tsx:555-556
already documents NumberInput as "clamps to [min, max] with integer-only
semantics, so it only ever hands back a valid page here." Before this PR
neither half was true. After the first two commits the range half was true. After
the delta the sentence is true as written — which is the cleanest evidence
that the delta finished the job rather than patching a review comment.
JUDGEMENT NEEDED: none — a defect fix against a contract the system had already written down.
Verdict: approve. GitHub will not take a self-approval, so the posted
verdict is mechanically comment, and the comment says which verdict it is
(R1c).
No findings. All three of gate-2's are closed:
gate-2 #1 clamp returns the bound without re-running the shape rule
→ CLOSED at NumberInput.tsx:517-527. isIntegerOnly max={9.5} + "20"
commits 9, not 9.5; measured in Chromium on prev-head and this head
in one session, and confirmed a second way (R16g) by reading
getCommittableBounds and by three new unit tests that fail without it
gate-2 #2 the per-keystroke commit is untouched, symptom survives mid-typing
→ NOT FIXED, and correctly so. The body states it under "Not fixed
here"; R13 says it is not this diff's, and it is pre-existing.
Nothing to ask for
gate-2 #3 min/max still documented as the old contract
→ CLOSED at NumberInput.doc.mjs:131 + :137 (en), :415 + :420 (zh), and
the TS JSDoc at NumberInput.tsx:354/:359
#4390 touches the same handlers and NumberInput.doc.mjs; whichever lands
second needs a merge (R11).
Thanks — drove the integer-only bound and the empty range in Chromium; both land inside the field's own rules now. Approve, comment only because it's mine.
MERGE: hold(min/max change meaning for every existing caller — nobody opted in)
(25 words, counted. The three pairs marked EMBED go in with it.)
None. Every line-anchored thing gate-2 raised is fixed at this head, and R14d is explicit that a finding the head already fixed gets deleted rather than rewritten thinner. Manufacturing a fourth would be the failure mode the loop is trying to eliminate (R3i).
- The stepper's clamp is safe against the case the delta fixes —
getSteppedValuealready refuses a non-integer result atNumberInput.tsx:627, and the increment control disables itself rather than committing9.5. I built a probe on the assumption that it did not before reading nine more lines; the finding died there, which is where it should die. - Enter still keeps the raw pending text when the entry is in range, so
03+ Enter leaves the field reading03on a value of 3. Pre-existing — the conditional clear only fires when the commit clamped, by design. -
valueForStepping(:846) parses the pending text with the strict parser, so stepping from an out-of-range entry steps from the last committed value rather than from what is on screen. Consistent with "typing is untouched", and out of this PR's stated scope. - 225 tests green locally (
vitest --project ui, NumberInput 132 + Pagination 93, 4.5s). The body says 364 for the same two components — a different selection or a different counter, not a claim I can reconcile, and not worth a word on the PR.
TIME total 27m
setup 4m worktree + fast-install (15s, cloned node_modules) + @astryxdesign/build
+ storybook dev on :6270 (warm main reused: yes — :6100 was one side
of the a11y and commit-path comparisons)
reading 13m brief 95KB, critic 237KB, harness, loop, gate-2, the diff, NumberInput +
Pagination + getSteppedValue source
measuring 8m 8 distinct probes, 15 executions, across 3 revisions of NumberInput.tsx
and 2 ports; 12 frames; 1 targeted test run. 2 re-measures
writing 2m draft + critic pass + the three fixes it found
waste 3m see below
-
waste, named: (1) ~2m re-measuring the bounds table because I reused
gate-2's probe and trusted its
isIntegerOnly:!truearg — the arg does not work, so the first run measured the wrong configuration and had to be thrown away. This is the single most valuable line in the file: it means gate-2's fractional-bound evidence was junk, and only its conclusion happened to be right. (2) ~1m writing and crashing a stepper probe on a hypothesis that nine more lines ofgetSteppedValuewould have killed — read the function to its end before probing it. (3) onevitestinvocation from the wrong cwd. -
Warm main reused: yes. No second worktree was cut; the three-way
before/after came from swapping
NumberInput.tsxinside the one worktree, which is what the harness prescribes and it cost seconds per swap. - Re-measures: 2 — the bounds table (broken instrument) and the stepper table (probe crashed on a legitimately disabled control).
-
New probes banked in
~/astryx/probe-kit/:numberinput-stepper-bounds.cjs(carries theargs=trap in its header),numberinput-stepper-overshoot.cjs.
-
CI has not finished on this head. Six runs so far and
CIisin_progress;pr-visual,pr-rtl,pr-a11yand the test job have not reported. So gate-2's open question — three visual shots CI attributed to the old head that neither round could reproduce locally — is still open, and the a11y row above is my own Chromium run rather thanpr-a11y's. Nothing should merge before those land, which theMERGE: holdcovers anyway. - Whether any consumer outside this repo passes a fractional bound to an
isIntegerOnlyfield, or a fractionalmaxwith an integralstep. In-repo the count is zero for both;PowerSearchValueEditor.tsx:212-215hands consumer-supplied bounds straight through, so the second one is reachable by an ordinary money field (max={99.99}) that nobody in this repo happens to have written.