feat(voice): add Read Aloud button for on-demand TTS - #2173
Conversation
Adds a single Read Aloud button next to the existing Voice Mode/Mic buttons in the chat input footer. Reads everything the assistant said since the user's last turn (not just the latest message), since an agent can emit several consecutive assistant messages — tool calls, intermediate steps, a final reply — before control returns to the user. Reuses the existing voice infrastructure end to end: - getAssistantMessagesAfterLastUser for the turn boundary - useVoiceMode's queueSentence/stopSpeaking for playback - flushForTts for markdown-to-speech normalization and chunking New useReadAloud hook owns its own useVoiceMode() instance and is disabled whenever live Voice Mode is active on the same surface, since the two instances share global voice state but have independent audio playback and can't stop each other's audio. Wired into all three chat surfaces: AiChatView, GlobalAssistantView, and SidebarChatTab. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
📝 WalkthroughWalkthroughRead-aloud support is added across three chat surfaces. Assistant text after the latest user turn is extracted, synthesized into queued audio, coordinated with Voice Mode, and exposed through a new footer control with playback and eligibility state. ChangesRead-aloud assistant playback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InputFooter
participant useReadAloud
participant readAloudPlayer
participant VoiceSynthesisAPI
participant AudioContext
User->>InputFooter: Click Read aloud
InputFooter->>useReadAloud: toggleReadAloud(messages)
useReadAloud->>readAloudPlayer: startReadAloud(chunks)
readAloudPlayer->>VoiceSynthesisAPI: POST /api/voice/synthesize
VoiceSynthesisAPI-->>readAloudPlayer: Audio data
readAloudPlayer->>AudioContext: Decode and play chunk
AudioContext-->>readAloudPlayer: onended
readAloudPlayer-->>InputFooter: Update isPlaying
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99d0a02d49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * independent audio playback, so one can't stop audio started by the other. | ||
| */ | ||
| export function useReadAloud() { | ||
| const { isSpeaking, queueSentence, stopSpeaking } = useVoiceMode(); |
There was a problem hiding this comment.
When the main chat and sidebar are mounted together, or Voice Mode is active under another surface's owner, every useReadAloud instance observes the same global isSpeaking and currentAudioId, while its audio source and queue remain local to that hook instance. Starting playback in one surface therefore marks every surface as reading; clicking Stop elsewhere clears the global state but cannot stop the original audio, allowing stale or overlapping playback and causing in-flight synthesis to be discarded. Track read-aloud ownership in the store or centralize the audio playback rather than treating global isSpeaking as local state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16db1f7: replaced the per-instance useVoiceMode() approach with a module-level playback singleton (apps/web/src/lib/voice/readAloudPlayer.ts) that owns the one real AudioContext/audio source. Every useReadAloud() call site — main chat, dashboard, and sidebar — now subscribes to that singleton via useSyncExternalStore instead of holding its own audio. Starting or stopping from any mounted surface now acts on the same audio, so there's no longer a second instance to desync from. Also switched the Voice-Mode-active gate in all three views from the surface-scoped isVoiceModeActive to the global isVoiceModeEnabled, since a live call on a different surface still plays through its own separate AudioContext and would have overlapped with read-aloud audio otherwise. Covered by a new test (readAloudPlayer.test.ts) that specifically asserts a stop triggered from one subscriber is observed by another, simulating the cross-surface case. Leaving this thread open for verification.
| [isSpeaking, stopSpeaking, queueSentence] | ||
| ); | ||
|
|
||
| return { isReadingAloud: isSpeaking, toggleReadAloud }; |
There was a problem hiding this comment.
When a user navigates away while read-aloud audio is playing, useVoiceMode's unmount cleanup stops and detaches the local AudioBufferSourceNode, but it does not reset the shared store's speaking state; detaching the source also prevents its onended handler from doing so. Since this wrapper registers no ownership-aware cleanup, the next chat surface remains stuck showing “Stop reading aloud,” and the first click only clears the stale state instead of starting playback. Release the global speaking state on unmount when this hook owns the active audio.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 16db1f7, by the same change as the sibling thread above: read-aloud playback no longer lives in useVoiceModeStore's shared speaking state at all. The new apps/web/src/lib/voice/readAloudPlayer.ts module singleton tracks its own playing flag independent of any component's mount lifecycle, so there's nothing for an unmounting surface to leave stuck — the singleton keeps existing and playing (or not) regardless of which component unmounts, and every remaining/future subscriber reads the real, current state via useSyncExternalStore. Leaving this thread open for verification.
Addresses two Codex review findings on useReadAloud.ts (PR #2173): 1. Multiple mounted chat surfaces (e.g. the right-sidebar chat tab alongside the main AiChatView/GlobalAssistantView content) each ran their own useVoiceMode() instance. All instances shared global voiceState/isSpeaking via the store, but each held its own local AudioContext/audioSource — so Stop clicked on one surface cleared shared state but couldn't reach audio actually playing from another surface's instance. 2. useVoiceMode's unmount cleanup stopped that instance's own audio but never reset the shared store's speaking state, so navigating away mid-playback could leave every other surface stuck showing "Stop reading aloud". Fix: apps/web/src/lib/voice/readAloudPlayer.ts is a module-singleton player (one AudioContext, one audioSource, one queue) completely decoupled from useVoiceMode/useVoiceModeStore's voiceState. Every useReadAloud() call site now just subscribes to it via useSyncExternalStore, so starting or stopping from any surface acts on the one real audio source everywhere. Also switches the "disable while Voice Mode is active" gate in all three chat views from the surface-scoped isVoiceModeActive to the global isVoiceModeEnabled flag — a live call on a DIFFERENT surface still plays through its own separate AudioContext and would overlap with read-aloud audio, which the surface-scoped check missed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
Existing tests only checked synchronous start/stop state transitions. Adds coverage for the actual promise chain: a chunk synthesizing and playing, multiple chunks playing back to back via onended, a failed synthesis being skipped without aborting the rest of the queue, and a chunk that finishes synthesizing after stop() was already called being correctly discarded rather than resurrecting playback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16db1f7935
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const buffer = await synthesize(text); | ||
| // Stopped while this chunk was being synthesized — discard the result. | ||
| if (!playing) return; |
There was a problem hiding this comment.
When a user stops while synthesis is pending and starts Read Aloud again before the old request resolves, the new run sets the shared playing flag back to true, so this check accepts the stale buffer. The old and new sources can then play simultaneously, and the stale run can overwrite audioSource or advance the new queue. Track a per-run generation/token and verify it after every await instead of relying only on the shared boolean.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in b6f3a1a with a monotonic generation token. Each playNext() call now captures the current generation and re-checks it after every await (both after synthesis and inside onended), instead of relying on the shared playing boolean alone. stopReadAloud() (called both directly and internally by startReadAloud()) bumps the token, so a stale run's synthesis resolving after a newer run has already begun is discarded rather than creating a second, overlapping AudioBufferSourceNode. Added a regression test that reproduces the exact interleaving you described (stop, then restart, then the stale run's fetch resolves after the new run has already started) and asserts only the new run ever creates a source.
| const canReadAloud = useMemo( | ||
| () => !isVoiceModeEnabled && getTextSinceLastUserTurn(plainMessages).trim().length > 0, | ||
| [plainMessages, isVoiceModeEnabled] |
There was a problem hiding this comment.
If Read Aloud is already playing and the user enters Voice Mode, this predicate merely prevents another read from starting; none of the three added handleVoiceModeToggle paths calls stopReadAloud. VoiceCallPanel then auto-starts microphone capture while the singleton audio continues, so synthesized speech can overlap with and be recorded as user input. Stop the shared player when Voice Mode is enabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b6f3a1a: all three handleVoiceModeToggle implementations (AiChatView, GlobalAssistantView, SidebarChatTab) now call stopReadAloud() in the enabling branch, before enableVoiceMode(). Since this runs in whichever surface's own toggle handler is clicked, it covers the cross-surface case too — read-aloud gets stopped no matter which surface's Voice Mode button triggers the enable.
| onClick={isVoiceProGated ? undefined : onReadAloudClick} | ||
| disabled={disabled || isVoiceProGated || (!isReadingAloud && !canReadAloud)} |
There was a problem hiding this comment.
When a user starts Read Aloud and then sends a new message, ChatInput passes disabled=true while the AI streams, and this condition disables the only local control that can stop the ongoing audio. For a long response or slow generation, the user is forced to keep listening until playback or streaming finishes; the button should remain actionable when isReadingAloud is true even if the rest of the footer is disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b6f3a1a: onClick and disabled now both special-case isReadingAloud — disabled is forced to false whenever isReadingAloud is true (bypassing the footer's disabled/Pro-gating/canReadAloud checks entirely), so Stop stays clickable even while a new message is streaming or the rest of the footer is otherwise disabled.
Addresses three fresh Codex findings on commit 16db1f7 (PR #2173): 1. (P1) readAloudPlayer.ts: a stop-then-restart while the stopped run's synthesis was still in flight could let the stale run's audio start playing after the new run had already begun, since both checked the same shared `playing` boolean. Added a monotonic `generation` token, captured per playNext() call and re-checked after every await, so a stale run is discarded even when a newer run has since flipped `playing` back to true. New regression test reproduces the exact interleaving (stale fetch resolves after the new run has started) and asserts only the new run ever creates an audio source. 2. (P1) None of the three handleVoiceModeToggle implementations (AiChatView, GlobalAssistantView, SidebarChatTab) stopped read-aloud playback before enabling Voice Mode, so VoiceCallPanel's mic capture could start while read-aloud audio was still playing — synthesized speech could get picked up as if it were user input. Now calls stopReadAloud() before enabling. 3. (P2) InputFooter's Read Aloud button was fully disabled whenever the footer's `disabled` prop was true (e.g. while a new message is streaming), which meant the only control that could stop ongoing playback became unreachable if the user sent a follow-up message mid-read. The button now stays enabled/clickable whenever isReadingAloud is true, regardless of the rest of the footer's disabled state. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
A 4-agent /simplify pass over the read-aloud diff (reuse, simplification, efficiency, altitude) surfaced four worthwhile changes: 1. (reuse) readAloudPlayer.ts's hand-rolled listener-set/notify/subscribe triad duplicated what zustand's create() already provides — and the sibling useVoiceModeStore.ts, one directory over, solves the exact same "shared cross-component voice state" problem that way already. Replaced it with a tiny zustand store holding just the observable `isPlaying` boolean; the actual audio machinery (AudioContext, queue, generation token) stays in plain module variables outside React, unchanged. 2. (altitude) The "disable Read Aloud while Voice Mode is enabled" check was duplicated verbatim in all 3 views. Folded into useReadAloud() itself as a canReadAloud(messages) function, so it's one property of the hook instead of three copies that could drift. 3. (altitude) The stopReadAloud() call added to fix Voice Mode overlap only existed in the 3 views' own handleVoiceModeToggle callbacks — meaning any future 4th entry point calling useVoiceModeStore.enable() directly would silently reintroduce the overlap bug. Moved enforcement into readAloudPlayer.ts itself via useVoiceModeStore.subscribe(), so it holds regardless of how/where Voice Mode gets enabled. This let the 3 views' duplicated stopReadAloud() calls be removed entirely. 4. (efficiency) canReadAloud was joining the full reply text with getTextSinceLastUserTurn just to check non-empty, recomputed on every render including every token of a live stream. Added hasTextSinceLastUserTurn(), which short-circuits on the first non-empty text part without building any string; canReadAloud now uses it. Also fixed two smaller things surfaced by the same pass: - InputFooter.tsx's Read Aloud button had a redundant onClick ternary (the disabled attribute already prevents the click from ever firing in the case the ternary tried to guard against). - readAloudPlayer.ts's onended handler had an `audioSource === source` check that can never be false given the generation-token invariant; removed with a comment explaining why. While rewriting stopReadAloud() for the zustand store I initially dropped the "only notify if something was actually playing" guard, which would have doubled subscriber notifications on every restart — caught by the existing test suite (one test failed) and fixed before committing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
…t time CI caught this immediately: AiChatView.test.tsx and AiChatView.realConversations.test.tsx mock @/stores/useVoiceModeStore as a bare vi.fn() selector (no .subscribe/.getState statics — the real zustand store has both, but nothing in those tests exercises the store's other methods). The previous commit's module-level `useVoiceModeStore.subscribe(...)` call in readAloudPlayer.ts ran the instant the module was imported, so merely rendering AiChatView (which now transitively imports readAloudPlayer via useReadAloud) crashed both test files at collection time with "subscribe is not a function" — 0 tests collected, both FAIL. Fix: register the subscription lazily, on first startReadAloud() call, guarded by a one-time flag, instead of unconditionally at module evaluation. In production useVoiceModeStore is always the real store, so this only delays registration to the moment it's actually needed — by the time any read-aloud audio could exist, startReadAloud() has always already run, so the invariant "Voice Mode enabling stops read-aloud" still holds regardless of entry point. Tests that import/render a consumer but never actually trigger playback (like the two above) never touch useVoiceModeStore.subscribe at all. Verified: both previously-crashing test files pass (74 tests total across AiChatView.test.tsx + AiChatView.realConversations.test.tsx), plus the full readAloudPlayer/getTextSinceLastUserTurn suite (33 tests). Ran the entire apps/web unit suite; the only other failures are pre-existing, unrelated to this branch (DB-connection-dependent tests failing locally because this worktree has no Postgres configured, and one timezone-dependent message- grouping test) — confirmed zero references to voice/read-aloud code in any of those files, and CI (which has a real database) did not flag them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx (1)
1-1: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMemoize
canReadAloudfor consistency with the sibling memoized derivations.All three surfaces compute
canReadAloud = canReadAloudFor(plainMessages)as a plain call on every render, while the neighboringlastAssistantMessageId/lastUserMessageIdderivations in each of these same files wrap the identicalplainMessagesdependency inuseMemo.hasTextSinceLastUserTurnshort-circuits so the per-call cost is small, but it's still an avoidable rescan on every unrelated re-render.
apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx#L585-593: wrapcanReadAloudinuseMemo(() => canReadAloudFor(plainMessages), [canReadAloudFor, plainMessages]).apps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsx#L440-448: sameuseMemowrap.apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsx#L546-554: sameuseMemowrap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx` at line 1, Memoize the canReadAloud derivation in AiChatView, GlobalAssistantView, and SidebarChatTab using useMemo with canReadAloudFor and plainMessages as dependencies, matching the neighboring memoized message derivations and preserving the existing computed value.apps/web/src/lib/voice/readAloudPlayer.ts (1)
63-101: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCancel stale synthesis requests when playback stops.
fetchWithAuthalready forwardsRequestInit, so wire anAbortControllerthroughsynthesize()and abort it fromstopReadAloud()to stop billed TTS requests immediately instead of only dropping their results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/voice/readAloudPlayer.ts` around lines 63 - 101, Update synthesize() to accept an AbortSignal and pass it in the fetchWithAuth() RequestInit options, creating and tracking the active AbortController for each playback synthesis request. In stopReadAloud(), abort the active controller before clearing playback state, and ensure the controller reference is cleaned up when synthesis completes so stale requests are cancelled rather than merely ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx`:
- Line 1: Memoize the canReadAloud derivation in AiChatView,
GlobalAssistantView, and SidebarChatTab using useMemo with canReadAloudFor and
plainMessages as dependencies, matching the neighboring memoized message
derivations and preserving the existing computed value.
In `@apps/web/src/lib/voice/readAloudPlayer.ts`:
- Around line 63-101: Update synthesize() to accept an AbortSignal and pass it
in the fetchWithAuth() RequestInit options, creating and tracking the active
AbortController for each playback synthesis request. In stopReadAloud(), abort
the active controller before clearing playback state, and ensure the controller
reference is cleaned up when synthesis completes so stale requests are cancelled
rather than merely ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f142f277-7742-478f-b384-6dda93c8124f
📒 Files selected for processing (10)
apps/web/src/components/ai/chat/input/ChatInput.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsxapps/web/src/components/ui/floating-input/InputFooter.tsxapps/web/src/hooks/useReadAloud.tsapps/web/src/lib/ai/streams/__tests__/getTextSinceLastUserTurn.test.tsapps/web/src/lib/ai/streams/getTextSinceLastUserTurn.tsapps/web/src/lib/voice/__tests__/readAloudPlayer.test.tsapps/web/src/lib/voice/readAloudPlayer.ts
Two nitpicks from CodeRabbit's first non-rate-limited review (on ad8090d): 1. (quick win) synthesize() now creates an AbortController per request and passes its signal to fetchWithAuth; stopReadAloud() aborts it. Previously stopping mid-synthesis only discarded the result via the generation-token check — the billed OpenAI TTS request kept running to completion in the background regardless. The abort is scoped to "still the active controller" in a finally block so it can't clobber a newer run's controller during the stop-then-restart race the generation token already guards against. New test asserts the captured AbortSignal is aborted after stopReadAloud(). 2. (trivial, consistency) canReadAloud is now wrapped in useMemo in all 3 views, matching the neighboring lastAssistantMessageId/lastUserMessageId derivations that memoize off the same plainMessages dependency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
|
Addressed both nitpicks in 43bd19c:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43bd19c650
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| variant="ghost" | ||
| size="sm" | ||
| onClick={onReadAloudClick} | ||
| disabled={!isReadingAloud && (disabled || isVoiceProGated || !canReadAloud)} |
There was a problem hiding this comment.
When the basic Mic control is already listening—or is started while playback is active—this condition still permits Read Aloud. ChatInput's useSpeechRecognition callback (lines 178–183) appends whatever the microphone hears to the draft, so speaker output from TTS can be transcribed back into the user's input. The commit only makes Read Aloud mutually exclusive with Voice Mode; the ordinary Mic and Read Aloud controls should also stop or disable one another.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this is a real gap, and separate from the Voice Mode mutual-exclusivity already in place. Fixed in 6584ad0: ChatInput.tsx now wraps the mic-toggle handler to call stopReadAloud() before starting to listen, and InputFooter.tsx's Read Aloud button is disabled while isListening is true (an existing prop already used for the Mic button itself), so it can't be started while dictating either. Didn't add a dedicated component test for this specific wiring since it's two lines of logic calling an already-thoroughly-tested stopReadAloud() — verified via typecheck plus the existing ChatInput/AiChatView/readAloudPlayer suites (80 tests, no regressions).
…on too Codex found this on the latest commit (43bd19c): the basic Mic control (useSpeechRecognition, ChatInput.tsx) transcribes whatever it hears straight into the draft via onTranscript, with zero coordination against Read Aloud's TTS audio. The previous fixes only made Read Aloud mutually exclusive with the full Voice Mode call feature — Mic dictation is a separate, per-ChatInput-instance control that could still run at the same time as Read Aloud, letting the assistant's own spoken reply get transcribed back into the user's input. Fix, mirroring the Voice Mode approach in both directions: - ChatInput.tsx: wraps the mic-toggle handler to call stopReadAloud() before starting to listen. - InputFooter.tsx: the Read Aloud button is now also disabled while isListening (an existing prop, already used for the Mic button's own styling) is true, so it can't be started while dictating either. Not adding a dedicated component test for this — the wrapper is two lines of trivial logic calling an already-thoroughly-tested stopReadAloud(), and a full ChatInput render-test harness for this narrow interaction felt like disproportionate effort for a P2 finding. Verified via typecheck plus the existing ChatInput/AiChatView/readAloudPlayer test suites (80 tests, all passing, no regressions). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6584ad0dcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function useReadAloud() { | ||
| const isReadingAloud = useReadAloudPlayerStore((s) => s.isPlaying); | ||
| const isVoiceModeEnabled = useVoiceModeStore((s) => s.isEnabled); |
There was a problem hiding this comment.
On a fresh page load where the user previously selected a non-default TTS voice, this hook never calls useVoiceModeStore.loadSettings(), while that initialization currently occurs only when useVoiceMode() mounts inside the conditionally rendered Voice Mode panel. Consequently, using Read Aloud before opening Voice Mode causes readAloudPlayer.synthesize() to read the store's default nova voice rather than the persisted choice; initialize the voice store when this hook mounts or before the first synthesis.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac734cb: readAloudPlayer.startReadAloud() now calls useVoiceModeStore.getState().loadSettings() on every start (idempotent and cheap, so not gated behind a one-time flag like the mutual-exclusion subscriptions below it), so a persisted TTS voice/speed choice is honored even if the user has never opened the full Voice Mode panel this session.
| onClick={onReadAloudClick} | ||
| disabled={!isReadingAloud && (disabled || isVoiceProGated || !canReadAloud || isListening)} |
There was a problem hiding this comment.
When the main chat and sidebar are mounted together, each ChatInput owns an independent useSpeechRecognition() instance, so isListening here describes only this footer. If dictation is active in one surface, the other surface still enables Read Aloud and can start TTS while the first microphone remains live, allowing the spoken reply to be transcribed into that draft. Use shared dictation activity or otherwise stop active recognizers before starting playback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in ac734cb. Added a small shared useDictationActivityStore (apps/web/src/hooks/useSpeechRecognition.ts) that every useSpeechRecognition() instance feeds via an activeCount (not a boolean, so two simultaneously-listening surfaces can't have one's stop clear the other's still-active state). readAloudPlayer now subscribes to it the same way it already does for Voice Mode, stopping playback the moment dictation starts on ANY surface, and useReadAloud()'s canReadAloud/toggleReadAloud check it too — reactively, so this component's own button correctly reflects a different surface's dictation state. That made the local isListening prop check in InputFooter's Read Aloud button fully redundant (a strict subset of the global check), so it's removed.
| export function stopReadAloud(): void { | ||
| generation++; | ||
| activeAbortController?.abort(); |
There was a problem hiding this comment.
When Stop is clicked while a chunk is being synthesized, this aborts only the browser's request. The /api/voice/synthesize handler starts its OpenAI request without request.signal and then executes AIMonitoring.trackUsage, so the server can finish the discarded synthesis and debit the user's credits even though playback was stopped. Propagate client disconnection through the route's upstream fetch so stopping the in-flight chunk actually cancels the billable work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac734cb: the route now passes signal: request.signal to the upstream OpenAI fetch, so a client-cancelled request actually cancels the provider call too, not just the client's own wait for a response. The existing hold-release-on-non-billed-path finally block already handles the resulting abort correctly (holdHandedOff stays false since trackUsage is never reached), so no other changes were needed there. Added a route test asserting the captured signal reflects the abort and that the hold is released without billing.
… billing on stop Three findings from Codex's review of 6584ad0 (PR #2173): 1. (P2) Load persisted voice settings before Read Aloud starts. synthesize() reads ttsVoice/ttsSpeed straight from useVoiceModeStore, but that store's loadSettings() (hydrates from localStorage) previously only ran when useVoiceMode() mounted inside the Voice Mode panel — never for Read Aloud, which no longer depends on that hook at all. A user's persisted voice choice would silently be ignored the first time they used Read Aloud without ever opening Voice Mode. Now called on every startReadAloud() (idempotent and cheap, so no one-time guard needed). 2. (P2) Coordinate mic dictation across mounted chat surfaces. The basic Mic control (useSpeechRecognition) is local per-ChatInput-instance state, unlike Voice Mode's single global store — so the earlier same-surface-only fix missed the case where a DIFFERENT surface's dictation is active while this surface starts Read Aloud, letting TTS audio get transcribed into that other draft. Added a small shared useDictationActivityStore (a count, not a boolean, so two simultaneously-listening surfaces don't have one's stop clear the other's still-active state) that every useSpeechRecognition() instance feeds. readAloudPlayer subscribes to it the same way it already does for Voice Mode, and useReadAloud()'s canReadAloud/toggleReadAloud now check it too — which also let the now-redundant local isListening prop check in InputFooter's Read Aloud button be removed. 3. (P2) Propagate the client's Stop cancellation to the OpenAI request itself. The previous AbortController fix only cancelled the browser's own fetch; /api/voice/synthesize started its upstream OpenAI request without forwarding request.signal, so a stopped chunk's provider call (and its billing) ran to completion regardless. Route now passes `signal: request.signal` to the upstream fetch — the existing hold-release-on-non-billed-path logic already handles the resulting abort correctly with no other changes needed. Also required a `@vitest-environment node` pragma on the synthesize route test file: it tests server code, but ran under the project's jsdom default, which has its own competing AbortController/AbortSignal globals — jsdom's Request constructor rejected a real AbortController's signal as "not an instance of AbortSignal" once a test actually exercised one. node is the more accurate environment for testing a route handler, not a workaround. Verified via typecheck plus the full related suite (92 tests: readAloudPlayer, both voice API routes, getTextSinceLastUserTurn, AiChatView ×2, ChatInput), all passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
…code gate useReadAloud.ts ended up using the reactive useDictationActivityStore((s) => s.activeCount > 0) selector directly instead of the standalone isDictationActive() helper, leaving it an unused export — caught immediately by the blocking knip:check gate on CI. Removed; useDictationActivityStore itself is still exported and used. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 836db6c0b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stopReadAloud(); | ||
| if (chunks.length === 0) return; | ||
| queue = [...chunks]; | ||
| setPlaying(true); |
There was a problem hiding this comment.
If Voice Mode or dictation becomes active after a chat surface renders but before its toggleReadAloud callback runs—for example, during rapid clicks across the mounted sidebar and main chats—the callback can still have blocked === false. startReadAloud does not inspect the stores' current state, and the subscriptions registered here only stop playback on a future inactive-to-active transition, so TTS can start while microphone capture is already active and be transcribed as user speech. Check the live Voice Mode and dictation state before queueing playback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in f73c45d. startReadAloud() now independently re-checks useVoiceModeStore.getState().isEnabled and useDictationActivityStore.getState().activeCount right before actually starting playback, closing the gap regardless of a stale caller closure or the subscriptions only catching future transitions. Added tests for both already-active cases.
| body: JSON.stringify({ text, voice: ttsVoice, speed: ttsSpeed }), | ||
| signal: controller.signal, | ||
| }); | ||
| if (!response.ok) return null; |
There was a problem hiding this comment.
When /api/voice/synthesize returns a systemic error such as out-of-credits (402), concurrency limiting (429), or unavailable configuration (503), this converts it to null, and playNext treats it as a skippable chunk. The control therefore stops with no explanation, and multi-chunk replies repeat the same doomed request for every remaining chunk. Propagate non-success responses to user-visible error handling and stop the queue rather than treating all HTTP failures as isolated audio-decoding failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f73c45d: a non-ok response now surfaces the server's error message via toast.error() and stops the whole run (through the existing generation-token mechanism), rather than converting it to null and having playNext() retry-skip every remaining chunk against the same doomed request. Network/decode failures in the catch block stay skip-and-continue, since those can genuinely be per-chunk-transient. New test asserts the toast fires with the server's message and that a second chunk is never even attempted.
…s failures Two more findings from Codex's review of 836db6c (PR #2173): 1. (P2) Recheck active microphone modes before starting playback. useReadAloud()'s toggleReadAloud can hold a stale `blocked` React closure, and readAloudPlayer's mutual-exclusion subscriptions only fire on a FUTURE inactive-to-active transition — neither catches "Voice Mode or dictation was ALREADY active by the time this specific startReadAloud() call runs" (e.g. rapid clicks across two mounted surfaces). startReadAloud() now independently re-checks live useVoiceModeStore/useDictationActivityStore state right before actually starting, closing the gap regardless of caller staleness. 2. (P2) Surface synthesis failures instead of silently skipping them. A systemic /api/voice/synthesize failure (out of credits, rate-limited, misconfigured) was converted to null and treated as one skippable chunk — every remaining chunk in a multi-chunk reply would then fail identically and silently, with the control just going quiet. Non-ok responses now surface the server's error message via toast.error() and stop the whole run (via the existing stopReadAloud()/generation-token mechanism) rather than retry-skipping a doomed request per chunk. Network/decode failures in the catch block remain skip-and-continue, since those genuinely can be per-chunk-transient. Verified via typecheck plus readAloudPlayer's suite (20 tests, +3 new: live Voice-Mode-already-on, live-dictation-already-active, and the systemic failure toast+stop behavior) and the full related suite (95 tests total), all passing. knip clean (5 baseline issues, no new ones). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f73c45ddc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| return null; |
There was a problem hiding this comment.
When fetchWithAuth, arrayBuffer, resume, or decodeAudioData throws for any reason other than an intentional Stop abort, this catch converts the failure to null; playNext then silently advances to the next chunk. A transient network or audio-decoding failure can therefore omit part of the assistant's reply—or produce no audio at all for a single-chunk reply—without notifying the user. Distinguish aborts from real failures and surface/stop or retry the latter rather than silently skipping them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in c5c343e, and it caught a real bug while I was writing the test for it: the catch block now distinguishes an intentional Stop-triggered AbortError (silent, expected) from any other exception (surfaced via toast + stopped, matching the non-ok-response treatment). I threaded the run's generation-token ID into synthesize() so both failure paths only toast/stop for a run that's still current — while writing that test I found the earlier version called stopReadAloud() unconditionally on any non-abort failure, so a stale, already-superseded run failing for its own unrelated reason after a stop-then-restart would incorrectly tear down a NEWER, legitimately-playing run. Both the toast and the stop are now gated behind the same current-run check.
…p bug Codex's review of f73c45d (PR #2173) found the catch block still silently dropped genuine failures — the earlier fix only handled non-ok HTTP responses, not exceptions from fetchWithAuth/arrayBuffer/resume/ decodeAudioData. Any of those throwing (network failure, a corrupted or undecodable response, AudioContext.resume() failing) got converted to null and treated as a skippable chunk, silently truncating or fully dropping a reply with zero explanation. Distinguished an intentional Stop-triggered AbortError (expected, silent) from any other exception (now gets the same surface-and-stop treatment as a non-ok response). Threaded the run's generation-token ID into synthesize() so both failure paths only toast/stop for a run that's still current — this caught a real bug while writing the test for it: the previous version called stopReadAloud() unconditionally on any non-abort failure, so a STALE, already-superseded run (from a stop-then-restart) independently failing for its own unrelated reason would incorrectly tear down a newer, legitimately-playing run. Both the toast and the stop are now gated behind the same "is this run still current" check, making a stale run's failure a full no-op rather than a spurious toast or interruption. Removed a now-outdated test whose entire premise (skip a failed HTTP response and continue to the next chunk) was the exact behavior the "surface systemic failures" fix from the previous commit intentionally replaced. Added tests for: catching a genuine exception (toast + stop), an intentional abort producing no toast, and — the one that caught the bug — a stale run's own independent failure producing no toast AND not disturbing a newer run already in progress. Verified via typecheck plus readAloudPlayer's suite (22 tests) and the full related suite (97 tests total), all passing. knip clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5c343e943
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * so this is a pre-check for starting a new read, not the only guard. | ||
| */ | ||
| export function useReadAloud() { | ||
| const isReadingAloud = useReadAloudPlayerStore((s) => s.isPlaying); |
There was a problem hiding this comment.
When the user closes the sidebar or navigates away from the last mounted chat surface during synthesis or playback, this hook only unsubscribes from the Zustand store and never calls stopReadAloud(). Because the fetch, queue, and audio source are module-level singletons, synthesis can continue to consume credits and the reply can continue playing after every Stop control has disappeared; add ownership/ref-counted cleanup or a navigation/session-level stop without interrupting playback while another surface remains mounted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 7b5edc2. Added a ref count of currently-mounted useReadAloud() consumers; stopReadAloud() only fires when the count drops to zero, so closing just the sidebar while the main chat stays open correctly leaves a main-chat-initiated read running, but navigating to a route with no chat surface at all now stops playback (and the in-flight synthesis) instead of leaving it running with no reachable Stop control. New test covers both the single-consumer and one-of-several-vs-last-of-several cases.
| // Created before the network await so the browser still credits this | ||
| // AudioContext to the user gesture that triggered playback. | ||
| const ctx = getAudioContext(); |
There was a problem hiding this comment.
When new AudioContext() throws—for example because Web Audio is unavailable or the browser's context quota is exhausted—the call occurs before synthesize() enters its try block. The resulting rejection escapes through the fire-and-forget playNext() call after isPlaying was set to true, leaving the UI permanently showing a playing/Stop state with no audio or error toast; create the context inside the handled failure path and reset the run on failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7b5edc2: moved new AudioContext() inside synthesize()'s try block, so a creation failure now gets the same surface-and-stop handling (toast + stopReadAloud()) as any other synthesis failure, instead of rejecting the fire-and-forget playNext() call unhandled and leaving isPlaying stuck true forever. Added a test in its own file (readAloudPlayer.audioContextFailure.test.ts) — the module's AudioContext is a lazy singleton that's never recreated once successfully created, so a throwing stub needed a fresh module registry rather than sharing the main test file where earlier tests already cache a working context.
…n failure Two more findings from Codex's review of c5c343e (PR #2173): 1. (P2) Stop playback when the final read-aloud surface unmounts. The player is deliberately independent of any single component's lifecycle (that's the point of the module singleton), but if EVERY mounted chat surface unmounts — e.g. the user navigates to a route with no chat UI at all — there's no longer a Stop control reachable anywhere, while synthesis keeps running (and billing) and audio keeps playing. Added a ref count of currently-mounted useReadAloud() consumers; stopReadAloud() only fires when the count drops to zero, so closing just the sidebar while the main chat stays open correctly leaves a main-chat-initiated read running. 2. (P2) Handle AudioContext creation failures. `new AudioContext()` ran before synthesize()'s try block, so a creation failure (quota exhausted, Web Audio unsupported) rejected the fire-and-forget playNext() call unhandled, leaving isPlaying stuck true forever with no audio and no explanation. Moved the call inside the try block so it gets the same surface-and-stop handling as any other failure. New tests: apps/web/src/hooks/__tests__/useReadAloud.test.ts covers the ref-counted unmount behavior (single consumer, and one-of-several vs. last-of-several). The AudioContext-failure test lives in its own file (readAloudPlayer.audioContextFailure.test.ts) rather than the main readAloudPlayer.test.ts — the module's AudioContext is a lazy singleton that, once created by an earlier test, is never recreated, so a throwing stub registered in a later test in the same file would never actually be invoked; a fresh file gives Vitest a fresh module registry instead. Verified via typecheck plus the full related suite (100 tests total, all passing). knip clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
getTextSinceLastUserTurnhelper joins all of them.apps/web/src/lib/voice/readAloudPlayer.ts), decoupled from any single component's lifecycle. This app can have multiple chat surfaces mounted at once (the right-sidebar chat tab alongside the main AiChatView/GlobalAssistantView content); everyuseReadAloud()call site subscribes to the one real audio source via a tiny Zustand store, so starting or stopping — or a failure — from any surface acts on the same audio.useDictationActivityStore(a count, not a boolean) letsreadAloudPlayerknow when dictation is active on ANYChatInputinstance, not just the current one.getAssistantMessagesAfterLastUserfor the turn boundary,flushForTtsfor markdown-to-speech normalization/chunking. No new TTS API routes — talks directly to the existing/api/voice/synthesize(OpenAI TTS, Pro+ gated), now updated to forward the caller's abort signal upstream./api/voice/synthesizeendpoint.Review history
Seven rounds of Codex/CodeRabbit automated review findings were addressed and replied to inline — every thread has a concrete reply explaining the fix and is left open for reviewer verification (not self-resolved, since all were fixed during this same review cycle):
useVoiceMode()couldn't be stopped cross-surface, and unmount never reset shared state → centralized into the module singleton.stopReadAloud(); Stop control unreachable during streaming → footer disabled-state fix./simplifypass: hand-rolled pub-sub → Zustand store (matching the siblinguseVoiceModeStoreconvention); voice-mode-exclusivity folded into the hook; centralized enforcement via a store subscription instead of duplicated call-site fixes; dead ternary/redundant check removed. (Caught and fixed a self-introduced double-notify regression along the way.)useDictationActivityStore.startReadAloudcould start even if Voice Mode/dictation had just become active with no transition to observe → live-state recheck at start; systemic synthesis failures (non-ok responses) were silently skip-retried → surfaced via toast + stop.stopReadAloud()was firing unconditionally on any non-abort failure, so a stale run's own independent failure could tear down a newer, legitimately-playing run — fixed by gating both the toast and the stop behind "is this run still current."AudioContextcreation failures rejected unhandled outside the try block, leaving the UI stuck showing "Stop" forever → moved inside the handled path.Latest Codex review (commit
7b5edc2e3): "Didn't find any major issues. Breezy!" — first fully clean pass after the above.Test plan
bun run typecheck(apps/web) — cleanbun run lint(apps/web) — no new warningsbun run knip:check— clean, no new dead-code findingsbunx vitest run— 100 tests acrossreadAloudPlayer(+ a dedicated AudioContext-failure file),useReadAloud(unmount lifecycle), both voice API routes,getTextSinceLastUserTurn,AiChatView×2,ChatInput— all passing, including several regression tests that caught real races/bugs during this review cyclemasterwith zero conflicts, verified repeatedly across 7 pushesDATABASE_URL/env config in the worktree) — recommend clicking through on a branch deploy or locally with env configured: send a prompt that triggers a tool call before the final reply, click Read Aloud, confirm it speaks all assistant turns in order; confirm Stop works immediately (including mid-stream and mid-synthesis-failure); confirm entering Voice Mode or starting mic dictation while Read Aloud is playing stops it first, from any surface; try it with both the sidebar chat tab and the main content view open at once; confirm navigating away entirely stops playback.🤖 Generated with Claude Code
https://claude.ai/code/session_01NHGBY8FgjREJXDGaPRAyig