fix(ui): cancel pending microphone acquisition on early release (#550)#580
Conversation
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29146649660 Artifacts expire in 7 days.
|
|
Gatekeeper review of latest head I reviewed this as a platform-specific MediaRecorder lifecycle fix, focusing on regressions, evidence for the proposed root cause, and integration with the other active speech PR. Blocking findings
The primary behavioral change is Issue #550 reports CodeNomad 0.15.0, while the current application and Electron version have changed. Green packaging checks do not execute MediaRecorder. Before merge, test the Linux/Electron artifact with at least three consecutive recordings and inspect the resulting payload/container contents, not only whether the request returns successfully.
A format-independent byte count cannot establish whether encoded audio frames exist:
Restrict the heuristic to the observed WebM failure signature, use format-aware validation, or remove this guard from the primary fix.
The same problem occurs if the component is unmounted while acquisition is pending: cleanup cannot invalidate a stream that has not resolved yet. This race predates the PR, but the new timeslice makes an orphan recorder periodically append chunks while keeping the microphone active. Add a
PR #579 adds Put the recorder start/finalization policy in the shared audio utility introduced by #579, or apply the mitigation consistently to both paths before both PRs merge. Important findings
The observed 61 KB first payload followed by 110-byte payloads establishes a client capture/encoding failure. It does not establish that omitting a timeslice specifically causes Chromium's encoder initialization failure. The earlier #550 investigation described timeslicing as a defensive mitigation to test because no matching Electron/Chromium report was found. Until the affected Linux/Electron scenario is reproduced and verified, this should be described as a workaround rather than a proven root-cause fix.
No test covers:
A fake MediaRecorder test can cover event ordering and cleanup even though the Chromium-specific encoder issue remains a manual platform test. What looks correct
Validation
VerdictDo not merge yet. The |
heunghingwan
force-pushed
the
fix/550-stt-empty-audio-subsequent-recordings
branch
from
df9172c to
0d010eb
Compare
heunghingwan
changed the title
heunghingwan
force-pushed
the
fix/550-stt-empty-audio-subsequent-recordings
branch
from
0d010eb to
da64542
Compare
|
Thanks for the thorough review @pascalandr. The PR has been completely reworked based on your feedback and new evidence. Revised root causeThe original hypothesis (timeslice encoder flush) was wrong. The actual root cause is a push-to-talk race condition in
This explains:
What was removedBased on your feedback, all speculative changes were removed:
P1-3 (orphan recording race) — fixedThis was the core finding. Added a
P1-4 (PR #579 second recorder path)
P1-2 (byte-size heuristic)Removed entirely as you suggested. Early
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29262563146 Artifacts expire in 7 days.
|
1 similar comment
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29262563146 Artifacts expire in 7 days.
|
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29263156342 Artifacts expire in 7 days.
|
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Gatekeeper re-review of da64542
The rework is substantially better and the speculative recorder changes are gone. One merge gate remains.
Blocking finding
- P1: the replacement fix still has no behavioral validation.
The new generation guard in packages/ui/src/components/prompt-input/usePromptVoiceInput.ts is logically sound: an early release invalidates the pending request, stale streams are stopped, stale recorder events cannot mutate state, and tracks are released before transcription. It also resolves the integration concern with PR #579 because both recorder paths now use the same cancellation pattern.
However, this head replaces the original root-cause hypothesis and the response explicitly defers tests. The green build/package matrix, typecheck, and bundle do not execute getUserMedia or MediaRecorder, and there is no comment showing that the new artifact was exercised. That leaves the actual #550 workflow unverified.
Before merge, provide either:
- an automated delayed-getUserMedia regression proving press -> release before resolution stops the returned stream, never creates/starts a recorder, never uploads audio, and allows the next recording; or
- a short manual result from the current da64542 artifact (Linux/Electron or the confirmed Windows/Chrome environment) covering early release plus at least three consecutive normal recordings, with no orphan microphone activity and no header-only upload.
This does not require introducing Vitest/jsdom if manual artifact verification is the proportionate option.
Non-blocking observation
On Electron, release during requestMicrophoneAccess still proceeds to getUserMedia before the stale-generation check at lines 94-103. The resulting stream is stopped, so it cannot become an orphan recorder, but an additional generation check before getUserMedia would avoid acquiring the device after cancellation.
Resolved from the previous review
- Removed the format-blind byte threshold.
- Removed speculative timeslicing and requestData behavior.
- Added stale request, event, finalization, cleanup, and unmount guards.
- Confirmed the #579 settings recorder already has generation protection.
- Moved track release ahead of the transcription round-trip.
Validation
- npm run typecheck in packages/ui: passed.
- npm run build in packages/ui: passed.
- git diff --check upstream/dev...HEAD: passed.
- Complete required GitHub build matrix: green.
Verdict
Changes requested. The code issue is addressed, but the behavioral gate from the previous review remains open until the current fix is exercised once on the affected workflow.
…alNomadsAI#550) The voice input button uses a push-to-talk pattern: press to start, release to stop. startRecording() is async (awaits getUserMedia) but beginVoicePress() fires it with void — it does not await. When the user releases the button before getUserMedia() resolves, stopRecording() finds no active recorder and silently returns. The pending getUserMedia() promise later resolves and starts an orphan recording with no matching stop. The user perceives this as: button doesn't turn red, then jumps directly to transcribing on the next press. On fast taps the orphan recording captures zero audio frames, producing a header-only WebM blob (~110 bytes) that the STT provider rejects with HTTP 400. This race also explains the original Linux/Electron report (NeuralNomadsAI#550): subsequent recordings fail because the orphan from the previous tap interferes with state. The issue is not platform-specific — it reproduces on Windows Chrome too. Fix: add a requestGeneration counter (mirroring the pattern already used in use-transcription-test.ts from PR NeuralNomadsAI#579). Each startRecording() call increments the generation; getUserMedia() checks it after resolving and discards the stream if stale. stopRecording() also increments the generation when called while state is still idle (pending acquisition), immediately invalidating any in-flight request. cleanupMedia() and onCleanup() increment it as well to cover unmount and error paths. The recording state machine is extracted into recording-controller.ts (dependency-injected, no browser/SolidJS imports) with 10 unit tests covering: normal start/stop, early-release cancellation, stream release on cancel, no-chunk skip, next-recording-after-cancel, transcribing lock, transcribe failure, cleanup-during-pending, and stale-generation-during-transcription. usePromptVoiceInput.ts is now a thin adapter wiring real browser APIs to the controller. Additionally, stopTracks(stream) is called before the transcription network round-trip instead of after, releasing the audio device sooner.
heunghingwan
force-pushed
the
fix/550-stt-empty-audio-subsequent-recordings
branch
from
da64542 to
d1f5c5c
Compare
pascalandr
left a comment
pascalandr
left a comment
There was a problem hiding this comment.
Final gatekeeper review of d1f5c5c
No blocking findings remain.
Verified resolution
- The speculative timeslice, requestData, and format-blind byte-threshold changes remain removed.
- The push-to-talk lifecycle is now isolated in a dependency-injected recording controller while the Solid/browser hook remains a thin adapter.
- Releasing before getUserMedia resolves invalidates the request, prevents recorder creation/start, and stops the late stream.
- Cleanup and stale transcription results are generation-guarded.
- A subsequent recording works after a cancelled acquisition.
- The Electron permission path now checks cancellation before calling getUserMedia, resolving the previous non-blocking observation.
- Tracks are released before the transcription network round-trip.
- The recorder path added by PR #579 remains compatible with the same generation-guard approach.
Regression coverage
The new 10-test controller suite covers normal start/stop, delayed acquisition cancellation, late stream release, no-chunk handling, recording after cancellation, transcribing lockout, transcription failure, cleanup during pending acquisition, and stale transcription completion.
Validation
- npx tsx --test src/components/prompt-input/recording-controller.test.ts: 10/10 passed.
- npm run typecheck in packages/ui: passed.
- npm run build in packages/ui: passed.
- git diff --check upstream/dev...HEAD: passed.
- Electron Linux/macOS/Windows and Tauri Linux/macOS/macOS ARM64/Windows: passed.
Non-blocking residual
The UI package still has no test script and the current packaging workflow does not execute the controller test automatically. The focused suite passes and closes this review gate, but wiring existing UI tests into CI would make the regression protection persistent.
Verdict
Approved. The behavioral gate from the prior review is closed and the fix is ready to merge.
|
PR builds are available as GitHub Actions artifacts: https://github.com/NeuralNomadsAI/CodeNomad/actions/runs/29269028586 Artifacts expire in 7 days.
|
|
Thanks @heunghingwan ! |
2 participants
Problem
The voice input button uses push-to-talk: press to start recording, release to stop. On the second and subsequent recordings, the button fails to enter the recording state (doesn't turn red) and jumps directly to "transcribing", producing empty or header-only audio (~110 bytes) that the STT provider rejects with HTTP 400.
This affects all platforms (Linux/Electron, Windows/Chrome), not just Linux as originally reported in #550.
Root Cause
startRecording()is async — it awaitsgetUserMedia(). ButbeginVoicePress()inprompt-input.tsxfires it withvoid(fire-and-forget):When the user releases the button before
getUserMedia()resolves (common on second+ recordings where mic permission is already granted and acquisition is fast),stopRecording()finds no active recorder (mediaRecorder === null,state() === "idle") and silently returns. The pendinggetUserMedia()promise later resolves and starts an orphan recording with no matching stop.The user perceives: button doesn't turn red → presses again → the orphan recorder is stopped → jumps to "transcribing". On fast taps, the orphan recording captures zero audio frames, producing a header-only WebM blob (~110 bytes).
Why it only manifests on second+ recordings
On the first recording,
getUserMedia()may show a permission prompt or have slower initial device setup, giving the user time to hold the button. On subsequent recordings, the device is already warm andgetUserMedia()resolves faster, but the user's tap duration remains the same — releasing before acquisition completes.What this is NOT
start()vsstart(timeslice)produce identical results across consecutive recordingsMediaStream+MediaRecorderper recordingFix
Add a
requestGenerationcounter (mirroring the pattern fromuse-transcription-test.tsin PR #579):startRecording()increments the generation before awaitinggetUserMedia()stopRecording()/cancelRecording()increment the generation when called whilestate() === "idle"(pending acquisition), immediately invalidating any in-flight requestdataavailable,stop) andfinalizeRecording()check the generation before mutating statecleanupMedia()andonCleanup()increment the generation to cover unmount and error pathsAdditionally,
stopTracks(stream)is called before the transcription network round-trip (instead of after), releasing the audio device sooner.Verification
tsc --noEmitpasses (packages/ui)Closes #550