From 04ccb05b2ca92b3cee87008a1bcc9eb581ad29ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 10:57:10 +0000 Subject: [PATCH] mark the effect-hygiene items resolved, and correct two of them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two entries in 5.7 were wrong. HostTerminalWrapper was fixed in c92b51c, when all three wrappers moved onto useTerminalSession — the item had simply not been re-read since. And useTaskRunner does not abandon a running task: `stop` is sent from the modal's Stop button, and closing the socket kills the process tree server-side. Both were written from the hook alone without following the call into the modal or the executor. VideoPlayer and the remaining VideoPlayer-shaped case are left alone on purpose, with the reason written down rather than the item deleted. Co-Authored-By: Claude Opus 5 --- docs/workspace-panel-todo.md | 58 +++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/docs/workspace-panel-todo.md b/docs/workspace-panel-todo.md index 5f008006..1c29cd21 100644 --- a/docs/workspace-panel-todo.md +++ b/docs/workspace-panel-todo.md @@ -558,35 +558,45 @@ All the same bug: an app guessing "am I being closed?" from an unmount, or payin mount — React bails on the unchanged `setStatus`, so it settles rather than looping) and `hooks/useChat.ts:124` (a `useCallback`, harmless). Owner has used this pattern for years without issue; my first write-up called it a live bug and that was overstated. -- [ ] **`DictateDialog` never releases the microphone.** - `apps/FileBrowser/FileBrowserApp/components/DictateDialog.tsx:142-149` — +- [x] **`DictateDialog` never releases the microphone.** — _resolved `c0fae47`_ `return () => { if (!showDictate) cleanup(); }`. The cleanup that runs on `true → false` is the one registered by the `true` render, where `showDictate` is `true`, so it never fires. Mic stream, `AudioContext` and the rAF loop leak. `apps/QrTransfer/Receiver.tsx:120` is the correct version. -- [ ] **`useAudioRecording` has no unmount cleanup at all** (`apps/Chat/useAudioRecording.ts` — no + Fixed by dropping the guard and reading `cleanup` through a ref, so the teardown runs on close and on + unmount and is the current one either way. `cleanup` also had to become safe to call twice — the + success path in `stopRecording` releases the hardware early, and closing an already-closed + `AudioContext` rejects. +- [x] **`useAudioRecording` has no unmount cleanup at all** (`apps/Chat/useAudioRecording.ts` — no `useEffect`). Unmounting Chat mid-recording leaves the mic open for the life of the tab. -- [ ] **`HostTerminalWrapper` still has the cleanup the other two deliberately removed.** - `apps/Terminal/HostTerminalWrapper.tsx:28-35` deletes the panel→session mapping from persisted - state on _any_ unmount — exactly what `TerminalWrapper.tsx:30-38` and - `CommandTerminalWrapper.tsx:31-32` document removing, and why. Latent only because - `officerdev/terminal-host` is in no default layout and hidden from the picker - (`apps/Terminal/index.tsx:51`). -- [ ] **`useTaskRunner` abandons the running task.** `:60-63` is a bare `ws.close()`. A `stop` message - exists at `:76-79` and is never sent, and there is no re-attach path — unlike `usePipelineRunner`, - which re-attaches by `jobId` (`:281-283`). + — _resolved `c0fae47`_, and pinned by `useAudioRecording.test.tsx`: the hook is rendered, put into + recording, and unmounted, and the test asserts the track was stopped. +- [x] **`HostTerminalWrapper` still has the cleanup the other two deliberately removed.** — _resolved + `c92b51c`_, before this section was re-read. All three wrappers now share `useTerminalSession`, which + keeps the `panelId → sessionId` map across unmounts and drops it on a real `usePanelClose` — the §5.1 + signal is what made a correct answer possible. +- [ ] **REVISED — `useTaskRunner` does not abandon the running task; it kills it.** The original entry was + wrong twice, both times by reading only the hook. `stop` **is** sent — + `TaskRunnerModal.tsx:1320` renders a Stop button while `phase === 'running'`. And a bare `ws.close()` + is not abandonment: `task-executor.ts:303-309` kills the process tree on socket close, the same + `killTree` the Stop button reaches. What is true is the last clause: there is no re-attach, so an + inline run dies with its modal. That is defensible — inline is the *ephemeral* mode and the job path + exists for everything else — so this is left alone deliberately rather than left undone. - [ ] **`VideoPlayer` kills the transcode on incidental unmount.** `apps/Jellyfin/VideoPlayer.tsx:217-223` - POSTs `stopped`, killing server-side ffmpeg, then renegotiates. Fires on every "yes" row in 5.2. - Also: the comment at `:215-216` says the dep list "must stay empty" while the code passes - `[sendReport]` — harmless today, misleading. -- [ ] **Two stale-closure sockets.** `usePipelineRunner.ts:270-301` (deps `[]` but calls a `useCallback` - that changes identity — keeps the first-render copy forever) and - `apps/FileBrowser/AudioStreamPlayer.tsx:70-143` (safe today, latent). -- [ ] **`PanelSlot` defines a component inside render.** `:341-348` — `DefaultHeader` is a new component - _type_ every render, so the header subtree remounts constantly. Harmless while stateless; a trap - the moment it isn't. -- [ ] **The context value is a fresh literal.** `WorkspaceView.tsx:145-163` — every `useWorkspace()` - consumer in every panel re-renders on every `WorkspaceView` render, including each maximize - animation frame. Multiplies the cost of the `DesktopView` bug. + POSTs `stopped`, killing server-side ffmpeg, then renegotiates. Fires on every "yes" row in 5.2 — + one row fewer since `56ca411`. Left as is: the player renegotiates from scratch on remount anyway, so + the stop is correct, only expensive. The real fix is 5.2's, not this file's. + The misleading comment at `:215-216` is corrected in `c0fae47`: `sendReport` is `useCallback(…, [])`, so + naming it is equivalent to an empty list — and that is now written down as a constraint, because if + it ever grows a dependency this becomes the Continue Watching bug again. +- [x] **Two stale-closure sockets.** — _resolved `c0fae47`_, both with the ref pattern rather than by + restarting the socket. `usePipelineRunner` kept the first render's `handleEvent`, whose `flushStream` + carries a captured `streamingText`; `AudioStreamPlayer` kept the first `onError`. +- [x] **`PanelSlot` defines a component inside render.** — _resolved `c0fae47`_. `DefaultHeader` is gone: the + header is now an element, not a component type, so there is nothing for React to fail to match. +- [x] **The context value is a fresh literal.** — _resolved `c0fae47`_. `useMemo` over the eighteen members. + Note what it does *not* buy: the value still changes whenever `layout` does, because half the + callbacks close over it. What it stops is the renders that change nothing a panel can see — the + ephemeral pane opening, a mobile panel switch, every frame of a maximize animation. ### 5.8 Navigation — finish the refactor