-
Notifications
You must be signed in to change notification settings - Fork 2
codex/audit-audio-flow-for-voice-mode-issues #529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
codex/audit-audio-flow-for-voice-mode-issues #529
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0de5823ce2
ℹ️ 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".
| if (!isEnabled || hasStartedOnActivateRef.current) return; | ||
| hasStartedOnActivateRef.current = true; | ||
| startListening(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard auto-start against concurrent listen attempts
Starting startListening() unconditionally on activation can race with a user pressing Space/tapping the mic before the first getUserMedia call resolves, because the UI still allows manual start while voiceState is still idle. In that case two startListening executions can run concurrently, and the later one overwrites streamRef/mediaRecorderRef, leaving the earlier recorder/track unmanaged and causing stuck mic capture or inconsistent state transitions. This regression is introduced by the new automatic start path and should be gated by an in-flight/listening check.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughVoice mode reliability is enhanced through three key additions: a Changes
Sequence DiagramsequenceDiagram
participant Overlay as VoiceModeOverlay
participant Hook as useVoiceMode Hook
participant Store as useVoiceModeStore
participant Media as Browser APIs
Overlay->>Store: Check isEnabled && hasLoadedSettings
activate Store
Store-->>Overlay: true
deactivate Store
Overlay->>Overlay: Guard via hasStartedOnActivateRef<br/>(runs only once per activation)
Overlay->>Hook: Call startListening()
activate Hook
Hook->>Hook: Check guards: isStartingListeningRef,<br/>not already listening/processing,<br/>feature still enabled
Hook->>Media: Initialize MediaStream
activate Media
Media-->>Hook: Stream acquired
deactivate Media
Hook->>Media: Initialize MediaRecorder
activate Media
Media-->>Hook: Recorder ready
deactivate Media
Hook->>Hook: Set listening state
Hook-->>Overlay: listening = true
deactivate Hook
Note over Overlay: If errors occur (mic permission,<br/>transcription, synthesis)
Hook->>Hook: Use standardized error message helper
Hook-->>Overlay: Display user-friendly error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/1.0-overview/changelog.md`:
- Around line 17-18: Add a blank line between the paragraph that ends with
"Intermittent "TTS but no listening indicator" behavior is reduced by keeping
state transitions explicit during activation." and the following heading "###
Monaco Worker Stability + CSP Hardening" so the two sections are visually and
semantically separated in Markdown.
🧹 Nitpick comments (1)
apps/web/src/hooks/useVoiceMode.ts (1)
302-305: Lint: implicit return value inforEachcallback.
track.stop()returnsvoid, but the concise arrow body still triggers Biome'suseIterableCallbackReturnrule. Add braces to make the intent explicit.🔧 Proposed fix
- stream.getTracks().forEach((track) => track.stop()); + stream.getTracks().forEach((track) => { track.stop(); });
| - Intermittent "TTS but no listening indicator" behavior is reduced by keeping state transitions explicit during activation. | ||
| ### Monaco Worker Stability + CSP Hardening |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing blank line between changelog sections.
Line 17 runs directly into the ### Monaco Worker Stability heading on Line 18. Add a blank line between these two sections for proper Markdown rendering and visual separation.
📝 Proposed fix
- Intermittent "TTS but no listening indicator" behavior is reduced by keeping state transitions explicit during activation.
+
### Monaco Worker Stability + CSP Hardening📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - Intermittent "TTS but no listening indicator" behavior is reduced by keeping state transitions explicit during activation. | |
| ### Monaco Worker Stability + CSP Hardening | |
| - Intermittent "TTS but no listening indicator" behavior is reduced by keeping state transitions explicit during activation. | |
| ### Monaco Worker Stability + CSP Hardening |
🤖 Prompt for AI Agents
In `@docs/1.0-overview/changelog.md` around lines 17 - 18, Add a blank line
between the paragraph that ends with "Intermittent "TTS but no listening
indicator" behavior is reduced by keeping state transitions explicit during
activation." and the following heading "### Monaco Worker Stability + CSP
Hardening" so the two sections are visually and semantically separated in
Markdown.
Summary
Updated voice-mode error handling to show user-friendly, actionable failure messages (instead of technical/raw error text) for microphone permission/device issues, transcription failures, and TTS playback failures. This now explicitly informs users what failed and what to do next.
Removed the console-only onError logger from VoiceModeOverlay so failures are surfaced through existing UI error state rather than just dev logs.
Kept the prior activation behavior intact (auto-start listening on activation) so mic prompt + visual listening indicator still happen, while now also giving descriptive user-visible failure messaging when the flow fails.
Testing
✅ VoiceModeOverlay.tsx
⚠️ pnpm --filter web typecheck (fails due to existing repo-wide baseline issues with unresolved @pagespace/lib/* modules and unrelated strict type errors)
Codex Task
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes