Skip to content

Review 5455

Jump to bottom
Cindy Zhang edited this page Aug 30, 2026 · 2 revisions

Review 5455 — replaceable busy-indicator spike

PR: #5455 · author cixzhang Posted review: https://github.com/facebook/astryx/pull/5455#pullrequestreview-5062038304

PRESENTATION

PR

#5455 spike(Indicator): make the loading visual replaceable — a stateless 'busy' family, by cixzhang (bucket: the maintainer)

HEAD REVIEWED

42e3a3bebc261a928edc862e440d3167ddc8434a

VERSIONS

LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.15

PRIOR REVIEW

Gate 12 reviewed 4247ceab and posted “Approve once you undraft” twice. The owner’s stronger direction on the PR remains authoritative: pursue the replaceable busy-indicator architecture, keep it a spike, and do not treat accepted direction as merge readiness. This gate extends that direction and does not call the PR merge-ready.

LANE

LANE: full WHY: new public indicator family/registry surface, 39 files, 11 commits, broad host adoption, theme behavior, and AT-facing semantics. A spike is allowed to carry the exploration; it is not eligible for a fast or merge-ready conclusion.

PROBLEM

WHY 1: A product cannot replace Astryx’s loading ring without replacing or forking each host that renders it. WHY 2: Builders either accept a visibly off-brand loading state or own divergent copies of controls they otherwise want Astryx to maintain. WHY 3: A themeable system with a hardcoded working state creates a recurring exception at one of the most visible moments in an interaction.

USER-FACING PROBLEM: A builder applying a product identity across Astryx cannot replace the loading visual through the theme, so users see a system-specific ring or the builder forks controls. PROBLEM SEVERITY: missing capability — current controls work, but the accepted theming job has no supported path. NEW FEATURE CASE: owner-backed; the PR’s 27 Aug direction comment explicitly accepts pursuing this capability while keeping it a spike. EARLY STOP: clear — need and ownership are settled for exploration.

VERDICT: clear

SOLUTION

Astryx resolves one stateless “busy picture” from the indicator registry, and loading hosts ask for that picture rather than constructing a ring. Hosts keep the meaning of busy; the indicator stays decorative. The rebase preserves newer main behavior around Button interaction and translated ChatToolCalls status text, and extends the same seam to the touch DateTimeInput host that main added after the prior review.

SOLUTION (7 architecture decisions · ~465 runtime lines; 39 files, +934/−423)

  1. Add a stateless busy indicator family and spinner registry entry.
  2. Move busy semantics to hosts while standalone Spinner keeps status semantics.
  3. Let each indicator family define its state and size prop domain.
  4. Pass host color through private spinner vars rather than a public shade prop.
  5. Preserve the shipped spinner theme target as a legacy alias.
  6. Keep arc and track theme vars independent.
  7. Give non-interactive running ChatToolCalls rows host-owned busy/status semantics.

The new TouchDateTimeField edit is not an eighth decision. It is one additional adoption of decision 1 after main introduced that host.

BURDEN: high — a new registry member, public export/theme target, 24+ host render sites, host semantics, private var plumbing, and broad regression surface. BURDEN MATCH: acceptable only as a spike — the capability is demonstrated, but this burden cannot be called production-ready without a smaller shipping plan and explicit layout/release contracts.

VERDICT: note — coherent exploration, intentionally not a merge candidate.

ARCHITECTURE

OWNER: the indicator registry owns the busy picture; each host owns busy semantics; standalone Spinner owns its own status semantics. TIER 1: useIndicator / getIndicator / defineTheme({indicators}), themeProps, existing host ARIA. TIER 2: family-specific size and semantic indicator roles. SEAMS: theme replacement, public spinner alias, host private color vars, standalone Spinner, in-control host layout, newly added touch DateTimeInput. BEHAVIOR UNIT: inline — no state machine or Effect; resolution stays in the existing registry. COMPLEXITY BUDGET: one picture owner, one semantics owner per host, one registry representation, zero parallel loading protocols. ACTUAL BURDEN: 39 files; ~465 runtime lines; one new family/entry/export; one registry source; 26 rendered BusyIndicator sites including the registry implementation and standalone wrapper; 12 focused test files / 595 tests. BURDEN TREND: first reviewed head 4247ceab → current 42e3a3be: model flat; one main-added host adopted, while newer main status/interaction behavior was retained rather than reimplemented. RESET TRIGGER: not triggered — no second registry, owner, or policy representation was added during the rebase.

domain fact one authoritative writable source generated / immutable projections other writable copies
busy visual implementation indicator registry spinner entry BusyIndicator resolution in hosts none
host busy meaning each host’s existing state/ARIA decorative BusyIndicator none
standalone status meaning Spinner wrapper resolved indicator picture none

Rebase integration checks:

  • Button keeps main’s inactive vs explicitly disabled visual split and the BusyIndicator overlay.
  • ChatToolCalls keeps main’s translated Pending/Running/Complete/Failed hidden text plus the spike’s host-owned aria-busy/status role.
  • DropdownMenuSubMenu keeps main’s viewport, overflow, and RTL work plus BusyIndicator and aria-busy.
  • TouchDateTimeField, added on main after gate 12, now uses BusyIndicator; no runtime <Spinner> host remains outside Spinner/Indicator docs and tests.

VERDICT: clear for the spike architecture; productionization remains intentionally unsettled.

IMPACT

Unthemed users keep the same Button ring colors and 89.21875×32 host box. A theme author can still recolor the arc without recoloring the track. Screen-reader users retain main’s translated “Running” text while the row exposes busy status. Touch DateTimeInput now follows the same replacement seam instead of becoming the one new host that ignores it.

VERDICT: clear

API

Real exploratory call site:

defineTheme({name: 'brand', indicators: {spinner: BouncingDots}})

The rebase adds no new API beyond the spike’s existing family, export, registry entry, alias, and family-size map. No changeset is present, correctly for a spike and necessarily before any shipping PR.

OSSIFICATION: high. The busy family, spinner registry key, legacy target, and size contract become permanent if shipped. This gate does not accept them for release; it only preserves the already accepted exploration.

VERDICT: note — valid spike surface, not accepted release surface.

THEMING

Fresh Chromium measurements at current head and the 2da7a39 runtime baseline (the final rebase base 0f462449 differs only by a documentation spec):

case main head
default Button track white @ 0.3 white @ 0.3
default Button arc white @ 1 white @ 1
theme sets only --spinner-color white track, pink arc white track, pink arc
Button box 89.21875×32 89.21875×32

The public arc and private host slots remain independent. No duplicate target/import/key appeared in the rebase.

VERDICT: clear

BREAKING

BEHAVIOR: main’s Button loading/disabled behavior and ChatToolCalls translated status text are preserved; TouchDateTimeInput changes only which implementation supplies the same visual. API: spike surface remains unreleased and intentionally without a changeset. VISUAL: computed painted strokes and host geometry match current main for default and arc-only theme arms. THEME: existing spinner target continues to reach the arc and track independently.

VERDICT: note — the spike still explores permanent surface; no new rebase regression found.

PERFORMANCE & RESOURCES

EFFECTS: zero added or changed by the rebase/maintenance fix. RENDER: existing spike cost remains one theme-registry resolution per busy host; the new host adds no new mechanism. LISTENERS/OBSERVERS: none added. LAYOUT: no read/write or forced reflow; fresh Button geometry matches main. BUNDLE: still unmeasured for the overall spike; this is one reason it remains non-merge-ready.

VERDICT: not looked at — overall bundle delta remains unmeasured; no new runtime cost was added by maintenance.

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: this spike changes replaceable painted content and claims unchanged default pixels.

Four current-main/current-head Chromium captures produced passing sensor receipts. The receipts match on story, theme, mode, direction, 420×140@1 viewport, media, aria-busy, circle count, target count, geometry, fonts, and no Storybook error; only Build differs. The captured PNGs were not manually inspected in this maintenance pass, so VISUAL remains not looked at. Computed real-browser strokes and geometry are recorded under THEMING.

VERDICT: not looked at — captured and sensor-valid, but not visually inspectable in this run.

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

No local a11y audit was run. Focused tests retain main’s translated status labels and host semantics. A real Chromium accessibility-tree probe on core-chattoolcalls--running reports:

  • DOM busy carrier: role=status, with Running in its content;
  • exposed AX node: status, busy=1;
  • exposed StaticText / InlineTextBox: Running.

This closes gate 12’s “nothing says Loading any more” note through newer main code without reimplementing it. A real AT announcement transition was not measured, and the spike remains non-merge-ready.

VERDICT: note — translated state text and busy exposure survive; real AT timing remains unverified.

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION note — coherent spike, high shipping burden
ARCHITECTURE clear for exploration
IMPACT clear
API note — unreleased spike surface
THEMING clear
BREAKING note
PERFORMANCE not looked at — bundle
VISUAL not looked at — pixels not rendered to reviewer
A11Y & I18N note — AT timing unverified

GOAL: met for maintenance — the replaceable busy-indicator direction survives current main; main’s Button and ChatToolCalls fixes survive; the only new busy host is now on the seam; focused tests and typecheck pass.

DISPOSITION:

  • Gate 12’s missing status word → closed by main’s translated ChatToolCalls labels, preserved in the rebase.
  • New TouchDateTimeField direct Spinner → fixed by applying the existing BusyIndicator seam.
  • Unmeasured bundle, uninspected frames, AT timing, changeset/docs, and layout-box contract → accepted only as reasons the spike stays a spike; not accepted for shipping.

ADVICE: omitted — no new design direction was requested or chosen.

AUTHOR CAN PROCEED: yes — continue exploration on this spike; no merge/undraft action is implied.

WORST OUTCOME: shipping the exploratory registry/layout contract as permanent surface without the missing evidence → incompatible with a merge-ready verdict, so this review does not give one.

JUDGEMENT NEEDED: none for this maintenance pass. Before productionizing, the owner still needs to settle the indicator’s layout box and release contract, then supply bundle/visual/AT evidence.

ENGINEERING VERDICT: maintenance clean; keep as spike. MECHANICAL POSTED VERDICT: comment.

REVIEW

Rebase is clean and preserves the replaceable busy-indicator direction. Main’s new status labels and Button behavior survive, and the new touch DateTimeInput uses the replacement seam. This remains a spike, not merge-ready.

[Reviewed by Robohands]

INLINE

None.

VERIFICATION

  • Rebase with rerere disabled; two content conflicts reconciled by retaining both main behavior and the spike seam.
  • Range-diff: 8/10 original commits patch-identical; two differ only where newer main added ChatToolCalls status text/tests and moved DateTimeInput context; one maintenance commit adopts TouchDateTimeField.
  • Focused Vitest: 12 files, 595 tests passed.
  • Core TypeScript typecheck passed.
  • pnpm check:repo passed from the maintenance commit hook.
  • Focused ESLint: 0 errors, 8 pre-existing warnings; no new warning in the maintenance file.
  • Public-repo guard and git diff --check passed.
  • Chromium Button paint/geometry, screenshot sensors, and ChatToolCalls AX probe passed.
  • No local a11y audit was run.

TIME

TIME total 25m

  • setup 4m — worktree, fast install, two current-main refreshes
  • reading 6m — prior gate, current main overlaps, both sides of conflicts
  • rebase/fix 5m — two conflict resolutions plus one newly added host
  • measuring 7m — 595 tests, typecheck, Chromium paint/AX, four receipts
  • writing 3m — gate and critic pass
  • waste 10m — a broad legacy color probe hung across stale story cases and was replaced by the decisive Button matrix; fast-moving documentation lands forced two no-overlap rebases

WHAT I COULD NOT VERIFY

  • Bundle delta.
  • Real assistive-technology announcement timing.
  • Human visual inspection of the captured PNGs was not completed in this maintenance pass.

Remote CI at 42e3a3be: build, sandbox, Storybook, test, lint, smoke, dependency, theme layers, fixture contrast, a11y, RTL, and stable visual regression all passed. visual-acceptance alone is red because stable visual evidence could not be published, the same evidence-publication gap already present before this rebase. The PR remains draft.

CRITIC PASS

#5455

VERDICT CHECK: correct — comment/keep-as-spike. The owner accepted the direction but explicitly withheld merge readiness; missing bundle, visual inspection, and AT evidence make an approval inappropriate. LENGTH: 32 words before attribution → within the comment/request-change cap. VIOLATIONS: none. WOULD SHE HAVE POSTED IT: yes — it leads with the maintenance result, names the three consequential integrations, and ends with the spike boundary rather than an invitation to merge. REWRITE: not required.

Template gate: all required slots, architecture budget/source/reset fields, visual obligation, remedy checkpoint, three judgement gates, author-can-proceed, and version pair are present. Prior review is reconciled; no design question is pushed to the author; the public review contains no internal context, local path, gate mechanics, or private control syntax; attribution is exact.

PATTERN

The only correctness gap introduced by newer main was one new busy host outside the replacement seam. Applying the existing seam was a maintenance fix, not a new architecture decision.

GATE 13: CLEAN FOR MAINTENANCE; THE PR REMAINS A SPIKE.

WHAT CINDY CHANGED BEFORE POSTING

Nothing; the current-head maintenance review was posted exactly as drafted.

Clone this wiki locally