From fab931fa28f8f446f23be17a3e4df14fa084e267 Mon Sep 17 00:00:00 2001 From: Jack Lau <72348727+jackulau@users.noreply.github.com> Date: Sat, 22 Aug 2026 15:40:31 -0500 Subject: [PATCH] docs(desktop): name the timer drain in the effect that performs it Review follow-up, comments and one type annotation. No behaviour change. The unmount effect now clears pending timers, but its leading comment is still entirely about the focus bus, so the cleanup reads as unrelated code that happens to be in the same block. Say why it lives there: both concerns are "this composer is going away", they unmount together by definition, and a sibling unmount-only effect would only be a second place to forget. In the regression test, the clearTimeout mock declared id as number. Nothing that reaches it is a number: jsdom under node returns a Timeout object, which is why the scheduled and cleared arrays are unknown[] and compared by identity. The annotation documented a shape the test never sees, so it is now unknown with the cast moved to the one call that genuinely wants a number. --- .../components/assistant-ui/thread/user-edit-composer.tsx | 5 +++++ .../assistant-ui/thread/user-message-edit.test.tsx | 8 ++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx b/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx index a61409b3ae..cd0d45eb34 100644 --- a/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx +++ b/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx @@ -140,6 +140,11 @@ export const UserEditComposer: FC = ({ cwd, gateway, sess // bus leaks: confirming or cancelling an edit tears the composer down while // `'edit'` is still the active target. Release it alongside the thread-scroll // cleanup so keyboard routing falls back to the visible chat composer. + // + // It also drains whatever `scheduleTimeout` still has pending, which is a + // second concern under the same heading rather than a separate one: both + // are "this composer is going away", they unmount together by definition, + // and a sibling unmount-only effect would only be a second place to forget. useEffect( () => () => { notifyThreadEditClose() diff --git a/apps/desktop/src/components/assistant-ui/thread/user-message-edit.test.tsx b/apps/desktop/src/components/assistant-ui/thread/user-message-edit.test.tsx index 5befc0515d..98449a0cb3 100644 --- a/apps/desktop/src/components/assistant-ui/thread/user-message-edit.test.tsx +++ b/apps/desktop/src/components/assistant-ui/thread/user-message-edit.test.tsx @@ -263,9 +263,13 @@ describe('Enter submission and latch behavior', () => { return id }) as typeof window.setTimeout) - vi.spyOn(window, 'clearTimeout').mockImplementation(((id?: number) => { + // `id` is typed `unknown` for the same reason the arrays above are: what + // actually arrives is whatever `setTimeout` returned, and under jsdom that + // is a Timeout object rather than the `number` the DOM lib promises. + // Declaring it `number` would have documented a shape this never sees. + vi.spyOn(window, 'clearTimeout').mockImplementation(((id?: unknown) => { cleared.push(id) - realClearTimeout(id) + realClearTimeout(id as number | undefined) }) as typeof window.clearTimeout) const onEdit = vi.fn(async () => {})