diff --git a/clients/web/src/test/leakedTimers.test.tsx b/clients/web/src/test/leakedTimers.test.tsx index ea51b4be..209efa25 100644 --- a/clients/web/src/test/leakedTimers.test.tsx +++ b/clients/web/src/test/leakedTimers.test.tsx @@ -17,9 +17,10 @@ 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; +/** Schedule through the wrapper the net installed, keeping its inferred handle + * type so it stays directly acceptable to `window.clearTimeout`. */ +function scheduleTracked(ms: number) { + return window.setTimeout(() => {}, ms); } describe("leaked-timer safety net", () => { @@ -52,6 +53,27 @@ describe("leaked-timer safety net", () => { expect(() => window.clearTimeout(id)).not.toThrow(); }); + it("queues a frame that would schedule a timer after the sweep", () => { + // The rAF race in miniature, and the ordering the net depends on. This + // synchronous test cannot let the frame callback run — it fires only after + // the test (and the whole afterEach) returns. If frames were not cancelled + // *before* the timer sweep, this callback would register `rafScheduled` + // after the drain and survive teardown. The next test proves it does not. + requestAnimationFrame(() => { + rafRace.ranFrame = true; + window.setTimeout(() => { + rafRace.ranTimer = true; + }, 5); + }); + expect(rafRace.ranFrame).toBe(false); + }); + + it("neither the queued frame nor its timer ever ran", async () => { + await new Promise((resolve) => setTimeout(resolve, 60)); + expect(rafRace.ranFrame).toBe(false); + expect(rafRace.ranTimer).toBe(false); + }); + 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 @@ -70,3 +92,6 @@ describe("leaked-timer safety net", () => { const leaked: { callback: ReturnType | null } = { callback: null, }; + +/** Records whether a queued frame — or the timer it would register — ever ran. */ +const rafRace = { ranFrame: false, ranTimer: false }; diff --git a/clients/web/src/test/renderWithMantine.tsx b/clients/web/src/test/renderWithMantine.tsx index 61c91077..ea291c6b 100644 --- a/clients/web/src/test/renderWithMantine.tsx +++ b/clients/web/src/test/renderWithMantine.tsx @@ -37,13 +37,20 @@ function makeWrapper(env: "test" | "default", colorScheme: MantineColorScheme) { }; } -// Default render helper. `env="test"` makes Mantine render transitions -// synchronously (no internal `setTimeout`). Without it, a `Transition`/`Modal` -// open/close timer can fire after happy-dom tears down `window` at the end of -// the run, throwing an uncaught `ReferenceError: window is not defined` that -// fails the whole run even when every assertion passed (#1760). This is the -// right default for the vast majority of tests, which don't assert on -// mid-transition state. +// Default render helper. `env="test"` makes Mantine skip the *animated render* +// of a transition, which is the right default for the vast majority of tests — +// they don't assert on mid-transition state. +// +// It does NOT stop the timers, contrary to what this comment used to claim +// (#1984). `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 this helper schedules three 200ms timers. A timer that outlives its +// file fires after happy-dom disposes that file's `window` and throws an +// uncaught `ReferenceError: window is not defined`, failing the whole run from +// an arbitrary innocent file (#1760). What actually prevents that is the +// leaked-timer safety net in `setup.ts`. export function renderWithMantine( ui: ReactElement, options?: MantineRenderOptions, diff --git a/clients/web/src/test/setup.ts b/clients/web/src/test/setup.ts index c84a313f..a564fa5d 100644 --- a/clients/web/src/test/setup.ts +++ b/clients/web/src/test/setup.ts @@ -113,17 +113,23 @@ Object.defineProperty(window, "matchMedia", { // `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. +// Rather than chase each component, track every timer *and every animation +// frame*, and clear whatever is still outstanding once the test's own teardown +// has had its turn. Ordering is load-bearing twice over: this runs *after* +// `cleanup()` below, so legitimate unmount cleanups clear their own timers and +// only true leaks are left; and frames are cancelled *before* timers are swept, +// because a queued frame callback would otherwise run after the sweep and +// register a fresh timer. See the ordering note on the `afterEach` itself. // // 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 pendingFrames = new Set(); const realSetTimeout = window.setTimeout.bind(window); const realClearTimeout = window.clearTimeout.bind(window); +const realRequestAnimationFrame = window.requestAnimationFrame.bind(window); +const realCancelAnimationFrame = window.cancelAnimationFrame.bind(window); window.setTimeout = (( handler: TimerHandler, @@ -156,11 +162,43 @@ window.clearTimeout = ((id?: number): void => { // Same DOM-vs-Node overload mismatch as `setTimeout` above. }) as unknown as typeof window.clearTimeout; +window.requestAnimationFrame = ((callback: FrameRequestCallback): number => { + const handle: number = realRequestAnimationFrame((time: number) => { + pendingFrames.delete(handle); + callback(time); + }); + pendingFrames.add(handle); + return handle; + // Same DOM-vs-happy-dom declaration mismatch as `setTimeout` above. +}) as unknown as typeof window.requestAnimationFrame; + +window.cancelAnimationFrame = ((handle: number): void => { + pendingFrames.delete(handle); + realCancelAnimationFrame(handle); + // Same DOM-vs-happy-dom declaration mismatch as `setTimeout` above. +}) as unknown as typeof window.cancelAnimationFrame; + 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. + + // Order is the whole point, and it is not interchangeable. + // + // Cancel queued animation frames FIRST. This `afterEach` is synchronous, so a + // frame callback already queued cannot run until it returns — at which point + // it would register a fresh `setTimeout` *after* the sweep below had already + // drained, and on the file's last test that timer would survive teardown. + // That is precisely the rAF race this net exists for, so sweeping timers + // without cancelling frames first would leave the original hole open. + // + // Cancelling first closes it deterministically: JS is single-threaded and + // nothing here yields, so no frame callback can run between these two loops. + for (const handle of pendingFrames) { + realCancelAnimationFrame(handle); + } + pendingFrames.clear(); + + // Then drop the timers. After cleanup(), anything still pending is a leak. for (const id of pendingTimers) { realClearTimeout(id); }