Repository navigation
fix(Spinner): route standalone loading through the shared announcement channel - #7910
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
community
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| ChatToolCalls | 27.7 kB (+1.7 kB) · 8.2 kB CSS (0) | — | — · — · — |
| Spinner · Storybook | 20.3 kB (+1.7 kB) · 5.7 kB CSS (0) | 351 (+24) | — · — · — |
| ChatMessageList | 23.8 kB (+884 B) · 6.8 kB CSS (0) | — | — · — · — |
110 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DropdownMenuSubMenu | 60.5 kB (+850 B) · 9.3 kB CSS (0) | — | — · — · — |
| CommandPaletteInput | 27.8 kB (+844 B) · 7.4 kB CSS (0) | — | — · — · — |
| lab/EmojiPicker · Storybook | 73.1 kB (+590 B) · 751 B CSS (+29 B) | 3,595 (+14) | — · — · — |
| lab/CommentThread | 268.8 kB (+541 B) · 2.4 kB CSS (+14 B) | — | — · — · — |
| lab/Comments | 274.3 kB (+525 B) · 2.5 kB CSS (+15 B) | — | — · — · — |
| lab/CommentComposer | 109.4 kB (+412 B) · 1.9 kB CSS (+25 B) | — | — · — · — |
| richtext/RichTextEditor | 284.0 kB (+395 B) · 2.6 kB CSS (0) | — | — · — · — |
| CheckboxInput | 54.1 kB (+392 B) · 8.9 kB CSS (0) | — | — · — · — |
| SideNav | 76.7 kB (+391 B) · 10.1 kB CSS (0) | — | — · — · — |
| PowerSearch | 260.8 kB (+379 B) · 22.6 kB CSS (0) | — | — · — · — |
| ChatSendButton | 47.6 kB (+375 B) · 8.7 kB CSS (0) | — | — · — · — |
| lab/DiffViewer | 73.5 kB (+373 B) · 2.1 kB CSS (0) | — | — · — · — |
| ChatDictationButton | 47.8 kB (+369 B) · 8.8 kB CSS (0) | — | — · — · — |
| Markdown | 154.1 kB (+369 B) · 19.2 kB CSS (0) | — | — · — · — |
| TableSelectionToolbar | 56.7 kB (+369 B) · 11.8 kB CSS (0) | — | — · — · — |
| IconButton | 47.2 kB (+368 B) · 8.7 kB CSS (0) | — | — · — · — |
| Calendar | 58.1 kB (+365 B) · 9.3 kB CSS (0) | — | — · — · — |
| richtext/RichTextView | 264.3 kB (+365 B) · 2.3 kB CSS (0) | — | — · — · — |
| Button | 47.2 kB (+364 B) · 8.7 kB CSS (0) | — | — · — · — |
| ChatLayoutScrollButton | 47.7 kB (+364 B) · 8.9 kB CSS (0) | — | — · — · — |
| lab/TransferListSelector | 87.5 kB (+364 B) · 1.9 kB CSS (0) | — | — · — · — |
| ComplexSelector | 58.0 kB (+362 B) · 10.4 kB CSS (0) | — | — · — · — |
| TopNavMenu | 61.4 kB (+362 B) · 9.9 kB CSS (0) | — | — · — · — |
| FileInput | 55.7 kB (+361 B) · 9.6 kB CSS (0) | — | — · — · — |
| Thumbnail | 51.5 kB (+360 B) · 10.2 kB CSS (0) | — | — · — · — |
| MobileNavToggle | 47.5 kB (+359 B) · 8.7 kB CSS (0) | — | — · — · — |
| SideNavCollapseButton | 48.0 kB (+359 B) · 8.7 kB CSS (0) | — | — · — · — |
| ToggleButton | 48.1 kB (+359 B) · 8.8 kB CSS (0) | — | — · — · — |
| BaseTypeahead | 57.5 kB (+357 B) · 9.6 kB CSS (0) | — | — · — · — |
| TabMenu | 58.6 kB (+357 B) · 9.8 kB CSS (0) | — | — · — · — |
| ChatComposer | 63.7 kB (+356 B) · 10.5 kB CSS (0) | — | — · — · — |
| Lightbox | 55.3 kB (+356 B) · 9.1 kB CSS (0) | — | — · — · — |
| AlertDialog | 90.6 kB (+355 B) · 13.1 kB CSS (0) | — | — · — · — |
| lab/TransferList | 80.4 kB (+355 B) · 1.7 kB CSS (0) | — | — · — · — |
| Banner | 49.2 kB (+354 B) · 9.2 kB CSS (0) | — | — · — · — |
| DialogHeader | 50.0 kB (+353 B) · 10.2 kB CSS (0) | — | — · — · — |
| DrawerHeader | 50.2 kB (+353 B) · 11.6 kB CSS (0) | — | — · — · — |
| Typeahead | 65.5 kB (+350 B) · 11.2 kB CSS (0) | — | — · — · — |
| MobileNav | 57.0 kB (+349 B) · 9.3 kB CSS (0) | — | — · — · — |
| SideNavHeading | 62.0 kB (+348 B) · 10.0 kB CSS (0) | — | — · — · — |
| Stepper | 48.8 kB (+347 B) · 8.8 kB CSS (0) | — | — · — · — |
| TextInput | 54.9 kB (+345 B) · 9.9 kB CSS (0) | — | — · — · — |
| LayerProvider | 64.9 kB (+344 B) · 9.4 kB CSS (0) | — | — · — · — |
| ChatLayout | 50.5 kB (+342 B) · 9.2 kB CSS (0) | — | — · — · — |
| DateTimeInput | 187.0 kB (+340 B) · 17.2 kB CSS (0) | — | — · — · — |
| TopNavHeading | 61.4 kB (+339 B) · 9.8 kB CSS (0) | — | — · — · — |
| NumberInput | 58.3 kB (+338 B) · 10.1 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 84.1 kB (+338 B) · 16.5 kB CSS (0) | — | — · — · — |
| Toast | 61.1 kB (+338 B) · 9.0 kB CSS (0) | — | — · — · — |
| TimeInput | 141.1 kB (+336 B) · 14.5 kB CSS (0) | — | — · — · — |
| TableCell | 84.6 kB (+335 B) · 16.7 kB CSS (0) | — | — · — · — |
| ContextMenu | 83.4 kB (+333 B) · 16.3 kB CSS (0) | — | — · — · — |
| SideNavItem | 57.1 kB (+333 B) · 9.8 kB CSS (0) | — | — · — · — |
| Switch | 90.5 kB (+333 B) · 9.5 kB CSS (0) | — | — · — · — |
| TopNavMegaMenu | 56.5 kB (+333 B) · 10.6 kB CSS (0) | — | — · — · — |
| CodeBlock | 61.1 kB (+332 B) · 10.0 kB CSS (0) | — | — · — · — |
| TextArea | 49.6 kB (+331 B) · 8.8 kB CSS (0) | — | — · — · — |
| CommandPalette | 59.0 kB (+329 B) · 12.3 kB CSS (0) | — | — · — · — |
| BreadcrumbItem | 71.8 kB (+328 B) · 11.3 kB CSS (0) | — | — · — · — |
| ChatComposerInput | 61.6 kB (+328 B) · 10.0 kB CSS (0) | — | — · — · — |
| ListItem | 59.0 kB (+328 B) · 10.7 kB CSS (0) | — | — · — · — |
| lab/ChatEmojiPicker | 69.4 kB (+328 B) · 773 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 70.0 kB (+328 B) · 1.1 kB CSS (0) | — | — · — · — |
| ChatComposerTokenElement | 61.6 kB (+327 B) · 10.0 kB CSS (0) | — | — · — · — |
| DateRangeInput | 76.2 kB (+327 B) · 11.2 kB CSS (0) | — | — · — · — |
| lab/Schedule · Storybook | 64.2 kB (+327 B) · 3.9 kB CSS (0) | 4,807 (+5) | — · — · — |
| CheckboxListItem | 60.0 kB (+324 B) · 10.7 kB CSS (0) | — | — · — · — |
| DateInput | 164.3 kB (+323 B) · 15.5 kB CSS (0) | — | — · — · — |
| AppShell | 76.6 kB (+322 B) · 12.2 kB CSS (0) | — | — · — · — |
| Pagination | 93.5 kB (+321 B) · 16.6 kB CSS (0) | — | — · — · — |
| Table | 92.8 kB (+321 B) · 16.9 kB CSS (0) | — | — · — · — |
| Carousel | 53.1 kB (+319 B) · 9.5 kB CSS (0) | — | — · — · — |
| TopNav | 58.6 kB (+314 B) · 9.6 kB CSS (0) | — | — · — · — |
| charts/Chart | 169.1 kB (+311 B) · 494 B CSS (0) | — | — · — · — |
| Tokenizer · Storybook | 75.1 kB (+311 B) · 17.1 kB CSS (0) | 2,223 (+3) | — · — · — |
| Popover | 55.7 kB (+309 B) · 12.8 kB CSS (0) | — | — · — · — |
| lab/ListInput | 85.8 kB (+307 B) · 1.6 kB CSS (0) | — | — · — · — |
| MoreMenu | 78.5 kB (+304 B) · 16.4 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 234.4 kB (+302 B) · 454 B CSS (0) | — | — · — · — |
| MultiSelector | 80.6 kB (+301 B) · 15.6 kB CSS (0) | — | — · — · — |
| Selector | 83.5 kB (+301 B) · 16.2 kB CSS (0) | — | — · — · — |
| lab/TourStep | 61.6 kB (+299 B) · 590 B CSS (0) | — | — · — · — |
| DropdownMenu | 78.2 kB (+290 B) · 16.4 kB CSS (0) | — | — · — · — |
| charts/ChartTooltip | 146.3 kB (+281 B) · 423 B CSS (0) | — | — · — · — |
| lab/CommentPin | 48.7 kB (+214 B) · 1.2 kB CSS (0) | — | — · — · — |
| EmptyState | 6.6 kB (+207 B) · 485 B CSS (0) | — | — · — · — |
| Link | 44.5 kB (+206 B) · 6.8 kB CSS (0) | — | — · — · — |
| AvatarGroupOverflow | 40.5 kB (+191 B) · 5.4 kB CSS (0) | — | — · — · — |
| Slider | 92.6 kB (+187 B) · 8.3 kB CSS (0) | — | — · — · — |
| RadioList | 43.5 kB (+185 B) · 6.4 kB CSS (0) | — | — · — · — |
| RadioListItem | 56.9 kB (+185 B) · 9.5 kB CSS (0) | — | — · — · — |
| InfoTip | 38.5 kB (+184 B) · 5.1 kB CSS (0) | — | — · — · — |
| StatusDot | 23.3 kB (+184 B) · 5.2 kB CSS (0) | — | — · — · — |
| Tooltip | 21.0 kB (+183 B) · 3.9 kB CSS (0) | — | — · — · — |
| AvatarGroup | 37.9 kB (+182 B) · 5.5 kB CSS (0) | — | — · — · — |
| Field | 42.1 kB (+180 B) · 6.3 kB CSS (0) | — | — · — · — |
| lab/SelectableCardGroup | 52.2 kB (+180 B) · 162 B CSS (0) | — | — · — · — |
| InputGroup | 44.3 kB (+179 B) · 7.3 kB CSS (0) | — | — · — · — |
| FieldLabel | 38.2 kB (+178 B) · 4.8 kB CSS (0) | — | — · — · — |
| SegmentedControl | 84.6 kB (+177 B) · 6.7 kB CSS (0) | — | — · — · — |
| Avatar | 35.2 kB (+176 B) · 5.4 kB CSS (0) | — | — · — · — |
| CheckboxList | 45.8 kB (+162 B) · 7.5 kB CSS (0) | — | — · — · — |
| charts/ChartLegend | 35.6 kB (+1 B) · 224 B CSS (0) | — | — · — · — |
| Text | 7.4 kB (+1 B) · 4.9 kB CSS (0) | — | — · — · — |
| Timer | 9.6 kB (+1 B) · 5.9 kB CSS (0) | — | — · — · — |
| FieldStatus · Storybook | 9.6 kB (0) · 3.6 kB CSS (0) | 164 (0) | — · — · — |
| Heading | 7.9 kB (0) · 4.9 kB CSS (0) | — | — · — · — |
| ProgressBar | 11.0 kB (0) · 6.2 kB CSS (0) | — | — · — · — |
| Timestamp | 22.9 kB (0) · 8.9 kB CSS (0) | — | — · — · — |
| TypeaheadItem | 6.4 kB (0) · 3.6 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.58 MB packed (+2.0 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 9 rows, core 8 rows; run pnpm api:report.
Wide change (8 components): add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
github-actions
Bot
removed
the
needs:design-review
## Exact decision needed
Whether merge or stable release may wait on real assistive-technology (NVDA, VoiceOver) evidence that contributors cannot produce.
## Existing authority searched
- `spec:AST-009` (current): FR4/FR8/FR17–FR19 required an explicit trigger classification, a named real-AT matrix, a scenario, and a durable release-block record, and blocked the first stable release until real-AT receipts passed or an owner exception applied.
- Linked records that restated the block or required screen-reader checks: `spec:AST-001` (browser evidence and FR7 row), `module:Table/useTableRowStatus` AR6, `module:Table/useTableColumnResize` verification, `spec:AST-056` (browser evidence line), `spec:AST-021` (release boundary phrase), `spec:AST-086` (FR6 real-AT row and platform line), `component:FieldStatus` AR5 and its verification row, `spec:AST-020` FR6 ("real-AT contract").
- Open PRs that add release-block sections to AST-009: #7732, #7275, #7272. Under this record those sections are no longer needed.
## Decision
- No requirement gates merge or release on evidence that cannot be produced (Intent, FR17, DEC-4).
- Merge and stable release need only the unit, DOM, accessibility-tree, axe, and real-browser evidence the owning records require (FR17).
- Announcements are proved by their routing: which element or channel carries them, their text, urgency, and update count per occasion (FR4, FR19).
- Real-AT observation is recommended follow-up when an outcome depends on AT behavior, may be tracked in a public issue, and holds no pull request or release (FR1–FR6, FR18).
- A change with an AT-dependent claim says so and names its lower-layer evidence; no matrix, scenario, or release record is needed to merge (FR8).
- Evidence-layer boundaries, the representative matrix, failure handling, and durable receipts stay, scoped to when a real-AT check runs.
- The exception process (old FR23–FR26) is removed because nothing remains to except.
## Rejected alternatives
- Holding stable releases until named real-AT receipts pass: fixes for known accessibility defects stall when no contributor can run the required pairings.
## Claim scope and non-goals
Only the evidence bar for merge and release, and the status of real-AT observation. Component accessibility requirements, evidence-layer boundaries, and focus/announcement behavior stay with their owners.
## Evidence and implementation relationship
Status-announcement conformance PRs (#7869, #7870, #7871, #7892, #7900, #7910) stall on the old real-AT release-block requirement because nobody in the review loop can run NVDA or VoiceOver. No implementation is included.
## Writing rubric results
- R1 narration: none; present-tense rules throughout. The two earlier holds are described as current follow-ups.
- R2 research in the record: none.
- R3 evidence naming a change: none; the earlier hold sections that named pull requests are replaced by the facts (story names and claims).
- R4 rejected alternatives over one line or outside a decision: one per decision, one line each.
- R5 pull request or issue references remaining: none in AST-009 or the edited lines.
- R6 ruled open questions still listing options: none.
## Scope
- [x] This PR contains specification records only.
- [x] It records observable behavior and preserves equivalent internal implementations.
- [x] Any named internal mechanism is intentionally public, names its dependent caller/system, and explains why an equivalent implementation would not satisfy the contract.
- [x] Verification states the evidence layer and failure signal without prescribing CI job/workflow topology.
- [x] It records one intentional durable decision, not a rescue for a separable tagalong.
- [x] The canonical knowledge owner, affected current claims, and overlapping open work are named; any ownership collision is resolved or called out for human decision.
- [x] Adjacent API, visual, accessibility, composition, and implementation facts remain outside scope unless this exact decision depends on them.
- [x] Public text and artifacts contain no internal Meta context.
## Validation
- `pnpm check:knowledge`
- Prettier
- `git diff --check`
3b69c37 to
aec7d13
Compare
b4d74a0 to
a261602
Compare
7de17ce to
ad6e9ae
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for routing standalone Spinner messages through the shared announcement channel. Approval waits on a maintainer to provide or identify the spec that governs a standalone Spinner's non-live role, deciding whether it is an indeterminate progressbar and which record states it, because AST-086 leaves that role outside its scope and no Spinner record exists yet. Independently of that decision, these need fixing:
- 24 existing tests in 9 files fail on this branch (CI
test-uishards 1–4), mostly consumer tests that still find a composed Spinner byrole="status". - Spinners composed inside busy controls (DateInput, Thumbnail, Typeahead, CommandPaletteInput and others) now announce through the channel, which AST-086 FR8 rules out, and an idle lab Schedule announces "Loading events" because its Spinner is hidden only with
visibility: hidden. - A ReactNode
labelwithoutaria-labelannounces "Loading" instead of the shown words (AST-086 FR7). - The test named for "a new appearance with the same words" mounts the Spinner only once, so it does not cover that case.
[Automated review]
| ref={hasLabel ? undefined : ref} | ||
| role="status" | ||
| ref={ringRef} | ||
| role="progressbar" |
There was a problem hiding this comment.
No current record chooses this replacement role: AST-086 leaves "the role and name it exposes without a live role" outside its scope, and there is no Spinner component record. This waits on a maintainer decision.
| if ( | ||
| !element || | ||
| element.closest('[aria-hidden="true"], [hidden], [inert]') |
There was a problem hiding this comment.
Only attribute-hidden ancestors silence the ring. Composed Spinners in busy controls (DateInput, Thumbnail, Typeahead, CommandPaletteInput and others) now announce, which AST-086 FR8 rules out, and the lab Schedule's Spinner, hidden with visibility: hidden, announces "Loading events" when an idle schedule mounts.
| (typeof label === 'string' ? label : undefined) ?? | ||
| t('@astryx.spinner.loading'); |
There was a problem hiding this comment.
For a ReactNode label without aria-label this resolves to "Loading", so the announcement no longer matches the shown words (AST-086 FR7).
| expect(registerProperty).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it('announces once per appearance, including a new appearance with the same words', () => { |
There was a problem hiding this comment.
This mounts the Spinner once, so it does not cover the "new appearance with the same words" in its name. Toggle loading off and on again, and rerender with the same label, asserting the call count each time.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Route standalone Spinner labels through the shared polite announcement channel once per appearance or label change, rather than mounting a status region with its name. Rerenders and StrictMode replay do not repeat the channel call; a new appearance with the same words does. Spinner's ring now exposes an indeterminate `role="progressbar"` instead of `role="status"`, without a fabricated `aria-valuenow`. Update tests that query a standalone Spinner by its old role. Visible labels, sizing, animation, and label naming are unchanged. |
There was a problem hiding this comment.
Composed Spinners change too: their role, and, until #7856 lands, their announcements. AST-086 asks release notes to name each affected component.
There was a problem hiding this comment.
Thanks for routing standalone Spinner loading through the shared announcement channel: on this head a Spinner that appears after page load announces its label once as a named progress bar, rings inside busy controls stay silent, and the focused tests pass. Approval waits on an engineering owner approving the Spinner.spec.md text at this head (AR1's progressbar role and silencing paths, the Ownership boundary, Compatibility and the OQ1 decision), because the earlier /approve-spec named a head the rebase replaced; an engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. One point for that decision: Core's TokenizerBottomSheet keeps its named search ring silent through role="img", while AR1 says Tokenizer uses the internal path.
[Automated review]
| and app code do; Typeahead, BaseTypeahead and Tokenizer, which keep their | ||
| search indicator named, silence it through an internal, non-exported path. A caller `role` other than |
There was a problem hiding this comment.
Core's TokenizerBottomSheet (on this branch from main) keeps its named search ring silent only through role="img" (packages/core/src/Tokenizer/TokenizerBottomSheet.tsx:800-804), the caller-role path the Compatibility section calls shipped compatibility and not a recommended mode, while this sentence says Tokenizer silences its search indicator through the internal path. The ring is silent either way; consider naming the sheet here, or moving it to the internal path.
…oined (#9070) ## Exact decision needed Whether `spec:AST-086` FR9's shared channel joins a component's superseded message about its current state with its next one, and the optional public `useAnnounce` option that asks for replacement. Today every message written to a region before the previous one is exposed is joined, so a component that states its current state on every input, such as a search's result count, has all its stale states spoken together when input outpaces a frame (a barcode scanner, dictation, automation): `1 result Create "Urg" Create "Urge" Create "Urgen" Create "Urgent"`. ## Existing authority searched - `spec:AST-086` FR9 (the join), FR6 (an identical message repeated in a frame is one message), FR8 (a busy start its outcome overtakes is dropped), FR13 and FR14 (own-content messages and modal containers). - `architecture:interaction-modality` INV8: superseded work must not change the current status or announcement, with supersession left to component contracts. - `spec:AST-075` DEC-2 and FR7: Astryx's comboboxes announce their result count with `useAnnounce`, the same hook products use. Tokenizer's record: its sheet's count and empty-state text go through `useAnnounce`. MultiSelector's record (draft): its result count and create row label go through the polite channel. - `spec:AST-002` FR1, FR2, FR5, FR7, FR16 and FR20 for the new public option; `has*` booleans such as `hasClear` and `hasPaging` for its name. - Open pull requests that mention AST-086 (#7871, #7902, #7910, #8175, #8211, #8583, #8001): none edits FR9. #8640 implements the replacement for MultiSelector and waits for this decision. ## Decision A `useAnnounce` instance created with `hasReplace` states one current state of its owner, such as a search's result count: each message it writes withdraws that instance's previous message while it is not yet exposed, whatever their urgencies, and is spoken in the region of its own urgency; a message already exposed has been heard and stays. Messages from other instances, and from instances without `hasReplace`, still join. `useAnnounce` takes an optional options object with one field, `hasReplace` (default `false`), and the `announce(message, politeness)` it returns is unchanged. FR14's modal containers withdraw the same way, and a picker's own empty-panel message comes from its search's `hasReplace` instance (FR13). Astryx's search counts and MultiSelector's create row use a separate `useAnnounce({hasReplace: true})` instance (`spec:AST-075` FR7, Tokenizer's and MultiSelector's records); selection messages keep their own plain instance. DEC-4 records it; its decider is the approving engineering owner, with INV8 and FR8 as its reasons. Public option (`spec:AST-002` FR5, FR20): two otherwise identical messages, `"7 results"` written twice in one frame, need different outcomes. As a search's count it replaces the earlier one; as two distinct notices both are spoken. Only the caller knows which it sends, and the channel cannot derive it. Before: `const announce = useAnnounce(); announce(t('resultCount', {count}));`. After: `const announceCount = useAnnounce({hasReplace: true}); announceCount(t('resultCount', {count}));`. Existing calls are unchanged. ## Rejected alternatives - Each owner debouncing its own messages: every announcement waits, and every owner repeats the rule. - An exception for every `useAnnounce` message: Astryx's comboboxes and Tokenizer announce through `useAnnounce`, so their counts would keep joining. - A string key on each message (`announce(message, politeness, {currentState})`): a third argument whose strings every call must keep consistent, where one instance already states one state. ## Claim scope and non-goals FR9's replacement within one `hasReplace` instance, the option that asks for it, and the AST-075, Tokenizer and MultiSelector sentences that describe their counts' announcement. Which messages a component sends through a `hasReplace` instance stays with its own record. ## Evidence and implementation relationship - On main, the MultiSelector CreateRow accessibility spec, which types into the create row at about four keystrokes per frame, fails 9 of 10 runs alone and 26 of 48 under load, its polite region reading the joined statuses. - #8640 implements the replacement for MultiSelector. With it, the spec passes 10 of 10 alone and 48 of 48 under load. It lands after this record changes, adopting `useAnnounce({hasReplace: true})`. ## Writing rubric results - R1 narration: none, in AST-086, AST-075, Tokenizer's or MultiSelector's record. - R2 research in the record: none. - R3 evidence naming a change: none. - R4 rejected alternatives over one line or outside a decision: one, inside DEC-4, one line; nothing added to the other records. - R5 pull request or issue references remaining: none. - R6 ruled open questions still listing options: none. ## Scope - [x] This PR contains specification records only. - [x] It records observable behavior and preserves equivalent internal implementations. - [x] Any named internal mechanism is intentionally public, names its dependent caller/system, and explains why an equivalent implementation would not satisfy the contract. - [x] Verification states the evidence layer and failure signal without prescribing CI job/workflow topology. - [x] It records one intentional durable decision, not a rescue for a separable tagalong. - [x] The canonical knowledge owner, affected current claims, and overlapping open work are named; any ownership collision is resolved or called out for human decision. - [x] Adjacent API, visual, accessibility, composition, and implementation facts remain outside scope unless this exact decision depends on them. - [x] Public text and artifacts contain no internal Meta context. ## Validation Local gates at 2b84901: `pnpm check:knowledge` (records and templates aligned), Prettier, `git diff --check`, `pnpm check:repo`.
|
/approve-spec c7f3fd5 |
github-actions
Bot
removed
the
needs:spec-owner-review
Preserve Spinner's rendered-text fallback alongside owner-provided announcers, keep direct module imports, and retain Comments' persistent error region together with its busy-start announcement.
There was a problem hiding this comment.
Thanks for routing standalone Spinner loading through the shared announcement channel: a Spinner that appears after page load now announces its label once as a named progress bar, rings inside busy controls stay silent, and the Spinner.spec.md text is owner-approved. One change is needed: the new lab useAnnounceBusyStart repeats what Comments' useStatusAnnouncement already does (announce a loading message once, re-arm when it goes), and CommentParts.tsx now announces its two loading messages through both hooks with different page-load and live-container rules. Please move useStatusAnnouncement to packages/lab/src/ and use it for the EmojiPicker, Schedule and Comments history busy starts (adding a page-load option if you want that silence), then delete useAnnounceBusyStart; if busy starts need their own path, say why in the hook's header.
[Automated review]
| export function useAnnounceBusyStart(isBusy: boolean, message: string): void { | ||
| const announce = useAnnounce(); | ||
| const wasBusy = useRef(false); | ||
| useEffect(() => { | ||
| const started = isBusy && !wasBusy.current; | ||
| wasBusy.current = isBusy; | ||
| if (started && document.readyState === 'complete') { | ||
| announce(message, 'polite'); | ||
| } | ||
| }, [announce, isBusy, message]); |
There was a problem hiding this comment.
This repeats what Comments' useStatusAnnouncement (packages/lab/src/Comments/useStatusAnnouncement.ts) already does: announce a message once through useAnnounce when it appears, re-arm when it goes. CommentParts.tsx now uses both, with different page-load and live-container rules. Consider moving useStatusAnnouncement to packages/lab/src/ (optional ref, a page-load option if wanted) and calling it with isBusy ? message : null from the three owners, or say here why busy starts need their own path.
There was a problem hiding this comment.
Thanks for moving the lab busy starts onto the existing useStatusAnnouncement: a standalone Spinner now announces its label once as a named progress bar, composed rings stay silent, and the earlier duplicate-hook concern is resolved. No further code changes are needed from this review. GitHub reports a conflict with main, where component records now live under docs/specs/components/; merging main should carry the Spinner.spec.md edits to docs/specs/components/Spinner/Spinner.spec.md, and the spec check will run again on that commit.
[Automated review]
Carry the approved Spinner contract to docs/specs/components while preserving the existing announcement changes and upstream Typeahead and Tokenizer updates.
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Thanks for routing standalone Spinner loading through the shared announcement channel. On this head, a Spinner that appears after page load announces its label once as a named progress bar, and composed rings stay silent.
Approval waits on one merge and one spec decision:
- Regression: the branch no longer merges into
main.docs/specs/components/Spinner/Spinner.spec.md,packages/core/src/Spinner/Spinner.tsx,Spinner.test.tsxanduseAnnounceRenderedText.adopters.test.tsxconflict withmain's Spinner rotor and generated-string changes, so please mergemainand keep both sets of changes. - The Spinner record's OQ1 decision is new direction that the current record leaves open: AR1's
role="progressbar"with noaria-valuenow, the composed-ring silencing paths and the decision-log entry. Its earlier approval does not carry to a new head, so once the merge is pushed, an engineering owner approves the record text by commenting/approve-spec <full head SHA>for that head (the author may do so if they are an engineering owner).
Two optional comment tidy-ups are noted inline. CI has not run on this head yet; a maintainer adds the ci:run label so it runs.
[Automated review]
|
rebased to main |
Resolves the conflicts with meta#8177, meta#9936 and meta#8256 (Spinner's rotor, wake-once-per-100-turns and CircularProgress) and the adopters test's PowerSearch and InfoTip changes. The Spinner record takes main's FR3, system_specs and verification rows, with this branch's approved "indicator" wording, AST-017/AST-020 additions and announcement notes applied onto them.
There was a problem hiding this comment.
Thanks for routing standalone Spinner loading through the shared announcement channel and for resolving the conflicts with main; approval still waits on two fixes and an engineering owner's spec decision.
Blocking:
- The merge brought back
packages/lab/src/Comments/useStatusAnnouncement.ts, a second copy of the hook this change moves topackages/lab/src/useStatusAnnouncement.ts, and since nothing imports it, please delete it. - Regression: with
main's comment-provenance lint rule now merged in, Lint as CI runs it reports 42@astryx/no-comment-provenanceerrors in 11 files this change adds or edits (for exampleSpinner.tsx:462-467), so keep each comment's reason and drop the record and claim ids.
The component:Spinner record now makes the indicator role="progressbar" with no aria-valuenow and records how composed rings stay silent; the current specs leave that choice open, and the approvals so far name earlier heads. Once the fixes are pushed, an engineering owner records the decision by commenting /approve-spec with that head's full SHA, and the author may do so if they are an engineering owner. CI has not run on this head yet: a maintainer adds the ci:run label so it runs.
[Automated review]
| * @param options.skipPageLoad Keep messages present during page load silent | ||
| * (FR11). Defaults to false to preserve Comments' error and progress delivery. | ||
| */ | ||
| export function useStatusAnnouncement( |
There was a problem hiding this comment.
The merge with main brought back packages/lab/src/Comments/useStatusAnnouncement.ts, the copy this file replaces. Nothing imports it now, so it is a second implementation of this hook; please delete it so the move stays a move.
| (typeof label === 'string' ? label : undefined) ?? | ||
| t('@astryx.spinner.loading'); | ||
|
|
||
| // Only a standalone Spinner is its own owner (`spec:AST-086` FR2, FR8): a |
There was a problem hiding this comment.
With main's @astryx/no-comment-provenance rule merged in, Lint as CI runs it (strict) reports the record and claim ids in this comment and the one at line 487 as errors. Keeping only the reason would read, for example: "A composed Spinner is silent; its owner announces. Core composers set SpinnerOwnerContext, and app code sets aria-hidden. A caller role other than progressbar also keeps it silent, because released app code relies on that."
The comments this branch added cite knowledge records (spec IDs and FR numbers), which strict lint's no-comment-provenance rule rejects. Each now keeps only the reason it gives; the records still hold the requirements. Comments only: no code changes.
|
/approve-spec 2ae0d0d |
There was a problem hiding this comment.
Thanks for routing standalone Spinner loading through the shared announcement channel and for clearing the comment-provenance lint errors; the Spinner record text is owner-approved for this head. One change is still needed:
packages/lab/src/Comments/useStatusAnnouncement.tsis still in the tree beside the movedpackages/lab/src/useStatusAnnouncement.ts, and since nothing imports it, please delete it so the move stays a move.
Deleting it adds a commit, so an engineering owner then re-approves the record text by commenting /approve-spec with that head's full SHA (the author may do so if they are an engineering owner). Two optional comment tidy-ups are noted inline. CI has not run on this head yet: a maintainer adds the ci:run label so it runs.
[Automated review]
| * @param options.skipPageLoad Keep messages present during page load silent. | ||
| * Defaults to false to preserve Comments' error and progress delivery. | ||
| */ | ||
| export function useStatusAnnouncement( |
There was a problem hiding this comment.
packages/lab/src/Comments/useStatusAnnouncement.ts, the copy this file replaces, is still in the tree and nothing imports it, so there are two implementations of this hook; please delete the Comments copy so the move stays a move.
| // Only a standalone Spinner announces. A Core component that composes it | ||
| // owns the task and silences it through the internal SpinnerOwnerContext; | ||
| // app code hides it with `aria-hidden`. A caller `role` other than | ||
| // progressbar also keeps it silent, as it shipped, so app code that | ||
| // silenced a composed ring that way keeps working; new code uses | ||
| // `aria-hidden`. |
There was a problem hiding this comment.
Optional: the last sentence runs 29 words and the one before joins two topics. A shorter version that keeps only the reasons:
// A composed Spinner is silent; its owner announces. Core composers set
// SpinnerOwnerContext, and app code sets aria-hidden. A caller role other
// than progressbar also keeps it silent, because released app code relies
// on that.| {/* Keep the announcement owner beneath Dialog: the shared channel | ||
| routes its busy start into this modal. */} |
There was a problem hiding this comment.
Optional: this repeats the reason already in BusyStatus's JSDoc ("Runs beneath Dialog so useAnnounce reaches its modal scope"); one of the two is enough.
This branch moved packages/lab/src/Comments/useStatusAnnouncement.ts to packages/lab/src/useStatusAnnouncement.ts; the merge of main brought the old copy back. Nothing imports it.
|
/approve-spec 72f8890 |
There was a problem hiding this comment.
Thanks for routing standalone Spinner loading through the shared announcement channel and for removing the leftover Comments copy of useStatusAnnouncement. The Spinner record text is owner-approved for this head, and no changes are needed.
Approved pending CI: CI has not run on this head yet, so a maintainer adds the ci:run label so it runs. One optional comment tidy-up is noted inline.
[Automated review]
| // Only a standalone Spinner announces. A Core component that composes it | ||
| // owns the task and silences it through the internal SpinnerOwnerContext; | ||
| // app code hides it with `aria-hidden`. A caller `role` other than | ||
| // progressbar also keeps it silent, as it shipped, so app code that | ||
| // silenced a composed ring that way keeps working; new code uses | ||
| // `aria-hidden`. |
There was a problem hiding this comment.
Optional: most Core composers hide the ring with aria-hidden; only the three search composers use SpinnerOwnerContext, and the last sentence runs 29 words. A shorter version that matches the record:
// A composed Spinner is silent, because its owner announces.
// Most composers and app code set `aria-hidden`; the search composers set SpinnerOwnerContext.
// A caller role other than progressbar also keeps it silent, because released app code relies on that.
User impact
A standalone Spinner rendered with
{loading && <Spinner label="Loading results" />}can appear without announcing that loading has started. Someone waiting for results with a screen reader can miss the indication that work is in progress.Fixes #7855.
Problem and solution fit
Spinner mounts its
role="status"element together with its name. A newly mounted live region does not reliably announce the content it arrives with.The fix uses the internal
useAnnounceRenderedTexthook to send standalone loading messages through the existing politeuseAnnouncechannel. The indicator becomes a named indeterminaterole="progressbar", with noaria-valuenow, rather than another live region. Content present during page load or hydration stays silent; later appearances and text changes announce once, without repeating on unchanged rerenders or StrictMode replay.Messages use the rendered label text. When there is no rendered text, the hook's internal
fallbackTextpath reads anyaria-labelledbyreferences after commit before using the resolved explicit or translated default name. Existing callers that omit the fallback retain their behavior.Composed indicators must not announce the same busy state as their owner. Most composing components and app code hide the ring with
aria-hidden. The search indicators updated in Typeahead, BaseTypeahead, and Tokenizer use an internal, non-exported context and keep their existing loading owner. A caller's explicit non-progressbarrole also remains silent, so existing app code needs no change. No public announcement prop is added.The lab EmojiPicker, Schedule, and Comments history loaders hide their rings and announce from the task owner. EmojiPicker and history keep the loading words readable without changing the ring size or label layout. Schedule preserves its reserved header space and keeps its ring silent, including while idle.
The review follow-up moves Comments' existing
useStatusAnnouncementtopackages/lab/src/and reuses it for all three busy starts, removinguseAnnounceBusyStart. Its optional page-load suppression preserves the busy loaders' initial silence; existing Comments error and progress calls retain their delivery rules. The history caller remains beneath Dialog souseAnnouncedelivers inside the modal.Expected behavior and authority
Spinner AR1 records the standalone progressbar role and silencing paths, following AST-020 FR15 and AST-017 FR53. The record text has owner approval on this PR and is unchanged by the hook-consolidation follow-up.
AST-086 FR2, FR4, FR6–FR8, and FR11–FR14 define transient delivery, live-container ownership, once-per-occasion announcements, rendered-text/name fallback, page-load behavior, and in-modal delivery. React runtime INV10 places Core's own-content announcements in the shared internal module.
Smallest restoration
The change covers Spinner's implementation, docs, approved component record, and tests; the internal hook's fallback and stacked-text extraction; three search composers; the lab loading-message and shared status-announcement paths; the status-message bindings; affected consumer test queries; and a patch changeset. Tests distinguish the named progressbar from the persistent announcement channel.
Existing standalone call sites stay unchanged:
A parent that already announces loading hides its composed ring:
If that parent also displays loading words, those words remain outside the hidden ring so they stay readable.
The fallback is internal; no public hook or export is added. The shared hook's visibility, live-container, and lifecycle gates are unchanged. No new Storybook story or rendering harness, visual changes, or changes to geometry, motion, layout, or theming are included.
Evidence
aria-labelledbyname changes. Hook tests cover reference order, unresolved references, and rendered-text precedence.progressbarrole overrides do not send their own message. Removing the silent-owner condition restores standalone delivery.Scope
Testing
Current local validation of the review follow-up (
1ac95c208c):pnpm exec vitest run --project ui packages/lab/src/useStatusAnnouncement.test.tsx packages/lab/src/Comments packages/lab/src/EmojiPicker packages/lab/src/Schedule --maxWorkers=4— 191 passed across 12 files.pnpm exec vitest run --project ui packages/core/src/Spinner/Spinner.test.tsx packages/core/src/hooks/useAnnounceRenderedText.test.tsx packages/core/src/hooks/useAnnounceRenderedText.adopters.test.tsx packages/core/src/FieldStatus packages/core/src/CheckboxList/CheckboxList.test.tsx packages/core/src/EmptyState/EmptyState.test.tsx --maxWorkers=4— 220 passed across 7 files.pnpm -F @astryxdesign/core typecheckandpnpm -F @astryxdesign/lab typecheck— implementation and component-doc checks passed.pnpm build— passed.pnpm lint:strict— passed with 0 errors and 105 warnings; repository checks passed.git diff --check, and normal pre-commit hooks — passed.pnpm test --maxWorkers=4 --bail=1stopped atpackages/cli/test/global-local.test.mjs:407: the expected upgrade notice was absent. The same case failed when run alone. The CLI files are unchanged from the PR base; this is not a passing full-suite result.The earlier description reported 13 passing Chromium status-message tests. Those browser tests were not rerun for this hook-consolidation follow-up.
Writing rubric results
For
Spinner.spec.md, whose approved text is unchanged by this follow-up:Merge main (conflict resolution)
mainchanged the Spinner record and code after this branch's last merge: #8177 (the ring keeps turning while JavaScript runs), #9936 (wake once per 100 turns) and #8256 (CircularProgress shares the rotor). This merge keeps both. In the code, the rotor, wake-once and CircularProgress changes come frommain; the shared announcement channel, theprogressbarrole andSpinnerOwnerContextcome from this branch. Every other file's diff from this pull request is unchanged.Record lines, side by side:
main(#8177)system_specsspec:AST-017,spec:AST-020spec:AST-108/FR4span" / "status box" → "indicator"main's wordingSpinnerMotion.a11y.chromium.spec.tsmain's rowsNo other record wording changed. Checks after the merge: the Spinner,
useAnnounceRenderedText, adopters (PowerSearch, InfoTip), CircularProgress, CheckboxList, FieldStatus, Typeahead, Tokenizer and Lab Comments/EmojiPicker/Schedule/useStatusAnnouncement suites pass (1,422 tests). The core type check is clean apart from tests that need a localtemporal-polyfill.