From c0fae47cc7af6408f94fb5fe32a45183a1108ee7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 10:56:58 +0000 Subject: [PATCH] release what a panel was holding when it goes away MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The microphone was the loud one: DictateDialog's teardown was guarded by `if (!showDictate)`, which can never be true, because a cleanup sees the props of the render that registered it and only the open render registers one. So it never ran, and the mic, the AudioContext and the rAF loop stayed alive for the life of the tab. useAudioRecording had no unmount cleanup at all — closing a Chat panel mid-recording did the same thing, with no way to switch the recording indicator back off. Both now release on unmount; the second is pinned by a test that records, unmounts, and asserts the track stopped. The rest is the same shape. Two sockets registered a listener once and held the first render's callback forever — usePipelineRunner's carried a captured streamingText, so a re-render mid-run would have folded every later event into a stale buffer. PanelSlot built its default header as a component *type* inside render, which React cannot match against the previous one. WorkspaceView handed every panel a fresh context object on every render, including each frame of a maximize animation. VideoPlayer's comment claimed a dependency list that the code did not have; the list is fine (sendReport never changes identity) and the comment now says why that has to stay true. Co-Authored-By: Claude Opus 5 --- .../src/apps/Chat/useAudioRecording.test.tsx | 83 +++++++++++++++++++ .../src/apps/Chat/useAudioRecording.ts | 18 +++- .../apps/FileBrowser/AudioStreamPlayer.tsx | 10 ++- .../components/DictateDialog.tsx | 23 +++-- .../components/usePipelineRunner.ts | 9 +- .../src/apps/Jellyfin/VideoPlayer.tsx | 6 +- .../src/components/Workspace/PanelSlot.tsx | 26 +++--- .../components/Workspace/WorkspaceContext.ts | 2 +- .../components/Workspace/WorkspaceView.tsx | 71 +++++++++++----- 9 files changed, 200 insertions(+), 48 deletions(-) create mode 100644 src/workspaces/officerdev/src/apps/Chat/useAudioRecording.test.tsx 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 && (