Review 4405
Internal review record. Nothing here was posted to the PR. This is an unapproved community PR adding core behaviour, so the loop's output is a draft for the maintainer regardless of the verdict.
| PR | #4405 |
| Author |
@AKnassa — outside contributor (fork, read permission, in neither owners file) |
| Verdict | request changes — 2 blocking, 1 note |
| AUTHOR CAN PROCEED | yes |
| Posted? | no — internal draft |
| Loop version | 1.6.0 |
| Audit rubric | 1.13 |
| Reviewed at head | 7f7460f |
| Evidence | assets/pr-4405 — 7 frames, 7 sensor receipts, 6 arm diffs |
#4405 feat(core): make Tokenizer InputGroup-compatible by AKnassa (bucket: contributor)
7f7460f3ad6f79b74f8c15bde2dbe80190aef738 <- every claim below was verified at this commit
Dispatch head was 6d90946b7ece9330153ce8b304d26f0564763239, 507 commits behind main.
The merge of origin/main (c6041cfd58ea832a79b50f98d7e0f64cd513852e) was textually
conflict-free — four of the PR's files auto-merged, zero conflicts — and was pushed
without force to AKnassa:ak-3520-tokenizer-inputgroup in 20s. Everything below is
the resulting exact head. No cixzhang decisive review exists; no prior review of any
kind exists (R1e: pulls/4405/reviews and /comments both empty).
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: two runtime decisions, an intended visual change with no covered overflow endpoint, and an unresolved question the author raises himself. Fast is unavailable on any one of those.
WHY 1: Putting a Tokenizer inside an InputGroup today produces two labels and two
field boxes — the group's and the Tokenizer's own — and the Tokenizer's box will not
fit the group's fixed-height row.
WHY 2: Someone filling in "To: [people]" sees a control that looks broken: a second rounded box floating inside the first, and once two tokens are picked the inner box grows past the group's border and hangs below it. A screen-reader user hears the field named twice.
WHY 3: InputGroup exists so a prefix/suffix addon and a control read as one input.
A control that cannot join that seam cannot be used for the one job the group is
for, so the tag-entry case — the most natural "To:" field there is — has no supported
path.
USER-FACING PROBLEM: A person adding recipients to a "To:" field sees a doubled,
overflowing control instead of one input, and a screen-reader user hears the field
announced twice.
PROBLEM SEVERITY: missing capability — Tokenizer was the last control on the issue's
own compatible-list with zero InputGroup references, and the composition renders
visibly broken today (measured: 59px control in a 32px group, 27px spill).
VERDICT: clear
Astryx has a wrapper that draws one border around a label, a small prefix and one input so they read as a single field. Until now the Tokenizer had no idea when it was inside one, so it drew its own label and its own box inside the wrapper's box. Now it notices the wrapper, hands the label, description and status over to it, and keeps only the chips and the typing area. Because the wrapper's row is a fixed height, the chips also stop stacking onto a second line and slide sideways instead.
SOLUTION (2 decisions · ~76 runtime lines of 249)
- Inside a group, stop rendering the Tokenizer's own field chrome and take the accessible name and description from the group instead — traces to the stated problem ("renders its own duplicate label and field chrome").
- Inside a group, chips stop wrapping; the row scrolls sideways with its scrollbar hidden — traces to the stated problem, recorded under the body's "Needs a maintainer call" heading.
Both decisions are written down. R29's failure mode is silence, not plurality, and there is none here: the second decision is the one the author flags himself, in the body, with its alternatives and its two costs named. That is the shape the rule wants.
BURDEN: low — zero Effects, zero listeners, zero timers, no new state, no new public
prop, no new theme target, no new dependency. One context read, one useId, two
style objects, one early return.
BURDEN MATCH: proportionate — a missing-capability problem answered with the same
mechanism seven landed siblings already use.
VERDICT: note — decision 2 is a design choice the author correctly declined to make alone; see REMEDY SEARCH, which found a landed answer for it.
OWNER: InputGroup owns the grouped-control contract; Tokenizer implements it, as
TextInput, NumberInput, TimeInput, DateInput, Typeahead, Selector and
MultiSelector already do (useInputGroup grep: 9 source files on the merged head).
TIER 1: InputGroupContext, getInputARIA, groupStyles.inGroup, VisuallyHidden
— all four reused, none reimplemented.
TIER 2: size and status handling unchanged.
SEAMS: the unfocusedLayer popover, the unfocusedInline OverflowList path, the
disabled-reason tooltip, and the group's own flex row.
BEHAVIOR UNIT: inline — correct here. No state machine is added; the diff adds zero
Effects, so the two-Effect threshold is not approached.
| seam | driven result |
|---|---|
| built-in inline row, 1 token | works — one .astryx-field, combobox named "Favorite fruits Selections", row height 32 = group height 32, spill 0 |
tokenOverflowBehavior="unfocusedInline" |
works — "+3 more" summary, scrollWidth − clientWidth = 0, nothing cut |
tokenOverflowBehavior="unfocusedLayer", closed |
works — the placeholder is the group-row member: h=32, start radius 0 (joined to the seam), spill 0; popover content 0-height |
unfocusedLayer, expanded |
works — popover content keeps its standalone 10px radius and flex-wrap: wrap, exactly as the comment at Tokenizer.tsx:766-769 claims |
| disabled-reason tooltip in a group | works — aria-describedby carries both the group id and the tooltip id, input is aria-disabled not disabled
|
isInGroupRow = inputGroup != null && !isLayerMode (Tokenizer.tsx:770) is the only
non-obvious placement call in the diff, and driving both layer states confirms it: the
element in the group's flex row gets the seam treatment, the element inside the
popover does not.
VERDICT: clear
Nobody is reached until someone opts in: no existing app renders a Tokenizer inside an
InputGroup, because until this PR the composition was undocumented and visibly
broken. For the builder who does opt in, this is the difference between a control
that hangs out of its own group and one that sits in it.
For the end user of an app that adopts it, the visible change is a single clean input row instead of a doubled box — and, in the state below, two of six chosen tags that are no longer on screen.
What landing this newly exposes: Tokenizer becomes the second multi-value control on
the group's compatible list, and the first one that can silently hide selections.
MultiSelector — already on the list — renders a summary string in the group row
(MultiSelector.tsx:1220,1234), never N chips, so the "many values in a 32px row"
problem never arises for it. Adding Tokenizer without the same treatment makes that
asymmetry real on the day this merges, not before.
VERDICT: note — a new asymmetry between the two multi-value controls on the same list
No API change. No prop added, removed or retyped; no export added; Tokenizer's
public signature is byte-identical. The diff's only surface movement is documentation:
Tokenizer is added to InputGroup.doc.mjs's compatible-controls list (three places)
and one guidance line is added to Tokenizer.doc.mjs in all three blocks — docs,
docsZh and docsDense. Both locales updated: en+zh.
OSSIFICATION: nothing new ossifies. The internals the diff reaches for —
getInputARIA, groupStyles.inGroup, useInputGroup, VisuallyHidden — are the
existing mechanism, not a near-miss being worked around; there is no new helper beside
a shipped one.
One behavioural note that is API-shaped without being an API change: inside a group,
Tokenizer's own description and status props stop having any rendered effect,
because Field is what rendered them. A builder writes status={{type:'error', message:'…'}} and gets the error border but no message anywhere on the page. That is
the InputGroup contract, identical for all seven siblings, and it is not this
contributor's to change (R13).
VERDICT: clear
No new theme targets. themeProps('tokenizer', …) is untouched; the diff adds no
astryx-* class. Greps on the changed source, at this head:
git diff origin/main...HEAD -- packages/core/src | grep -E "^\+.*(#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow)" → 0
git diff origin/main...HEAD -- packages/core/src | grep -E "^\+.*themeProps\(|^\+.*astryx-" → 0 (4 hits, all test assertions)
T2 — is a themeable surface removed? In a group the astryx-field,
astryx-field-label and astryx-field-description targets stop being emitted for a
Tokenizer. That is the group taking them over, it is what all seven siblings do, and
the group emits its own. Not a removal.
One value is pinned and not themeable: scrollbarWidth: 'none' at
Tokenizer.tsx:311, alongside flexWrap: 'nowrap' and overflowX: 'auto'. A theme
cannot restore a scrollbar or re-enable wrapping in a grouped Tokenizer. This is not a
T-rule violation — none of the three is a colour, spacing, radius or shadow token —
but it is the styling half of the finding below, and it is worth naming here because
it means no theme can opt out of it.
VERDICT: note — three layout decisions pinned beyond a theme's reach
BEHAVIOR: yes, and it is the point — a Tokenizer with an InputGroup ancestor now
renders a different tree. Walked the six states against the diff: controlled vs
uncontrolled — untouched, value/onChange unchanged, and token removal inside a
group is covered by a new test. disabled — covered, and driven: input stays
focusable with aria-disabled="true", not disabled, so focus is not dropped (A11).
boundary — one token and six tokens both driven. empty / loading / error: not
reachable — the diff adds no state, no default and no prop; the only conditional it
adds is "am I inside a group", which is not a runtime state transition.
Outside a group, nothing changes: inputGroup is null and every new branch is
inert.
API: no — nothing stops compiling and no existing call site changes meaning. An
existing <Tokenizer> not inside an InputGroup takes the identical path.
VISUAL: yes, inside a group only, and measured both directions. Outer box: the control goes from 59px (wrapping, spilling 27px below a 32px group) to exactly 32px, spill 0. It got smaller, so R15d's growth case does not apply and no neighbour reflows. Frames below.
THEME: no — every existing target and override still resolves; the grouped path emits
strictly fewer Field targets, and the group already emits its own. Drove the far side
of the one bound the diff adds: height: 100% from groupStyles.inGroup overriding
styles.wrapper's height: auto — measured 32px at md, matching the group, with
minHeight from sizeStyles consistent rather than fighting it.
VERDICT: note — an intended, measured visual change confined to the grouped path
EFFECTS: zero added, zero changed, zero moved.
git diff origin/main...HEAD -- packages/core/src | grep -E "^[+-].*use(Layout)?Effect" → 0
No Effect table is owed. The diff adds one useId and one use(InputGroupContext)
read per Tokenizer.
RENDER: no reachable change. The two mergeRefs(...) calls at Tokenizer.tsx:774 and
:898 build a fresh callback ref every render, so React detaches and reattaches on
each one — but the code they replace was an inline ref={el => {…}} arrow, which was
equally unstable. Same cost before and after; not a regression, and not a finding
against this author. It is a lint error for a different reason — see JUDGEMENT 1.
LISTENERS/OBSERVERS: none added. The disabled-reason tooltip's listeners keep the lifetime they already had.
LAYOUT: no forced synchronous reflow added — no getComputedStyle, offsetWidth or
getBoundingClientRect in the diff. overflowX: 'auto' makes the grouped row a
scroll container, which is a compositor-side cost, not a per-render one.
BUNDLE: no new dependency. Two imports (VisuallyHidden, groupStyles) that
packages/core already ships and that six sibling controls already pull in.
VERDICT: clear
VISUAL CHECK: manual frames required
WHY: pixels are intended to change in the grouped path, and the changed visible
endpoint — a grouped row whose selections exceed it — is not covered by any story. The
WithTokenizer story the PR adds sets maxEntries={3}, which is precisely the cap
that keeps that endpoint off screen. The repo's Visual acceptance job was cancelled
on this head, so there is no gate to lean on either.
All frames: real Chromium, headed ({launch: {headless: false}} — headless never
paints a macOS scrollbar, and a scrollbar is the subject), each opened with the read
tool, each with a captureWithSensors() receipt written beside it.
SENSOR RECEIPT (identical for every frame except Build and Viewport): story
core-inputgroup--with-tokenizer · theme neutral · colorMode light · direction
ltr · dpr 1 · forcedColors false, reducedMotion false, coarsePointer false, hover
true · targetCount 1 on .astryx-input-group · fonts loaded · zero page and
Storybook errors · build 7f7460f3ad6f79b74f8c15bde2dbe80190aef738. Receipts, arm diffs and full-size frames:
assets/pr-4405.
One sensor disclosure, because R38 forbids loosening an expectation after a
failure and I did loosen one. The narrow-overflow pair was first written
expecting three tokens; the sensor failed at two, because the story's
maxEntries={3} cap plus one preselected item leaves the click path room for one
more, not two. I investigated the mismatch rather than assuming, then relabelled the
arm "two tokens" and lowered the expectation to match the scenario the arm actually
drives. No finding rests on that pair — it is the control/second-width frame. The
decisive frame, overflow6, asserted six tokens from the arm's own contract and
passed on the first run. One further change was a sensor correction, not a
loosening: the remedy frame counts scrollWidth − clientWidth instead of
.astryx-token elements, because OverflowList renders aria-hidden measurement
copies and a raw element count reads nine for six selections — the hidden-sizer trap
the harness warns about.
Both arms ran in the same worktree, same install, same browser session. BEFORE is
Tokenizer.tsx reverted to origin/main in place (12 insertions, 64 deletions, one
file, banked as before.diff), not a second checkout. Semantic state sensor: BEFORE
asserts 2 .astryx-field, AFTER asserts 1 — written from the contract before looking
at the page, and both passed.
| Before (main) | After (this PR) |
|---|---|
 |
 |
Difference: intentional. Source is the PR's own first sentence — "instead of
rendering its own duplicate label and field chrome". Before, the Tokenizer's own
rounded box sits inside the group's box and stops ~180px in, leaving the group's
border running on past it; after, one continuous border. .astryx-field count 2 → 1,
.astryx-field-label 2 → 1.
| Before (main) | After (this PR) |
|---|---|
 |
 |
Measured, same run, 560px viewport, tokenOverflowBehavior at its 'none' default:
| before | after | |
|---|---|---|
| control height / group height | 59px / 32px | 32px / 32px |
| spill below the group's border | 27px | 0px |
scrollWidth − clientWidth |
0 | 202px |
| selections fully on screen | 6 of 6 | 4 of 6 |
computed scrollbar-width
|
auto |
none |
Difference: partly intentional, partly a finding. Intentional and sourced — the body says the grouped variant "keeps tokens on one scrolling row", and the spill is gone. Not sourced, and the finding: “Dragonfruit” is cut mid-word at the border and “Watermelon” is not on screen at all, with no scrollbar, no fade and no “+N”. The row ends flush at the border, so it reads as the complete set. The author names this cost in the body ("the scrollbar is hidden, so overflow isn't visually signalled"); naming it is not the same as it being decided.
Same six selections, same 560px row, tokenOverflowBehavior="unfocusedInline":
scrollWidth − clientWidth = 0, nothing cut, "+3 more" carries the rest. This is
the control's own shipped public value, not a proposal.
| Before (main) | After (this PR) |
|---|---|
 |
 |
At 260px the before-arm's control again grows past the group. After, one row. This is the control/attribution pair for the pair above — the same fix at a second width.
The body says "a focused token's outline can clip at the scroll edge". Driven, it does
not, at md: the focused remove button's 2px ring clears the scrollport by 3px top
and bottom, and when the row overflows the browser scrolls the focused button into
view (scrollLeft 0 → 12.5) rather than clipping it. Worth telling the author, since
it is one of his two stated costs and it is not real.
Render-matrix rows deliberately dropped, with reasons: text zoom 200% and
forced colors — the diff adds no colour, no font size and no painted state, so
neither can change an answer the base control does not already own. icon-only — a
Tokenizer has no icon-only path. hug vs fill — groupStyles.inGroup sets
flex: 1; min-width: 0, identical to the seven siblings, so the width source is the
group in every case. RTL and the width sweep are covered above and in A11Y & I18N.
VERDICT: BLOCKS — at default settings a grouped Tokenizer puts 202px of the person's own selections off screen with no affordance of any kind
TRIGGERED BY: the proven visual defect above — 2 of 6 selections off screen, one cut
mid-word, no scrollbar, no fade, no overflow summary, at tokenOverflowBehavior's
default.
ELAPSED: 3m
OUTCOME: existing-pattern
EXISTING PATTERN: Tokenizer's own tokenOverflowBehavior="unfocusedInline"
(Tokenizer.tsx:436 default, :484-485 the truncation branch) — a landed, exported,
public value of a public prop. Driven inside the group at the same head and viewport:
scrollWidth − clientWidth = 0, "+3 more", nothing cut. Corroborated by the sibling:
MultiSelector, the other multi-value control on the group's compatible list, already
renders a summary string in the group row (MultiSelector.tsx:1220,1234) rather than
N chips.
REQUESTED BY: none
EXTENSION ELAPSED: 0m
HUMAN HANDOFF: not applicable — a landed public pattern was found inside the cutoff, so this is not a design question anyone has to answer. Ownership, public shape and constraints are all settled; the author owns HOW.
Driven in real Chromium at this head unless marked otherwise.
-
A1 accessible name — checked by me. The grouped combobox resolves to "Favorite
fruits Selections"; both
aria-labelledbyids resolve to real elements. In LTR and RTL. -
A12 input ARIA from props — checked by me, and this is where the one defect is.
With the Tokenizer's own
descriptionandstatusalso set inside a group, the input emitsaria-describedby="<group-id> _r_7_ _r_8_"and the last two point at elements that were never rendered, becauseFieldis what would have rendered them. Reproduced in a browser, not inferred:Tokenizer.tsx:725(status?.message ? statusMessageId : null) is unguarded. Convention gate —!inputGroup && …statusacross the seven group-aware controls: 4 guard it (TextInput.tsx:348,NumberInput.tsx:608,MultiSelector.tsx:823,Selector.tsx:902), 3 do not (Typeahead,TimeInput, and nowTokenizer).TextInput.tsx:346-347carries the comment explaining exactly why. Thedescriptionid is unguarded in 7 of 7, so that half is genuinely inherited and not this contributor's (R13). Impact: browsers and AT ignore ids that resolve to nothing, so nobody is harmed today — anote, not a block. The PR body's own justification is what is slightly off: it says "TypeaheadandMultiSelectorhave exactly the same wart", andMultiSelectordoes not. -
A11 busy never disables — checked by me.
isDisabledinside a group leaves the input focusable witharia-disabled="true"; focus is not dropped. Covered by a new test too. -
A4 full keyboard support — checked by me. Shift+Tab from the input reaches
"Remove Banana" then "Remove Apple"; when the row overflows the browser scrolls the
focused control into view (
scrollLeft0 → 12.5). Every selection stays reachable by keyboard even when it is off screen, which is why the overflow finding is about discoverability and not reachability. -
A15 focus ring visible — checked by me. 2px ring, 3px of clearance top and
bottom inside the scrollport at
md. Not clipped. - A18 axe — auto-covered, and not re-run locally per the harness. Note that the a11y job is one of the ~12 checks missing on this head; see JUDGEMENT.
-
A16 / I1 strings — no new user-facing or AT-facing string.
grep -E "^\+.*(useTranslator|t\('@astryx)"on the changed source → 0. The one new text node is<VisuallyHidden>{label}</VisuallyHidden>, which re-renders the caller's own already-translatedlabel. -
I8/I9 logical properties, I10-I12 direction — checked by me. Driven in RTL:
computed direction
rtl,flex-wrap: nowrapandoverflowX: autohold, and the first chip sits at the inline start (the right edge).groupStyles.inGroupis entirely logical (marginInlineStart,borderStartStartRadius); the diff adds no physical inset ortranslate, so I11 cannot fire. -
A6/A7 announcements — not looked at. The diff does not touch
useAnnounce, but I did not check whether the count of off-screen selections is announced, which is the question the overflow finding raises.
VERDICT: note — two aria-describedby ids resolve to nothing when a grouped Tokenizer
sets its own description or status; no user-reachable harm, and 4 of 7 siblings
already guard the status half
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | note — 2 decisions, both recorded; the second is a design choice, answered in REMEDY SEARCH |
| ARCHITECTURE | clear |
| IMPACT | note — new asymmetry between the two multi-value controls on the compatible list |
| API | clear |
| THEMING | note — nowrap / overflow / hidden-scrollbar pinned beyond a theme's reach |
| BREAKING | note — intended, measured visual change, grouped path only, control got smaller |
| PERFORMANCE | clear |
| VISUAL | BLOCKS — 202px of selections off screen at default settings, no affordance |
| A11Y & I18N | note — two dangling aria-describedby ids |
Re-read against the bodies: no note above hides a sentence a user would call a bug.
The A11Y note is genuinely inert (missing ids are ignored by AT), the THEMING and
IMPACT notes are consequences of the VISUAL block and dissolve with it, and the
BREAKING note is the fix working. The one thing filed in the wrong slot would have
been the lint error, which PERFORMANCE noticed while pricing the ref churn and which
is not a performance fact at all — it is carried below as finding 1.
GOAL: partly met — the stated goal ("Tokenizer now works properly inside an
InputGroup") is met for the seam, the ARIA and the chrome, all driven and all
correct; it is not met for the case the group's fixed height creates, where the
default configuration hides the person's own selections. The uncovered member of the
class is "more selections than the row holds", and it cannot follow separately because
it is the state this PR newly makes reachable.
DISPOSITION:
- Default grouped overflow hides selections with no affordance → blocks now
- Lint red at the merged head,
@astryx/no-unstable-merged-refs×2 → blocks now - Two dangling
aria-describedbyids → not blocking; one-line guard, cheap in the same push, and inert until then -
description/statussilently dropped in group mode → accepted (pre-existing, all seven siblings, the group's contract by design; not this contributor's) - The PR body's "MultiSelector has the same wart" is inaccurate → not blocking; folded into the finding above rather than raised separately
- Missing CI checks on this head (7 of ~19; a11y and visual among them) → accepted (mechanism, not merit): workflow approval for a fork contributor, and the original head 6d90946 ran a green Lint before my merge. Named here so approving is a choice about a known gap.
-
mergeRefsrender-churn → accepted (no regression; the code it replaces was an equally unstable inline arrow)
ADVICE: proven existing pattern — tokenOverflowBehavior="unfocusedInline", driven at
this head with the frame above, plus MultiSelector's summary row as the sibling
precedent. For finding 1, useMergedRefs at Typeahead.tsx:430, the exact line this
PR mirrors.
AUTHOR CAN PROCEED: yes — both blocking items have a cited landed answer and
neither needs a decision from us. Acceptance criteria: (1) pnpm lint is clean at the
merged head; (2) a grouped Tokenizer at tokenOverflowBehavior's default, with more
selections than the row holds, leaves none of them visually cut off without an
affordance — verified by scrollWidth − clientWidth === 0 on .astryx-tokenizer in
the group, with a story covering that state. The author picks the mechanism; making
unfocusedInline the grouped default is the obvious one and is free of API cost,
because grouped Tokenizer has no callers yet.
WORST OUTCOME: "“Dragonfruit” is cut mid-word at the border and “Watermelon” is not on screen at all, with no scrollbar, no fade and no “+N”. The row ends flush at the border, so it reads as the complete set." → request changes. That sentence cannot coexist with approve, and it is exactly what the frame shows.
JUDGEMENT NEEDED: none — the author raised the overflow treatment as a maintainer call, and it turned out to have a landed answer inside the five-minute search. No new concept, no new axis, no uncovered design, no measured cost to accept.
request changes
-
[BLOCKS] At
tokenOverflowBehavior's'none'default, a grouped Tokenizer puts 202px of chips outside a scrollport whose scrollbar is set tonone→ someone who picked six tags sees four and a half, with the row ending flush at the border and nothing saying there are more;unfocusedInlinealready fits all six with "+3 more" ·Tokenizer.tsx:302-311Confirmed a second way: found by measurement, confirmed by driving theunfocusedInlinearm in the same session —scrollWidth − clientWidthgoes 202 → 0, so the mechanism is the cause and the shipped fix removes it. -
[BLOCKS]
mergeRefscalled during render trips@astryx/no-unstable-merged-refs, twice; Lint is red on the merged head → the PR cannot merge until it is green, andTypeahead— the control this diff mirrors — already usesuseMergedRefsat the same position ·Tokenizer.tsx:774,:898Confirmed a second way: found by CI on the merged head, confirmed byeslint . -f jsonlocally (2 errors, both these lines) and by grep — 35ref={useMergedRefs(sites inpackages/core/srcagainst 2ref={mergeRefs(, and both of those two are this diff's. The rule landed in #5429, after this branch was cut; Lint was green on the original head. My merge is what surfaced it, and the review says so. -
[not blocking]
Tokenizer.tsx:725referencesstatusMessageIdin a group, whereFieldnever rendered it → a grouped Tokenizer with its ownstatusemits anaria-describedbypointing at nothing; AT ignores it, so nobody is harmed — butTextInput.tsx:348guards it with!inputGroupand 3 other siblings do too
Thanks for this — the group seam is the fiddly part and you got it right; I drove the grouped name, the RTL row and the disabled path in a browser and they're all clean.
Two things first.
I merged main in to test this (the branch was a month behind) and Lint now fails:
mergeRefs during render trips @astryx/no-unstable-merged-refs, which landed after
you branched. Typeahead uses useMergedRefs in the same place.
The bigger one is the single-line row. Six selections at the default leaves two off the edge — no scrollbar, no "+N", the row just ends mid-word:
Your own story sets maxEntries={3}, which dodges it.
tokenOverflowBehavior="unfocusedInline" already fits all six with nothing cut:
Should the grouped default just be unfocusedInline?
If you'd rather talk it through with someone, we're in Discord.
- packages/core/src/Tokenizer/Tokenizer.tsx:774 —
mergeRefsduring render trips the new lint rule;Typeahead.tsx:430usesuseMergedRefshere. - packages/core/src/Tokenizer/Tokenizer.tsx:311 — Hidden scrollbar plus nowrap leaves overflow with no signal at all.
- packages/core/src/Tokenizer/Tokenizer.tsx:725 — This id isn't rendered in a group.
TextInput.tsx:348guards it with!inputGroup.
- The changeset is honest:
patchtier with a[feat]body matches 8 of 8 sampled[feat]changesets on main, and it credits@AKnassaper convention. - Focused tests are green at the merged head: 88 passed across
Tokenizer.test.tsx(69) andInputGroup.test.tsx(19), in 8.8s. No full-suite run. - The author's stated worry about a clipped focus ring does not reproduce — 3px of clearance, and the browser scrolls the focused control into view.
TIME total 35m
setup 6m worktree + merge main (20s) + fast-install (2m, clone+reconcile) + one @astryxdesign/build build + storybook dev on 6265 (69s) (warm main reused: yes, as the BEFORE reference commit — no second checkout cut)
reading 8m brief, critic, loop mechanics, harness, the diff, Tokenizer, and the six sibling group-aware controls
measuring 11m 6 probe runs across 4 scripts, 7 frames with sensor receipts, 3 arm mutations (2 reverts + 1 overflow story), 1 focused vitest (88 tests, 8.8s), 1 local eslint
writing 9m 6 gates, 5 critic passes, wiki page + index row
waste 1m first probe run used the wrong story id (`inputgroup--with-tokenizer`, missing the `core-` prefix); one frame run asserted 3 tokens where the story's maxEntries cap lets the click path reach 2; two review-only story exports were appended before I found that storybook dev does not re-index new exports, so that arm had to be redone as an in-place edit of the existing story
- Whether the count of off-screen selections is announced to a screen reader — the
diff does not touch
useAnnounce, but I did not drive it, and it is the question finding 1 raises. - The ~12 CI checks that did not run on this head (a11y and visual acceptance among them). The original head ran a green Lint; this head's Lint is red for the reason in finding 2.
Not posted. Held as an internal draft under the community-PR rule: this adds core behaviour and has no maintainer approval, so the loop does not post or approve it whatever the verdict.
Five gates. The verdict, both blocking findings and the one note were correct in the first draft and never moved. Every gate corrected the evidence rather than the judgement:
| gate | what the critic caught |
|---|---|
| 1 | first presentation |
| 2 | R15-gate — a blocking visual claim in the posted text with no frame a reader could look at; R14e — an anchor pointing at a comment line rather than the code |
| 3 | R15-gate again, one sentence later — the remedy claim had no frame either; R16 — the WORST OUTCOME quote had been re-typed rather than quoted |
| 4 | R38 — one frame's expectation had been lowered after a sensor failure without the record saying so |
| 5 | clean |
Mirror note for whoever maintains the kit: Critic-Rules on this wiki has drifted
from the kit. The wiki carries two lines the kit does not, under R0, about
requesting removal of unnecessary executable additions rather than asking to run
them. The brief says the kit is the source of truth and a drifted mirror is a bug —
but the drift here is extra rule text on the wiki, so overwriting it from the kit
would delete someone's edit. Flagged rather than resolved; it needs whoever wrote
that line to put it in the kit. Review-Presentation also differs, in a path
placeholder only, which is that page's public-safe form and expected.