From 1e0f94bb31a9034159f4b1b5895b01f43ca5869f Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 12 Aug 2026 00:16:05 -0400 Subject: [PATCH] fix(test): clear leaked timers so one cannot fail an unrelated CI run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A window.setTimeout that outlives its test file fires after happy-dom disposes that file's window; React's dispatchSetState then throws an uncaught ReferenceError: window is not defined. Vitest attributes it to whichever file was running, so a single leak fails the ENTIRE run with every test passing — landing on an innocent file under a step named "Enforce per-file coverage gate." Seen twice in CI, both on docs-only branches. Not a Mantine bug, and not a badly-written test. Both hooks do clear on unmount (useTransition's clearAllTimeouts, useLockScroll's effect cleanup). It is a rAF race: clearAllTimeouts cancels the *pending* rAF, but the transition schedules rAF -> rAF -> setTimeout, so if the inner callback is already in flight when the unmount lands, cancelAnimationFrame is a no-op and that callback then schedules a timer after cleanup has run. Nothing owns it. Needs a loaded machine, which is why it only ever appeared in CI. Track every timer and clear whatever is still outstanding after cleanup(). The ordering is load-bearing: legitimate unmount cleanups get their turn first, so only true leaks are dropped. Fake timers pass through untracked, which is correct — a fake timer cannot outlive the environment. Also corrects the claim, in AGENTS.md and in setup.ts's own comment, that env="test" prevents this. It does not: env is read only by Transition.mjs at its render branch, while useTransition runs before that check (hooks cannot be conditional) and still schedules real timers. Measured — opening a through renderWithMantine schedules three 200ms timers. Believing that guarantee is why this went unexamined. The regression test is mutation-verified: with the net disabled it fails, with it restored it passes. It deliberately does not assert that Mantine schedules no timers — it does, and that is normal; the contract is that none survive. Closes #1984 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3 Signed-off-by: cliffhall --- AGENTS.md | 2 +- clients/web/src/test/leakedTimers.test.tsx | 72 ++++++++++++++++++++ clients/web/src/test/setup.ts | 79 ++++++++++++++++++++-- 3 files changed, 148 insertions(+), 5 deletions(-) create mode 100644 clients/web/src/test/leakedTimers.test.tsx diff --git a/AGENTS.md b/AGENTS.md index 5f9e44bc..8584c231 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -635,7 +635,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - **Side-by-side (`.test.tsx` next to the source) — the default for web's own `src/` code.** Components, hooks, `lib/`, `utils/`. This is the overwhelming majority; a web-owned test living under `src/test/` instead of beside its source is a bug (fixed one such straggler, `downloadFile.test.ts`, in #1776). - **`src/test/` — the three things that _cannot_ be co-located:** (1) tests of the repo-root **`core/`** package (`src/test/core/…`, mirroring the `core/` folder layout — `core/` physically lives at `/core` outside `clients/web/`, is consumed via the `@inspector/core` alias, and has no test harness of its own, so co-locating would pollute the shared isomorphic package with web-only test infra); (2) the **`integration`** vitest project (`src/test/integration/…`, node env, 30s — placement _is_ the manifest, see above); (3) **shared test infrastructure** (`renderWithMantine.tsx`, `setup.ts`, `fixtures/`, `scrollAreaStoryAssertions.ts`) — not tests _of_ a source file, so nothing to sit beside. - **The above is web only.** The Node clients (**cli, tui, launcher**) keep **all** their tests in a top-level **`__tests__/`** dir, not beside their source — their `tsconfig.json` excludes `**/*.test.*` and their `tsconfig.test.json` includes `__tests__/**/*` (plus, for launcher, its root `vitest.config.ts`), so a co-located `src/**/*.test.*` lands in **no** tsconfig project and fails `npm run verify:typecheck-coverage` (#1791). Put a new cli/tui/launcher test under `__tests__/`. -- Use `renderWithMantine` from `src/test/renderWithMantine.tsx` to render components — it wraps in `MantineProvider` with the project theme. It sets `env="test"` so Mantine renders transitions synchronously (no internal `setTimeout`); this prevents a `Transition`/`Modal` timer from firing after happy-dom tears down `window` at end-of-run and failing the whole run with an uncaught `ReferenceError: window is not defined` (#1760). **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. +- 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 `` 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. ### Responding to Code Reviews diff --git a/clients/web/src/test/leakedTimers.test.tsx b/clients/web/src/test/leakedTimers.test.tsx new file mode 100644 index 00000000..ea51b4be --- /dev/null +++ b/clients/web/src/test/leakedTimers.test.tsx @@ -0,0 +1,72 @@ +/** + * Regression tests for the leaked-timer safety net in `setup.ts` (#1984). + * + * A `window.setTimeout` that outlives its test file fires after happy-dom has + * disposed that file's `window`, and React then throws an uncaught + * `ReferenceError: window is not defined` that fails the whole run — from an + * arbitrary innocent file, with every test passing. These lock down the net that + * prevents it. + * + * Note what is deliberately NOT asserted: that Mantine schedules no timers. It + * does (measured: three 200ms timers when a `Modal` opens, even under + * `env="test"`), and that is the library behaving normally. The contract here is + * only that nothing survives the test that scheduled it. + */ + +import { describe, it, expect, vi } from "vitest"; +import { Modal } from "@mantine/core"; +import { renderWithMantine } from "./renderWithMantine"; + +/** Ids the net is tracking, read back through the wrapper it installed. */ +function scheduleTracked(ms: number): number { + return window.setTimeout(() => {}, ms) as unknown as number; +} + +describe("leaked-timer safety net", () => { + it("wraps window.setTimeout rather than leaving the native one in place", () => { + // If the wrapper were absent the net would silently track nothing, so assert + // the instrumentation exists before relying on the behavior it enables. + expect(window.setTimeout.toString()).not.toContain("[native code]"); + }); + + it("clears a timer the test leaves pending, so it cannot fire later", async () => { + const fired = vi.fn(); + window.setTimeout(fired, 20); + // Deliberately do not clear it: the afterEach net owns it from here. The + // next test asserts it never ran. + leaked.callback = fired; + expect(fired).not.toHaveBeenCalled(); + }); + + it("the previous test's leaked timer never fired", async () => { + // 20ms of real time, comfortably past the leaked timer's deadline. + await new Promise((resolve) => setTimeout(resolve, 60)); + expect(leaked.callback).not.toBeNull(); + expect(leaked.callback).not.toHaveBeenCalled(); + }); + + it("clearTimeout stops tracking, so an explicitly-cleared timer is not double-cleared", () => { + const id = scheduleTracked(1000); + expect(() => window.clearTimeout(id)).not.toThrow(); + // Clearing twice must stay a no-op — the net also clears at teardown. + expect(() => window.clearTimeout(id)).not.toThrow(); + }); + + it("a Modal's own transition timers do not survive the test that opened it", () => { + // The real-world shape of #1984: ServerRemoveConfirmModal-style tests toggle + // `opened`, and Mantine schedules real timers for the transition and the + // scroll lock. Rendering here is enough — the net clears them at teardown, + // and a regression would surface as the run-killing uncaught ReferenceError + // rather than as a failure of this assertion. + const { rerender } = renderWithMantine( + {}} />, + ); + rerender( {}} />); + expect(document.body).toBeTruthy(); + }); +}); + +/** Carries the leaked callback across the two tests above. */ +const leaked: { callback: ReturnType | null } = { + callback: null, +}; diff --git a/clients/web/src/test/setup.ts b/clients/web/src/test/setup.ts index d818a2ce..c84a313f 100644 --- a/clients/web/src/test/setup.ts +++ b/clients/web/src/test/setup.ts @@ -65,10 +65,15 @@ Object.defineProperty(globalThis, "fetch", { // disable Mantine transitions: `useTransition` only honors reduced motion when // `theme.respectReducedMotion` is true, and Mantine 8 defaults it to `false` // (the project theme doesn't override it). The actual protection against the -// #1760 post-teardown `window is not defined` leak is `env="test"` in -// `renderWithMantine`, which forces transitions synchronous regardless. Tests -// that need specific media results still mock `@mantine/hooks` or stub -// `matchMedia` themselves. +// #1760 post-teardown `window is not defined` leak is the leaked-timer safety +// net below — NOT `env="test"`, which an earlier version of this comment +// claimed. `env` is read only by `Transition.mjs`, and only at its *render* +// branch (`transitionDuration === 0 || env === "test"`); `useTransition()` is +// called before that check, since hooks cannot be conditional, so it still +// schedules real `window.setTimeout`s on every `mounted` change. Measured: +// opening a `` through `renderWithMantine` schedules three 200ms timers +// (#1984). Tests that need specific media results still mock `@mantine/hooks` +// or stub `matchMedia` themselves. Object.defineProperty(window, "matchMedia", { configurable: true, writable: true, @@ -91,7 +96,73 @@ Object.defineProperty(window, "matchMedia", { }) as unknown as MediaQueryList, }); +// --------------------------------------------------------------------------- +// Leaked-timer safety net (#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 that to +// whichever file happened to be running, and one such error fails the ENTIRE +// run with every test passing — so it reads as a defect in an innocent file. +// +// Mantine's own hooks do clear their timers on unmount (`useTransition`'s +// `clearAllTimeouts`, `useLockScroll`'s effect cleanup), so this is not a +// library bug. It is a race: `clearAllTimeouts` cancels the *pending* rAF, but +// if that rAF callback is already in flight when the unmount lands, +// `cancelAnimationFrame` is a no-op and the callback goes on to schedule a +// `setTimeout` after cleanup has already run. Nothing owns that timer. It needs +// a loaded machine to hit, which is why it only ever appears in CI. +// +// Rather than chase each component, track every timer and clear whatever is +// still outstanding once the test's own teardown has had its turn. Ordering is +// load-bearing: this runs *after* `cleanup()` below, so legitimate unmount +// cleanups clear their own timers and only true leaks are left. +// +// Under `vi.useFakeTimers()` the wrapper is swapped out for vitest's fake +// implementation, so nothing is tracked while fake timers are installed — which +// is correct, since a fake timer cannot outlive the environment. +const pendingTimers = new Set(); +const realSetTimeout = window.setTimeout.bind(window); +const realClearTimeout = window.clearTimeout.bind(window); + +window.setTimeout = (( + handler: TimerHandler, + timeout?: number, + ...args: unknown[] +): number => { + const id: number = realSetTimeout( + (...cbArgs: unknown[]) => { + pendingTimers.delete(id); + if (typeof handler === "function") { + handler(...cbArgs); + } + }, + timeout, + ...args, + ); + pendingTimers.add(id); + return id; + // happy-dom types `setTimeout` with Node's overloads (returning `Timeout`), + // while the DOM lib types it as `number`. The implementation above honors the + // DOM shape the app codes against; this cast bridges the two declarations, + // which TS cannot relate structurally. +}) as unknown as typeof window.setTimeout; + +window.clearTimeout = ((id?: number): void => { + if (typeof id === "number") { + pendingTimers.delete(id); + } + realClearTimeout(id); + // Same DOM-vs-Node overload mismatch as `setTimeout` above. +}) as unknown as typeof window.clearTimeout; + afterEach(() => { cleanup(); window.localStorage.clear(); + // After cleanup(), anything still pending is a leak — drop it so it cannot + // fire past this file's environment teardown. + for (const id of pendingTimers) { + realClearTimeout(id); + } + pendingTimers.clear(); });