docs: the renderWithMantine rule no longer rests on timer safety

Review caught a second-order effect of the fix: making the leaked-timer net
global invalidated the rationale of a neighbouring rule.

"Do not hand-roll a bare MantineProvider — that reintroduces the leak class"
stopped being true the moment the net moved into setup.ts, which covers every
unit test regardless of how it renders. The rule itself is still right, so keep
it and replace the justification: a hand-rolled provider skips the project theme
and the helper's options and drifts from every other test. Both copies say
explicitly that the old reason no longer holds, so nobody re-derives it.

Mirrored into .github/copilot-instructions.md in the same PR, as AGENTS.md
requires for review-relevant guidance — it carried the identical stale claim.

That makes four places this one belief had propagated to. A fix can create stale
docs by making a previously-true statement false, which is easy to miss when
only the changed lines get read.

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>
This commit is contained in:
cliffhall 2026-08-12 01:11:05 -04:00
commit b451949c9e
2 changed files with 2 additions and 2 deletions

View file

@ -92,7 +92,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
### Rendering components in tests
- **Always render through `renderWithMantine`** from `src/test/renderWithMantine.tsx`. Never hand-roll a bare `MantineProvider` — that reintroduces a real failure class where a `Transition`/`Modal` timer fires after happy-dom tears down `window` and fails the entire run.
- **Always render through `renderWithMantine`** from `src/test/renderWithMantine.tsx`. A hand-rolled bare `MantineProvider` skips the project theme and the helper's options, and drifts from every other test. (It does *not* reintroduce the timer-leak class — an older version of this rule said so; the leaked-timer net in `setup.ts` is global and covers every unit test regardless of how it renders.)
- For a forced color scheme, pass the option — `renderWithMantine(ui, { colorScheme: "dark" })` — rather than a hand-rolled `defaultColorScheme` provider.
- Only when asserting _mid-flight_ transition state, use `renderWithMantineTransitions`, passing `settleMs` derived from the component's real animation duration. Do **not** combine it with `vi.useFakeTimers()`, and use the `unmount()` it returns if the test unmounts the tree itself.

View file

@ -635,7 +635,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab
- **Side-by-side (`<Name>.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"`, 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.
- 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** — 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. 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