Merge pull request #1986 from modelcontextprotocol/v2/fix/1984-mantine-transition-timer-leak

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

clients/web/src/test/leakedTimers.test.tsx

@@ -0,0 +1,104 @@
+/**
+ * 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";
+import { pendingTimerCount } from "./setup";
+
+/** 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", () => {
+  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 untracks the timer", () => {
+    // Asserted against the net's own bookkeeping, not against "clearTimeout
+    // doesn't throw" — the latter holds whether or not tracking works, since
+    // clearTimeout is idempotent, so it proved nothing. That vacuity hid a real
+    // bug: happy-dom's handle is a `Timeout` object, so the original
+    // `typeof id === "number"` guard never matched and nothing was untracked.
+    const before = pendingTimerCount();
+    const id = scheduleTracked(1000);
+    expect(pendingTimerCount()).toBe(before + 1);
+    window.clearTimeout(id);
+    expect(pendingTimerCount()).toBe(before);
+  });
+
+  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
+    // 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(
+      <Modal opened={false} onClose={() => {}} />,
+    );
+    rerender(<Modal opened={true} onClose={() => {}} />);
+    expect(document.body).toBeTruthy();
+  });
+});
+
+/** Carries the leaked callback across the two tests above. */
+const leaked: { callback: ReturnType<typeof vi.fn> | null } = {
+  callback: null,
+};
+
+/** Records whether a queued frame — or the timer it would register — ever ran. */
+const rafRace = { ranFrame: false, ranTimer: false };

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 `<Modal>`
+// 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,

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 `<Modal>` 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,143 @@ 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 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.
+// Handles are held as `unknown`, deliberately. The DOM lib declares these as
+// `number`, but happy-dom returns objects at runtime — verified here:
+// `setTimeout` yields a `Timeout` and `requestAnimationFrame` an `Immediate`.
+// An earlier revision typed the sets `Set<number>` and guarded removal with
+// `typeof id === "number"`; that guard never matched, so explicitly-cleared
+// timers were never untracked. Harmless in effect (the teardown sweep clears
+// them again and `clearTimeout` is idempotent) but the bookkeeping was a
+// fiction, and the test asserting it passed vacuously. Treat the handle as
+// opaque and pass it straight back to the matching canceller.
+const pendingTimers = new Set<unknown>();
+const pendingFrames = new Set<unknown>();
+const realSetTimeout = window.setTimeout.bind(window);
+const realClearTimeout = window.clearTimeout.bind(window);
+const realRequestAnimationFrame = window.requestAnimationFrame.bind(window);
+const realCancelAnimationFrame = window.cancelAnimationFrame.bind(window);
+
+/** Cancel an opaque handle through the real canceller it came from. */
+function cancelTimer(id: unknown): void {
+  // The handle came from `realSetTimeout`, so it is exactly what
+  // `realClearTimeout` expects; only the *declared* type disagrees.
+  realClearTimeout(id as Parameters<typeof realClearTimeout>[0]);
+}
+
+function cancelFrame(handle: unknown): void {
+  // As above, for the rAF pair.
+  realCancelAnimationFrame(
+    handle as Parameters<typeof realCancelAnimationFrame>[0],
+  );
+}
+
+/** Test-only introspection, so the regression tests can assert on the
+ *  bookkeeping itself rather than on behavior that would hold anyway. */
+export function pendingTimerCount(): number {
+  return pendingTimers.size;
+}
+
+window.setTimeout = ((
+  handler: TimerHandler,
+  timeout?: number,
+  ...args: unknown[]
+): unknown => {
+  const id: unknown = realSetTimeout(
+    (...cbArgs: unknown[]) => {
+      pendingTimers.delete(id);
+      if (typeof handler === "function") {
+        handler(...cbArgs);
+      }
+    },
+    timeout,
+    ...args,
+  );
+  pendingTimers.add(id);
+  return id;
+  // The wrapper's public signature must match the DOM declaration the app codes
+  // against, while its body traffics in the runtime handle described above. TS
+  // cannot relate the two, hence the cast.
+}) as unknown as typeof window.setTimeout;
+
+window.clearTimeout = ((id?: unknown): void => {
+  // No `typeof` guard: the handle is an object here, so a numeric test would
+  // reject every real one. Anything defined is worth untracking.
+  if (id !== undefined && id !== null) {
+    pendingTimers.delete(id);
+  }
+  cancelTimer(id);
+  // Same declaration-vs-runtime mismatch as `setTimeout` above.
+}) as unknown as typeof window.clearTimeout;
+
+window.requestAnimationFrame = ((callback: FrameRequestCallback): unknown => {
+  const handle: unknown = realRequestAnimationFrame((time: number) => {
+    pendingFrames.delete(handle);
+    callback(time);
+  });
+  pendingFrames.add(handle);
+  return handle;
+  // Same declaration-vs-runtime mismatch as `setTimeout` above.
+}) as unknown as typeof window.requestAnimationFrame;
+
+window.cancelAnimationFrame = ((handle?: unknown): void => {
+  if (handle !== undefined && handle !== null) {
+    pendingFrames.delete(handle);
+  }
+  cancelFrame(handle);
+  // Same declaration-vs-runtime mismatch as `setTimeout` above.
+}) as unknown as typeof window.cancelAnimationFrame;
+
 afterEach(() => {
   cleanup();
   window.localStorage.clear();
+
+  // 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) {
+    cancelFrame(handle);
+  }
+  pendingFrames.clear();
+
+  // Then drop the timers. After cleanup(), anything still pending is a leak.
+  for (const id of pendingTimers) {
+    cancelTimer(id);
+  }
+  pendingTimers.clear();
 });