XS✔ ◾ fix: recover from stuck Stop button when a shave's audio never opened#956
Conversation
…#950) stop() in useScreenRecording silently no-op'd when mediaRecorderRef was null/inactive (e.g. audio/mic setup for a shave never completed), leaving isRecording stuck true with no error shown. It now resets to idle state and surfaces a toast whenever there's nothing to stop, and any unexpected error during the real stop path now also guarantees the same reset via a shared try/finally instead of silently resolving null. Closes #950
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
github-actions
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes the “stuck Stop button” screen-recording/shave failure mode (#950) by ensuring useScreenRecording.stop() always returns the UI to an idle state and provides user feedback when there’s nothing valid to stop.
Changes:
- Added a
resetToIdle()helper and used it to recover when noMediaRecorderexists / is inactive, including an actionable error toast. - Hardened the normal stop flow with explicit error handling (
recorder.onerror) and centralized cleanup/state reset. - Added new unit tests covering the “stop with no recorder” regression scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ui/src/hooks/useScreenRecording.ts |
Adds stuck-state recovery in stop() and improves stop-path error handling/cleanup. |
src/ui/src/hooks/useScreenRecording.test.ts |
Adds regression unit tests for stopping when audio/recorder never started. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } finally { | ||
| cleanup(); | ||
| setIsRecording(false); | ||
| setIsProcessing(false); | ||
| } |
There was a problem hiding this comment.
Agreed, fixed in f9018c5. finally now awaits both hideControlBar() and cleanup() (previously cleanup() was fire-and-forget), and hideControlBar() is called unconditionally in finally instead of only inside the onstop happy path — so an onerror/throw path (or a synchronous throw from recorder.stop() itself) can no longer leave the control bar stuck visible while isRecording/isProcessing have already reset. Removed the now-redundant hideControlBar() call that used to live inside onstop.
| recorder.onerror = (event) => { | ||
| reject(new Error(`MediaRecorder error: ${String((event as ErrorEvent).error ?? event)}`)); | ||
| }; |
There was a problem hiding this comment.
Agreed — same issue tomek-i flagged separately below; fixed together in f9018c5 with a local MediaRecorderErrorEventLike interface (extends Event, adds error?: DOMException) instead of casting to the unrelated DOM ErrorEvent type.
tomek-i
added
armada:reviewing
|
🔭 crows-nest: picked up by ARMADA — dispatching to the review→merge pipeline. |
| try { | ||
| await window.electronAPI.screenRecording.hideControlBar().catch(() => {}); | ||
| try { | ||
| return await new Promise((resolve, reject) => { |
There was a problem hiding this comment.
[major] Concurrent stop() calls clobber each other's onstop/onerror handlers, hanging the first caller forever
stop() has no re-entrancy guard beyond the disabled state of the main window's Stop button. The recording control bar is a separate Electron BrowserWindow whose own Stop button fires the stop-recording-request IPC event independently of the main renderer's isProcessing state. If the control-bar Stop and the main-window Stop fire close together, both calls read the same mediaRecorderRef.current and each assigns recorder.onstop/onerror on the same MediaRecorder instance — the second call's assignment silently overwrites the first's handlers, so the first call's Promise never resolves or rejects and its finally never runs, leaving that invocation permanently dangling. This is the same class of stuck-forever bug this PR sets out to fix, just relocated. Suggested fix: guard stop() with an in-flight ref (e.g. isStoppingRef) so a second concurrent invocation is a no-op/early return instead of re-registering handlers on the live recorder.
_flagged by:
There was a problem hiding this comment.
Agreed, fixed in f9018c5. Verified independently first: there's actually only one useScreenRecording() instance (in the main window — ScreenRecorder.tsx); the control bar doesn't run its own hook. But the bug is real for a related reason — the control bar's Stop button forwards over IPC (stopFromControlBar → stop-recording-request) to that same single stop() closure that the in-window Stop button also calls, and nothing prevented a second concurrent call before the first's onstop/onerror fired. Added an `isStoppingRef" in-flight guard so a second concurrent call is now a no-op/early-return instead of re-registering handlers on the live recorder.
| // MediaRecorder was ever created because audio/mic setup for this shave | ||
| // never completed, so there is nothing to call .stop() on, but the UI must | ||
| // not be left believing a recording is still active. | ||
| const resetToIdle = useCallback(async () => { |
There was a problem hiding this comment.
[minor] resetToIdle() never stops the backend recording timer if recorder goes inactive mid-session
resetToIdle() calls hideControlBar() and the local cleanup() (stops local MediaStream tracks / closes the AudioContext) but never calls window.electronAPI.screenRecording.stop(...). If a MediaRecorder was created/started (so the backend timer is running) but then transitions to state === 'inactive' on its own (e.g. underlying track/stream ends unexpectedly) before the user clicks Stop, stop() takes the recovery branch and calls resetToIdle(), which never tells the backend to stop its timer — it keeps firing recording-time-update events until the next recording start or app quit. Consider having resetToIdle() also invoke the backend stop/cleanup IPC so backend-side timer/session state is torn down symmetrically.
_flagged by:
There was a problem hiding this comment.
Agreed, fixed in f9018c5. resetToIdle() now also calls window.electronAPI.screenRecording.stop(new Uint8Array()) — handleStopRecording() stops the backend timer as its first action and safely no-ops before writing anything when videoData is empty, so this reuses existing plumbing without a new IPC channel.
| toast: { success: vi.fn(), error: vi.fn(), info: vi.fn(), warning: vi.fn() }, | ||
| })); | ||
|
|
||
| describe("useScreenRecording – stop() when audio/recorder was never opened (#950)", () => { |
There was a problem hiding this comment.
[minor] New tests don't cover the 'recorder exists but inactive' branch or the rewritten onstop/onerror path
The stop() guard is if (!recorder || recorder.state === 'inactive'), but all three new tests only exercise the !recorder (never-created) half — none construct a MediaRecorder-like ref with state === 'inactive' to hit the other half, which is the literal sibling guard the PR description calls out as related. The rewritten in-progress-recording path (the new onerror handler alongside onstop, and the try/catch/reject restructuring) has zero test coverage. The 'second Stop click' test also awaits sequentially rather than firing overlapping calls, so it provides no coverage of the concurrent-invocation race noted separately.
_flagged by:
There was a problem hiding this comment.
Agreed, fixed in f9018c5. Added coverage for the recorder-exists-but-inactive branch (via a fake MediaRecorder driven through start()), the rewritten onstop/onerror path (both the happy-path resolve and the onerror-rejects-without-hanging case), and a genuine concurrent-invocation test (two overlapping stop() calls where the second is a no-op and doesn't clobber the first's handlers).
| filePath: result.filePath, | ||
| fileName: result.fileName || result.filePath, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
[nit] onerror event cast uses the wrong DOM type name
recorder.onerror casts the event to ErrorEvent, but MediaRecorder's error event is actually a MediaRecorderErrorEvent (with an .error: DOMException property), not a generic ErrorEvent. Works at runtime since .error exists on the real event, but the annotation is misleading. Consider a local { error?: DOMException } shape instead of ErrorEvent.
_flagged by:
There was a problem hiding this comment.
Agreed, fixed in f9018c5. Added a local MediaRecorderErrorEventLike interface (extends Event, adds error?: DOMException) since lib.dom doesn't ship a MediaRecorderErrorEvent type, and cast to that instead of the unrelated ErrorEvent.
| const resetToIdle = useCallback(async () => { | ||
| await window.electronAPI.screenRecording.hideControlBar().catch(() => {}); | ||
| await cleanup(); | ||
| setIsRecording(false); |
There was a problem hiding this comment.
[nit] stop() now calls recorder.stop() unconditionally, dropping the old recording-state guard
The previous code only called recorder.stop() when recorder.state === 'recording'; the new code calls it whenever the recorder is neither undefined nor 'inactive', which includes 'paused'. No pause functionality exists today so this is currently a no-op risk, but worth a comment or restoring the explicit state check so a future pause feature doesn't hit an unguarded .stop() call.
_flagged by:
There was a problem hiding this comment.
Agreed, fixed in f9018c5. Restored the explicit if (recorder.state === "recording") guard before calling recorder.stop(). Since onstop/onerror won't fire without an actual stop() call, the else branch now rejects explicitly (Cannot stop recorder in unexpected state) instead of leaving the promise dangling — a plain restore of the old guard would have hung on 'paused' once a pause feature lands, so I closed that gap too.
🔭 muster review — PR #956Two independent lenses reviewed this diff in parallel (code-review + codex-rescue), neither saw the other's notes. Bottom line: 0 blocking, 1 major, 2 minor, 1 nit — not a hard blocker, but the major finding is a real relocation of the same bug class this PR fixes.
Major
Minor
Nits
Lenses agreed there's nothing blocking; codex-rescue's root-cause pass surfaced the concurrency edge case the conventions lens didn't probe for. Handing to shipwright's address-review mode to triage. |
|
🚀 Pre-release build is available for this PR: |
…ery, fix onerror path (#956) - Guard stop() with an isStoppingRef so a second concurrent invocation (control-bar Stop button vs. in-window Stop button both routing to the same handler) is a no-op instead of clobbering the first call's onstop/onerror handlers and hanging it forever. - resetToIdle() now also calls the backend stop/cleanup IPC so a never-started or self-inactivated recording doesn't leave the backend timer running until the next recording or app quit. - Restore the explicit state === 'recording' guard before calling recorder.stop(), rejecting instead of leaving the promise dangling for any other reachable state. - Model the MediaRecorder error event's actual shape (a DOMException on .error) instead of casting to the unrelated DOM ErrorEvent type. - Always hide the control bar in stop()'s finally block (not just on the onstop happy path) so an onerror/throw path can't leave it stuck visible. - Add test coverage for the recorder-exists-but-inactive branch, the onstop/onerror in-progress path, and the concurrent-stop no-op guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
shipwright — address-review summaryAll 7 review threads (5 from muster/tomek-i's consolidated review + 2 from Copilot's separate automated pass) triaged and addressed. New head: Agreed and fixed (7/7):
Declined: none. Validation on the new head:
Note: the repo-root |
|
🚀 Pre-release build is available for this PR: |
tomek-i
removed
the
armada:reviewing
|
✅ reviewed, addressed, green — awaiting human merge (auto-merge off). Round 1 review: 0 blocking, 1 major, 2 minor, 2 nit — posted inline + summary.
This didn't reach Gate: 0 blocking · review not degraded · CI green (all checks SUCCESS/SKIPPED) · not draft · mergeable · no branch protections configured · |
|
🔭 crows-nest: address-review complete (head |
tomek-i
added
armada
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
tomek-i
added
the
armada:record
| // this shave) or it's already inactive — there's nothing to stop, but we | ||
| // must still bring the app back to an idle state instead of silently | ||
| // no-oping and leaving the Stop button stuck on a dead recording. | ||
| if (!recorder || recorder.state === "inactive") { |
There was a problem hiding this comment.
[major] isStoppingRef re-entrancy guard is not applied to the null/inactive recovery branch
Both lenses independently flagged this: isStoppingRef.current is only set true inside the live recorder stop path — the new !recorder || recorder.state === "inactive" guard clause (the exact branch this PR is fixing) never touches it. Two Stop clicks landing close together while the recorder is null/inactive (the #950 stuck-shave scenario itself, and the dual-trigger race the guard's own comment describes) both pass the check and both await resetToIdle() concurrently — duplicate IPC calls (hideControlBar, screenRecording.stop), duplicate cleanup(), and duplicate error toasts.
Suggested fix: set/reset isStoppingRef.current around the guard-clause branch too (e.g. wrap both branches in the same try/finally that manages the flag), not just the live-recorder path.
flagged by: code-review + codex-rescue
There was a problem hiding this comment.
Fixed in f397632 — the recovery branch now sets isStoppingRef.current = true before resetToIdle() and clears it in a finally, mirroring the live-recorder path's guard.
| try { | ||
| await window.electronAPI.screenRecording.hideControlBar().catch(() => {}); | ||
| try { | ||
| return await new Promise((resolve, reject) => { |
There was a problem hiding this comment.
[major] New re-entrancy guard can itself wedge forever if onstop/onerror never fire
isStoppingRef.current is set true before awaiting the stop promise and only cleared in the finally block once that promise settles — which requires recorder.stop() to reliably fire either onstop or the new onerror handler. If neither ever fires (device revoked mid-stop, stream/track already dead, or a stop-event quirk), the promise never resolves/rejects, finally never runs, and isStoppingRef.current stays true permanently — reproducing the exact 'Stop becomes a permanent no-op' symptom this PR fixes, just relocated and now silent (stop() early-returns null, no toast). No test exercises 'neither event ever fires.'
Suggested fix: bound the wait with a timeout (e.g. Promise.race against a short timeout that force-resets state) so a missing event can't wedge the guard indefinitely.
flagged by: codex-rescue
There was a problem hiding this comment.
Fixed in a5864d5 — added a STOP_EVENT_TIMEOUT_MS (15s) that rejects the stop Promise if neither onstop nor onerror fires, with clearTimeout on every other settlement path so it can't fire spuriously.
tomek-i
removed
the
armada:record
|
🎬 crows-nest: on-demand walkthrough failed — no |
|
🚀 Pre-release build is available for this PR: |
tomek-i
removed
the
armada:reviewing
|
✅ crows-nest / muster pipeline complete — reviewed, addressed, re-validated green — awaiting human merge (auto-merge off).
Gate: |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
| // Bounds this Promise so it can never hang forever if onstop/onerror | ||
| // never fire (e.g. the recording device was revoked, or the browser | ||
| // drops the event) — cleared on every settlement path below. | ||
| const timeoutId: ReturnType<typeof setTimeout> = setTimeout(() => { |
There was a problem hiding this comment.
[major] Timed-out stop() never detaches recorder.onstop/onerror, so a late event still fires stale side effects
When STOP_EVENT_TIMEOUT_MS elapses, the promise rejects and the outer finally runs cleanup(), which nulls mediaRecorderRef.current and resets chunksRef.current. But the local recorder variable inside the Promise executor still holds a live reference, and recorder.onstop/recorder.onerror are never cleared on any settlement path. If the browser/device fires the event late — after the timeout has already rejected and the UI has shown a failure toast and reset to idle — the stale onstop handler still runs: it builds a Blob from the now-cleared chunksRef.current (empty), calls the stop IPC a second time, and calls toast.success(...), directly contradicting the failure toast already shown. Suggest nulling recorder.onstop/recorder.onerror in the timeout callback (and other settlement branches) so a late-firing event is a no-op.
flagged by: codex-rescue
There was a problem hiding this comment.
| // this shave) or it's already inactive — there's nothing to stop, but we | ||
| // must still bring the app back to an idle state instead of silently | ||
| // no-oping and leaving the Stop button stuck on a dead recording. | ||
| if (!recorder || recorder.state === "inactive") { |
There was a problem hiding this comment.
[nit] isProcessing not set true during recovery-branch resetToIdle() await
In the live-recorder stop path, setIsProcessing(true) is called before the async work begins, so the record button is disabled while stopping is in flight. The !recorder || recorder.state === "inactive" recovery branch never calls setIsProcessing(true) before awaiting resetToIdle() (two IPC round-trips + cleanup()) — only the internal isStoppingRef guard (invisible to the UI) prevents a second click from re-entering. Not a functional bug, just an inconsistency between the two branches' UI feedback during their async windows.
flagged by: code-review
There was a problem hiding this comment.
Fixed in 06e25d3 — the recovery branch now calls setIsProcessing(true) before awaiting resetToIdle(), matching the live-recorder path so the button reflects in-flight state on both branches.
|
[minor] Missing regression test for the STOP_EVENT_TIMEOUT_MS timeout path ( The PR's headline safety mechanism — the 15s timeout wrapped around the onstop/onerror Promise so flagged by: code-review + codex-rescue (independently) |
🔭 muster review — PR #956Lenses: code-review + codex-rescue (both ran — no degrade) Summary: 0 blocking, 1 major, 1 minor, 1 nit — no blocking findings, but the major finding is worth addressing before merge (a stale-listener bug in the new timeout path that can fire a false success toast after the failure toast already shown).
Bottom line: no blocking findings — review advisory, but the major finding should be fixed (dangling event handlers on timeout) before this ships, since it can produce a contradictory 'success' toast after the UI has already shown failure. 🔭 crows-nest / muster — automated review, not a merge decision. |
…th (#956 thread 3567819413) Timed-out stop() left recorder.onstop/onerror attached to the live MediaRecorder, so a late-firing event after the timeout rejected could still build a Blob from chunks cleanup() already cleared, call the stop IPC a second time, and show a contradictory success/error toast. Null both handlers out on every settlement path (timeout, onstop success/error, onerror, and the unexpected-state branch) so a late event is a no-op.
…path (#956 thread 3567819890) The live-recorder stop() path sets isProcessing(true) before its async work so the button reflects in-flight state; the !recorder || state === "inactive" recovery branch never did, leaving the UI looking idle while resetToIdle() is still tearing down. Not a functional bug (isStoppingRef still guards re-entry) — just UI-feedback consistency between the two branches.
…eout path (#956 top-level comment) Adds fake-timer coverage for the 15s stop() timeout: one test drives vi.advanceTimersByTimeAsync(15000) to confirm the promise settles to null and isRecording/isProcessing reset when neither onstop nor onerror ever fires, and a second confirms a late-firing onstop after the timeout has already settled is a safe no-op (no duplicate stop IPC call, no contradictory success toast) now that recorder.onstop/onerror are detached on the timeout path.
|
Addressed: missing regression test for the Agreed — added in
Both pass; full UI suite is green at 205/205. |
shipwright — address-review summaryAll 3 findings from the latest muster review triaged and implemented (3/3 agreed, 0 discussed, 0 declined). None were blocking.
Validation: New head sha: Threads left unresolved for human review. |
|
🚀 Pre-release build is available for this PR: |
tomek-i
added
armada:blocked
|
🔭 crows-nest: pipeline complete — review (0 blocking) → address (3/3 findings fixed, pushed as Reason: branch protections / required reviews not satisfied — this repo's ruleset reports Action needed: a human reviewer approve this PR on GitHub, then merge ( |
|
🎬 Walkthrough video: https://github.com/SSWConsulting/SSW.YakShaver.Desktop/releases/download/logbook-pr-956/walkthrough-pr-956.mp4 Chapters (1:35, narrated):
walkthrough-pr-956.mp4 |
Consolidating toward 2-4 R&D Core Activities per year: the cross-Space overlay bug and the already-fixed stuck-Stop-button bug (#956) are two facets of the same recording-lifecycle-reliability question, so #921 is broadened to cover both rather than staying scoped to one symptom. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
✅ Automated Release Created Successfully Release Details:
You can monitor the build progress in the Actions tab. |
* Scaffold FY2027 R&D Core Activity docs for #921 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Broaden #921's activity to cover the related, shipped #956 fix Consolidating toward 2-4 R&D Core Activities per year: the cross-Space overlay bug and the already-fixed stuck-Stop-button bug (#956) are two facets of the same recording-lifecycle-reliability question, so #921 is broadened to cover both rather than staying scoped to one symptom. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Calum Simpson <calum@ssw.com.au> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Summary
If a shave's microphone/audio setup never fully completed,
useScreenRecording'sstop()would silently returnnullwithout ever resettingisRecording/isProcessingor telling the user anything went wrong. The Stop button would then remain visible/enabled but every click became a permanent no-op — the exact "stuck shave" reported in the issue.Closes #950
What changed
src/ui/src/hooks/useScreenRecording.tsstop()no longer silently no-ops when theMediaRecorderis missing/inactive: it now resets recording state to idle, clears stale refs, and surfaces an error toast. The recorder's real stop path was also hardened with arecorder.onerrorhandler and a sharedtry/finallyso any unexpected failure during stop still guarantees the UI returns to idle instead of hanging mid-promise.src/ui/src/hooks/useScreenRecording.test.tsDecisions
resetToIdle()helper (hide control bar, runcleanup(), flipisRecording/isProcessingfalse) and call it both from the "nothing to stop" guard and implicitly via the samefinallyblock on the real stop path.stop()call still believing it's recording, regardless of which branch was taken — closing off the whole class of bug rather than just the one reported branch.!mediaRecorderRef.current). Rejected because the siblingrecorder.state === "inactive"guard had the identical silent-no-op bug and would have been left stuck too.toast.error("Nothing to stop — this recording never started properly. You can try again.")when Stop is clicked with no usable recorder.sonnertoast.error, same phrasing style as the neighboringFailed to start recording: .../Failed to save recording: ...calls) and gives the user both a clear explanation and the recovery action required by AC2 — the button becomes "Start Recording" again immediately after, so they can just try again.Acceptance criteria
stop()now always resolves and returns the UI to idle, even when noMediaRecorderwas ever created.toast.erroris shown and the Stop button reverts to "Start Recording", so the user can immediately retry.isRecording/isProcessingare unconditionally reset in every code path (guard clause and the real stop path'sfinally).Testing
src/ui/src/hooks/useScreenRecording.test.ts, 3 new tests)npm run build)npm rebuild better-sqlite3 --build-from-source && npx vitest run --exclude 'src/ui/**'— 658 passed;npm --prefix src/ui test— 198 passed)npm run lint— biome check clean)Bug repro evidence
MediaRecorderwas never created/started does nothing — no state change, no user feedback,isRecordingstaystrue.useScreenRecording.test.ts) that callsstop()on a freshly-mounted hook wherestart()was never invoked — i.e.mediaRecorderRef.currentisnull, the exact state you land in when audio was never opened for a shave — and asserts an error toast is shown.stop(): it resolved (so it didn't hang), but the "surfaces a clear, actionable error toast" assertion failed —toast.errorwas called 0 times, confirming the silent-no-op / no-recovery-action defect called out in AC2.stop()resolvesnull,isRecording/isProcessingarefalse, andtoast.erroris called with an actionable "Nothing to stop... You can try again." message. A second consecutive Stop click also resolves cleanly (no re-stuck state).Follow-up items
None — the fix is self-contained to the
stop()guard clauses and the surrounding error handling in the same hook.🤖 Generated with Claude Code