From 94c154e225862d3538d1b8e8a89edb95b5435e1b Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Sat, 14 Nov 2026 11:44:57 +1100 Subject: [PATCH] test(desktop): unmount every TerminalSubstrate tree before the window closes (#9440) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `desktop/src/features/terminal/TerminalSubstrate.test.mjs` can pass every test and then run the Desktop JS job out of memory several minutes later. This moves the React Testing Library unmount from `beforeEach` to `afterEach`, so no tree is still mounted when the jsdom window closes. Only the test file changes (6 insertions, 2 deletions). Commit (cherry-picked unchanged from a local flake-fix series): - `test(desktop): unmount every TerminalSubstrate tree before the window closes` ### CI sighting One sighting was found in the CI runs from 7 to 12 November 2026 that were searched: - 9 November 2026, "Desktop Domain / Desktop JS", run 42706004493 attempt 1 (on #9227; the rerun passed): - https://github.com/block/buzz/actions/runs/42706004493/job/129463488242 In that job the suite reported 6,950 of 6,951 passing. The log shows `An update to TerminalSubstrate inside a test was not wrapped in act(...)` at 21:54:33Z, then `FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory` at 22:02:17Z, and the file is reported as failed at file level (`test at src/features/terminal/TerminalSubstrate.test.mjs:1:1`) after 504.6 s. ### Root cause The file called `cleanup()` only in `beforeEach`, so the last test's tree was still mounted when `after(() => dom.window.close())` ran. If ThemeProvider's asynchronous palette update landed after that close, it re-rendered the substrate, the splash effect's palette guard passed, and `requestAnimationFrame` on the closed window started a fresh jsdom interval that nothing cleared. That 60 Hz loop repainted the banner into the test's recording `getContext` stub indefinitely, and `paintLog` grew until the heap limit. No individual test hangs, so a per-test timeout would not catch it. ### Fix `afterEach(() => cleanup?.())` unmounts every tree, including the last, before `after()` closes the window. The `beforeEach` call became redundant and is removed. No assertions change, and no timeouts, retries or sleeps are added. ### Related issue N/A (flake fix). No existing issue or PR for this failure was found. Three open PRs touch the same test file (#5057, #5485, #7777); none of them changes where `cleanup` runs. ### Testing Human testing is still required. This PR does not carry `buzz-review-completed`; the author has not yet run this test themselves on this branch. #### Verification of the fix (before this branch was split out) - The test file, 5 consecutive runs: 25 of 25 tests passed each time in about 6.8 s, with no `act` warning. - The CI out-of-memory failure was not reproduced with the real test file. Its mechanism was reproduced in a scratch harness with the same stubs: without cleanup before the window close, 3 of 3 runs left the palette unresolved at close, added 11,556 `paintLog` entries per 100 ms, kept one live timer, and never exited. With cleanup before the close, 3 of 3 runs added no entries, left no timers, and exited on their own. #### Local checks on this branch The push ran the repository's full pre-push hook set from a checkout of this branch's head (`7f4117340`), with no lanes excluded and without `--no-verify`. The branch is one commit behind `main`; that commit changes only mobile and docs files, none of them touched here. Ran and passed: - `branch-skew`, `push-head-scope` and `file-size-check` - `desktop-check` (`pnpm check`) - `desktop-typecheck` (`pnpm typecheck`) - `desktop-test` (`pnpm test`, the full desktop unit suite, which includes `TerminalSubstrate.test.mjs`) Did not run: - `rust-tests`, `desktop-tauri-checks` and `mobile-checks`. The hook selects lanes by changed path, and this branch changes no Rust, Tauri or mobile file. - Desktop builds and the Playwright E2E suites, which are CI-only. - `just ci`. - The 5-run repetition and the scratch harness above were not repeated on this branch. A green run does not prove the flake is gone: the failure needs the palette update to land after the window closes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: mtoohey Signed-off-by: Matt Toohey Co-authored-by: Claude Fable 5.1 --- desktop/src/features/terminal/TerminalSubstrate.test.mjs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/desktop/src/features/terminal/TerminalSubstrate.test.mjs b/desktop/src/features/terminal/TerminalSubstrate.test.mjs index d6113cbf08..0ce197f79d 100644 --- a/desktop/src/features/terminal/TerminalSubstrate.test.mjs +++ b/desktop/src/features/terminal/TerminalSubstrate.test.mjs @@ -1,5 +1,5 @@ import assert from "node:assert/strict"; -import { after, before, beforeEach, test } from "node:test"; +import { after, afterEach, before, beforeEach, test } from "node:test"; import { JSDOM } from "jsdom"; @@ -67,8 +67,12 @@ before(async () => { }); after(() => dom.window.close()); +// Unmount after every test, not before the next one: a tree still mounted when +// the window closes can be re-rendered by ThemeProvider's late palette update, +// whose `requestAnimationFrame` on the closed window starts a jsdom interval +// that nothing clears and that fills `paintLog` until the heap limit. +afterEach(() => cleanup?.()); beforeEach(() => { - cleanup?.(); reducedMotion = false; paintLog.length = 0; dom.window.localStorage.clear();