Skip to content

fix(desktop): rearm voice conversation after playback - #73880

Merged
austinpickett merged 1 commit into
NousResearch:mainfrom
carbongotfound:agent/voice-conversation-rearm
Jul 31, 2026
Merged

fix(desktop): rearm voice conversation after playback#73880
austinpickett merged 1 commit into
NousResearch:mainfrom
carbongotfound:agent/voice-conversation-rearm

Conversation

@carbongotfound

Copy link
Copy Markdown
Contributor

What changed

  • capture the voice-playback interruption sequence after normal streaming setup
  • do the same after whole-text fallback setup
  • add renderer coverage for a complete normal speech cycle returning to listening

Root cause

Both startSpeechStream() and playSpeechText() call stopVoicePlayback() as normal setup, which increments the global playback sequence. The voice-conversation hook captured its baseline before that increment, then interpreted the setup increment as an explicit user Stop when playback completed. As a result, it suppressed the next microphone cycle.

The hook now records the post-setup sequence. A later sequence change still represents an external interruption and continues to suppress automatic re-arming.

Closes #73649.

Validation

  • npm run --workspace apps/desktop test:ui -- src/app/chat/composer/hooks/use-voice-conversation.test.tsx
  • npm exec --workspace apps/desktop -- eslint src/app/chat/composer/hooks/use-voice-conversation.ts src/app/chat/composer/hooks/use-voice-conversation.test.tsx
  • npm run --workspace apps/desktop typecheck
  • git diff --check

@carbongotfound
carbongotfound marked this pull request as ready for review July 29, 2026 05:58
@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) tool/tts Text-to-speech and transcription P3 Low — cosmetic, nice to have labels Jul 29, 2026
@Adolanium

Copy link
Copy Markdown
Contributor

The root cause analysis matches what I found, and the fix works. I checked out the branch and ran the new test, and I also confirmed the same scenario on main does not re-arm, so this does close #73649.

One issue I ran into while testing. On the live path the baseline moved from the top of openLiveSpeech to after the await startSpeechStream() call. Between those two points resolveSpeakStreamUrl() is doing its IPC round trip, and the Stop button is already on screen during the preparing state. If someone clicks Stop in that window, the sequence bump gets absorbed into the new baseline, so playback starts anyway and the mic re-opens after. I wrote a quick vitest to check this: on this branch the stop is ignored and the mic re-arms, on main the same stop is honored. The window is small, but it's a real user click being ignored, and it's new behavior compared to main.

Could we keep a capture at openLiveSpeech entry, and after the await check whether the sequence moved by more than the single setup bump? If it did, treat it as an external stop and skip the re-arm. The fallback path doesn't need this since it's synchronous.

Two smaller things on the tests:

Not blocking, but while you're in this file: openLiveSpeech overwrites stopBargeMonitorRef.current without stopping the previous monitor (around line 442). The fallback path stops it first (line 402). Looks like the old monitor can leak and keep holding the mic if openLiveSpeech re-enters.

@carbongotfound
carbongotfound force-pushed the agent/voice-conversation-rearm branch from 12d8df5 to 1bad2fd Compare July 30, 2026 01:06

Copy link
Copy Markdown
Contributor Author

Addressed in 1bad2fd. You were right about the async setup window.

Changes:

  • Capture the playback sequence at openLiveSpeech entry.
  • After startSpeechStream() resolves, allow exactly the one expected setup increment. A second increment is treated as an external Stop, the new session is stopped, and the mic is not re-armed.
  • If stream discovery returns no session after a Stop, skip fallback playback entirely.
  • Capture the fallback baseline after playSpeechText() performs its synchronous setup increment.
  • Preserve the current upstream full-duplex barge monitor logic. It now uses the idempotent ensureBargeMonitor(), so openLiveSpeech no longer overwrites a live monitor without stopping it.

Added regression coverage for:

  1. Normal streaming completion re-arms.
  2. Stop during async stream preparation is honored.
  3. Stop during failed stream discovery does not start fallback playback.
  4. External Stop during active streaming suppresses re-arm.
  5. Normal fallback playback re-arms.

Validation passed: 11 focused voice-conversation tests, ESLint on the changed files, and the full desktop TypeScript check.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fix. Current main still captures the fallback baseline before playSpeechText() at apps/desktop/src/app/chat/composer/hooks/use-voice-conversation.ts:461, while that helper synchronously calls stopVoicePlayback() at apps/desktop/src/lib/voice-playback.ts:438-450; the streaming path has the same ordering around startSpeechStream() (use-voice-conversation.ts:487-497, voice-playback.ts:324-342).

Commit 1bad2fd corrects both paths and covers normal streaming/fallback re-arm plus the explicit-Stop cases raised in the review discussion. No substantive correctness or design-fit problems were found in the PR diff.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026

@austinpickett austinpickett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Verified on a merge-with-main worktree.

Root cause holds on current main: the baseline sequence was captured before the setup-time stopVoicePlayback() increment, so normal completion read as a user Stop and the mic never re-armed. Fixed for both the streaming and fallback paths.

All four points from the review discussion are covered: the async setup-window Stop (stoppedDuringStart guard), the Stop-during-stream-discovery case, fallback-path coverage, and the external-stop regression test for #70955. The barge monitor no longer leaks now that ensureBargeMonitor is idempotent.

Local checks green: 11 voice-conversation tests (5 new + 6 existing), typecheck across all three tsconfigs, eslint on the changed files.

Thanks @carbongotfound for the fix and @Adolanium for the review catches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Desktop voice conversation never re-arms mic after normal playback

5 participants