Review 4977
Round 2 (delta re-review). Verdict: approve — recommended to land, merge
left to a person (R26 subsystem; the repo's own high-risk gate agrees) · posted at exact head · exact head
74c1f21b90fbc52b6f60c4b49456b29940403d1a · prior reviewed head
cd995b3dbffea4299576aff517d5fee20b2ddcba · author bucket: external
contributor.
The round-1 blocker — a pattern whose literals contain a digit reading its own literal back as typed data — is fixed and proven at this head with a driven before/after arm. Both round-1 inline questions are answered. No finding remains.
Reconstructed from the PR record; the round-1 page was never written.
Blocker. stripToRaw pulled every digit out of the displayed string, so
it could not tell a mask literal from a typed one. On the PR's own
"Phone with country code" story (mask={{pattern: '(+1) ### ### ####'}}):
type 5, 5 → "(+1) 155" onChange("155")
type 5551234567 → "(+1) 111 111 1115" onChange("1111111115")
rawIndexForCaret counted digits the same way, so the caret went with it. The
round-1 review named the direction — read the display against the pattern — and
asked the author how they wanted to recover the digits, because doing so
threatened the messy-paste case their tests covered.
Two inline questions.
-
maskEngine.ts:41— "This can't tell a mask literal from a typed digit. Same assumption atrawIndexForCaret, :86." -
InputMask.tsx:437— "Clear handsonChangea null event and skipschangeAction. Might need the synthetic-target casthandleCompositionEnduses." -
InputMask.doc.mjs:168— "The guidance at :188 says store and submit the raw digits; this submits the formatted string. Which one wins?"
Round 1 closed with "once the digits round-trip, this is an approve."
#4977 feat(lab): add InputMask masked text input by AKnassa
(bucket: external contributor — read permission, in neither owners file,
labelled community)
74c1f21b90fbc52b6f60c4b49456b29940403d1a
Trigger is valid. Two commits since the reviewed head:
ba3da359 is a merge of upstream/main (not an author reply), and
74c1f21b fix(lab): read InputMask edits against the pattern is
single-parent and author-authored, touching only the five InputMask files
(+431/−114). The PR's own contribution is still the same 8 files; no
dependency, lockfile, workflow, or script change — verified from the changed-file
list before any install.
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: the delta carries four runtime decisions, changes a public prop's submitted value, and answers a standing changes-requested round — three independent promotion triggers, so fast was never available.
SUBSYSTEM: yes — the PR introduces a concept the system does not have (a mask
and caret-mapping engine; the PR establishes that DateInput is free-text with
lenient parsing, not segmented) and gives it a module of its own. R26 therefore
holds the merge, not the verdict: the loop reviews it, recommends, and leaves
the landing to a person. The repo's own gate says the same thing independently —
the PR carries needs:code-review and its review-required check is pending
"Waiting on code review: community contribution".
The design question a subsystem normally raises is not open here and needs no ruling: the package is canary-only lab, the author explicitly frames the API as "a working basis" for the RFC's spec protocol rather than a core claim, and round 1 scoped this thread to the correctness defect. Promotion out of lab is where the API question gets asked.
WHY 1: at cd995b3d the engine recovered digits by scraping every digit out of
the rendered display, so a pattern literal that happened to be a digit was
indistinguishable from something the person typed.
WHY 2: someone filling in the country-code field typed their own number and the
field kept a different one, moving the caret with it.
WHY 3: a masked input exists so a fixed-shape value is easier to enter
correctly; one that rewrites the value is worse than the plain text box it
replaces.
USER-FACING PROBLEM: typing 5551234567 into (+1) ### ### #### showed
(+1) 111 111 1115 and reported 1111111115.
PROBLEM SEVERITY: broken task — reproduced at the prior head by rendering it,
not by reading it (frames below).
VERDICT: clear
The component now hands the engine both pictures — what the field showed before the edit and what it shows after — plus where the caret landed and whether the edit was a deletion. The engine lines the old picture up against the pattern, so a character sitting in a literal position is a literal whatever it looks like, and recovers only the digits that were in slots. Whatever the edit inserted is read against the pattern when it lines up (a pasted formatted value) and as loose digits when it does not (a messy paste), which is what keeps both cases working. Because the engine can tell a slot from a literal, the pattern's own punctuation never comes back as data.
SOLUTION (4 decisions · ~190 runtime lines of 431)
- Replace display-scraping with pattern-aligned edit resolution — serves the stated blocker.
- Take the deletion direction from the input event's type rather than a remembered key — serves the same blocker where no key is reported (virtual keyboards, IME, autocorrect).
-
htmlNameposts the raw digits through a hidden input; the visible input drops itsname— answers round-1 inline 3. - Disabled cursor
not-allowed→default— conformance with the disabled-cursor gate that landed inmainunder this PR.
BURDEN: low — no Effect added, no listener, no timer, no new state. One ref and
one useCallback removed. All new logic is pure and lives in a module with no
React and no DOM.
BURDEN MATCH: proportionate — the machinery is a pure function serving a proven
correctness defect.
VERDICT: clear
OWNER: the mask engine owns the string and caret math; the component owns DOM
and React.
TIER 1: the Field wrapper, the input-container hook, the input ARIA helper, the
status-icon hook, the tooltip hook, VisuallyHidden, Spinner, the shared
clear button, useOptimistic/useTransition.
TIER 2: size, status.
SEAMS: controlled vs uncontrolled · the async change action · the clear button ·
IME composition · form submission.
BEHAVIOR UNIT: pure utility — resolveEdit is testable with no DOM, and carries
29 engine tests.
| seam | driven result |
|---|---|
controlled value + onChange
|
works — digits round-trip in Chromium at every fill level |
uncontrolled defaultValue
|
works — unit |
| async change action on clear | works — it now runs, with a real event target |
| clear button | works — value empties, onChange('') fires, focus returns to the field |
| IME composition | works — a composed 555 settles to (+1) 555 / raw 555
|
htmlName inside a form |
works — FormData carries the raw digits |
Effects are unchanged in count and shape: one pre-existing layout effect restoring the caret, keyed on an edit counter. Nothing about the delta adds a second Effect, so the multi-Effect architecture rule is not engaged.
VERDICT: clear
Anyone typing into a mask whose pattern contains a digit now gets their own
digits back, and the caret stays where they put it. For every other pattern the
displayed result is unchanged. Two contracts move underneath: htmlName submits
the raw digits instead of the formatted string, and a disabled field answers the
pointer with default instead of not-allowed. Both are documented; the
component is unreleased canary-only lab surface, so nobody is migrated.
VERDICT: note — two deliberate contract changes ride along with the fix, each documented at this head.
Smallest real call site, from the PR's own story:
<InputMask mask={{pattern: '(+1) ### ### ####'}} label="Phone with country code" />| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
~ |
mask: {pattern, placeholder?} — unchanged signature; doc now states literals may be digits and are never read back as typed data |
barrel | — | JSDoc + doc.mjs | ok |
~ |
htmlName?: string — unchanged signature; submitted value changes from the formatted display to the raw digits |
barrel |
NumberInput does exactly this: visible input drops name when the display is formatted and a hidden input posts the canonical value (NumberInput.tsx:926) |
JSDoc + doc.mjs + dense docs | ok |
− |
stripToRaw, rawIndexForCaret
|
module-internal | — | — | ok |
+ |
clampRaw, resolveEdit, DeleteDirection, DisplayEdit
|
module-internal | — | — | ok |
The barrel still exports only InputMask, InputMaskProps, InputMaskSize and
MaskProp, so nothing added or removed here is public surface.
OSSIFICATION: no public prop, accepted value, export, default or theme target is
added. The one changed line makes the component internally consistent — one
string for value, onChange and the submitted field — which is the class the
Field-family canonical-value inputs already occupy, and it is the answer the
nearest landed sibling gives. Cost of being wrong is low while the package is
canary-only.
VERDICT: clear
No theme target added, removed or renamed. The component's theme props carry the
same four keys. The only style change is the disabled cursor, which is not a
theme surface and is set by the repo's own zero-tolerance disabled-cursor gate.
VERDICT: clear
BEHAVIOR: yes, three — value and caret for digit-literal patterns (the fix);
htmlName submits raw digits; disabled cursor. All on an unreleased canary-only
package with no consumer.
API: no — no signature change, and the removed helpers were never exported from
the barrel.
VISUAL: yes, one — the disabled cursor. Frame banked below; both disabled
variants answer default, which is the value the landed gate requires.
THEME: no.
VERDICT: note — three intentional behavior changes, each with a source: two from
the round-1 review, one from a gate that landed in main.
EFFECTS: one, pre-existing and unchanged.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
useLayoutEffect [editSeq] InputMask.tsx:356
|
the input's selection range | the caret must be restored after React writes the re-formatted value, which has not happened during the handler | unchanged — same single effect, same key, before and after | per render; nothing to clean up | caret assertions in 37 component tests |
RENDER: no reachable change. Per keystroke the work goes from one pass over the
display to one pass over the pattern plus the inserted text; both are the length
of a phone number. One fewer ref write per keydown, because the remembered key
is gone.
LISTENERS/OBSERVERS: none added.
LAYOUT: the hidden input measured display: none, 0 × 0 in Chromium — no box,
no flex gap in the input row.
BUNDLE: no dependency change.
VERDICT: clear
VISUAL CHECK: manual frames required WHY: the delta's entire user-visible effect is which characters land in a text field and where the caret sits. No CI job renders that, and the round-1 defect was invisible to the unit suite until the pattern with a literal digit was driven.
Same Storybook, same story, same viewport, same keystrokes. The before arm is
the prior head's maskEngine.ts and InputMask.tsx checked out into the same
running dev server, so only those two files differ between the rows.
| keystrokes | Before — cd995b3d
|
After — 74c1f21b
|
|---|---|---|
5, 5
|
 display (+1) 155 · raw 155
|
 display (+1) 55 · raw 55
|
5551234567 |
 display (+1) 111 111 1115 · raw 1111111115
|
 display (+1) 555 123 4567 · raw 5551234567
|
The before row reproduces the round-1 review's transcript character for character, which is what makes the after row mean something.
SENSOR RECEIPT — build: exact head 74c1f21b (after) and the prior head's two
engine files swapped into the same build (before), arm diff banked at
203ac393b2c9c38ff5674c3c3381a03109ed3ca41ce57d9302b6a43ba08cc174; story:
ZzReview/4977 → Country, a controlled harness over the PR's own
(+1) ### ### #### pattern; theme neutral, light; direction LTR; viewport
760×220 at DPR 2; semantic state: focused, after the listed keystrokes, render
settled 250 ms past the last key; geometry: the input element's own box; zero
page errors, and the only console warning is the pre-existing runtime-theme
notice that is present on main.
the PR's own story, after typing 5551234567 into the country field |
the four states |
|---|---|
 |
 |
The states frame is where the disabled-cursor change lands: the two disabled
rows read cursor: default under the pointer, the read-only and idle rows read
text. No hidden-input box appears in any row, which is the layout half of the
htmlName change.
| backspace over a literal | forward-delete over a literal | after clear |
|---|---|---|
 (555) 124-567 caret 8 |
 (555) 123-567 caret 10 |
 empty, focus back in the field |
Backspace and forward-delete arrive at the browser as the same value and the same caret; only the input event's type separates them, and taking the direction from there is what makes these two rows differ correctly.
| 390 px, DPR 3, coarse pointer | RTL |
|---|---|
 |
 |
On coarse pointer the value and the per-keystroke digit sequence are identical
to desktop, and backspacing across the space literal lands on
(+1) 551 234 567 with the caret at 7. In RTL the value string round-trips
unchanged; the bidi rendering of the parenthesised group is the same before and
after this commit and is not part of the delta.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
Auto-covered. pr-a11y and pr-rtl are both green at this head, and the
PR touches no a11y baseline file, so nothing was bought with silence.
Checked by me, driven in Chromium at this head.
-
A1 — the field carries its label; the clear button's accessible name is
Clear Phone number. -
A4/A5 — the hidden input adds no tab stop: the next stop after the field
is the clear button, and after clearing, focus is back on the text input, not
on
<body>. The button is not removed from the DOM to express state; it is removed because the value is empty, which is its documented condition. -
A12 —
aria-describedbyresolves toFormat: (555) 555-5555;aria-invalidandaria-requiredstill wire from props;aria-busyis present only while busy. -
A11 — busy does not disable:
disabledremains a function ofisDisabledand the disabled-message path only, untouched by the delta. -
A3 — the ghost hint sits inside an
aria-hiddentrack, so the remaining shape is not announced twice;inputmode="numeric"survives. -
A14/pointer — the two disabled rows answer
cursor: default, the value the landed gate requires and the valueTextInputandNumberInputalready use. - Composition — an IME composition echoes unmasked while composing and settles to the masked value on end, with the caret at the end of the digits.
I18N. The delta adds no user-facing or AT-facing string. The one AT-facing
string in the component, the format hint, is unchanged by this commit and still
reaches aria-describedby. No key is added, renamed or removed. Direction: the
typed value round-trips in RTL and pr-rtl is green.
Nothing the delta touches is unchecked.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | note — two documented contract changes ride along |
| API | clear |
| THEMING | clear |
| BREAKING | note — three intentional behavior changes, each with a source |
| PERFORMANCE | clear |
| VISUAL | clear |
| A11Y & I18N | clear |
Every round-1 finding rechecked at this head.
| round-1 finding | state at 74c1f21b
|
evidence |
|---|---|---|
| Blocker — a digit literal is read back as typed data, value and caret both | fixed | driven before/after arm above; 29 engine + 37 component tests green |
Clear hands onChange a null event and skips the change action |
fixed | clear now routes through the same commit path; the change action runs and the callback gets a real event target |
| The doc says store and submit raw digits; the component submitted the formatted string | fixed | raw wins: a hidden input posts the digits, the visible input drops its name, and the prop doc, the doc file and the dense docs all say so |
None remains, so none is carried forward.
Radius rechecked, nothing new found. Loose paste behaves identically on a
pattern with no digit literals and on one with them — pasting
+1 555 123 4567 gives 1555123456 under both (+1) ### ### #### and
(###) ###-#### — so that is the documented capacity clamp on over-long loose
input, not a surviving instance of the round-1 defect. Formatted paste, messy
paste, mid-value insert, select-all-replace, over-capacity typing, both delete
directions, IME, coarse pointer and RTL were each driven and each landed where
the pattern says they should.
GOAL: met — the round-1 blocker's own repro, rendered at both heads, moves from
(+1) 111 111 1115 / 1111111115 to (+1) 555 123 4567 / 5551234567.
DISPOSITION:
-
htmlNamenow submits raw digits instead of the formatted string → accepted. Nobody pays: the package is canary-only and unreleased, so there is no consumer to migrate. It is the answer the round-1 review asked for, and it matches the nearest landed sibling (NumberInput.tsx:926) rather than inventing a shape. - Disabled cursor
not-allowed→default→ accepted. Required by the repo's own zero-tolerance disabled-cursor gate, which landed inmainunder this PR and admits exactly one value;TextInputandNumberInputalready use it. - The Vercel preview deployment is red at this head → not a finding. It is red on unrelated PRs in the same window, every build job in CI is green, and the PR touches neither the docsite nor deployment config. Recorded as unverified rather than attributed.
ADVICE: omitted — nothing is asked of the author, so there is no remedy to cite or invent.
AUTHOR CAN PROCEED: yes — nothing is required of the author on this PR.
WORST OUTCOME: none found at this head. The strongest user-impact sentence
anywhere in this record — "typing 5551234567 into (+1) ### ### #### showed
(+1) 111 111 1115 and reported 1111111115" — describes the prior head
cd995b3d and is rendered fixed at 74c1f21b, so it is compatible with
approve.
JUDGEMENT NEEDED: none — the fix answers the round-1 question against the existing contract, and the one contract that moved took the nearest landed sibling's shape rather than a new concept.
approve — every touched slot is clear or note, every negative fact has
a disposition, and the author is not blocked. The merge is not the loop's to
make (R26, above): reviewed, recommended, waiting on a person. Plainly: I would
land it once a human has done the code review the repo's high-risk gate asks
for.
No numbered findings: none remain.
Reading the display against the pattern was the right answer, and it kept the messy paste working — the part I wasn't sure about when I asked. All three from my last round are closed:
(+1) ### ### ####round-trips5551234567, clear runschangeAction, and the raw digits win through a hidden input the way NumberInput does.Not merging it here — a new component still wants a human's code review, which the PR is already waiting on.
If you'd rather talk it through with someone, we're in Discord.
[Reviewed by Robohands]
None — nothing is line-anchored on an approve with no findings.
- While an async change action is in flight, the hidden input carries the committed digits while the field shows the optimistic ones; a form submitted in that window posts the pre-action value. Defensible as written — the optimistic value is by definition unconfirmed — and it needs a form submit during an in-flight action to observe.
- In RTL the browser renders the value's parenthesised group mirrored
(
4567 123 555 (1+)). Identical before and after this commit, so it is not the delta's; worth a look when the component leaves lab. - Backspacing the pattern's leading literal is a no-op that moves the caret forward to the first digit slot rather than leaving it. Harmless, and only reachable inside a leading literal run.
TIME total 33m
setup/rules 14m wiki clone + kit read + worktree + dependency install
reading 6m round-1 reconstruction from GitHub, delta, engine, tests, history
build/server 3m Storybook dev at the exact head (warm main not applicable — the
before arm is the prior PR head, driven in the same dev server)
browser/a11y 11m 3 Chromium probe runs, 2 arms, frames, a11y and RTL passes
focused tests 0m 13s — 29 engine + 37 component tests, all green
critique/wiki 5m 2 critic passes, presentation, wiki, assets branch
CI wait 0m read the exact-head results already on the PR
waste 6m the first dependency install spent five minutes scanning for a
donor worktree; restarting it with the donor named took 7s
Re-measures: one, to prove the after arm returned after the before arm was
swapped out. Warm main port 6100 was not used and not needed: the comparison
this review owes is against the prior PR head, not against main.
- Why the Vercel preview deployment is red; it is red on unrelated PRs in the same window and no build job in CI reproduces it.
Posted as drafted — approved at exact head 74c1f21b. No merge or auto-merge action was taken.