fix(test): treat timer handles as opaque; the numeric guard never matched

Review was right, and I had assumed the handle type rather than checking it.
Probed both under happy-dom:

  setTimeout            -> typeof "object", constructor Timeout
  requestAnimationFrame -> typeof "object", constructor Immediate

So `typeof id === "number"` in the clearTimeout wrapper never matched, and an
explicitly-cleared timer was never untracked. Both `Set<number>` annotations and
`const id: number` were fictions TypeScript could not catch, since the DOM lib
DECLARES number while happy-dom RETURNS an object.

The effect was benign — the teardown sweep re-clears them and clearTimeout is
idempotent — but the sharper problem is that the test asserting this passed for
the wrong reason. "clearTimeout doesn't throw" holds whether or not tracking
works, so it proved nothing and hid the broken guard.

Hold handles as `unknown`, route cancellation through two small helpers that
carry the one justified cast each, and drop the numeric guards. The test now
asserts the net's own bookkeeping through an exported pendingTimerCount(), and
is mutation-checked: restoring the numeric guard fails it (1 failed | 6 passed).

That is the second test in this PR that passed without proving its claim. Both
had the same tell — the assertion would have held with the feature removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Signed-off-by: cliffhall <cliff@futurescale.com>

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

@@ -16,6 +16,7 @@
 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`. */
@@ -46,11 +47,17 @@ describe("leaked-timer safety net", () => {
     expect(leaked.callback).not.toHaveBeenCalled();
   });
 
-  it("clearTimeout stops tracking, so an explicitly-cleared timer is not double-cleared", () => {
+  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(() => window.clearTimeout(id)).not.toThrow();
-    // Clearing twice must stay a no-op — the net also clears at teardown.
-    expect(() => window.clearTimeout(id)).not.toThrow();
+    expect(pendingTimerCount()).toBe(before + 1);
+    window.clearTimeout(id);
+    expect(pendingTimerCount()).toBe(before);
   });
 
   it("queues a frame that would schedule a timer after the sweep", () => {

clients/web/src/test/setup.ts

@@ -124,19 +124,48 @@ Object.defineProperty(window, "matchMedia", {
 // 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<number>();
-const pendingFrames = new Set<number>();
+// 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[]
-): number => {
-  const id: number = realSetTimeout(
+): unknown => {
+  const id: unknown = realSetTimeout(
     (...cbArgs: unknown[]) => {
       pendingTimers.delete(id);
       if (typeof handler === "function") {
@@ -148,34 +177,37 @@ window.setTimeout = ((
   );
   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.
+  // 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?: number): void => {
-  if (typeof id === "number") {
+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);
   }
-  realClearTimeout(id);
-  // Same DOM-vs-Node overload mismatch as `setTimeout` above.
+  cancelTimer(id);
+  // Same declaration-vs-runtime mismatch as `setTimeout` above.
 }) as unknown as typeof window.clearTimeout;
 
-window.requestAnimationFrame = ((callback: FrameRequestCallback): number => {
-  const handle: number = realRequestAnimationFrame((time: number) => {
+window.requestAnimationFrame = ((callback: FrameRequestCallback): unknown => {
+  const handle: unknown = realRequestAnimationFrame((time: number) => {
     pendingFrames.delete(handle);
     callback(time);
   });
   pendingFrames.add(handle);
   return handle;
-  // Same DOM-vs-happy-dom declaration mismatch as `setTimeout` above.
+  // Same declaration-vs-runtime 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.
+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(() => {
@@ -194,13 +226,13 @@ afterEach(() => {
   // 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);
+    cancelFrame(handle);
   }
   pendingFrames.clear();
 
   // Then drop the timers. After cleanup(), anything still pending is a leak.
   for (const id of pendingTimers) {
-    realClearTimeout(id);
+    cancelTimer(id);
   }
   pendingTimers.clear();
 });