fix(test): clear leaked timers so one cannot fail an unrelated CI run

[cliffhall]

Closes #1984

A window.setTimeout that outlives its test file fires after happy-dom has disposed that file's window, and React's dispatchSetState then throws an uncaught ReferenceError: window is not defined. Vitest attributes it to whichever file happened to be running, so one leak fails the entire run with every test passing — under a step named "Enforce per-file coverage gate", which sends the reader somewhere else entirely:

Test Files  318 passed (318)
Tests       4929 passed (4929)
Errors      1 error          ← this is what failed the step

Seen twice in CI, both on unrelated docs-only branches, both attributed to the innocent ServerRemoveConfirmModal.test.tsx.

It is neither a Mantine bug nor a badly-written test

Both hooks do clear their timers on unmount:

useEffect(() => () => { clearAllTimeouts(); }, []);   // use-transition
return () => window.clearTimeout(timeout.current);    // use-lock-scroll

It is a rAF race. The transition schedules rAF → rAF → setTimeout, and clearAllTimeouts cancels only the pending rAF. If the inner callback is already in flight when the unmount lands, cancelAnimationFrame is a no-op and that callback goes on to schedule a setTimeout after cleanup has already run. Nothing owns that timer. Hitting the window requires a loaded machine, which is why it only ever appeared in CI.

The guarantee we thought we had does not exist

Both AGENTS.md and setup.ts's own comment assert that env="test" makes transitions synchronous and is "the actual protection" against this. It isn't — and believing it is why the leak went unexamined.

env is read only by Transition.mjs, at its render branch:

// components/Transition/Transition.mjs:45
if (transitionDuration === 0 || env === "test") { /* render children directly */ }

useTransition() is called at line 32, before that check — hooks cannot be conditional — and its useDidUpdate(…, [mounted]) still runs the timer path. Measured, opening a <Modal> through renderWithMantine:

scheduledTimeouts: [200, 200, 200]

Three real timers, in the configuration we believed suppressed them. Both docs are corrected here.

The fix

Track every timer in setup.ts and clear whatever is still outstanding after cleanup(). Ordering is load-bearing: legitimate unmount cleanups get their turn first, so only true leaks are dropped. Under vi.useFakeTimers() the wrapper is swapped out and nothing is tracked — correct, since a fake timer cannot outlive the environment.

A component-by-component hunt was the alternative; this closes the class instead, including non-Mantine sources.

Rejected: respectReducedMotion: true

The obvious one-liner, since matchMedia is already pinned to prefers-reduced-motion: reduce. Measured before proposing:

Configuration	Timers scheduled
current (env="test")	[200, 200, 200]
+ respectReducedMotion: true	[200]

One survives, from ModalBase/use-lock-scroll.mjs. Landing it alone would have made the flake rarer and harder to diagnose without fixing it.

Verification

The regression test is mutation-verified — a test that cannot fail on the regression it names is worse than none:

State	Result
net disabled	1 failed | 4 passed
net restored	5 passed

It deliberately does not assert that Mantine schedules no timers. It does, and that is normal; the contract is only that none survive the test that scheduled it.

Full npm run ci → EXIT=0.

No screenshots — test infrastructure, no web-UI or TUI surface.

Sibling: #1985 is a different flake with the same consequence (a red run on an innocent PR) — a fixed 300 ms sleep racing a real subprocess death in the integration project. Unrelated cause, tracked separately.

[Copilot]

Pull request overview

Adds a test-wide timer safety net intended to prevent happy-dom teardown flakes.

Changes:

• Tracks and clears pending test timers after cleanup.
• Adds regression coverage for leaked timers and Mantine modals.
• Corrects timer-behavior documentation.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
clients/web/src/test/setup.ts	Adds timer tracking and teardown cleanup.
clients/web/src/test/leakedTimers.test.tsx	Tests leaked-timer handling.
AGENTS.md	Corrects Mantine test guidance.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

This is the best catch of the review — the net still had the exact hole it was written to close, and I had described that hole in the commit message while leaving it open.

You are right about the mechanism: the afterEach is synchronous, so a queued frame callback cannot run until it returns, and it then registers a fresh setTimeout after the sweep has already drained. On the file's last test that timer survives teardown.

Fixed by tracking animation frames as well and cancelling them before sweeping timers. The ordering is the fix, not an incidental detail — JS is single-threaded and nothing between the two loops yields, so no frame callback can run in between:

for (const handle of pendingFrames) realCancelAnimationFrame(handle);
pendingFrames.clear();
// then, and only then:
for (const id of pendingTimers) realClearTimeout(id);
pendingTimers.clear();

Covered deterministically as you asked, with a test that queues a frame which would register a timer, and a following test asserting neither ever ran. Mutation-checked so it cannot rot into a test that always passes:

State	Result
frames not cancelled first	1 failed | 6 passed
frames cancelled first	7 passed

[cliffhall]

Review round 1 — all three valid; one was a hole in the fix itself (c4e8ed9)

Mirroring at PR level, since the inline threads go outdated on push.

1. The sweep could still miss the race. The most important comment, and correct. The afterEach is synchronous, so a queued frame callback cannot run until it returns — and then registers a fresh setTimeout after the sweep has drained. On a file's last test that timer survives teardown. I had described this exact race in the commit message and then left it open in the fix for it.

Now tracks animation frames too and cancels them before sweeping timers. The order is the fix: nothing between the two loops yields, so no frame callback can run in between. Mutation-checked:

State	Result
frames not cancelled first	1 failed | 6 passed
frames cancelled first	7 passed

2. renderWithMantine.tsx still documented the disproven behavior. Fixed. That was the third copy of the claim (AGENTS.md, setup.ts, the helper), and the worst one to leave — it is the file contributors are pointed at. One confidently-worded comment had been copied outward until three files asserted it and none had been checked against Transition.mjs.

3. Unjustified as unknown as. Fixed. It broke the repo's own double-cast rule, which requires a justification for why nothing better exists — and here something better plainly did.

Verification: npm run ci → EXIT=0.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

AGENTS.md:638

• This review-relevant guidance now says the global setup.ts safety net is what prevents the leak, but the same sentence still claims that a bare MantineProvider reintroduces it. The net is installed for every unit test regardless of render helper, so that rationale is now stale. The stale claim also remains in .github/copilot-instructions.md:95; AGENTS.md requires review guidance to be mirrored there in the same PR. Update both copies to preserve the renderWithMantine rule without attributing timer safety to the helper.

- Use `renderWithMantine` from `src/test/renderWithMantine.tsx` to render components — it wraps in `MantineProvider` with the project theme. It sets `env="test"`, which makes Mantine skip the transition's animated **render** — but be clear on what that does *not* buy you: it does **not** stop the timers. `env` is read only by `Transition.mjs`, at its render branch (`transitionDuration === 0 || env === "test"`), while `useTransition()` runs before that check (hooks cannot be conditional) and still schedules real `window.setTimeout`s on every `mounted` change. Measured: opening a `<Modal>` through `renderWithMantine` schedules three 200ms timers (#1984). A timer that outlives its file fires after happy-dom disposes that file's `window`, and React's `dispatchSetState` then throws an uncaught `ReferenceError: window is not defined` that fails the **whole run** — attributed to whichever file was running, so it lands on an innocent one (#1760). What actually prevents that is the **leaked-timer safety net in `src/test/setup.ts`**, which tracks every timer and clears whatever is still pending after `cleanup()`; see the comment there for the rAF race that makes Mantine's own unmount cleanup insufficient. **Always render through `renderWithMantine`; do not hand-roll a bare `MantineProvider` in a test** (that reintroduces the leak class). To exercise a **forced color scheme** (e.g. the `useComputedColorScheme` dark branch) pass the `colorScheme` option — `renderWithMantine(ui, { colorScheme: "dark" })` — instead of hand-rolling a `defaultColorScheme="dark"` provider (#1786). Only when a test must assert _mid-flight_ transition state (e.g. a `data-anim="out"` cell during an exit crossfade) use `renderWithMantineTransitions` (real transitions). Such a test can leak the #1760 class because waiting for one cell to unmount does **not** settle a concurrent _enter_ (a completed enter leaves no DOM signal to `waitFor`), so the helper **automatically drains the in-flight animation after the test**. The rule for using it: pass `settleMs` derived from the component's real animation duration — its `Transition` `duration`/`exitDuration` plus any `enterDelay`/`exitDelay` plus rAF slack — e.g. `renderWithMantineTransitions(ui, { settleMs: HEADER_ANIM_MS + 200 })` (so the window can't silently become insufficient when that duration changes); do **not** also use `vi.useFakeTimers()` in the same test (the auto-settle no-ops under fake timers — it warns, but anything the test left pending on the _real_ clock is then unprotected, so the test depends on which clock was installed at teardown); and if the test unmounts the tree itself, use the `unmount()` the helper returns (it drops that tree from the settle's liveness check, while still draining — a bare mid-body `cleanup()` on a still-armed tree would trip the check). The mechanism behind all three — why the drain is `act`-wrapped, the fake-timer hazard, the `afterEach`-before-`cleanup()` ordering and its `container.isConnected` self-checks, and the exported `settleTransitions(ms)` for manual mid-body settling — is documented at length on the helper in `renderWithMantine.tsx`; read there before changing it.

[cliffhall]

Review round 2 — correct, and a second-order effect I had missed (b451949)

Good catch, and a subtler class than the earlier ones: the fix invalidated a neighbouring rule's rationale.

Once the net moved into setup.ts it became global — every unit test, regardless of render helper — so "do not hand-roll a bare MantineProvider (that reintroduces the leak class)" stopped being true in the same PR that made it stop being true. I changed the sentence around it and did not re-read what followed.

The rule still stands, so it stays; only the justification changes:

Always render through renderWithMantine; do not hand-roll a bare MantineProvider in a test — a hand-rolled provider skips the project theme and the helper's options and drifts from every other test. Note this rule used to be justified by the leak class, which is now wrong: the setup.ts net is global and covers every unit test however it renders. The rule stands on consistency, not on timer safety.

Both copies say outright that the old reason no longer holds, so the next person does not reconstruct it from memory.

You were also right that .github/copilot-instructions.md:95 carried the identical stale claim and that AGENTS.md requires it mirrored in the same PR — updated there too.

That is four places this one belief had reached. The through-line for this whole PR: a plausible mechanism claim gets copied outward and nobody re-derives it — and a fix can create stale docs by making a previously-true statement false, which is invisible if you only read the changed lines.

Verification: npm run ci → EXIT=0.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

clients/web/src/test/setup.ts:160

• Happy DOM returns a NodeJS.Timeout object at runtime, so an explicitly cleared timer fails this numeric guard and remains in pendingTimers; teardown then treats it as leaked and clears it again. This also means the new “stops tracking” test passes without proving its claim because clearTimeout is idempotent. Delete any defined handle regardless of its runtime representation.

window.clearTimeout = ((id?: number): void => {
  if (typeof id === "number") {
    pendingTimers.delete(id);
  }

[cliffhall]

Review round 3 — correct on a fact I assumed instead of checking (c0e4b89)

Probed it rather than reasoning about it:

Handle	typeof	Constructor
setTimeout	object	Timeout
requestAnimationFrame	object	Immediate

So typeof id === "number" never matched, exactly as you said, and no explicitly-cleared timer was ever untracked. Both Set<number> annotations and const id: number were fictions TypeScript could not catch, because the DOM lib declares number while happy-dom returns an object — the declaration and the runtime disagree and only the declaration is visible to tsc.

The runtime effect was benign, as you noted. The part that matters is your second sentence: the test asserting this passed for the wrong reason. expect(() => clearTimeout(id)).not.toThrow() holds whether or not tracking works, since clearTimeout is idempotent — so it proved nothing and concealed the broken guard.

Now:

• handles held as unknown, with two small helpers carrying one justified cast each;
• no numeric guards — anything defined is untracked;
• the test asserts the net's own bookkeeping via an exported pendingTimerCount(), mutation-checked so it cannot go vacuous again: restoring the numeric guard fails it (1 failed | 6 passed).

Worth naming the pattern, since this is the second test in this PR you have caught passing without proving its claim (the first was the teardown test that only exercised removeSafe). Both had the same tell: the assertion would have held with the feature removed. I have been writing tests that confirm the code runs rather than that it works.

Verification: npm run ci → EXIT=0.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.