Skip to content

test(desktop): stop the focus-resume test racing a fixed 10ms sleep #6278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mfethe1 wants to merge 2 commits into
base: main
Choose a base branch
from
+20 −3
Open

test(desktop): stop the focus-resume test racing a fixed 10ms sleep #6278

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fa6de7
test(desktop): stop the focus-resume test racing a fixed 10ms sleep
Aug 19, 2026
e62d1df
docs(desktop): correct the deferral chain this test actually exercises
Aug 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions desktop/src/shared/lib/useDocumentVisible.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,26 @@ 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.

@jemiahw jemiahw Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description/comment incorrectly says this test exercises:

setTimeout(0) → requestAnimationFrame → setTimeout(0)

The test creates jsdom without pretendToBeVisual: true, so requestAnimationFrame is undefined. Production code therefore takes the fallback path:

setTimeout(0) → setTimeout(0)

See useDocumentVisible.test.mjs:254-257 and foregroundReady.ts:28-34.

@mfethe1 mfethe1 Aug 19, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, and thank you — I checked and this jsdom exposes no requestAnimationFrame at all:

plain jsdom rAF: undefined
pretendToBeVisual rAF: function

So scheduleAfterForegroundReady takes the fallback branch and the chain here is setTimeout(0) → setTimeout(0), then a React re-render and effect flush. No frame, so frame cadence explained nothing — I reasoned from the production code path without checking which branch this fixture actually reaches.

Fixed in e62d1df, and the PR description is corrected too. The real reason 10ms was not enough: two chained timer turns plus an effect are not bounded by a fixed sleep once Node's timer phase is starved under load, which is what the measurement showed (4 failures in 12 runs with six busy loops saturating the CPU; 0 in 12 after).

The fix and the numbers are unchanged — only the explanation was wrong. Worth correcting anyway, since a wrong mechanism in a comment about a race points the next reader at the wrong subsystem.

//
// `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) {
await new Promise((resolve) => window.setTimeout(resolve, 5));
}
});
assert.deepEqual(observed, [1_000, false, 1_000]);

await act(async () => root.unmount());
Expand Down