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.
#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: contributor/collaborator, treated as internal)
1c3426021c4d46632d69dd8c1fc5d4affaa8cdae
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
LANE: full WHY: R1e re-review with a standing changes-requested review, intended visual/layout change, internal transport on a public props type, two added Effects, a ResizeObserver, and a measured performance claim.
WHY 1: An async Typeahead or Tokenizer used a static clock as progress, placed it over the clear action, and omitted the combobox busy state. WHY 2: A person waiting on remote results could misread the icon, lose part of the clear target, or type under end-lane controls. WHY 3: Search-as-you-type needs visible and assistive progress feedback without obscuring the query or its escape action. USER-FACING PROBLEM: A person waiting on remote suggestions gets ambiguous progress feedback in the same space as the query and clear control. PROBLEM SEVERITY: harmful friction — search remains possible, but progress, text, and cancellation are harder to perceive or use.
VERDICT: clear
The search engine continues to own whether a request is active. It tells each field when that state changes, and each field renders the standard Spinner in its own end lane while padding the input by the measured lane width. Direct users of the base engine keep its built-in loading status, and the input exposes aria-busy.
SOLUTION (3 decisions · 307 runtime changed lines of 540 total changed lines)
- Replace the clock with the canonical Spinner and add
aria-busy— stated defects 1 and 3. - Hand loading state to Typeahead and Tokenizer through an internal callback while preserving BaseTypeahead's default status — stated defect 2 and the prior review's direct-caller finding.
- Measure each absolute end lane and reserve that width from the input — stated defect 2 and the prior review's narrow-query finding.
BURDEN: high — two wrapper states, a layout effect, a cleanup effect, one observer per rendered lane, synchronous initial geometry, dynamic StyleX padding, and browser-only layout verification. BURDEN MATCH: disproportionate in its current performance shape — the user-facing fix works, but every search now drives whole-field and token-list commits that the parent did not.
VERDICT: BLOCKS — the corrected layout adds a measured render regression on every async search.
OWNER: BaseTypeahead owns search lifecycle and input ARIA; Typeahead/Tokenizer own field chrome; the shared resize-observer utility owns pooled element measurement.
TIER 1: Field/input anatomy, getInputARIA, Spinner, and observeResize/unobserveResize.
TIER 2: size cascade and processing-status representation.
SEAMS: direct BaseTypeahead, Typeahead, Tokenizer with arbitrary endContent, selected-token edit mode, threshold cancellation, LTR/RTL.
BEHAVIOR UNIT: useEndLaneReserve named hook — coherent boundary, but its browser path has no focused test and bypasses the existing pooled observer.
| seam | driven result |
|---|---|
| Typeahead, selected value + clear + long active query at 280px | current head reserves 43px; text ends 8px before the lane |
| Tokenizer, token + endContent + clear + active query | current head reserves 132px; text ends 3.72px before the lane |
| direct BaseTypeahead | visible named Spinner status preserved |
| 3→2 threshold cancellation | busy state clears; stale request cannot restore it |
| RTL and reduced motion | lane remains separated; direction mirrors; Spinner duration changes 0.525s→3s |
VERDICT: note — placement and seams work, but the helper should use the existing shared observer and needs focused browser coverage.
Typeahead and Tokenizer users now see the standard Spinner, retain a fully separate clear control, keep query text clear of the lane, and expose aria-busy; direct BaseTypeahead users keep their visible status. Builders pay a new render cost on every async search: a Tokenizer token renderer runs six times per token instead of once in the same-worktree comparison.
VERDICT: BLOCKS — the visible fix succeeds, but the render cost scales with every selected token.
The public call site is unchanged.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
BaseTypeaheadProps.__onLoadingChange?: (isLoading: boolean) => void |
emitted on the public props type, intentionally internal by __/@internal
|
__queryEntries is the landed wrapper-transport precedent |
intentionally omitted from caller docs | ok under the prior review's explicit internal-transport direction |
OSSIFICATION: no supported caller API is added. The double-underscore transport follows the adjacent landed convention; changing it later is internal cleanup rather than a documented consumer migration.
VERDICT: clear
No theme target, CSS variable, or token contract changes. Existing spacing/size tokens position the lane, and Design Conventions' Processing row explicitly calls for “a spinner in the field's end slot; dimensions stay stable.”
VERDICT: clear
BEHAVIOR: search, selection, clear, threshold cancellation, creation, and direct-Base loading feedback pass; wrapper render frequency increases. API: no supported public call site changes; the transport is an internal double-underscore prop. VISUAL: the intended clock→Spinner and lane separation hold; matched sensors show the prior head covered query text and this head does not. THEME: no target, token, selector, or override changes.
VERDICT: BLOCKS — behavior remains functionally correct, but the measured render contract regresses on each search transition.
EFFECTS:
-
+ useEffect []useEndLaneReserve.ts:61— cleanup for an element ResizeObserver; no state update inside the Effect; lifetime is mount→unmount; callback-ref null already disconnects. -
+ useIsomorphicLayoutEffect [__onLoadingChange]BaseTypeahead.tsx:472— syncs the latest callback into a ref before paint; no state update, listener, layout read, or extra commit from the Effect itself; no focused test.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
useEffect [] useEndLaneReserve.ts:61
|
ResizeObserver cleanup | cleanup must survive unmount; callback-ref null already handles node removal | the Effect sets no state; lane width state adds one full-field commit on mount and unmount | one Effect per field; disconnect on ref change and unmount | none for the real observer path |
useIsomorphicLayoutEffect [__onLoadingChange] BaseTypeahead.tsx:472
|
none; latest React callback ref | avoids a discarded render leaving an abandoned callback | source has one ref write and no state; same-worktree ablation isolates the extra commits to wrapper loading state and lane-width state | every callback identity change; no cleanup needed | callback-count unit test, no Effect-specific browser test |
RENDER: same worktree/browser/harness, parent→head commits are Typeahead 1+1→3+2 and Tokenizer 2+1→4+2 (search start+settle). Token renderer calls are N=1 1→6, N=6 6→36, N=20 20→120. Disabling only the reserve returns them to 3/18/60, independently confirming half the head cost comes from width state; the remaining half is the wrapper loading mirror. maxEntries is optional, so selected-token N is consumer-grown; the largest checked-in fixed composition has four Tokenizers on one page.
LISTENERS/OBSERVERS: one new ResizeObserver per rendered lane; it disconnects on node replacement and unmount. sharedResizeObserver.ts already pools this class of observation.
LAYOUT: one synchronous getBoundingClientRect() on lane mount; observer callbacks normally consume borderBoxSize, with offsetWidth only as the fallback.
BUNDLE: no dependency added; exact-head build and dependency checks are green.
VERDICT: BLOCKS — the same prior-review render-cost concern remains: a 20-token search multiplies token renders 6×.
VISUAL CHECK: manual frames required WHY: the PR intentionally changes the busy indicator and end-lane geometry, and the prior review's blocking visual state had to be re-run at the new head.
| Prior reviewed head | Current head |
|---|---|
 |
 |
SENSOR RECEIPT: prior a9d858eb014e744ea1e892be8e4fb00853f742b5 and current 1c3426021c4d46632d69dd8c1fc5d4affaa8cdae; story review-pr5555--busy-pair; neutral/light/LTR; 420×430@1; focused Typeahead with the same 46-character pending query; target box 256×56 at (44,44); one running animation; zero Storybook/page errors. Only Build and the asserted outcome differ: textCovered true→false. Both frames have passing JSON receipts and an identical review-arm diff.
I invoked the image reader for both PNGs, but this session's renderer returned only file metadata and explicitly did not show the pixels. Geometry and sensor assertions pass; visual judgement itself was not available.
VERDICT: not looked at — the client could not render the captured PNGs to this reviewer.
Exact-head Chromium exposes aria-busy="true" on all three comboboxes and a translated, named status "Loading" for Typeahead, Tokenizer, and direct BaseTypeahead. Clear keeps focus on the combobox; LTR/RTL interaction passes; reduced motion changes Spinner duration from 0.525s to 3s. pr-a11y and pr-rtl both ran and passed on this head with no baseline change.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — disproportionate render cost |
| ARCHITECTURE | note — bypasses pooled observer; browser path lacks focused test |
| IMPACT | BLOCKS — token work scales 6× |
| API | clear |
| THEMING | clear |
| BREAKING | BLOCKS — render frequency regresses |
| PERFORMANCE | BLOCKS — parent→head 20→120 token renders |
| VISUAL | not looked at — PNG renderer unavailable; sensors/geometry pass |
| A11Y & I18N | clear |
R1e: cixzhang’s prior review requested lane reservation, preserved direct-Base feedback, internal transport, no Effect-driven state mirror, a changeset, and a render-count rerun. This head satisfies the first five; the rerun shows the render-cost concern remains.
GOAL: partly met — the prior overlap, direct-Base status, internal transport, no-state-in-Effect, and changeset asks are addressed; the prior render-cost concern remains under a new state owner. DISPOSITION: wrapper/lane render multiplication → blocks now; per-instance observer and missing focused browser test → included in the same correction rather than a separate follow-up; unavailable pixel rendering → recorded, with passing exact-state sensors and no approval claim. ADVICE: bounded direction — preserve zero text/lane overlap while removing the full-wrapper commits caused by loading and lane-width state; the existing shared observer settles pooling, but the state-free update shape remains the author’s choice. AUTHOR CAN PROCEED: yes — keep the current visual, direct-Base, ARIA, threshold, and clear behavior, while removing full-wrapper commits caused by loading and lane-width mirrors; re-run the banked N=1/6/20 probe. WORST OUTCOME: “a Tokenizer token renderer runs six times per token instead of once in the same-worktree comparison.” → request changes.
JUDGEMENT NEEDED: none — this is a measured regression against an existing contract, not a new API or design choice.
request changes
- [BLOCKS] loading and lane width are mirrored into wrapper React state.
→ every async search re-renders all selected tokens six times instead of once; at N=20 that is 20→120 calls ·
packages/core/src/Tokenizer/Tokenizer.tsx:472;packages/core/src/Field/useEndLaneReserve.ts:58 - [not blocking] the new helper allocates its own observer instead of the shared pool.
→ a page with several fields creates one observer per visible end lane ·
packages/core/src/Field/useEndLaneReserve.ts:88
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]
-
packages/core/src/Field/useEndLaneReserve.ts:88— Please use the existing shared observer instead of allocating one per field lane.
- Exact-head targeted Vitest: 130/130 pass; two pre-existing Tokenizer
act(...)warnings. Core typecheck passes. - All 19 named CI checks passed at the reviewed head;
pr-a11y,pr-rtl, Stable visual regression, build, test, and lint all ran. - Current
mainchanged onlyInputClearButtoninternals in this review's paths; GitHub's generated merge ref is conflict-free.
TIME total 17m setup 3m isolated worktree + APFS-cloned install + Storybook 6555 (warm main reused: yes) reading 5m kit, rubric 1.13, prior review, body, diff, source, history measuring 6m 130 tests, 8 browser paths, matched frames, 3 A/B perf arms, 1 re-measure writing 3m presentation + two critic passes + wiki record waste 3m initial checkout used FETCH_HEAD after a multi-ref fetch; two sensor-script callback mistakes
- The captured PNG pixels: the image reader returned metadata but did not render media in this session.
- VoiceOver speech; Chromium accessibility state and status-node lifecycle were checked.
VERDICT CHECK: correct — the same performance concern from the prior review remains, independently measured at the current head. LENGTH: 58 words excluding image markup → ok for request-changes. VIOLATIONS: none. WOULD SHE HAVE POSTED IT: yes — it thanks the author, reconciles every prior ask, gives the measured user/library cost, asks for an outcome the evidence proves, and keeps the shared-observer note line-anchored. REWRITE: none.
The clean gate separates a proven performance acceptance criterion from an implementation choice the review did not build.