* 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>
a7b2fdb2ef