XS✔ ◾ fix: recover from stuck Stop button when a shave's audio never opened (#956)

* fix: recover from stuck stop button when a shave's audio never opened (#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

* address review: guard stop() re-entrancy, stop backend timer on recovery, 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>

* address review: guard isStoppingRef in recovery branch (#956 thread 3567622365)

The null/inactive recovery branch of stop() called resetToIdle() without
setting isStoppingRef, so a second concurrent Stop click while recovery was
in flight wasn't short-circuited by the top-of-function re-entrancy guard.
Now mirrors the live-recorder path: set the ref before resetToIdle() and
clear it in a finally.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* address review: bound stop() promise with a timeout (#956 thread 3567622398)

If recorder.stop() never fires onstop or onerror (device revoked, dropped
event), the stop Promise never settled, so isStoppingRef stayed true forever
and the finally cleanup never ran. Add STOP_EVENT_TIMEOUT_MS (15s) that
rejects the promise if neither event fires in time, clearing the timeout on
every other settlement path so it can't fire spuriously after a normal stop.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* address review: defensively guard cleanup() teardown steps (#956 thread 3567622425)

track.stop() and AudioNode.disconnect() calls in cleanup() could throw
synchronously and propagate out of stop()'s finally / resetToIdle(),
skipping the isRecording/isProcessing resets that are the point of this fix.
Wrap each teardown step in its own try/catch (logging via console.warn) and
move the ref resets into cleanup()'s own finally so they always run.

Also adds a regression test that forces a track's stop() to throw during the
no-recorder recovery path and asserts isRecording/isProcessing still end up
false, covering the top-level review summary's minor "no test for
cleanup()/resetToIdle() itself throwing" note.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* address review: detach recorder.onstop/onerror on every settlement path (#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.

* address review: set isProcessing(true) in the recovery-branch stop() 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.

* address review: add regression coverage for STOP_EVENT_TIMEOUT_MS timeout 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.

---------

Co-authored-by: armada-lookout <armada-lookout@users.noreply.github.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

Author: 3 people