fix(Typeahead,Tokenizer): the busy indicator is a Spinner in the field's end lane, not a clock on top of the clear button - #5555
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Negative control, run after the fact rather than asserted — the same two stories and the same probe, against
Gates: 12285 tests pass, |
cixzhang
left a comment
There was a problem hiding this comment.
Thanks — the Spinner and aria-busy are the right direction, but moving loading out of BaseTypeahead leaves three regressions.
At 280px the absolute lane covers the live query: Typeahead’s Spinner overlaps 14×14px of the input and clear overlaps 17×20px (both zero on the parent); Tokenizer has the same Spinner/input overlap. The caret and trailing characters render under the controls while search runs. Could the input reserve the rendered lane width?
| Parent | Head | Settled control |
|---|---|---|
![]() |
![]() |
![]() |
BaseTypeahead is released. Direct callers previously got a visible, named “Loading” status; this head removes it and adds an undocumented public callback, so existing callers silently lose that feedback. Its Effect also doubles wrapper commits at search start and settlement. Could we preserve the default status, keep wrapper transport internal, report the transition without an Effect, and add a changeset?
[Reviewed by Robohands]
1eb979d to
404864d
Compare
|
All four are in, and you were right about the regression — thanks for measuring it. The overlap. My "pre-existing, filing separately" note was wrong in the state you tested. The in-flow indicator I removed was reserving its own width, so moving it into the lane is what took the reserve away while a search is out. The lane is out of flow and reserves nothing, and no CSS fixes that from the input's side: it cannot see a sibling's width, and a custom property set on the lane cannot travel sideways to it. So That also folds in the case I had punted, since it is the same missing reserve without a spinner in it. At 280px, overlap of the input's content box with each lane control, against a build of current
The released base. The Effect is gone. The report goes out at the call site through a ref, so the wrapper's Changeset added. One process note: #5385 landed while I was working on this, so the branch is replanted on |
|
Audited the change rather than just re-running my own tests. Two things fixed, three verified, one worth your judgement. Fixed
Verified
Your call, not a defect
One correction to my earlier comment: I claimed Selector, MultiSelector, CommandPalette and DateTimeInput were direct |
cixzhang
left a comment
There was a problem hiding this comment.
Thanks — the prior overlap, direct-Base, internal-transport, and changeset asks are fixed. The render-cost ask is still open: same-worktree A/B takes a 20-token search from 20 to 120 token renders, and disabling only the reserve returns it to 60. Could we keep the zero-overlap result without adding full-field commits at search start and settlement?
| Prior head | Current head |
|---|---|
![]() |
![]() |
[Reviewed by Robohands]
| return; | ||
| } | ||
|
|
||
| const observer = new ResizeObserver(entries => { |
There was a problem hiding this comment.
Please use the existing shared observer instead of allocating one per field lane.
…d's end lane, and the input reserves it Three defects in one block. The indicator a search painted was `<Icon icon="clock">` — a static glyph, byte-identical to TimeInput's, in a family where every other input paints busy with a Spinner and where `clock` otherwise means *time*. It was an in-flow item at the row's inline end, which is where each field independently parks its clear button, so the two landed on each other: 17x20px of overlap in Typeahead, 19x20px in Tokenizer, the latter leaving part of the clear glyph unclickable. And the combobox never carried `aria-busy`. The engine reports the busy state instead of painting it, and each field paints it in the one inline-end lane it already owns beside its clear button and end content. A direct `BaseTypeahead` caller keeps the visible, named status it has always had — as a Spinner now, so the fix reaches those callers too — and passing the callback is what hands the indicator over, so a field never renders two. The lane is out of flow (these wrappers wrap, and an in-flow sibling gets pushed onto a second row by a token), so it reserved nothing and the query ran underneath it at a narrow width. `useEndLaneReserve` measures the rendered lane and returns the padding the input needs. Measured rather than assumed, because what the lane holds varies with the field's state and, in Tokenizer, includes arbitrary `endContent`. At 280px, all six states measured, overlap of the input's content box with the lane's controls: | | main | here | |---|---|---| | Typeahead, value settled | clear 17px | 0 | | Typeahead, value + search in flight | 0 | 0 | | Tokenizer, value idle / settled | clear 25px | 0 | | Tokenizer, value + search in flight | 0 | 0 | The two rows that were already 0 were only 0 because the in-flow indicator reserved its own width; moving it out is what would have regressed them, and the reserve is what holds them. The other two are the pre-existing case of the same bug, with no spinner in it at all. Reporting goes out at the call site through a ref rather than from an Effect, so the field's state change batches into the commit React was already doing instead of forcing a second one, and it is edge-triggered, so the redundant clear on every keystroke below the query threshold reports nothing. Stacked on #5385 until it landed; replanted on main now that it has.
… during render Two audit findings, neither user-visible. The latest-callback ref was assigned during render. The repo's own convention splits on exactly this: plain values are assigned during render (`snapPointsRef` in BottomSheetPanel), but the latest-callback refs beside them — `onMotionStartRef`, `onMotionCompleteRef` — are synced in a layout effect, because a render React discards must not leave the ref pointing at a callback from the abandoned pass. This effect writes a ref and nothing else, so it commits nothing and no wrapper re-renders for it; the doubled commits the review asked about stay gone. And the ResizeObserver's fallback for a browser with no `borderBoxSize` read `contentRect`, which is the content box — equal today, since the lane has no padding or border, and silently short the moment one is added. `offsetWidth` on the observed element is border-box, like the primary path.
The reserve fixed the overlap and paid for it in commits: a 20-token
search went from 20 renders to 120, and the reserve alone accounted for
the second half. `useEndLaneReserve` held the measured width in state,
so the lane changing size — which it does exactly twice a search, as the
spinner arrives and as it leaves — re-rendered the whole field to carry
a number no JavaScript ever reads.
It reaches CSS as a custom property written straight to the field
wrapper and inherited by the input, so the padding follows the lane
without React seeing the value at all. The rule is static now: one
class, `calc(inset + var(--_astryx-end-lane-width, 0px))`, generated
once instead of regenerated per width.
Measured, same test either way — `Profiler` around the field, one search
start to settle:
state-held reserve 2 commits at search start (fails)
custom property 1 commit (passes)
which is the doubling, and it holds for the settle edge too. jsdom
reports every width as 0, so a state-held reserve never re-renders
there and the regression is invisible; the test stubs a ResizeObserver
that reports a width, which is the smallest thing that makes it
reproducible in CI.
The observer is shared as well — `observeResize`, the same singleton
`useTruncation` uses. Three fields on a page created three observers
before and create one now, so the browser dispatches one callback a
frame rather than N.
Verified in Chromium at 280px that the accepted fix still holds: while
busy, the input's content box ends at 268 and the lane starts at 276 —
zero overlap with the caret, and the reserve is released when the lane
goes.
1c34260 to
b3ea3b1
Compare
|
Render cost fixed — the reserve now costs zero commits of its own ( Root cause. Fix. The measurement reaches CSS as a custom property written straight to the field wrapper and inherited by the input. The rule is static now: One class, generated once, instead of a new one per width. React never sees the value. Measured, same test against both implementations —
The remaining two are the ones the search itself owes: the spinner arriving and the spinner leaving. Worth flagging how that test is built, because the obvious version does not work: jsdom reports every width as 0, so a state-held reserve never re-renders there and the regression is invisible. The test stubs a And the observer is shared. It now goes through The accepted fix still holds. Re-measured in Chromium at 280px while busy: and on settle the property is removed, so the input takes the room back rather than keeping a stale reserve. Changeset updated with both the property and the shared observer. |
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsField (@astryxdesign/core) · View in Storybook
Tokenizer (@astryxdesign/core) · View in Storybook
Typeahead (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: 1 accessibility violation(s) found — 1 serious. Tokenizer - 1 issue(s)
Visual Regression634 added · 0 removed. View the report To accept these exact frames: Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
|
/accept-visual The report lists 634 added, zero changed — new baselines for stories that had none, and not a single existing shot moved. That is the expected result: this round is a render-cost refactor (the lane width moved from React state to a CSS custom property), and the computed padding is identical either way. Verified in Chromium at 280px — content box ends 268, lane starts 276, same as before. |
|
Visual acceptance refused: copy the exact |
|
Re-verified on the current head (main merged, 4 commits), including the Tokenizer case I had not actually measured before. Tokenizer is the stronger proof, because its lane genuinely changes width — the story carries a token,
The padding tracks a 22px swing in both directions — Typeahead at 280px, for completeness: busy → content box ends 268, lane starts 276, overlap 0, Also confirmed, since these are easy to claim and easy to get wrong:
|








Summary
No longer stacked — #5385 landed, so this is replanted on
mainand reviewable on its own. Closes #5554.What was wrong
Three defects in one block, all on
main:BaseTypeahead.tsxrendered<Icon icon="clock" size="sm" color="secondary">— byte-identical toTimeInput.tsx, and in coreclockotherwise means time. Every sibling input paints busy with<Spinner size="sm" />. Nothing spun during a search:getAnimations({subtree: true})finds nothing running on it.flex: 1, so it was pushed to the row's inline end — which is where each wrapper independently parks an absolutely-positioned clear affordance. 17×20px at Typeahead md (13 of the 16px glyph covered) and 19×20px in Tokenizer, where the indicator painted over the button and left part of the ✕ unclickable.aria-busy, unlikeTextInput.The change
The engine reports the busy state rather than painting it, and each field renders
<Spinner size="sm" />in the one lane it already owns at its inline end — Tokenizer'sendSection, which already holdsendContentand the clear button, and Typeahead's clear-button box, widened into anendLaneflex row.aria-busygoes on the input.A caller using
BaseTypeaheaddirectly is unaffected: it still renders its own visible, named "Loading" status, as a Spinner now rather than the clock, so the fix reaches those callers too. Passing the callback is what hands the indicator over, so a field never paints two.The input reserves the lane. The lane is absolutely positioned — both wrappers are
flexWrap: 'wrap', so an in-flow sibling gets pushed onto a second row by a token — and an out-of-flow box reserves nothing, so at a narrow width the live query ran underneath it.useEndLaneReservemeasures the rendered lane and returns the padding the input needs. Measured rather than assumed: the lane holds a clear button that comes and goes with the value, an indicator that comes and goes with the search, and, in Tokenizer, arbitraryendContent. There is no CSS that does this — the input cannot see a sibling's width, and a custom property set on the lane cannot travel sideways to it.Measured in Chromium, at 280px
Overlap of the input's content box (where text and the caret may go) with each lane control, across the six states, against a build of current
main:The two in-flight rows were already 0 on
main— but only because the in-flow indicator reserved its own width. Moving it into the lane is what would have taken that away, which is the regression @cixzhang caught; the reserve is what holds them at 0. The other rows are the pre-existing case of the same bug, with no spinner in it at all — I had filed that away as separate and it belongs here, since it is the same missing reserve.Indicator, same probe:
Icon/0 animations on main →Spinner/1 animation here, both fields.aria-busyabsent on main →trueon both.Review changes since the last revision
BaseTypeaheadPropsis re-exported from the package entry point, so the base has callers this repo cannot see, and they painted no indicator of their own. It renders whenever no wrapper has taken it over.__onLoadingChange, marked@internal, following__queryEntriesfrom feat(core): minQueryLength — hold the typeahead search until the query is long enough #5385 andDefinedTheme.__inputTokens.Coverage
There was none — no story rendered either component loading, and neither test file exercised
isLoading, which is how a clock survived in the busy slot. Adds aLoadingstory to each (async source, 1.2s), the Tokenizer one withhasClearandendContentso all three lane occupants are present at once, plus unit tests for the three contracts jsdom can hold: the default status for a direct caller, the handover (and that the base then renders none), and the edge-triggered reporting.Test plan
pnpm build,pnpm lint:strict(0 errors; the same 80 warningsmainreports),pnpm check:repo,pnpm -F @astryxdesign/core typecheck— all clean.pnpm exec vitest run packages/core/src/Typeahead packages/core/src/Tokenizer: 132 tests pass.