mark the effect-hygiene items resolved, and correct two of them
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user