diff --git a/src/workspaces/officerdev/src/apps/Chat/useAudioRecording.test.tsx b/src/workspaces/officerdev/src/apps/Chat/useAudioRecording.test.tsx new file mode 100644 index 00000000..ef12456b --- /dev/null +++ b/src/workspaces/officerdev/src/apps/Chat/useAudioRecording.test.tsx @@ -0,0 +1,83 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import { act, cleanup, render } from '@testing-library/react'; +import { useAudioRecording } from './useAudioRecording'; + +/** + * The microphone is the one resource in this app the browser tells the user about: a recording indicator + * stays lit in the tab strip until every track is stopped. Nothing here released it on unmount, so closing + * a Chat panel mid-recording left the light on for the life of the tab with no way to switch it off. + * + * The stubs are the smallest thing that can answer "was the track stopped": a stream that counts stops and + * a recorder that reports its state. Neither the real MediaRecorder nor a real device is involved. + */ + +const stopped: string[] = []; + +const fakeStream = (label: string) => ({ + getTracks: () => [{ stop: () => stopped.push(label) }], +}); + +class FakeMediaRecorder { + state: 'recording' | 'inactive' = 'inactive'; + stream = fakeStream('track'); + mimeType = 'audio/webm'; + ondataavailable: ((ev: { data: Blob }) => void) | null = null; + onstop: (() => void) | null = null; + start() { + this.state = 'recording'; + } + stop() { + this.state = 'inactive'; + this.onstop?.(); + } +} + +const install = () => { + stopped.length = 0; + Object.assign(globalThis, { MediaRecorder: FakeMediaRecorder }); + Object.defineProperty(globalThis.navigator, 'mediaDevices', { + configurable: true, + value: { getUserMedia: async () => fakeStream('track') }, + }); +}; + +type Handle = { toggle: () => Promise; recording: boolean }; + +const handle: Handle = { toggle: async () => {}, recording: false }; + +const Probe = () => { + const runner = useAudioRecording(() => {}); + handle.toggle = runner.toggleRecording; + handle.recording = runner.recording; + return null; +}; + +afterEach(cleanup); + +describe('useAudioRecording', () => { + test('releases the microphone when its component goes away mid-recording', async () => { + install(); + const view = render(); + + await act(async () => { + await handle.toggle(); + }); + expect(handle.recording).toBe(true); + expect(stopped).toEqual([]); + + view.unmount(); + + // Not "the user pressed stop" — the panel was closed, swapped or remounted while still recording, and + // that is the case that used to leak. + expect(stopped).toEqual(['track']); + }); + + test('unmounting when nothing was ever recorded does nothing', () => { + install(); + const view = render(); + + view.unmount(); + + expect(stopped).toEqual([]); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/Chat/useAudioRecording.ts b/src/workspaces/officerdev/src/apps/Chat/useAudioRecording.ts index cbca46bd..c14434d6 100644 --- a/src/workspaces/officerdev/src/apps/Chat/useAudioRecording.ts +++ b/src/workspaces/officerdev/src/apps/Chat/useAudioRecording.ts @@ -1,4 +1,4 @@ -import { useState, useRef } from 'react'; +import { useState, useRef, useEffect } from 'react'; import { toast } from 'sonner'; export function useAudioRecording(onTranscription: (text: string) => void) { @@ -85,6 +85,22 @@ export function useAudioRecording(onTranscription: (text: string) => void) { } }; + // Nothing here released the microphone on unmount, and a panel can go away mid-recording — closing the + // Chat panel, switching dashboards, a layout change that remounts the slot. The recorder is only ever + // stopped by the user pressing the button again, so without this the stream outlives the component and + // the browser keeps showing the recording indicator for the life of the tab. + useEffect( + () => () => { + const recorder = mediaRecorderRef.current; + if (!recorder) return; + if (recorder.state !== 'inactive') recorder.stop(); + recorder.stream?.getTracks().forEach((t) => t.stop()); + mediaRecorderRef.current = null; + chunksRef.current = []; + }, + [], + ); + return { recording, transcribing, toggleRecording }; } diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/AudioStreamPlayer.tsx b/src/workspaces/officerdev/src/apps/FileBrowser/AudioStreamPlayer.tsx index 558c065a..c5c4fd67 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/AudioStreamPlayer.tsx +++ b/src/workspaces/officerdev/src/apps/FileBrowser/AudioStreamPlayer.tsx @@ -67,6 +67,12 @@ export const AudioStreamPlayer = ({ wsUrl, onError }: AudioStreamPlayerProps) => const wsRef = useRef(null); const gainRef = useRef(null); + // The effect below runs once per `wsUrl` and registers listeners that outlive every render after it, so a + // named `onError` dependency would either tear the stream down on each render or freeze the first render's + // callback. A ref is the third option: one stream, current callback. + const onErrorRef = useRef(onError); + onErrorRef.current = onError; + useEffect(() => { let disposed = false; let audioCtx: AudioContext | null = null; @@ -115,7 +121,7 @@ export const AudioStreamPlayer = ({ wsUrl, onError }: AudioStreamPlayerProps) => }); ws.addEventListener('error', () => { - if (!disposed) onError?.('Audio stream connection failed'); + if (!disposed) onErrorRef.current?.('Audio stream connection failed'); }); ws.addEventListener('close', () => { @@ -123,7 +129,7 @@ export const AudioStreamPlayer = ({ wsUrl, onError }: AudioStreamPlayerProps) => }); } catch (err) { if (!disposed) { - onError?.(err instanceof Error ? err.message : 'Audio playback failed'); + onErrorRef.current?.(err instanceof Error ? err.message : 'Audio playback failed'); } } }; diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/DictateDialog.tsx b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/DictateDialog.tsx index c200d6b3..a8edfa39 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/DictateDialog.tsx +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/DictateDialog.tsx @@ -19,11 +19,13 @@ export const DictateDialog = ({ fileBrowserManager }: DictateDialogProps) => { const animFrameRef = useRef(0); const streamRef = useRef(null); + // Safe to call twice: the success path in `stopRecording` releases the hardware as early as it can, and + // the effect cleanup then runs over the same refs. Closing an already-closed AudioContext rejects. const cleanup = () => { cancelAnimationFrame(animFrameRef.current); mediaRecorderRef.current?.stream?.getTracks().forEach((t) => t.stop()); streamRef.current?.getTracks().forEach((t) => t.stop()); - audioCtxRef.current?.close(); + if (audioCtxRef.current && audioCtxRef.current.state !== 'closed') void audioCtxRef.current.close(); mediaRecorderRef.current = null; streamRef.current = null; audioCtxRef.current = null; @@ -32,6 +34,11 @@ export const DictateDialog = ({ fileBrowserManager }: DictateDialogProps) => { setRecording(false); }; + // The effect below closes over `cleanup` from the render that opened the dialog. Reading it through a + // ref means the teardown always runs the current one rather than a stale capture. + const cleanupRef = useRef(cleanup); + cleanupRef.current = cleanup; + const startRecording = async () => { try { const stream = await navigator.mediaDevices.getUserMedia({ audio: true }); @@ -139,13 +146,15 @@ export const DictateDialog = ({ fileBrowserManager }: DictateDialogProps) => { } }; + // Teardown runs when the dialog closes and when the panel unmounts. It used to be guarded by + // `if (!showDictate)`, which can never be true: a cleanup sees the props of the render that registered + // it, and only the `showDictate === true` render registers one. So it never fired, and the microphone, + // the AudioContext and the rAF loop stayed alive for the life of the tab. `QrTransfer/Receiver.tsx` is + // the same shape done right. useEffect(() => { - if (showDictate) { - startRecording(); - } - return () => { - if (!showDictate) cleanup(); - }; + if (!showDictate) return; + startRecording(); + return () => cleanupRef.current(); }, [showDictate]); useEffect(() => { diff --git a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/usePipelineRunner.ts b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/usePipelineRunner.ts index 76d96494..cbf37899 100644 --- a/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/usePipelineRunner.ts +++ b/src/workspaces/officerdev/src/apps/FileBrowser/FileBrowserApp/components/usePipelineRunner.ts @@ -267,6 +267,13 @@ export function usePipelineRunner() { } }, [flushStream, stopTimer, addCost]); + // The socket below is opened once and must stay open, so its listener is registered once too — and would + // hold the first render's `handleEvent` forever. That closure carries `flushStream`'s captured + // `streamingText`, so a re-render mid-run would leave every later event folding into a stale buffer. + // Reading through a ref keeps the socket long-lived and the handler current. + const handleEventRef = useRef(handleEvent); + handleEventRef.current = handleEvent; + useEffect(() => { const token = localStorage.getItem('BEARER_TOKEN'); if (!token) return; @@ -287,7 +294,7 @@ export function usePipelineRunner() { ws.addEventListener('message', (ev) => { try { const msg = JSON.parse(ev.data) as ServerMessage; - handleEvent(msg); + handleEventRef.current(msg); } catch { // ignore } diff --git a/src/workspaces/officerdev/src/apps/Jellyfin/VideoPlayer.tsx b/src/workspaces/officerdev/src/apps/Jellyfin/VideoPlayer.tsx index 041e3bef..ffc7356e 100644 --- a/src/workspaces/officerdev/src/apps/Jellyfin/VideoPlayer.tsx +++ b/src/workspaces/officerdev/src/apps/Jellyfin/VideoPlayer.tsx @@ -212,8 +212,10 @@ export const VideoPlayer = ({ id }: { id: string }) => { }, [mode, plan, token]); // The stop report, on the way out. Also the only place the server learns a transcode is no longer wanted. - // The dependency list is empty and must stay empty — this cleanup nulls `planRef`, so anything that made it - // re-run mid-playback would silently switch off every report that follows. + // This must run exactly once, at unmount: the cleanup nulls `planRef`, so anything that made it re-run + // mid-playback would silently switch off every report that follows. `sendReport` is `useCallback(…, [])` + // and never changes identity, which is what makes naming it here equivalent to an empty list — and is why + // it has to stay that way. If it ever grows a dependency, this becomes the Continue Watching bug again. useEffect( () => () => { sendReport('stopped'); diff --git a/src/workspaces/officerdev/src/components/Workspace/PanelSlot.tsx b/src/workspaces/officerdev/src/components/Workspace/PanelSlot.tsx index 597ce9a3..443af0c1 100644 --- a/src/workspaces/officerdev/src/components/Workspace/PanelSlot.tsx +++ b/src/workspaces/officerdev/src/components/Workspace/PanelSlot.tsx @@ -440,17 +440,19 @@ export const PanelSlot = ({ ); } - // All apps get a chrome header — custom HeaderComponent or default from registry icon+name - const DefaultHeader = entry - ? () => ( - <> - - {entry.name} - - ) - : null; - - const ResolvedHeader = HeaderComponent ?? DefaultHeader; + // All apps get a chrome header — a custom HeaderComponent, or the registry's icon and name. The default + // used to be a component declared here, which makes it a new component *type* on every render: React + // cannot match it to the previous one, so the header subtree unmounted and remounted continuously. It is + // harmless while the default is stateless, and stops being harmless the moment it isn't. Elements, not + // types, are the thing to build inside a render. + const header = HeaderComponent ? ( + + ) : entry ? ( + <> + + {entry.name} + + ) : null; const trafficLights = interactive && !isMobile ? ( @@ -484,7 +486,7 @@ export const PanelSlot = ({ const headerContent = (
{mobileBackButton} - {ResolvedHeader && } + {header} {!mobileBackButton && onClose && (