test(web): settle real-timer Transitions so the coverage gate can't leak 'window is not defined' (#1786) (#1793)

* test(web): settle real-timer Transitions so the coverage gate can't leak 'window is not defined' (#1786)

The web coverage gate was intermittently failing with an unhandled
`ReferenceError: window is not defined` thrown from a Mantine `Transition`
timer that fired after happy-dom tore down `window` (the #1760 class). The
three `ViewHeader.test.tsx` tests that use `renderWithMantineTransitions`
(real, env="default" timers) each drove their *primary* transition to
completion, but the concurrent *enter* on a sibling cell leaves no unmount to
`waitFor`, so its rAF→setTimeout chain could still be pending at teardown and
fire post-teardown, failing an unrelated later file's run.

Fix: after each such test, flush pending transition timers against the
still-mounted tree via `settleTransitions()` (a wait longer than the 300ms
header duration + rAF slack), so the settling setState targets a live tree and
nothing is left to fire after cleanup unmounts.

Also close the related nit: `renderWithMantine`/`renderWithMantineTransitions`
now accept a `colorScheme` option, so `ViewHeader.test.tsx` no longer
hand-rolls a bare `defaultColorScheme="dark"` MantineProvider (the #1760
anti-pattern). AGENTS.md documents both the option and the settle requirement.

Closes #1786

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): address review — export settleTransitions, derive settle from HEADER_ANIM_MS, guard fake timers, correct comments

Review feedback on PR #1793:

1. Derive the settle window from the real animation duration: export
   `HEADER_ANIM_MS` from ViewHeader.tsx and compute
   `TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200` in the test, so bumping the
   header duration can't silently make the settle insufficient (the invariant
   is now structural, not a comment).

2. Move `settleTransitions` into the shared `renderWithMantine.tsx` helper
   (exported, optional `ms` arg) next to `renderWithMantineTransitions`, since
   AGENTS.md points every future caller at it — shared test scaffolding belongs
   in src/test/, not a single test file. AGENTS.md now references the helper.

3. Guard `settleTransitions` against `vi.useFakeTimers()`: it awaits a real
   setTimeout, so under fake timers it would hang to the test timeout. Throw a
   clear message instead.

4. Correct the diagnosis comments: the hazard isn't a Mantine timer surviving
   unmount (its effect cleanup cancels timers) — it's the React state update
   Mantine's inner rAF schedules outside `act`, resolving post-teardown. Also
   fix the stale setup.ts matchMedia comment: the reduce-motion stub does not
   disable transitions (Mantine defaults respectReducedMotion=false); env="test"
   is the actual #1760 protection.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): tighten the review-trail comments (round 2)

Comment-accuracy follow-ups from PR #1793 round 2 (test infra only, no
behavior change):

- DEFAULT_SETTLE_MS: drop the false "Mantine's default header transitions run
  ~300ms" attribution (300ms is ViewHeader's HEADER_ANIM_MS; Mantine's Transition
  default is 250ms). Justify 500ms generically — a typical few-hundred-ms
  transition plus rAF slack.
- settleTransitions doc: stop asserting a single escape mechanism as fact. State
  what's observed (a post-teardown dispatchSetState throwing from
  resolveUpdatePriority, which runs synchronously inside dispatchSetState), what's
  known (useTransition has an unmount cleanup), and that the exact escape route
  isn't pinned down — the act-wrapped drain fixes it regardless.
- setup.ts: happy-dom v20 does implement matchMedia, so reframe the block as a
  deliberate override (pinning prefers-reduced-motion: reduce) rather than a
  polyfill for a missing API.
- Export MantineRenderOptions so callers can name the options type.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): resolve the last comment-contradiction + tighten the matchMedia override (round 3)

Round-3 review of PR #1793 (test infra only):

- setup.ts finding 1: the inner `value:` comment still said "happy-dom doesn't
  implement it", contradicting the block comment two lines up (which now
  correctly states happy-dom v20 DOES implement matchMedia and this is an
  override). Dropped the false clause; kept the part that actually justifies the
  `as unknown as MediaQueryList` double cast (the stub omits most of the
  interface).
- setup.ts finding 5 (folded in rather than deferred): anchored the `matches`
  regex to `/prefers-reduced-motion:\s*reduce/` so the override no longer also
  answers `true` to `(prefers-reduced-motion: no-preference)`.
- renderWithMantine.tsx nit 4: the `env="test"` rationale sat on `makeWrapper`,
  which also builds the `env="default"` wrappers (opposite purpose). Moved it
  onto `renderWithMantine`, mirroring the `env="default"` rationale on
  `renderWithMantineTransitions`; `makeWrapper` keeps a one-liner.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): make renderWithMantineTransitions own the settle (round 4)

Round-4 review of PR #1793 — implements the note-2 hardening in-PR rather than
deferring it, plus note 1 and the nit:

- Note 2 (settle was a convention AGENTS.md had to police): the real-transitions
  helper now OWNS the settle. `renderWithMantineTransitions` arms an automatic
  post-test drain via a helper-registered `afterEach`, so a real-transitions
  test can no longer leak the #1760 class by forgetting to settle a concurrent
  enter. Verified empirically that the helper's `afterEach` runs BEFORE the
  global cleanup() in setup.ts (afterEach is LIFO; setup.ts's cleanup is a
  setupFile registered first) — the ordering the drain depends on. The three
  explicit `settleTransitions()` calls in ViewHeader.test.tsx are gone; each
  render passes `settleMs: HEADER_ANIM_MS + 200` instead, keeping the window
  duration-derived. The auto-settle skips under fake timers (and no
  transitions test uses them). `settleTransitions` stays exported for the rare
  manual mid-body case.

- Note 1 (settle formula was duration-only): the helper doc + the new
  `settleMs` option doc now say the window must exceed the longest JS chain —
  duration PLUS any enterDelay/exitDelay — plus rAF slack.

- Nit: ViewHeader.test.tsx now states why HEADER_ANIM_MS is exported (the
  settle window must track the animation it settles).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): pin + self-check the auto-settle hook ordering; fix fake-timer doc; keep longest settle (round 5)

Round-5 review of PR #1793:

- Note 2 (load-bearing hook ordering was unpinned/undefended): the
  auto-settle's afterEach must run before setup.ts's cleanup(). Added BOTH
  guards the review suggested. (1) Runtime self-check: renderWithMantineTransitions
  captures the render container; the afterEach throws a clear error if it's
  already detached (cleanup ran first). (2) Pinned `sequence.hooks: "stack"` on
  the unit project. While verifying, found the ordering actually holds in EVERY
  sequence.hooks mode (stack/list/parallel) because setupFile hooks are outer
  relative to the helper's import-registered hook — so the comments now state
  that true mechanism (the pin is defense-in-depth; the container.isConnected
  check is the real enforcement), rather than claiming it "depends on stack".
  Full 3533-test unit suite passes under "stack".

- Note 1 (fake-timer claim was backwards): AGENTS.md no longer says the leak
  "would return" under fake timers (fake clocks park Mantine's setTimeout/rAF
  and are discarded, not drained). The real hazard — the auto-settle silently
  no-ops, so real-clock work is unprotected — is now stated. The afterEach also
  console.warns on the fake-timer skip (was silent; the manual settleTransitions
  throws — this closes that asymmetry).

- Note 3 (arming was last-write-wins): armedSettleMs now keeps the longest
  window via Math.max, so a later short-animation render can't under-settle an
  earlier long one.

- Nit: documented `settleMs: 0` as a working opt-out for a test that drove every
  transition to completion itself (act flush, no wait).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): post-await self-check + correct the hook-ordering mechanism (round 6)

Round-6 review of PR #1793:

- Finding 1 (self-check blind in parallel): added the post-await
  container.isConnected re-check the review suggested (factored both checks into
  assertConnectedForSettle(container, "before"|"after")). While verifying it, I
  established the actual mechanism, which differs from what both prior comments
  and the review assumed: setup.ts's cleanup() is a *setupFile (outer)* hook, and
  Vitest runs inner afterEach hooks to completion before outer ones in EVERY
  sequence.hooks mode — so cleanup never races this settle. Verified: ViewHeader's
  real-transitions tests pass under stack/list/parallel with neither check
  tripping, and a same-suite `afterEach(cleanup)` probe (where the ordering CAN
  break) does trip them (pre-check). So sequence.hooks isn't load-bearing here;
  the comments in renderWithMantine.tsx, vite.config.ts, and AGENTS.md now say
  that. The "stack" pin stays as documentation of intent / defense-in-depth, and
  the two checks are the real guard against a future same-level-cleanup
  regression.

- Nit: documented why last-write-wins on armedContainer is fine (cleanup detaches
  every container at once, so the last is a faithful "cleanup ran" proxy).
- Nit: the fake-timer skip warning now interpolates the current test name so it's
  actionable at teardown.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): disarm the auto-settle on self-unmount; concurrency + message nits (round 7)

Round-7 review of PR #1793:

- Finding 1 (guard misdiagnoses a self-unmount): the detached-tree check fired
  on ANY detachment and blamed a broken cleanup ordering, but a test calling the
  unmount() returned by renderWithMantineTransitions also detaches the container
  (a common pattern here). Fixed at the source: the helper now wraps unmount() to
  disarm the auto-settle (clear armedSettleMs/armedContainer) — once the test has
  unmounted there's nothing to settle against a live tree. The guard message also
  now names the self-unmount cause. Verified: a probe that unmounts a
  real-transitions render no longer trips the guard; ViewHeader unaffected.

- Nit: documented that the module-level arming assumes sequential tests (not safe
  under test.concurrent/describe.concurrent).
- Nit: the fake-timer skip warning falls back to "(unknown test)" when
  currentTestName is undefined.

- Finding 3 (doc weight): trimmed the AGENTS.md bullet to the *rule* (use the
  transitions variant only for mid-flight assertions; pass a duration-derived
  settleMs; no fake timers; use the returned unmount()) and pointed at the
  helper's own comments for the full mechanism, which now carries it at more
  depth.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): downgrade (not disarm) the auto-settle on self-unmount; per-container liveness (round 8)

Round-8 review of PR #1793:

- Finding 1 (round-7 disarm was too blunt + self-contradictory): disarming on
  self-unmount removed the drain entirely for the mid-flight-unmount test that
  most needs #1760 protection, and its "useTransition cleanup applies" rationale
  contradicted this file's own settleTransitions doc (which says that cleanup
  can't be relied on to stop the rAF). Now the wrapped unmount() DOWNGRADES: the
  drain still runs (mechanism-independent — flushes queued rAF/setTimeout while
  window is alive; a setState on the unmounted tree is a React no-op), only the
  liveness assertion for that container is dropped.

- Finding 2 (disarm undid round-5 max-wins arming): arming is now per-container
  (a Set of live containers) instead of all-or-nothing. Unmounting one tree drops
  only its own container; a second still-mounted tree keeps its longer window and
  its liveness check. The isConnected assertions run over the still-live set.

Verified with probes: returned unmount() drains without tripping the guard; a
second still-mounted tree is unaffected when the first self-unmounts; a bare
mid-body cleanup() STILL trips the guard (it's live, not decoration); ViewHeader
unchanged. Comments/AGENTS.md updated so the self-unmount path and the
settleTransitions doc no longer contradict each other.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): make the auto-settle drain best-effort so a guard trip doesn't also leak (round 9)

Round-9 review of PR #1793 (merge-verdict; optional robustness note):

- Finding 1 (pre-settle throw skipped the drain): on the ordering regression the
  guard exists to catch, throwing before the drain meant the maintainer got BOTH
  the explanatory error AND an uncaught post-teardown `window is not defined` in
  an unrelated file. The afterEach now captures a liveness failure, always runs
  the act-wrapped drain (flushing queued rAF/setTimeout while window is alive),
  and throws afterward — so the explanatory error stands alone. Verified: a bare
  mid-body cleanup() still throws the guard error (now after draining); ViewHeader
  unchanged.

- Nits: AGENTS.md now names the reachable bare-detach path (a mid-body cleanup()
  on a still-armed tree) rather than "a bare RTL unmount"; the settleMs doc notes
  arming is per-test, so in a multi-render test the longest window wins (a `0` is
  superseded by a longer sibling render).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): fix the regressed fake-timer doc + guarantee the liveness error survives a drain throw (round 10)

Round-10 review of PR #1793 (merge-verdict):

- Finding 1 (AGENTS.md fake-timer clause regressed in the round-7 doc trim): it
  had reverted to "silently dropping the protection", which round 5 had corrected
  — "silently" is now false (the auto-settle console.warns on the fake-timer
  skip) and "dropping the protection" reinstates a walked-back framing (fake
  clocks park Mantine's timers on a discarded clock, so the no-op doesn't
  obviously reopen #1760). Reworded to match the helper: it warns, and the hazard
  is that real-clock work is then unprotected, so the test depends on which clock
  was installed at teardown.

- Finding 3 (nit): matched the console.warn string to that same corrected
  framing, so AGENTS.md and the warning agree.

- Finding 2 (nit): a drain-time throw could discard the captured liveness error
  (the substitution round 9 set out to prevent, one step further). Now the drain
  error is captured too and the liveness error is preferred, so the explanatory
  error always wins. Verified a bare mid-body cleanup() still throws the liveness
  error after the best-effort drain; lint clean (no throw-in-finally); ViewHeader
  unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): preserve the drain error as cause + route the guard message to the reachable cause (round 11)

Round-11 review of PR #1793 (merge-verdict; two optional polish items):

- Finding 1: when both a liveness error and a drain-time throw occur, the drain
  error was dropped entirely. It's now attached as the liveness error's `cause`
  (when the liveness error is an Error), so an act-surfaced component error from
  the settling render is preserved without changing which error fails the test.

- Finding 2: the guard's error message pointed the reader first at the
  `sequence.hooks: "stack"` pin, which the file itself documents as
  defense-in-depth and NOT the ordering guarantee. Reworded to name the actually-
  reachable cause first — cleanup moved to a same-level afterEach instead of the
  setup.ts setupFile (outer) hook — and demote the pin to a parenthetical, so a
  maintainer who trips the check isn't sent to investigate a non-causal setting.

Verified: a bare mid-body cleanup() throws the reworded message; eslint clean;
ViewHeader unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): guard message states observation before candidate causes; log the preserved drain error (round 12)

Round-12 review of PR #1793 (merge-verdict):

- Finding 1: the round-11 rewording had dropped the "Expected cause:" hedge and
  asserted setup.ts's cleanup() as the fact, but the check only observes that a
  still-live armed tree was detached — it can't identify what detached it.
  Reworded to state that observation first, then list candidate causes: (a) a
  bare mid-body cleanup() in the test (the reachable case today), or (b) cleanup
  moved off the setup.ts setupFile hook to a same-level afterEach. No single
  cause asserted as fact.

- Nits 2a/2b: when both a liveness error and a drain error fire, the drain error
  is now also console.error'd (in addition to being attached as `cause`), so it's
  readable regardless of whether the reporter renders `error.cause` and isn't
  silently dropped even if the liveness error somehow isn't an Error.

Verified: a bare mid-body cleanup() throws the reworded message; eslint clean;
ViewHeader unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): final comment/string polish on the auto-settle guard (round 13)

Round-13 review of PR #1793 (merge-verdict; string/comment polish only):

- Finding 1: the console.error text claimed the drain error was "reported as the
  liveness error's cause" unconditionally, but the cause is only attached when
  the liveness error is an Error — misleading on the very non-Error path the
  hoisted log exists to serve. Dropped that parenthetical so the log is true on
  both paths.
- Nit: trimmed the now-duplicated outer comment to just the priority rationale
  (the inner comment, which mentions the log + instanceof conditional, is the
  accurate one).
- Nit: guard message says "drained against a dead tree" (exact for both the
  before- and during-settle detach) instead of "drained nothing".
- Nit: the message now names renderWithMantine.tsx rather than "the ordering note
  here", so it's self-contained out of context.

eslint clean; ViewHeader unchanged; npm run ci green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): correct the sequence.hooks default claim (it's "stack" in vitest 4, not "parallel") (round 14)

Round-14 review of PR #1793 (merge-verdict):

- Finding 1: verified against installed vitest 4.1.0 — `resolved.sequence.hooks
  ??= "stack"` (coverage chunk), so the runtime default is "stack"; the CLI
  help-text's "parallel" is stale in vitest itself. The vite.config.ts comment
  claimed the pin overrides a "parallel" default; corrected to say the pin is
  Vitest 4's own default made explicit (documents intent + guards a future
  default change), which is a truer reason to keep it than an override.

- Nit: tied the two additions together — the post-settle check's same-level race
  can only arise if someone changes sequence.hooks to "parallel" (same-level
  hooks run sequentially under "stack"/"list"), so the pin and the post-check now
  reference each other rather than sitting in opposed positions.

ViewHeader unchanged; npm run ci green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

* test(web): document the isolate:true dependency (empirically confirmed) + fix pre-check-blindness wording (round 15)

Round-15 review of PR #1793 (merge-verdict; comment-only):

- Finding 1: the auto-settle's module-level afterEach depends on isolate:true
  (the default) to re-register per test file; under isolate:false the module
  evaluates once per worker and the hook binds to only the first importing file,
  so other files arm with no consuming hook — the drain silently never runs and
  the state leaks across files. Reproduced it (vitest --no-isolate --pool=threads
  --no-file-parallelism over two real-transitions files leaves one file's arming
  unconsumed) and documented the assumption + the onTestFinished escape hatch on
  the arming-state comment.

- Nit: the "sequence.hooks is why the pre-check can't see the concurrent detach"
  clause was wrong — the pre-check is blind because it runs synchronously before
  the await (a property of where it sits, not the hook mode). Reworded to state
  that, and that the concurrent case *additionally* needs sequence.hooks:
  "parallel".

ViewHeader unchanged; npm run ci green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall 2026-07-26 15:49:33 -04:00 • committed by GitHub
commit a7b2fdb2ef
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 334 additions and 71 deletions

View file

@ -205,7 +205,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i
- **Test placement: side-by-side by default, `src/test/` only for what can't be co-located.** These look like competing conventions but aren't — the split is: *tests live beside their source, **except** tests for the repo-root `core/` package (which lives outside `clients/web/`) and shared test scaffolding — both of which live under `src/test/`, with `core/` tests mirroring the `core/` layout and integration tests under `src/test/integration/`.*
- **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.
- 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). 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) — and that test MUST drive the transition to completion (`await waitFor`/`findBy`) so its timer resolves before teardown.
- 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.
### Responding to Code Reviews
- When asked to respond to a code review of a PR,

View file

@ -7,24 +7,18 @@ import {
screen,
waitFor,
} from "../../../test/renderWithMantine";
import { render } from "@testing-library/react";
import { MantineProvider } from "@mantine/core";
import { theme } from "../../../theme/theme";
import { ViewHeader } from "./ViewHeader";
import { HEADER_ANIM_MS, ViewHeader } from "./ViewHeader";
// Render under a forced-dark MantineProvider so `useComputedColorScheme`
// returns "dark" (the light/dark icon + logo branches). `env="test"` disables
// timer-driven transitions, matching the shared `renderWithMantine` wrapper and
// avoiding post-teardown `window is not defined` races (#1760).
function renderDark(ui: React.ReactElement) {
return render(ui, {
wrapper: ({ children }) => (
<MantineProvider theme={theme} defaultColorScheme="dark" env="test">
{children}
</MantineProvider>
),
});
}
// The `renderWithMantineTransitions` tests use real (env="default") Mantine
// transitions to observe mid-flight "in"/"out" cells. Waiting for one cell to
// unmount doesn't settle the *sibling* enter cells (a completed enter leaves no
// DOM signal to `waitFor`), which would leak the #1760 timer class. Passing
// `settleMs` arms the helper's automatic post-test settle to drain the in-flight
// animation before teardown (#1786). Derive the window from the component's real
// duration plus rAF scheduling slack, so bumping HEADER_ANIM_MS can't silently
// make the settle insufficient. (This is why ViewHeader.tsx exports
// HEADER_ANIM_MS — the settle window must track the animation it settles.)
const TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200;
// Mock @mantine/hooks so we can control useMediaQuery results per test.
const mediaQueryMock = vi.hoisted(() => ({ value: false }));
@ -243,10 +237,12 @@ describe("ViewHeader", () => {
it("crossfades on disconnect: tab bar exits while the title enters, then the bar unmounts (#1450)", async () => {
mediaQueryMock.value = true;
// Asserts the mid-flight exit ("out") state, so it needs real transitions;
// it drives them to completion below (waitFor unmount).
// Asserts the mid-flight exit ("out") state, so it needs real transitions.
// The title's concurrent *enter* leaves no unmount to await, so the armed
// settle (settleMs) drains it before teardown (#1786).
const { rerender } = renderWithMantineTransitions(
<ViewHeader {...connectedProps} />,
{ settleMs: TRANSITION_SETTLE_MS },
);
expect(screen.getAllByRole("radio").length).toBeGreaterThan(0);
@ -314,10 +310,12 @@ describe("ViewHeader", () => {
it("animates the server name and disconnect controls in on connect, out on disconnect (#1450)", async () => {
mediaQueryMock.value = true;
// Asserts the mid-flight exit ("out") state, so it needs real transitions;
// it drives them to completion below (waitFor unmount).
// Asserts the mid-flight exit ("out") state, so it needs real transitions.
// The concurrent enter cells leave no unmount to await, so the armed settle
// (settleMs) drains them before teardown (#1786).
const { rerender } = renderWithMantineTransitions(
<ViewHeader {...connectedProps} />,
{ settleMs: TRANSITION_SETTLE_MS },
);
// Connected: the server name and the Disconnect control are in their
// enter cells.
@ -372,7 +370,9 @@ describe("ViewHeader", () => {
it("renders the dark-scheme icon/logo branch under a dark color scheme", () => {
// Rendering under a forced-dark provider exercises the
// `colorScheme === "dark"` branches for the theme icon and the logo src.
renderDark(<ViewHeader {...connectedProps} />);
renderWithMantine(<ViewHeader {...connectedProps} />, {
colorScheme: "dark",
});
expect(
screen.getByRole("button", { name: "Toggle color scheme" }),
).toBeInTheDocument();
@ -381,8 +381,10 @@ describe("ViewHeader", () => {
it("crossfades the title out and the connected header in on connect (#1450)", async () => {
mediaQueryMock.value = true;
// Asserts the mid-flight exit ("out") state, so it needs real transitions;
// it drives them to completion below (findByText for the entering header).
// Asserts the mid-flight exit ("out") state, so it needs real transitions.
// The title's exit and the server name's enter stay in flight past the
// assertions (the enter leaves no unmount to await), so the armed settle
// (settleMs) drains them before teardown (#1786).
// Start disconnected: the title cell is the entering one.
const { rerender } = renderWithMantineTransitions(
<ViewHeader
@ -390,6 +392,7 @@ describe("ViewHeader", () => {
onToggleTheme={vi.fn()}
onOpenClientSettings={vi.fn()}
/>,
{ settleMs: TRANSITION_SETTLE_MS },
);
expect(
screen

View file

@ -59,7 +59,7 @@ export type ViewHeaderProps = ConnectedProps | UnconnectedProps;
// Disconnect controls (right) all fade + slide-down, staggered by half this
// duration. The motion itself is CSS (`.header-anim`); keep the 300ms /
// 150ms-stagger there in sync with this value.
const HEADER_ANIM_MS = 300;
export const HEADER_ANIM_MS = 300;
// Grace window after a connection is established before the new-tab glow arms
// (#1450). Primitive lists (prompts/resources/tasks) are fetched asynchronously
// just after the handshake, so their tabs appear a few renders into the

View file

@ -1,47 +1,287 @@
import type { ReactElement, ReactNode } from "react";
import { render, type RenderOptions } from "@testing-library/react";
import { MantineProvider } from "@mantine/core";
import { act, render, type RenderOptions } from "@testing-library/react";
import { MantineProvider, type MantineColorScheme } from "@mantine/core";
import { afterEach, expect, vi } from "vitest";
import { theme } from "../theme/theme";
// `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.
function TestEnvWrapper({ children }: { children: ReactNode }) {
return (
<MantineProvider theme={theme} defaultColorScheme="light" env="test">
{children}
</MantineProvider>
);
}
// Opt-in wrapper that keeps Mantine's timer-driven transitions enabled, for the
// few tests that assert on transition/animation state that only exists mid-flight
// (e.g. a `data-anim="out"` cell during an exit crossfade). Such a test MUST
// drive the transition to completion (`await waitFor`/`findBy`) so its timer
// resolves before teardown; otherwise it reintroduces the #1760 leak.
function TransitionsWrapper({ children }: { children: ReactNode }) {
return (
<MantineProvider theme={theme} defaultColorScheme="light" env="default">
{children}
</MantineProvider>
);
// Options accepted by both render helpers: the standard RTL options (minus
// `wrapper`, which we own) plus an optional forced `colorScheme`. The default
// is "light"; pass "dark" to exercise `useComputedColorScheme` dark branches
// without hand-rolling a bare `MantineProvider` (the #1760 anti-pattern).
export type MantineRenderOptions = Omit<RenderOptions, "wrapper"> & {
colorScheme?: MantineColorScheme;
};
// Options for the real-transitions variant. `settleMs` is the window the
// automatic post-test settle waits (see below); pass the component's longest JS
// timer chain — its `Transition` `duration`/`exitDuration` plus any
// `enterDelay`/`exitDelay` plus two-frame rAF slack. Omit to use the generic
// default. Pass `0` as a deliberate opt-out for a test that provably drove every
// transition to completion itself: the `act` flush still runs, but with no wait.
// Arming is per *test*, not per render: in a test that renders more than one
// tree the longest `settleMs` wins (so a `0` here is superseded by a longer
// sibling render).
export type MantineTransitionsRenderOptions = MantineRenderOptions & {
settleMs?: number;
};
// Build a MantineProvider wrapper for the given Mantine `env` + forced color
// scheme. See the two render helpers below for what each `env` value is for.
function makeWrapper(env: "test" | "default", colorScheme: MantineColorScheme) {
return function Wrapper({ children }: { children: ReactNode }) {
return (
<MantineProvider theme={theme} defaultColorScheme={colorScheme} env={env}>
{children}
</MantineProvider>
);
};
}
// 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.
export function renderWithMantine(
ui: ReactElement,
options?: Omit<RenderOptions, "wrapper">,
options?: MantineRenderOptions,
) {
return render(ui, { wrapper: TestEnvWrapper, ...options });
const { colorScheme = "light", ...rest } = options ?? {};
return render(ui, { wrapper: makeWrapper("test", colorScheme), ...rest });
}
// Opt-in variant that keeps Mantine's timer-driven transitions enabled, for the
// few tests that assert on transition/animation state that only exists mid-flight
// (e.g. a `data-anim="out"` cell during an exit crossfade). Calling this **arms
// an automatic settle** (see `settleTransitions` / the `afterEach` below) so the
// test can't leak the #1760 class by forgetting to drain a concurrent enter that
// has no DOM signal to `waitFor`. Pass `settleMs` derived from the component's
// real animation duration so the settle window can't silently become
// insufficient if that duration changes.
export function renderWithMantineTransitions(
ui: ReactElement,
options?: Omit<RenderOptions, "wrapper">,
options?: MantineTransitionsRenderOptions,
) {
return render(ui, { wrapper: TransitionsWrapper, ...options });
const {
colorScheme = "light",
settleMs = DEFAULT_SETTLE_MS,
...rest
} = options ?? {};
// Keep the LONGEST armed window if a test renders more than one real-
// transitions tree, so a later short-animation render can't under-settle an
// earlier long one (last-write-wins would).
armedSettleMs = Math.max(armedSettleMs ?? 0, settleMs);
const result = render(ui, {
wrapper: makeWrapper("default", colorScheme),
...rest,
});
liveArmedContainers.add(result.container);
// If the test unmounts this tree itself, *downgrade* — don't disarm. The drain
// still runs (it's mechanism-independent: flushing the queued rAF/`setTimeout`
// while `window` is alive is what avoids the #1760 leak, and a `setState`
// landing on the now-unmounted tree is a React no-op — this is exactly the
// mid-flight-unmount case that most needs the drain). We only drop this
// container from the liveness set, so the auto-settle's `isConnected` check
// doesn't misread the test's own `unmount()` as a broken cleanup ordering, and
// — because it's per-container — unmounting one tree doesn't cancel the settle
// for another still-mounted one. Wrap rather than mutate RTL's result so
// `rerender`/`container`/etc. pass through unchanged.
const downgradingUnmount = () => {
liveArmedContainers.delete(result.container);
result.unmount();
};
return { ...result, unmount: downgradingUnmount };
}
// Fallback settle window: ≈500ms clears a typical few-hundred-millisecond
// transition plus the two-frame rAF scheduling slack. Callers that know their
// component's animation duration should pass `settleMs` (its longest JS timer
// chain — duration plus any `enterDelay`/`exitDelay` — plus slack) rather than
// rely on this default.
const DEFAULT_SETTLE_MS = 500;
// Set by `renderWithMantineTransitions`; consumed once by the `afterEach` below.
// `armedSettleMs === null` means no real-transitions render happened this test,
// so nothing to settle (every `renderWithMantine` / `env="test"` test skips the
// wait entirely). `liveArmedContainers` holds the containers this test rendered
// and did NOT unmount itself — the `afterEach` asserts each is still connected
// (a still-mounted tree that got detached means `cleanup()` ran too early). A
// test's own `unmount()` removes its container from the set (its detach is
// legitimate) but leaves `armedSettleMs` armed so the drain still runs.
//
// Two config assumptions this module-level state depends on:
// 1. **Sequential tests.** It isn't concurrency-safe, so don't use
// `renderWithMantineTransitions` under `test.concurrent` /
// `describe.concurrent` (two in-flight tests would share this arming).
// 2. **`isolate: true`** (Vitest's default; nothing in vite.config.ts
// overrides it). The `afterEach` below is registered when this module is
// first evaluated during a test file's collection, so isolation — a fresh
// module registry per file — is what re-registers it for every file. Under
// `isolate: false` the module evaluates once per worker, so the hook binds
// to only the first importing file; other files then arm with no hook to
// consume it — the drain silently never runs (reopening #1760/#1786 with no
// guard trip) and this state leaks across files. Confirmed by probe:
// `vitest --no-isolate --pool=threads --no-file-parallelism` over two
// real-transitions files leaves one file's arming unconsumed. If isolation
// is ever disabled, move arming to `onTestFinished` inside
// `renderWithMantineTransitions` (per-test, immune to module caching) — but
// mind that its ordering vs `setup.ts`'s `cleanup()` must be re-established.
let armedSettleMs: number | null = null;
const liveArmedContainers = new Set<HTMLElement>();
function resetArming() {
armedSettleMs = null;
liveArmedContainers.clear();
}
// Flush any in-flight `renderWithMantineTransitions` animation before the test
// ends. What's observed when it isn't flushed: an in-flight Mantine transition
// produces an uncaught, post-teardown `dispatchSetState` that throws
// `ReferenceError: window is not defined` from react-dom's
// `resolveUpdatePriority` (React 19 reads `window.event` there) — after
// happy-dom has torn down `window` — failing the whole run even though every
// assertion passed (#1760/#1786). What's known: Mantine's `useTransition` does
// have an unmount cleanup (`useEffect` clearing its rAF + `setTimeout`), yet the
// failing frame is an rAF callback executing *after* teardown, so the precise
// escape route (a cancelAnimationFrame that doesn't fully cancel under
// happy-dom, an rAF re-armed after cleanup, or a scheduler-flushed
// continuation) isn't pinned down. The fix doesn't depend on which: awaiting a
// real timer inside `act` drains the queued rAF callbacks, the terminal
// `setTimeout`, and the React work they schedule against the still-mounted tree
// — whichever is the actual escapee, it resolves on a live component before
// cleanup unmounts. Because it awaits a *real* `setTimeout`, it deadlocks under
// fake timers, so guard against that with a clear message rather than a 5s
// test-timeout hang.
export async function settleTransitions(ms: number = DEFAULT_SETTLE_MS) {
if (vi.isFakeTimers()) {
throw new Error(
"settleTransitions() awaits a real setTimeout and cannot run under " +
"vi.useFakeTimers(); call vi.useRealTimers() first, or advance the " +
"faked timers manually to settle the transition.",
);
}
await act(async () => {
await new Promise((resolve) => setTimeout(resolve, ms));
});
}
// Auto-settle any armed real transition. This must run — and *complete* — before
// `setup.ts`'s `cleanup()` unmounts the tree, so the settling `setState` targets
// a still-live component. That holds because `cleanup()` is a setupFile hook,
// which Vitest treats as *outer* relative to this import-registered inner hook:
// an inner afterEach runs to completion before an outer one, in *every*
// `sequence.hooks` mode. Verified — ViewHeader's real-transitions tests pass
// under "stack", "list", and "parallel" with neither check below tripping, and a
// same-suite `afterEach(cleanup)` probe (where the ordering *can* break) does
// trip them. So `sequence.hooks` isn't actually load-bearing here; the unit
// project still pins "stack" (conventional LIFO) as defense-in-depth.
// Enforcement is the `container.isConnected` checks over the still-live armed
// containers, which guard against a future regression that makes cleanup run
// before this settle finishes — e.g. cleanup moved to a *same-level* hook: the
// pre-settle check catches it running entirely first, the post-settle check
// catches it detaching the tree concurrently while this hook awaits. The
// pre-check runs synchronously *before* the await, so it structurally can't
// observe a mid-settle detach — that's why the post-check exists. That
// concurrent case additionally needs the unit project's `sequence.hooks` set to
// "parallel" (same-level hooks run sequentially under "stack"/"list", so they
// can't overlap this hook's await). Neither fires in the current outer-cleanup
// setup. A container the test unmounted itself isn't in
// the set, so its legitimate detach is never mistaken for that regression.
afterEach(async () => {
const ms = armedSettleMs;
const liveContainers = [...liveArmedContainers];
resetArming();
if (ms === null) return;
// A `renderWithMantineTransitions` test that also used fake timers can't be
// drained by a real-timer wait; skip, but warn — silence here is how the leak
// sneaks back (unlike the manual `settleTransitions`, which throws). No such
// test exists today.
if (vi.isFakeTimers()) {
const testName = expect.getState().currentTestName ?? "(unknown test)";
console.warn(
`renderWithMantineTransitions auto-settle skipped for "${testName}": ` +
"the test is under vi.useFakeTimers(). A real-transitions test should " +
"not use fake timers — anything left pending on the real clock is then " +
"unprotected (this settle can't drain it), so the test depends on which " +
"clock was installed at teardown.",
);
return;
}
// Drain regardless of whether any tree is still mounted, and even if the
// liveness check has already failed: a test that unmounted mid-transition still
// needs the queued rAF/`setTimeout` flushed while `window` is alive (the #1760
// case), and on the ordering regression the checks exist to catch, draining
// best-effort keeps the explanatory error below from *also* racing an uncaught
// post-teardown `window is not defined` in some unrelated file. So capture a
// liveness failure, always drain, then throw. The pre-settle check catches
// cleanup() running entirely first; the post-settle check (only meaningful if
// the pre one passed) catches a same-level concurrent cleanup() detaching a
// still-live tree mid-settle, which the pre-check can't see. Both apply only to
// trees the test left mounted.
let livenessError: unknown;
const captureLiveness = (when: "before" | "after") => {
try {
assertLiveContainersConnected(liveContainers, when);
} catch (error) {
livenessError ??= error;
}
};
captureLiveness("before");
let drainError: unknown;
try {
await settleTransitions(ms);
} catch (error) {
drainError = error;
}
if (livenessError === undefined) captureLiveness("after");
// A liveness error explains the actual regression; a drain-time throw is a
// downstream symptom — so prefer the liveness error, else rethrow the drain's.
if (livenessError !== undefined) {
if (drainError !== undefined) {
// Preserve the drain error rather than dropping it when both fire: attach
// it as the liveness error's `cause` (when that's an Error) *and* log it,
// so it's readable regardless of whether the reporter renders `cause` and
// on the (currently unreachable) path where the liveness error isn't an
// Error and no cause can be set.
if (livenessError instanceof Error) livenessError.cause = drainError;
console.error(
"renderWithMantineTransitions auto-settle: the drain also threw:",
drainError,
);
}
throw livenessError;
}
if (drainError !== undefined) throw drainError;
});
// Throw if any still-live armed tree was detached at the given point relative to
// the settle — meaning something unmounted it before this settle finished, so it
// drained against a dead tree and the #1760 leak is reopened. Only trees the
// test left mounted are passed here; a tree the test unmounted itself (via the
// wrapped `unmount()`) is excluded, so its legitimate detach never lands here.
function assertLiveContainersConnected(
containers: HTMLElement[],
when: "before" | "after",
) {
if (containers.some((c) => !c.isConnected)) {
// The check observes only that a tree the test left mounted is no longer
// connected — it can't identify *what* detached it — so state that
// observation, then list the candidate causes rather than asserting one.
throw new Error(
`renderWithMantineTransitions auto-settle: a still-mounted armed tree was ` +
`detached ${when === "before" ? "before the settle started" : "while the settle was in flight"}, ` +
"so the settle drained against a dead tree and the #1760 leak is " +
"reopened. Candidate causes: (a) a bare mid-body `cleanup()` in the test " +
"— use the `unmount()` returned by renderWithMantineTransitions instead " +
"(it drops the tree from the liveness set); or (b) `cleanup()` moved off " +
"setup.ts's setupFile (outer) hook to a same-level afterEach, so it no " +
"longer runs after this one (the `sequence.hooks` pin is defense-in-" +
"depth, not the guarantee — see the auto-settle ordering note in " +
"renderWithMantine.tsx).",
);
}
}
export * from "@testing-library/react";

View file

@ -57,25 +57,30 @@ Object.defineProperty(globalThis, "fetch", {
),
});
// happy-dom doesn't implement `matchMedia`, so Mantine's `useReducedMotion`
// resolves to "motion allowed" and every `Transition` (ScreenStage, Modal, …)
// schedules real enter/exit `setTimeout`s. A timer that outlives its test fires
// after the environment is torn down and throws `window is not defined` from
// deep in react-dom — an unhandled error that fails the run even though every
// test passed. Report `prefers-reduced-motion: reduce` so Mantine transitions
// render instantly (no timers); all other queries stay `false`, matching the
// prior (absent-matchMedia) behavior so `useMediaQuery`-driven layout is
// unchanged. Tests that need specific media results still mock `@mantine/hooks`
// or stub `matchMedia` themselves.
// happy-dom (v20) *does* implement `matchMedia`/`MediaQueryList`, so this isn't
// a polyfill for a missing API — it's a deliberate **override** of happy-dom's
// implementation, pinning `prefers-reduced-motion: reduce` and reporting `false`
// for every other query (preserving the historical absent-matchMedia layout
// behavior for `useMediaQuery`-driven code). Note this does *not*, on its own,
// 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.
Object.defineProperty(window, "matchMedia", {
configurable: true,
writable: true,
// Partial `matchMedia` mock: happy-dom doesn't implement it, and the members
// below are all the app touches. The stub omits the rest of `MediaQueryList`,
// so the double cast bridges the deliberately-incomplete shape.
// Partial `matchMedia` override: the members below are all the app touches.
// The stub omits the rest of `MediaQueryList`, so the double cast bridges the
// deliberately-incomplete shape. The `matches` regex is anchored to the
// `reduce` form so the stub doesn't also answer `true` to
// `(prefers-reduced-motion: no-preference)` (claiming both preferences at
// once); Mantine only ever queries the `reduce` form.
value: (query: string): MediaQueryList =>
({
matches: /prefers-reduced-motion/.test(query),
matches: /prefers-reduced-motion:\s*reduce/.test(query),
media: query,
onchange: null,
addEventListener: () => {},

View file

@ -290,6 +290,21 @@ export default defineConfig(({ command }) => {
// Integration tests run in the integration project below (node env).
exclude: [integrationGlob],
setupFiles: [path.join(dirname, "src/test/setup.ts")],
// Pin after-hooks to LIFO (reverse registration). This is Vitest 4's
// own default (`resolved.sequence.hooks ??= "stack"` — the CLI
// help-text's "parallel" is stale), so this line documents intent and
// guards a future default change rather than overriding anything. It's
// defense-in-depth, NOT load-bearing: the real-transitions auto-settle
// in `src/test/renderWithMantine.tsx` needs its `afterEach` to complete
// before `setup.ts`'s setupFile `cleanup()` unmounts the tree (avoiding
// the #1760 post-teardown `window is not defined` leak, #1786), and
// that already holds in *every* `sequence.hooks` mode because
// `cleanup()` is a setupFile (outer) hook, which Vitest runs after
// inner afterEach hooks regardless of this setting (verified across
// stack/list/parallel). The settle's own before/after
// `container.isConnected` self-checks are the real guard against a
// future regression.
sequence: { hooks: "stack" },
},
},
{