From 7fa6de717d3fad496281af7d42421fe5f6e3c4f1 Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Tue, 18 Aug 2026 21:46:18 -0400 Subject: [PATCH 1/2] test(desktop): stop the focus-resume test racing a fixed 10ms sleep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `focused polling pauses on blur and resumes after activation yields` waited a single `setTimeout(…, 10)` for the resume to land, then asserted the observed sequence. The resume is a three-hop chain — `scheduleAfterForegroundReady` does `setTimeout(0)` → `requestAnimationFrame` → `setTimeout(0)` — and jsdom fires rAF on a ~16ms cadence, so 10ms could never reliably cover it. The test passed on an idle machine and failed whenever the event loop was contended, which is exactly what a full `pnpm test` run does. It now polls until the resume is observed (2s ceiling) instead of guessing a duration. The assertion is unchanged and just as strict: the same `assert.deepEqual(observed, [1_000, false, 1_000])` still runs, so a resume that never fires still fails, with the same diff. Measured on x86_64-pc-windows-msvc, running the file 12x with six busy loops saturating the CPU: - before: 8 passed, 4 failed - after: 12 passed, 0 failed Full desktop suite with the fix: `pnpm test` 5037 passed, 0 failed (it had been reporting 5036/1 on this test); `pnpm check` exit 0. Signed-off-by: Michael Feth --- .../src/shared/lib/useDocumentVisible.test.mjs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/desktop/src/shared/lib/useDocumentVisible.test.mjs b/desktop/src/shared/lib/useDocumentVisible.test.mjs index bd3d54cbb37..cb86761bd43 100644 --- a/desktop/src/shared/lib/useDocumentVisible.test.mjs +++ b/desktop/src/shared/lib/useDocumentVisible.test.mjs @@ -287,9 +287,19 @@ describe("visibility-gated hooks", () => { focused = true; await act(async () => window.dispatchEvent(new window.Event("focus"))); assert.deepEqual(observed, [1_000, false]); - await act( - async () => new Promise((resolve) => window.setTimeout(resolve, 10)), - ); + // Wait for the resume to actually land rather than for a fixed duration. + // `scheduleAfterForegroundReady` is a three-hop chain — + // setTimeout(0) → requestAnimationFrame → setTimeout(0) — and jsdom fires + // rAF on a ~16ms cadence, so a single 10ms sleep could never reliably + // cover it: the test passed only when the event loop happened to be idle + // and failed under parallel load. Polling keeps the assertion exactly as + // strict while removing the race. + await act(async () => { + const deadline = Date.now() + 2_000; + while (observed.length < 3 && Date.now() < deadline) { + await new Promise((resolve) => window.setTimeout(resolve, 5)); + } + }); assert.deepEqual(observed, [1_000, false, 1_000]); await act(async () => root.unmount()); From e62d1df2da6ac9e525c637331bf0019ca84bf4aa Mon Sep 17 00:00:00 2001 From: Michael Feth Date: Wed, 19 Aug 2026 13:05:00 -0400 Subject: [PATCH 2/2] docs(desktop): correct the deferral chain this test actually exercises MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch from @jemiahw, and it is right. The comment claimed the resume runs `setTimeout(0) → requestAnimationFrame → setTimeout(0)` and blamed jsdom's ~16ms frame cadence for outrunning the old 10ms sleep. This jsdom is constructed without `pretendToBeVisual`, so it exposes no `requestAnimationFrame` at all — verified directly: plain jsdom rAF: undefined pretendToBeVisual rAF: function `scheduleAfterForegroundReady` therefore takes its fallback branch (`foregroundReady.ts`), and the real chain here is `setTimeout(0) → setTimeout(0)`, followed by a React re-render and effect flush. No frame is involved, so frame cadence explains nothing. The fix and its measurement are unaffected — two chained timer turns plus an effect are still not bounded by a fixed 10ms sleep once Node's timer phase is starved under load, which is what the 4-failures-in-12 measurement showed. Only the stated mechanism was wrong, and a wrong explanation in a comment about a race is worse than none: it sends the next reader to the wrong subsystem. Comment rewritten to describe the fallback path. No behaviour change. `node --test src/shared/lib/useDocumentVisible.test.mjs`: 5 passed. `pnpm check`: exit 0. Signed-off-by: Michael Feth --- .../shared/lib/useDocumentVisible.test.mjs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/desktop/src/shared/lib/useDocumentVisible.test.mjs b/desktop/src/shared/lib/useDocumentVisible.test.mjs index cb86761bd43..a702a9d91bf 100644 --- a/desktop/src/shared/lib/useDocumentVisible.test.mjs +++ b/desktop/src/shared/lib/useDocumentVisible.test.mjs @@ -288,12 +288,19 @@ describe("visibility-gated hooks", () => { await act(async () => window.dispatchEvent(new window.Event("focus"))); assert.deepEqual(observed, [1_000, false]); // Wait for the resume to actually land rather than for a fixed duration. - // `scheduleAfterForegroundReady` is a three-hop chain — - // setTimeout(0) → requestAnimationFrame → setTimeout(0) — and jsdom fires - // rAF on a ~16ms cadence, so a single 10ms sleep could never reliably - // cover it: the test passed only when the event loop happened to be idle - // and failed under parallel load. Polling keeps the assertion exactly as - // strict while removing the race. + // + // `scheduleAfterForegroundReady` defers through `setTimeout(0)` and then, + // because this jsdom is built without `pretendToBeVisual` and so exposes no + // `requestAnimationFrame`, through the fallback `setTimeout(0)` rather than + // a frame (`foregroundReady.ts`). Two timer turns, then React has to + // re-render and flush the effect that records `observed`. + // + // None of that is bounded by 10ms once the event loop is contended: Node + // starves the timer phase under load, so the second callback and the effect + // can both land after a fixed sleep has already resolved. Measured on this + // file with six busy loops saturating the CPU, the old form failed 4 runs + // in 12. Polling keeps the assertion exactly as strict while removing the + // race. await act(async () => { const deadline = Date.now() + 2_000; while (observed.length < 3 && Date.now() < deadline) {