Review 5555
Review #5555 — busy indicator ownership
freddymeta · collaborator/internal bucket · OPEN · view on GitHub
#5555 fix(Typeahead,Tokenizer): the busy indicator is a Spinner in the field's end lane, not a clock on top of the clear button by freddymeta (bucket: collaborator, treated as internal)
a9d858eb014e744ea1e892be8e4fb00853f742b5
LOOP VERSION: 1.5.0
AUDIT RUBRIC: 1.12
LANE: full
WHY: intended pixel/layout change, a new public callback, a new Effect mirroring state, changed ARIA, a released base component, and an active stacked parent with changes requested.
Clear. The facebook/astryx head matched the requested SHA. The exact arm changes six source/test/story files only; no dependency, lockfile, workflow, script, or secret-reading surface changed. The worktree was isolated, the diff was read before execution, and no public PR action was taken.
WHY 1: An async Typeahead or Tokenizer displays a static clock in the same physical space as the clear control and never marks its combobox busy.
WHY 2: A person waiting on remote results can misread the clock as a time affordance, and part of the clear target is covered exactly when they may want to cancel.
WHY 3: Search-as-you-type needs unambiguous progress feedback without hiding either the query or its escape action.
USER-FACING PROBLEM: A person waiting on remote suggestions gets the wrong progress symbol over the clear affordance and no busy state on the combobox.
PROBLEM SEVERITY: harmful friction — the search and clear actions remain possible, but feedback is misleading and the clear target is partly obstructed.
VERDICT: clear
The search engine keeps owning whether a request is active and tells each field when that state changes. Each field then draws the standard Spinner in its own end lane, beside the clear action, while the input exposes aria-busy. That separates the two controls visually, but the reporting mechanism and the lane still have to preserve the public base component and the text area the lane covers.
SOLUTION (3 decisions · 93 runtime changed lines of 185 total changed lines)
- Replace the static clock with Spinner and move it from the search engine into each field's end lane — stated problems 1 and 2.
- Add
onLoadingChangeplus a state-mirroring Effect so wrappers can own placement — stated problem 2. - Add
aria-busyto the combobox while a request is active — stated problem 3.
BURDEN: medium — two wrapper states, one new Effect, one public callback, two changed visual layouts, browser/a11y state, and focused tests/stories.
BURDEN MATCH: disproportionate as written — the visual fix does not need to remove a released default or double wrapper renders.
VERDICT: BLOCKS — the mechanism fixes indicator/clear overlap but creates a query-lane collision, removes direct-Base feedback, and mirrors React state through an Effect.
OWNER: BaseTypeahead owns search lifecycle and input ARIA; Typeahead/Tokenizer own field chrome and the end lane.
TIER 1: Field/input anatomy and getInputARIA; canonical Spinner reused.
TIER 2: semantic processing indicator and status semantics.
SEAMS: Typeahead, Tokenizer with arbitrary endContent, direct public BaseTypeahead, minQueryLength cancellation, Tokenizer creation, selected-token edit mode.
BEHAVIOR UNIT: inline — loading is a small search-lifecycle state; the added Effect is cross-owner synchronization rather than an external-system boundary.
| seam | driven result |
|---|---|
| Typeahead, selected value + clear + long active query at 280px | Spinner and clear do not overlap; their absolute lane covers the focused input/caret |
Tokenizer, selected token + endContent + clear + long active query |
Spinner/clear do not overlap; Spinner overlaps the input by 14×14px |
direct released BaseTypeahead
|
parent has a visible named Loading status; head has only aria-busy and no visible/status node |
minQueryLength={3}, 3→2 while in flight |
loading/status clears, menu closes, stale request cannot restore it |
Tokenizer hasCreate + minQueryLength={3}
|
QA is offered and committed with zero source calls |
| keyboard clear during loading | clear remains clickable and focus returns to the combobox |
VERDICT: BLOCKS — ownership is split in a way that breaks the direct public seam and duplicates state across an Effect.
Typeahead and Tokenizer users get a standard animated Spinner, a fully clickable clear control, and aria-busy; RTL and reduced-motion behavior hold. In a narrow active query, those same users now type behind the absolute lane. A builder using the released BaseTypeahead directly gets no visible or named loading status unless they discover and adopt a new undocumented callback. Every async search also incurs extra wrapper commits, including re-rendering every Tokenizer token.
VERDICT: BLOCKS — the change improves the reported collision but introduces visible, semantic, and render-cost regressions on reachable paths.
Existing wrapper call sites remain unchanged. The exact new surface is:
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
BaseTypeaheadProps.onLoadingChange?: (isLoading: boolean) => void |
yes — Typeahead/index.ts and core barrel; BaseTypeaheadProps shipped in v0.5.0 |
nearest naming precedent is onOpenChange; no landed loading-state callback |
missing from en, zh, and dense BaseTypeahead.doc.mjs
|
finding |
OSSIFICATION: the callback is used only by the two wrappers in this arm. The active parent review already requires its adjacent wrapper-only queryEntries seam to remain internal; the same established __ pattern avoids publishing implementation plumbing. If this callback ships, removing it later is breaking; if the default indicator removal ships, every existing direct caller changes without opting in.
VERDICT: BLOCKS — new undocumented public surface accompanies a backward-incompatible default for a released component.
No theme target, CSS variable, token, or theme contract changes. The end lane uses logical positioning and existing spacing/size tokens; Spinner is the canonical processing visual required by Design Conventions, System Interaction States: “a spinner in the field's end slot; dimensions stay stable.”
VERDICT: clear
BEHAVIOR: wrapper search, selection, clear, threshold cancellation, and creation still work; direct BaseTypeahead no longer renders its prior loading status.
API: additive optional callback, but existing callers must adopt it to preserve default visual/status feedback.
VISUAL: intentional clock→Spinner and indicator/clear separation; unintended input/caret overlap grows from 0 to 14×14px under the Spinner and 0 to 17×20px under Typeahead's clear control in the measured busy state.
THEME: no changed target, state selector, token, or override.
VERDICT: BLOCKS — a released direct caller silently loses feedback, and narrow active query text is newly covered.
EFFECTS:
+ useEffect [isLoading, onLoadingChange] BaseTypeahead.tsx:579 — synchronizes React state to wrapper React state; no external system; each loading transition can report where it originates.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
useEffect [isLoading,onLoadingChange] :579 |
none | not established; all true/false transitions already have explicit setters | Typeahead commits start/settle 1→2 each; Tokenizer 2→4 / 1→2; token render calls at N=1/6/20 change 1/6/20→3/18/60 on start and 0→1/6/20 on settle |
component lifetime; no cleanup | none for callback/render cost |
RENDER: the Effect doubles Typeahead commits and doubles Tokenizer commits across a search cycle. Tokenizer's extra parent renders map every selected token; N=6 is the largest tracked Storybook case and N=20 proves linear scaling.
LISTENERS/OBSERVERS: no new listener or observer.
LAYOUT: no forced layout read in production; browser geometry was evidence only.
BUNDLE: Spinner import replaces Icon in Base and adds Spinner to two wrappers; no dependency change, exact stack has no bundle CI.
VERDICT: BLOCKS — a React-to-React mirror adds measured full-wrapper work on the per-search path.
VISUAL CHECK: manual frames required
WHY: pixels and layout intentionally change, the stack runs only three lightweight checks, and the new loading stories do not cover selected Typeahead + clear, direct BaseTypeahead, RTL, or a long narrow query.
Parent 94785a3 — busy |
Head a9d858e — busy |
Head — settled control |
|---|---|---|
 |
 |
 |
| Sensor | Expected | Observed | Pass? |
|---|---|---|---|
| Build | parent 94785a3; head a9d858e; identical review-only story arm |
exact SHAs; arm diff | yes |
| Story | review-pr5555--busy-pair |
exact id in all three receipts | yes |
| Theme / mode / direction | neutral / light / LTR | matched | yes |
| Viewport / media | 420×430@1; normal colors/motion; fine pointer; hover | matched | yes |
| Semantic busy state | same 46-character query in three fields; three unresolved searches; Typeahead focused | matched parent/head exactly | yes |
| Control state | same queries; all searches settled; Typeahead focused | matched | yes |
| Geometry | one visible 280px review root in viewport | matched; no Storybook/page errors | yes |
The intentional difference is the PR body's “Spinner in the one lane it already owns”: 340/380px² clock-clear overlap becomes zero and two 0.525s Spinner animations appear. The unintentional differences are that both wrapper inputs now extend under that lane and the direct Base status disappears. OCR of the opened frames shows the focused Typeahead's trailing query characters distorted under the controls; the settled control removes the Spinner but keeps the pre-existing clear-lane overlap.
Parent receipt · head receipt · control receipt · measurements
VERDICT: BLOCKS — the intended Spinner/clear separation is real, but so are the new query collision and missing direct-Base indicator.
Auto-covered: no exact-head pr-a11y/pr-rtl jobs ran because this is stacked; no baseline or locale file changed. The focused unit files passed 126/126 tests.
Checked in Chromium: wrapper comboboxes expose aria-busy=true; each has one visible status "Loading"; the direct Base accessibility snapshot loses that status at head; clear remains keyboard-focusable/clickable and restores combobox focus; ArrowDown/Escape do not drop focus; RTL mirrors lane order with zero Spinner/clear overlap; reduced-motion changes both Spinner durations from 0.525s to 3s.
Stack interaction: below threshold there is no search, busy state, status, or open menu; at three characters there is one search plus busy/status; backspacing 3→2 clears both; hasCreate commits QA below threshold with zero search calls.
Not verified: actual VoiceOver speech. Both parent and wrapper head insert a named status node with content rather than mutating a persistent node, so the pre-existing announcement-timing question is not charged to this arm.
VERDICT: BLOCKS — direct public BaseTypeahead users lose the named Loading status; wrapper behavior, direction, focus, and threshold/create interactions pass.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — three regressions from loading ownership |
| ARCHITECTURE | BLOCKS — direct seam broken, React state mirrored |
| IMPACT | BLOCKS — query occlusion, lost Base feedback, extra work |
| API | BLOCKS — public callback undocumented; released default changed |
| THEMING | clear |
| BREAKING | BLOCKS — visual/semantic behavior changes for existing calls |
| PERFORMANCE | BLOCKS — measured extra wrapper/token renders |
| VISUAL | BLOCKS — exact matched frame proves new input/lane collision |
| A11Y & I18N | BLOCKS — direct Base status disappears |
GOAL: partly met — exact Chromium proves Spinner replaces the clock, animates, carries the translated name, exposes aria-busy, and no longer overlaps clear; it also proves the new lane covers the live query and direct Base loses the status.
DISPOSITION: input/end-lane collision → blocks now; released Base default and undocumented callback → blocks now; React-state Effect and measured render growth → blocks now; missing changeset → blocks now as part of preserving/releasing the consumer-visible change.
ADVICE: bounded direction — let fields own presentation without covering the input, preserve direct Base's default visible/named busy feedback, keep wrapper-only transport internal, and report transitions without an Effect. Re-run the same exact-arm story at narrow LTR/RTL plus the render-count probe.
AUTHOR CAN PROCEED: yes — acceptance criteria are explicit and do not require a new design decision: zero query/lane overlap, unchanged default direct-Base status, no React-to-React Effect mirror, documented/release-noted public behavior, and the passing threshold/create/focus/RTL/reduced-motion controls.
WORST OUTCOME: “A builder using the released BaseTypeahead directly gets no visible or named loading status unless they discover and adopt a new undocumented callback.” → request changes.
JUDGEMENT NEEDED: none — this is a defect fix against existing public behavior; the adjacent active-stack review already establishes that wrapper-only Base props stay internal.
request changes
- The absolute field lane is not reserved from the input.
→ a person typing a long query in a narrow loading field has the caret and trailing characters covered by Spinner/clear ·
packages/core/src/Typeahead/Typeahead.tsx:514;packages/core/src/Tokenizer/Tokenizer.tsx:846 - Moving the status out of released
BaseTypeaheadchanges its default while publishing an undocumented callback. → a direct caller's user loses visible and named loading feedback without any call-site change ·packages/core/src/Typeahead/BaseTypeahead.tsx:174 - The loading callback is dispatched from an Effect that mirrors child state into parent state.
→ every remote search adds full wrapper renders; a 20-token Tokenizer invokes its token renderer 40 extra times at start and 20 at settlement ·
packages/core/src/Typeahead/BaseTypeahead.tsx:579
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]
-
packages/core/src/Typeahead/Typeahead.tsx:514— This lane covers the focused query. Can we reserve its rendered width from the input? -
packages/core/src/Typeahead/BaseTypeahead.tsx:174— This ships publicly without docs while direct callers lose the prior default status. -
packages/core/src/Typeahead/BaseTypeahead.tsx:579— This React-state mirror doubles wrapper commits on every loading transition.
- The shipped
BaseTypeaheadCustomSearchblock is a concrete direct consumer of the released public component. - The exact parent is the current #5385 head, and this commit changes a test introduced there.
- Spinner/clear geometry and action pass in both directions; reduced motion slows 0.525s rotation to 3s.
TIME total 42m
setup 5m — safety, two fresh wiki clones, exact-head worktree, fast install/build
reading 10m — kit 1.5.0, rubric 1.12, guidance, body, exact arm, source/history/stack
measuring 18m — 126 tests, matched builds, 12 browser scenarios, 3 visual captures, OCR, render scale
writing 9m — two drafts and two critic passes
waste 4m — first focused-test path matched no files; first capture counted persistent live regions; one short query was not decisive
- Actual VoiceOver speech; Chromium's accessibility tree and status-node lifecycle were inspected.
- Full CI: stacked PRs run only CLA/Vercel here; no
pr-a11y,pr-rtl, visual, build, lint, typecheck, or full-suite job ran at this head.
STACKED, not independent or competing. a9d858e's parent is exactly current #5385 head 94785a3; it changes the loading test added by that parent and relies on its 3→2 cancellation cleanup. The parent's standing request to make queryEntries internal touches adjacent Base props/destructuring and the Tokenizer call site, so this child will need a restack and likely conflict resolution after that fix.
- Focused Vitest: 126/126 passed across Typeahead and Tokenizer; two pre-existing Tokenizer
act(...)warnings. - Build helper:
@astryxdesign/buildpassed; Storybook compiled the review-only arm at both exact commits. - Browser: LTR/RTL, 280px, focus/ArrowDown/Escape/clear,
aria-busy, AX status tree, reduced motion, 3→2 threshold cancellation, and below-threshold creation all driven in Chromium. - Exact arm: harness diff · probe · measurements · focused test log.
Two critic passes completed. Pass 1 kept request-changes but caught an arithmetic overstatement (20→60 token renders is +40, not +60), “two regressions” naming three asks, and a docs ask that contradicted keeping the transport internal. Gate 2 corrected all three. Pass 2 found no violations: final public text is 139 words excluding image markup; each inline is ≤20 words; verdict, impact, anchors, independent confirmations, screenshot receipts, final ask, attribution, and public-safety scan all pass.
Nothing was posted to the PR. This page and its evidence assets are the required public fork-wiki record.
- Remote head at final check:
a9d858eb014e744ea1e892be8e4fb00853f742b5. - Current
origin/main:5b5f1903a8eeccde0e404062a44a168e81677279; no commits since the stack parent touch the six reviewed paths. - Dedicated worktree
review-5555-a9d858: clean before removal; removed; registry entry absent. - Review Storybook on port 6555: stopped; no listener remains.
- Public PR actions: none.