Skip to content

Review 3934

Jump to bottom
Cindy Zhang edited this page Aug 27, 2026 · 4 revisions

Review 3934 — Chat speech hook and message button

Current status: Round 2 is the completed exact-head review at 3364f381ccd86803c6d8d7c90206d5e04906399d, after the branch was refreshed from current main. Proposed action: request changes. No GitHub review, comment, branch push, merge, or auto-merge action was taken.

The earlier stale-head work is retained below as historical preliminary work only and remains invalidated.

PRELIMINARY STALE-HEAD ANALYSIS — HISTORICAL ONLY

The remaining sections record what the invalidated run observed at the stale head. They are not review findings, acceptance criteria, or advice to the author, and must not be copied into a future review without re-verification after refreshing from main.

Pull request · exact head

PR

#3934 feat(chat): add useChatSpeech hook and ChatSpeechButton for text-to-speech by jiunshinn (bucket: contributor)

HEAD REVIEWED

98c48c502080f68f551e7b8685bd9521ae32ccdf

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: New exported hook, component, types, semantic icon, state, Effects/listener, continuous animation, and public API; the fork is untrusted and the branch conflicts with current main.

PRIOR REVIEW

No human review or inline review comment exists. The only PR comment is Vercel’s failed deployment notice.

PROBLEM

WHY 1: Chat users who cannot or do not want to read an assistant response have no built-in way to hear it. WHY 2: They must switch tools or abandon a hands-free conversation flow. WHY 3: Chat is meant to support accessible, low-friction interaction across input and output modalities.

USER-FACING PROBLEM: A person using an Astryx chat cannot ask the product to read an assistant response aloud from the message itself. PROBLEM SEVERITY: missing capability — issue #1320 and the Q3 tracker #3635 establish the need; no current Astryx TTS seam serves it.

VERDICT: clear

SOLUTION

The browser’s speech synthesizer reads caller-supplied message text and reports playback state through one React owner. A small message action renders that state and starts or stops playback. A semantic speaker icon and animated bars communicate the idle and speaking endpoints.

SOLUTION (5 decisions · ~350 runtime lines of 1,073 additions)

  1. Export a browser-backed speech controller with voice/rate/pitch/volume, lifecycle callbacks, voices, and speak/stop/pause/resume — serves the TTS capability.
  2. Add isPaused beyond the issue-designed return — explicitly awaiting owner confirmation.
  3. Export a per-message ChatSpeechButton that consumes one controller plus message text — serves message playback.
  4. Recommend one shared controller across message buttons while deriving every button’s active state from one global isSpeaking boolean — breaks that documented composition.
  5. Add speaker to the semantic icon registry and render fixed-loop bars while speaking — serves the new visual endpoint but leaves every complete theme registry unsynchronized.

BURDEN: high — permanent public surface, global browser state, two Effects, one mount-lifetime listener, five hook states/controls, a semantic registry expansion, visual motion, docs, and a browser test matrix. BURDEN MATCH: proportionate for a demonstrated missing capability, but the public state addition still needs its named owner decision and the documented multi-message path must work.

VERDICT: BLOCKS — the shared-controller message journey and registry integration are incomplete; isPaused also needs human judgement.

ARCHITECTURE

OWNER: useChatSpeech owns the browser synthesizer lifecycle; ChatSpeechButton is the message-level rendered affordance. TIER 1: existing Button, translator, motion-safety, and public DOM contracts. Button is reused; translator and reduced-motion patterns are not. TIER 2: semantic icons via IconRegistry. SEAMS: one button; one shared controller across message footers; unsupported hidden/visible; pause/resume; error; unmount; several independent hook owners. BEHAVIOR UNIT: useChatSpeech — a named external-system hook with focused unit tests; browser tests are missing from the PR.

seam driven result
one interactive button, actual Chromium start, pause, resume, stop, restart, and unmount cleanup all worked; focus stayed on the activated control
one shared controller, two message buttons fails: after starting the first, both buttons become “Stop reading”/pressed; activating the second records stop, not speak:Second
unsupported, hidden static story renders no control
unsupported, visible renders a focusable, enabled control even though the supplied controller is unsupported
error unit mock clears both state flags and calls onError; a real platform error could not be forced safely

The root cause of the multi-message failure is ownership: the public controller reports only global speech state, while each button needs to know whether its own text/utterance is active. The docs require sharing one controller, so independent hook instances are not the intended escape.

VERDICT: BLOCKS — the lifecycle owner cannot preserve the documented per-message composition seam.

IMPACT

The change is opt-in: existing chats render identically until a builder adds the hook/button. In supported Chromium, one button successfully controls native playback. In the documented shared-controller message list, every message announces and paints itself as active, and selecting another message stops the first without starting the chosen one.

VERDICT: BLOCKS — the primary multi-message journey gives users false state and requires a second action to switch messages.

API

const speech = useChatSpeech();

<ChatMessageMetadata
  footer={<ChatSpeechButton speech={speech} text={content} />}
/>
change public? class doc’d? verdict
+ useChatSpeech(options?: UseChatSpeechOptions): UseChatSpeechReturn yes — @astryxdesign/core/Chat behavior hook + thin component matches useChatDictation/ChatDictationButton hook API only in source/JSDoc; button en+docsZh (English prose) blocked by shared-owner state
+ UseChatSpeechOptions: voice?, rate?, pitch?, volume?, onStart?, onEnd?, onError? yes issue-designed speech controls JSDoc mechanically sound
+ return: isSupported, isSpeaking, isPaused, voices, speak, stop, pause, resume yes state + controls JSDoc isPaused needs owner decision
+ ChatSpeechButtonProps: speech, text, size, isHiddenWhenUnsupported, label, BaseProps yes ChatDictationButton sibling en+docsZh active identity missing
+ IconName += 'speaker' yes — Icon subpath microphone semantic sibling Icon docs + CLI docs BLOCK: complete registries not updated

OSSIFICATION: The hook/button concept and tier were already selected in owner-authored issue #1320. isPaused is the only explicitly undecided addition. Removing or changing any of these exports after release requires a deprecation cycle; adding speaker to the closed IconName union also requires every complete registry to grow in lockstep.

Fourteen complete registries are missing speaker: seven shipped theme packages and their seven CLI templates. Exact-head CI stops first at packages/themes/neutral/src/icons.tsx:50 with TS2741.

VERDICT: BLOCKS — public registry sync is incomplete; [Needs human judgement] remains for isPaused.

THEMING

No existing target or token is removed. Bars use the global accent and full-radius tokens, but the component adds no themeProps('chat-speech-button') target even though current ChatDictationButton carries its component target. The 900ms duration and ease-in-out are literals rather than motion tokens.

Current main’s sibling has already moved to translated labels, a component target, and reduced-motion handling. This stale branch predates those changes and conflicts with current main; the contributor should not be charged for main drift, but the new component must adopt the current contract before merge.

VERDICT: BLOCKS — continuous motion ignores reduced motion; target and motion-token parity also need current-main reconciliation.

BREAKING

BEHAVIOR: additive and opt-in for existing chat consumers; the documented shared-controller composition itself is broken. API: yes in practice — widening the closed IconName union makes complete IconRegistry values fail compilation until they add speaker; exact-head CI proves it in the Neutral theme. VISUAL: additive, intended — idle speaker becomes four animated bars while speaking. No existing component moves. THEME: existing theme registries do not compile at this head; no existing target or token is removed.

VERDICT: BLOCKS — all complete icon registries and generated theme templates must stay synchronized.

PERFORMANCE & RESOURCES

EFFECTS: two.

Effect + deps external system why render/handler cannot do it measured render cost lifetime + cleanup focused test
useEffect([]) at useChatSpeech.ts:131 speechSynthesis.voiceschanged voices may arrive outside a React event after mount at documented N=1: one dispatched voiceschanged produced +1 hook render and +1 commit one global listener from mount to unmount; removed on cleanup unit test covers update; Chromium Profiler measured 2→3 renders/commits
useEffect([]) at useChatSpeech.ts:146 global speech synthesizer owner unmount has no event handler that can survive teardown zero render work; cleanup only mount to unmount; calls cancel() unit + actual Chromium unmount both stop playback

RENDER: initial exact-head harness settled in two StrictMode renders/commits; voiceschanged added one at N=1. LISTENERS/OBSERVERS: one global listener per mounted hook; docs recommend one shared hook per message list, making realistic N=1; cleanup was verified. LAYOUT: bars animate compositor-friendly transform; no layout reads or writes. Missing reduced-motion is an accessibility/design block, not a measured layout cost. BUNDLE: no dependency change; PR Analysis could not complete because the workspace build is red.

VERDICT: clear — measured costs are bounded feature work and cleanup holds.

VISUAL EVIDENCE

VISUAL CHECK: manual frames required WHY: The PR adds a visible control and a speaking animation; exact-head visual CI is pending/skipped behind the red build, and the multi-message and reduced-motion endpoints are not covered by a green visual gate.

Shipped stories at exact head

Idle Speaking Speaking under reduced motion Unsupported but visible

Documented shared-controller journey after starting the first message

Both message controls show the active sound-wave state although only speak:First fired. Clicking the second then produces speak:First|stop.

Screenshot sensor receipts

frame story rendered state media geometry settled/error receipt
idle core-chatspeech--idle “Read aloud”, not pressed light/ltr, motion allowed 32×32, visible fonts loaded; 0 animations; 0 errors JSON
speaking core-chatspeech--speaking “Stop reading”, pressed light/ltr, motion allowed 32×32, visible fonts loaded; 4 running animations; 0 errors JSON
speaking, reduce core-chatspeech--speaking “Stop reading”, pressed light/ltr, reduced motion 32×32, visible fonts loaded; 4 running 900ms animations; 0 errors JSON
unsupported visible core-chatspeech--unsupported-visible “Read aloud”, enabled light/ltr 32×32, visible fonts loaded; 0 animations; 0 errors JSON
shared after first isolated review harness both “Stop reading”, both pressed; only speak:First light/ltr, motion allowed two 32×32 controls fonts loaded; 8 running animations; 0 errors JSON

Every receipt records build 98c48c502080f68f551e7b8685bd9521ae32ccdf, Neutral/light, ltr, 900×400@1, desktop pointer media, semantic state, target count, geometry, fonts, animation count, and zero page/Storybook errors. The shared-controller capture used an isolated story; its exact arm diff is recorded in the review source and changed no production code.

VERDICT: BLOCKS — reduced-motion still runs all four animations, and shared playback paints every message as active.

REMEDY SEARCH

REMEDY SEARCH: not triggered — author owns HOW; acceptance criteria are sufficient.

The visual defects require no new design choice: current motion conventions require zero continuous animation under reduced motion, and only the active message may use the already-chosen speaking treatment.

A11Y & I18N

Checked in Chromium: native button semantics; 32×32 target; Enter/Space activation through Button; focus retained through start/stop/restart; supported lifecycle; unsupported hidden/visible; unmount; and accessible state/name. Exact-head axe did not run because the build failed.

Three objective failures remain. First, the APG toggle-button contract says the label must stay stable when aria-pressed changes; this button changes “Read aloud” to “Stop reading” while also changing aria-pressed, so assistive technology exposes “Stop reading, toggle button, pressed.” Second, both default labels bypass useTranslator(). Third, reduced-motion still has four running 900ms animations.

Support absence was exercised through the shipped static unsupported stories. This Mac’s Chromium supports the API and reported 180 voices, so it cannot prove behavior in a genuinely unsupported engine. Error was verified with the unit mock; a real platform error was not forced.

VERDICT: BLOCKS — toggle semantics, AT-string localization, and reduced-motion handling do not meet the current accessibility contract.

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION BLOCKS + human API decision
ARCHITECTURE BLOCKS — shared controller loses per-message identity
IMPACT BLOCKS — every message reports active; switching needs two actions
API BLOCKS — 14 registries missing; isPaused unresolved
THEMING BLOCKS — reduced motion and current target parity missing
BREAKING BLOCKS — complete registries fail compilation
PERFORMANCE clear — measured external-system cost
VISUAL BLOCKS — 4 animations persist under reduced motion; shared state mispaints
A11Y & I18N BLOCKS — toggle label, localization, motion

GOAL: partly met — actual Chromium proves the complete single-owner lifecycle, but the documented shared-message journey fails and exact-head builds cannot complete. DISPOSITION: registry sync/build → blocks now; shared active-utterance identity/switching → blocks now; stable toggle semantics + translated labels + reduced motion → blocks now; isPaused → owner decision required before public surface freezes. ADVICE: proven existing patterns + bounded outcome criteria — current ChatDictationButton supplies translator, target, and reduced-motion precedents; one shared controller must expose exactly one active message and switch in one action; every complete icon registry/template must compile. AUTHOR CAN PROCEED: no — the code defects are actionable, but the owner must decide whether Phase 1 permanently exposes isPaused or keeps the issue-designed return minimal. WORST OUTCOME: “Every message announces and paints itself as active, and selecting another message stops playback without starting the one the user chose.” → request changes.

JUDGEMENT NEEDED: API — keep isPaused in the first public return, or drop it until a demonstrated pause/resume UI requires it. Recommendation: keep it; the already-approved pause()/resume() controls otherwise provide no declarative state for a caller’s controls, and actual Chromium confirms the state is coherent.

request changes, withheld pending the one API decision

  1. [BLOCKS] Adding speaker leaves every complete theme registry and CLI template stale. → a theme builder cannot build the workspace at this head; CI fails TS2741 before Storybook/a11y/visual jobs can run · packages/core/src/Icon/globalIconRegistry.tsx:54 Confirmed independently by exact-head CI and a full-tree registry enumeration: seven theme packages plus seven templates omit the required key.

  2. [BLOCKS] The shared controller has no active-message identity. → after one message starts, every message says “Stop reading”; choosing another stops playback instead of starting that message · packages/core/src/Chat/ChatSpeechButton.tsx:126-135 Confirmed independently in real Chromium with the documented two-button composition: speak:First, both pressed, then stop from the second button.

  3. [BLOCKS] The active-state accessibility contract is incomplete. → localized and reduced-motion users get English-only labels and perpetual motion; screen readers receive a changing label plus changing pressed state · packages/core/src/Chat/ChatSpeechButton.tsx:52-79,127,168 Confirmed independently by current sibling/convention search, the WAI-ARIA APG toggle rule, and exact-head Chromium reporting four running animations with reduced motion enabled.

REVIEW

[not written — human API decision unresolved]

INLINE (0–3, only when genuinely line-anchored)

[not written — human API decision unresolved]

EVIDENCE I DID NOT SPEND

  • The isHiddenWhenUnsupported={false} story exposes an enabled no-op control; this mirrors the older Dictation sibling and is omitted behind higher-impact findings.
  • BaseProps/ref land on the wrapper rather than the primary button; the current sibling shares that debt, so it is not charged to this contributor.
  • docsZh is English prose; documentation localization is lower priority than the runtime blockers.

CI

  • Exact head has 19 check runs. Lint, dependency, smoke, component, docsite, registry, and review-signal jobs ran and passed.
  • build, build-storybook, and build-sandbox fail because speaker is required but missing from packages/themes/neutral/src/icons.tsx:50. The test job’s 6,662 tests pass, then its build substep fails on the same error.
  • pr-a11y, preview deployment, and visual acceptance did not run to completion; Vercel also failed. No CI wait was needed because the exact-head jobs were terminal.
  • Current PR state is dirty/not mergeable against main. Current main changed the Chat and Icon families substantially after this one-commit branch; no current-main drift is charged as an author defect.

TESTS AND EVIDENCE

  • Focused Vitest: 2 files, 20/20 passing in 4.89s.
  • Actual Mac Chromium: native support present, 180 voices; start/pause/resume/stop/restart/unmount all passed and focus stayed intentional.
  • Shared-controller Chromium harness: first click → both controls pressed; second control → stop only. Wrapper ARIA also lands on the span, not the button.
  • Effect-cost Profiler: documented N=1, voiceschanged causes +1 render/+1 commit; listener and global speech cleanup both verified.
  • Five exact-head screenshot receipts passed and were published at immutable asset commit cca4211adc90a52a8fe056898165e27cf9afdacb.

TIME

TIME total 40m setup/rules 5m full loop 1.6.0 kit, rubric 1.13, safety screen, fresh wikis, exact-head worktree build/server 3m fast install, one build, guarded Storybook 6278; warm main 6100 reused and untouched browser/a11y 13m 4 shipped states, actual lifecycle, shared seam, effect cost, 5 sensored frames focused tests 5m one over-scoped invocation plus corrected 2-file/20-test run code/history 8m full diff/body/issues/reviews, API/design guidance, current main, competitors, CI diagnosis critique/wiki 6m draft, critic pass, durable evidence, page/index update CI wait 0m exact-head jobs already terminal waste 10m over-scoped Vitest invocation and three probe retries before fail-closed selector/cleanup corrections

WHAT I COULD NOT VERIFY

  • Real unsupported-browser behavior: this Mac’s Chromium implements SpeechSynthesis; only the shipped unsupported stories and unit mock cover absence.
  • A real synthesis error event: the deterministic unit mock covers state/callback behavior, but the platform error could not be forced.
  • Safari and Firefox behavior; this Mac permits Chromium only, so no cross-browser claim is made.

Invalidation correction

This round is invalidated and withheld by the mandatory refresh-before-review gate. The stale-head findings above are preliminary, non-author-facing, and historical only. The branch has one true content conflict in packages/core/src/Icon/Icon.doc.mjs: the PR adds speaker to three icon-name lists while current main adds chevronsLeft and chevronsRight to those same lists and independently expands the surrounding docs. The mechanical resolution is to preserve all three names plus current-main documentation; it requires no author judgement. Chat/index.ts, defaultIcons.tsx, globalIconRegistry.tsx, and packages/cli/assets/docs/icons.doc.mjs auto-merge in the trial merge.

Tracked exit: awaiting branch conflict resolution before a future full review. No GitHub action was taken.

What changed before posting

Nothing posted. The round was invalidated before any GitHub action because it did not refresh the aged PR from current main before review.

Round 2 — exact-head full review at 3364f381ccd86803c6d8d7c90206d5e04906399d

Pull request · exact head · evidence assets: 8686cb87

PR

#3934 feat(chat): add useChatSpeech hook and ChatSpeechButton for text-to-speech by jiunshinn (bucket: contributor)

HEAD REVIEWED

3364f381ccd86803c6d8d7c90206d5e04906399d

VERSIONS

LOOP VERSION: 1.6.0
AUDIT RUBRIC: 1.13

LANE

LANE: full
WHY: New exported hook, component, types, semantic icon, state, Effects/listener, continuous animation, and public API surface.

PROBLEM

WHY 1: Chat users who cannot or do not want to read an assistant response have no built-in way to hear it.
WHY 2: They must switch tools or abandon a hands-free conversation flow.
WHY 3: Chat is meant to support accessible, low-friction interaction across input and output modalities.

USER-FACING PROBLEM: A person using an Astryx chat cannot ask the product to read an assistant response aloud from the message itself.
PROBLEM SEVERITY: missing capability — issue #1320 and tracker #3635 establish the need.

VERDICT: clear

SOLUTION

The browser speech synthesizer reads caller-supplied message text and reports playback state through one React owner. A message action renders that state and starts or stops playback. A semantic speaker icon and animated bars communicate the idle and speaking endpoints.

SOLUTION (5 decisions · ~350 runtime lines of 1,073 additions)

  1. Export a browser-backed speech controller with voice/rate/pitch/volume, lifecycle callbacks, voices, and speak/stop/pause/resume — serves the TTS capability.
  2. Add isPaused beyond the issue-designed return — explicitly awaiting maintainer confirmation.
  3. Export ChatSpeechButton that consumes one controller plus message text — serves message playback.
  4. Recommend one shared controller across message buttons while deriving every button’s active state from one global isSpeaking boolean — breaks that documented composition.
  5. Add speaker to the semantic icon registry and render fixed-loop bars while speaking — serves the new visual endpoint but leaves every complete theme registry unsynchronized.

BURDEN: high — permanent public surface, global browser state, two Effects, one mount-lifetime listener, hook state/controls, a semantic registry expansion, visual motion, docs, and a browser test matrix.
BURDEN MATCH: proportionate for the missing capability, but the registry and shared-controller paths must work before the surface freezes.

VERDICT: BLOCKS — the shared-controller journey and registry integration are incomplete.

ARCHITECTURE

OWNER: useChatSpeech owns the browser synthesizer lifecycle; ChatSpeechButton owns the message-level rendered affordance.
TIER 1: existing Button, translator, motion-safety, and public DOM contracts. Button is reused; translator and reduced-motion patterns are not.
TIER 2: semantic icons via IconRegistry.
SEAMS: one button; one shared controller across message footers; unsupported hidden/visible; pause/resume; error; unmount; several independent hook owners.
BEHAVIOR UNIT: useChatSpeech — a named external-system hook with focused unit tests.

seam driven result
one interactive button, Chromium start, pause, resume, stop, restart, and unmount cleanup all worked; focus stayed on the activated control
one shared controller, two message buttons fails: after starting the first, both buttons become “Stop reading”/pressed; activating the second records stop, not speak:Second
unsupported, hidden static story renders no control
unsupported, visible renders a focusable, enabled control even though the supplied controller is unsupported
error unit mock clears both state flags and calls onError; a real platform error was not forced

The root cause of the multi-message failure is ownership: the public controller reports only global speech state, while each button needs to know whether its own text/utterance is active. The docs require sharing one controller, so independent hook instances are not the intended escape.

VERDICT: BLOCKS — the lifecycle owner cannot preserve the documented per-message composition seam.

IMPACT

The change is opt-in: existing chats render identically until a builder adds the hook/button. In supported Chromium, one button successfully controls native playback. In the documented shared-controller message list, every message announces and paints itself as active, and selecting another message stops the first without starting the chosen one.

VERDICT: BLOCKS — the primary multi-message journey gives users false state and requires a second action to switch messages.

API

const speech = useChatSpeech();

<ChatMessageMetadata
  footer={<ChatSpeechButton speech={speech} text={content} />}
/>
change public? class doc’d? verdict
+ useChatSpeech(options?: UseChatSpeechOptions): UseChatSpeechReturn yes — @astryxdesign/core/Chat behavior hook + thin component matches useChatDictation/ChatDictationButton hook API in source/JSDoc; button docs blocked by shared-owner state
+ UseChatSpeechOptions: voice?, rate?, pitch?, volume?, onStart?, onEnd?, onError? yes issue-designed speech controls JSDoc mechanically sound
+ return: isSupported, isSpeaking, isPaused, voices, speak, stop, pause, resume yes state + controls JSDoc isPaused needs maintainer judgement
+ ChatSpeechButtonProps: speech, text, size, isHiddenWhenUnsupported, label, BaseProps yes ChatDictationButton sibling component docs active identity missing
+ IconName += 'speaker' yes — Icon subpath microphone semantic sibling Icon docs + CLI docs BLOCK: complete registries not updated

OSSIFICATION: The hook/button concept and tier were already selected in owner-authored issue #1320. isPaused is the only explicitly undecided addition. Adding speaker to the closed IconName union also requires every complete registry to grow in lockstep.

Fourteen complete registries are missing speaker: seven shipped theme packages and seven CLI templates. The exact-head check finds the new union at packages/core/src/Icon/globalIconRegistry.tsx:59 and no matching key in those registries/templates.

VERDICT: BLOCKS — public registry sync is incomplete; [Needs human judgement] remains for isPaused.

THEMING

No existing target or token is removed. Bars use the global accent and full-radius tokens, but the component adds no themeProps('chat-speech-button') target even though current ChatDictationButton carries its component target. The 900ms duration and ease-in-out are literals rather than motion tokens.

VERDICT: BLOCKS — continuous motion ignores reduced motion; target and motion-token parity should be reconciled with the current sibling.

BREAKING

BEHAVIOR: additive and opt-in for existing chat consumers; the documented shared-controller composition itself is broken.
API: yes in practice — widening the closed IconName union makes complete IconRegistry values fail compilation until they add speaker.
VISUAL: additive, intended — idle speaker becomes four animated bars while speaking. No existing component moves.
THEME: existing theme registries do not compile at this head; no existing target or token is removed.

VERDICT: BLOCKS — all complete icon registries and generated theme templates must stay synchronized.

PERFORMANCE & RESOURCES

EFFECTS: two.

Effect + deps external system why render/handler cannot do it measured render cost lifetime + cleanup focused test
useEffect([]) at useChatSpeech.ts:131 speechSynthesis.voiceschanged voices may arrive outside a React event after mount one dispatched voiceschanged produced +1 hook render and +1 commit at N=1 one global listener from mount to unmount; removed on cleanup unit test covers update; Chromium Profiler measured 2→3 renders/commits
useEffect([]) at useChatSpeech.ts:146 global speech synthesizer owner unmount has no event handler that can survive teardown zero render work; cleanup only mount to unmount; calls cancel() unit + Chromium unmount both stop playback

RENDER: initial exact-head harness settled in two StrictMode renders/commits; voiceschanged added one at N=1.
LISTENERS/OBSERVERS: one global listener per mounted hook; docs recommend one shared hook per message list, making realistic N=1; cleanup was verified.
LAYOUT: bars animate compositor-friendly transform; no layout reads or writes. Missing reduced-motion is an accessibility/design block, not a measured layout cost.
BUNDLE: no dependency change; PR Analysis could not complete because the workspace build is red.

VERDICT: clear — measured costs are bounded feature work and cleanup holds.

VISUAL EVIDENCE

VISUAL CHECK: manual frames required
WHY: The PR adds a visible control and a speaking animation; exact-head visual CI did not complete behind the red build, and the multi-message and reduced-motion endpoints are not covered by a green visual gate.

Shipped stories at exact head

Idle Speaking Speaking under reduced motion

Documented shared-controller journey after starting the first message

Both message controls show the active sound-wave state although only speak:First fired. Clicking the second then produces speak:First|stop.

Screenshot sensor receipts

frame story rendered state media geometry settled/error receipt
idle core-chatspeech--idle “Read aloud”, not pressed light/ltr, motion allowed 32×32, visible fonts loaded; 0 animations; 0 errors JSON
speaking core-chatspeech--speaking “Stop reading”, pressed light/ltr, motion allowed 32×32, visible fonts loaded; 4 running animations; 0 errors JSON
speaking, reduce core-chatspeech--speaking “Stop reading”, pressed light/ltr, reduced motion 32×32, visible fonts loaded; 4 running 900ms animations; 0 errors JSON
shared after first isolated review harness both “Stop reading”, both pressed; only speak:First light/ltr, motion allowed two 32×32 controls fonts loaded; 8 running animations; 0 errors JSON

Every public receipt records build 3364f381ccd86803c6d8d7c90206d5e04906399d, Neutral/light, ltr, 900×400@1, desktop pointer media, semantic state, target count, geometry, fonts, animation count, and zero page/Storybook errors.

VERDICT: BLOCKS — reduced-motion still runs all four animations, and shared playback paints every message as active.

REMEDY SEARCH

REMEDY SEARCH: not triggered — author owns HOW; acceptance criteria are sufficient.

The visual defects require no new design choice: current motion conventions require no continuous animation under reduced motion, and only the active message may use the already-chosen speaking treatment.

A11Y & I18N

Checked in Chromium: native button semantics; 32×32 target; Enter/Space activation through Button; focus retained through start/stop/restart; supported lifecycle; unsupported hidden/visible; unmount; and accessible state/name. Exact-head axe did not run because the build failed.

Three objective failures remain. First, the APG toggle-button contract says the label should stay stable when aria-pressed changes; this button changes “Read aloud” to “Stop reading” while also changing aria-pressed, so assistive technology exposes both changes at once. Second, both default labels bypass useTranslator(). Third, reduced-motion still has four running 900ms animations.

Support absence was exercised through the shipped static unsupported stories. Error was verified with the unit mock; a real platform error was not forced.

VERDICT: BLOCKS — toggle semantics, AT-string localization, and reduced-motion handling do not meet the current accessibility contract.

JUDGEMENT

slot verdict
PROBLEM clear
SOLUTION BLOCKS
ARCHITECTURE BLOCKS — shared controller loses per-message identity
IMPACT BLOCKS — every message reports active; switching needs two actions
API BLOCKS — 14 registries missing; isPaused unresolved
THEMING BLOCKS — reduced motion and current target parity missing
BREAKING BLOCKS — complete registries fail compilation
PERFORMANCE clear — measured external-system cost
VISUAL BLOCKS — 4 animations persist under reduced motion; shared state mispaints
A11Y & I18N BLOCKS — toggle label, localization, motion

GOAL: partly met — Chromium proves the complete single-owner lifecycle, but the documented shared-message journey fails and exact-head builds cannot complete.
DISPOSITION: registry sync/build → blocks now; shared active-utterance identity/switching → blocks now; stable toggle semantics + translated labels + reduced motion → blocks now; isPaused → maintainer decision required before public surface freezes.
ADVICE: proven existing patterns + bounded outcome criteria — current ChatDictationButton supplies translator, target, and reduced-motion precedents; one shared controller must expose exactly one active message and switch in one action; every complete icon registry/template must compile.
AUTHOR CAN PROCEED: yes on the verifiable blockers; maintainer still needs to confirm isPaused before approval.
WORST OUTCOME: “Every message announces and paints itself as active, and selecting another message stops playback without starting the one the user chose.” → request changes.

JUDGEMENT NEEDED: API — keep isPaused in the first public return, or drop it until a demonstrated pause/resume UI requires it. Recommendation: keep it; the already-approved pause()/resume() controls otherwise provide no declarative state for a caller’s controls, and Chromium confirms the state is coherent.

request changes

  1. [BLOCKS] Adding speaker leaves every complete theme registry and CLI template stale.
    → a theme builder cannot build the workspace at this head; exact-head CI stops before Storybook/a11y/visual jobs can complete · packages/core/src/Icon/globalIconRegistry.tsx:59
  2. [BLOCKS] The shared controller has no active-message identity.
    → after one message starts, every message says “Stop reading”; choosing another stops playback instead of starting that message · packages/core/src/Chat/ChatSpeechButton.tsx:126-135
  3. [BLOCKS] The active-state accessibility contract is incomplete.
    → localized and reduced-motion users get English-only labels and perpetual motion; screen readers receive a changing label plus changing pressed state · packages/core/src/Chat/ChatSpeechButton.tsx:76-79,127,168

REVIEW

Thanks for picking up the first TTS slice — the hook/button split matches the issue, but I’d hold this before the API ships.

  • Adding speaker to IconName makes every complete registry provide it; the seven theme registries and seven CLI templates still omit it, so CI stops before Storybook/a11y can run.
  • The documented shared-hook pattern does not know which message is active. After one message starts, every speech button becomes pressed/“Stop reading”, and clicking another message stops playback instead of starting that text.
  • The button also needs the current a11y parity before merge: translated default labels, stable toggle semantics if it keeps aria-pressed, and no running wave animation under reduced motion.

Could you tighten those, then I’ll recheck the public API shape? If you'd rather talk it through with someone, we're in Discord.

[Reviewed by Robohands]

INLINE (0–3, only when genuinely line-anchored)

  • packages/core/src/Icon/globalIconRegistry.tsx:59 — Adding a required IconName means every complete theme registry/template needs the same speaker key, or the workspace stops compiling.
  • packages/core/src/Chat/ChatSpeechButton.tsx:131 — With one shared speech, this treats every message button as active, so the second message click calls stop() instead of speaking the second message.
  • packages/core/src/Chat/ChatSpeechButton.tsx:127 — These defaults are AT-facing strings, so they need useTranslator() and a stable toggle-name contract if the button keeps aria-pressed.

EVIDENCE I DID NOT SPEND

  • The isHiddenWhenUnsupported={false} story exposes an enabled no-op control; the older Dictation sibling has similar debt, so it was omitted behind higher-impact findings.
  • BaseProps/ref land on the wrapper rather than the primary button; the current sibling shares that debt, so it is not charged to this contributor.
  • docsZh is English prose; documentation localization is lower priority than the runtime blockers.

CI

  • Exact head has a red CI rollup: build, build-storybook, build-sandbox, Vercel, and visual-acceptance are red/pending; review-required remains pending.
  • Lint, dependency, smoke, component, docsite, and theme-layer checks passed. At the last read, the Actions test job was still in progress, but the build failure was already terminal for merge readiness.
  • The red build path is consistent with the local registry enumeration: speaker is required by IconName and omitted by all 14 complete registry/template files.

TESTS AND EVIDENCE

  • Focused Vitest: 2 files, 20/20 passing.
  • One local build for @astryxdesign/build: pass.
  • Chromium: idle/speaking/reduced-motion/shared-controller frames captured with public-safe sensor receipts.
  • Shared-controller Chromium harness: first click → both controls pressed; second control → stop only.
  • Effect-cost probe: documented N=1, voiceschanged causes +1 render/+1 commit; listener and global speech cleanup both verified.

TIME

TIME total 18m
setup/rules 3m full loop 1.6.0 kit, rubric 1.13, safety screen, fresh wikis, exact-head worktree
build/server 4m fast install, one build, guarded Storybook; warm main reused and untouched
browser/a11y 3m shipped states, reduced motion, shared seam, sensored frames
focused tests 1m one over-scoped invocation plus corrected 2-file/20-test run
code/history 3m full diff/body/issues/reviews, API/design guidance, current main, CI diagnosis
critique/wiki 4m critic pass, final presentation, public-safe evidence assets, page/index update
CI wait 0m terminal build failures were visible before reporting
waste 2m over-scoped Vitest invocation and one Storybook readiness timeout

WHAT I COULD NOT VERIFY

  • Exact-head axe/visual acceptance could not run because the CI build failed first.
  • A real unsupported SpeechSynthesis browser and a real platform synthesis error were not available; both were covered with shipped static stories/unit mocks instead.

What changed before posting

No public PR action taken; this review remains drafted only.

Clone this wiki locally