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

[cliffhall]

Closes #1786

Problem

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 failure class AGENTS.md documents. Because it's timer-timing dependent, a green run was non-evidence: it failed a docs-only commit on PR #1784 while all 4470 tests passed.

Root cause

ViewHeader.test.tsx has three tests that opt into real (env="default") Mantine transitions via renderWithMantineTransitions to observe mid-flight data-anim="in"/"out" cells. Each drove its primary transition to completion (waitFor the exiting cell to unmount), but the concurrent enter on the sibling crossfade cell leaves no unmount to waitFor — a completed enter has no DOM signal. So its rAF → setTimeout(300ms) chain could still be pending at test end; when it fired after teardown, react-dom's dispatchSetState → resolveUpdatePriority read a torn-down window and threw, failing whatever unrelated file was running at that instant.

Fix

• settleTransitions() at the end of each of the three transitions tests: an act-wrapped wait (500ms > the 300ms header duration + two-frame rAF slack) that drains every pending transition timer against the still-mounted tree, so the settling setState targets a live component and nothing remains to fire after cleanup unmounts. Deterministic — the wait strictly exceeds the JS timer chain (the stagger is CSS-only; no Mantine enterDelay/exitDelay).
• colorScheme option on renderWithMantine / renderWithMantineTransitions (closes the related nit): ViewHeader.test.tsx no longer hand-rolls a bare defaultColorScheme="dark" MantineProvider — the Flaky test teardown: Mantine Transition timer fires after happy-dom window teardown (window is not defined) #1760 anti-pattern the AGENTS.md rule forbids. It now calls renderWithMantine(ui, { colorScheme: "dark" }).
• AGENTS.md documents both the new option and the "settle every enter you open" requirement.

Verification

• ViewHeader.test.tsx (20 tests) run repeatedly — clean every time, no window is not defined.
• npm run ci green end-to-end locally: validate → the per-file ≥90 coverage gate → verify:build-gate → smoke → 460 Storybook tests.

Acceptance

• Coverage gate no longer throws window is not defined; repeated runs clean.
• renderWithMantineTransitions tests drive transitions to completion / settle before teardown.
• ViewHeader.test.tsx no longer hand-rolls a bare MantineProvider (helper gains a colorScheme option).
• npm run ci green.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 5m 13s —— View job

---

Review: PR #1793 — settle real-timer Transitions

• Read the PR diff
• Read ViewHeader.test.tsx + ViewHeader.tsx for context
• Check renderWithMantine.tsx helper and its other callers
• Verify the diagnosis against Mantine 8.3.17 useTransition + setup.ts cleanup
• Post review

Verdict: good change — the colorScheme option is a clean win, the helper dedup is correct, and the settle is the right shape of mitigation. Nothing blocking. Four notes, one of which I'd fix before merge (the magic duration) and one diagnosis nuance worth recording so the next person reading this trail isn't misled.

What's right

• makeWrapper(env, colorScheme) collapses two near-identical wrappers without behavior change, and the per-call wrapper identity is safe: RTL's rerender reuses the wrapper captured from the initial render options, so the three rerender-driven transitions tests don't remount their trees.
• MantineRenderOptions keeps wrapper omitted, so a caller still can't bypass the provider, and colorScheme = "light" preserves behavior for every existing caller.
• The colorScheme option genuinely retires the anti-pattern: after this PR, grepping clients/web/src/**/*.test.tsx for MantineProvider returns only a comment in ContentViewer.test.tsx:221 — no test hand-rolls a provider anymore.
• settleTransitions() is act-wrapped and runs before cleanup, so the settling setState lands on a live tree. That's the correct ordering; a bare await sleep() outside act would have produced "update not wrapped in act" noise instead.
• AGENTS.md wording is accurate and the "a completed enter leaves no DOM signal to waitFor" observation is the real insight here.

1. TRANSITION_SETTLE_MS = 500 is decoupled from the duration it must exceed (worth fixing)

ViewHeader.test.tsx:15 hardcodes 500 and the comment above it names HEADER_ANIM_MS (300ms) — but HEADER_ANIM_MS is module-private (ViewHeader.tsx:62) so nothing links them. Bump the header animation to, say, 600ms and the settle silently stops settling: the trailing setTimeout fires after the wait, the leak returns, and — by this PR's own "a green run is non-evidence" argument — nothing in CI reliably tells you. Exporting HEADER_ANIM_MS and computing const TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 makes the invariant structural instead of a comment. Fix this →

2. settleTransitions() is now a documented convention living in one test file

AGENTS.md now tells every future renderWithMantineTransitions caller to "flush any such straggler … (see settleTransitions in ViewHeader.test.tsx)". A cross-file "go copy this function" is exactly the kind of shared test scaffolding AGENTS.md's own placement rule assigns to src/test/ ("not tests of a source file, so nothing to sit beside"). Exporting settleTransitions(ms?) from src/test/renderWithMantine.tsx, right next to renderWithMantineTransitions, means the next caller imports it and AGENTS.md can point at the helper rather than a test file. Today ViewHeader.test.tsx is the only consumer (I grepped — the three call sites are all in this file), so this is cheap now and gets more expensive later. Fix this →

3. Minor footgun: settleTransitions hangs under fake timers

It awaits a real setTimeout, and this same file has two tests inside vi.useFakeTimers() (ViewHeader.test.tsx:288, :434). Calling it from one of those would block until the 5s test timeout with a confusing failure. Harmless today; if it becomes a shared export (note 2) it's worth a vi.isFakeTimers() guard that throws a clear message, or at least a line in the doc comment. Fix this →

4. Diagnosis nuance (non-blocking, but the comments will be read as canon)

I checked the mechanism the comments assert. @mantine/core@8.3.17's useTransition ends with:

useEffect(() => () => { clearAllTimeouts(); }, []);
// clearAllTimeouts: clearTimeout(transitionTimeoutRef) + clearTimeout(delayTimeoutRef) + cancelAnimationFrame(rafRef)

and src/test/setup.ts runs cleanup() in afterEach. So a Mantine transition timer that is merely pending at test end is already cancelled when RTL unmounts the tree — which means "a straggler setTimeout survives to post-teardown" can't be the whole story. The residue that actually escapes is more likely the React work scheduled by the out-of-act setStatus("entering"/"exiting") inside Mantine's inner rAF — which matches the stack you quote (dispatchSetState → resolveUpdatePriority). Your fix handles that identically (the act-wrapped drain runs the rAF callbacks and flushes React while mounted), so the change stands as-is; I'd just soften the "drain every pending timer … before the global cleanup() unmounts" phrasing to say what's actually being flushed (queued rAF callbacks + the terminal timeout + the React updates they schedule), so the next reader doesn't conclude that Mantine leaks timers past unmount.

Related, and worth knowing since it's the same mitigation stack: setup.ts:62-71 claims the matchMedia stub reporting prefers-reduced-motion: reduce makes "every Transition … render instantly (no timers)". It doesn't — useTransition only honors it when theme.respectReducedMotion is true, Mantine's default is false (@mantine/core@8.3.17 default-theme.mjs:23), and src/theme/theme.ts doesn't override it. Your three env="default" tests observably do run real timers, which confirms it. The real protection is env="test", exactly as the renderWithMantine comment says. Out of scope for this PR, and please don't flip respectReducedMotion in the shared theme (it would neutralize renderWithMantineTransitions entirely) — but the stale comment is worth a follow-up issue. Fix this →

Cost note

Three real 500ms sleeps add ~1.5s to the unit suite and again to the coverage run. Fine at this scale — just don't let the pattern spread past the handful of tests that genuinely need real transitions, which is what the AGENTS.md wording already guards against.

CI for this push is still in progress at the time of writing.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Thanks for the thorough review — all four notes addressed in 5834cd2.

1. TRANSITION_SETTLE_MS decoupled from the duration (the one you'd fix before merge) — Fixed structurally. HEADER_ANIM_MS is now exported from ViewHeader.tsx, and the test computes const TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200. Bumping the header duration now carries the settle window with it; the invariant is no longer a comment.

2. settleTransitions should be shared scaffolding, not live in one test file — Done. It's now exported from src/test/renderWithMantine.tsx, right next to renderWithMantineTransitions, with an optional ms argument (defaults to 500). ViewHeader.test.tsx imports it and passes the header-derived duration. AGENTS.md now points at the exported helper instead of the test file.

3. Fake-timers footgun — Added a guard: settleTransitions throws a clear message when called under vi.useFakeTimers() (it awaits a real setTimeout, so it would otherwise hang to the 5s test timeout). The message tells the caller to useRealTimers() first or advance the faked timers manually.

4. Diagnosis nuance — Corrected the comments so the trail isn't misleading. settleTransitions's doc now states the hazard is not a Mantine timer surviving unmount (its useEffect cleanup cancels the rAF + setTimeout) but the React state update Mantine's inner rAF schedules outside act (setStatus(...) → the terminal setStatus), which resolves post-teardown — matching the dispatchSetState → resolveUpdatePriority stack. The act-wrapped drain runs those queued rAF callbacks and flushes the React work while mounted. I also fixed the stale setup.ts:62 matchMedia comment per your related finding: the prefers-reduced-motion: reduce stub does not disable transitions (Mantine 8 defaults respectReducedMotion to false, and the project theme doesn't override it) — env="test" is the actual #1760 protection. Left respectReducedMotion untouched as you advised (flipping it would neutralize renderWithMantineTransitions), and handled the comment here rather than as a follow-up.

npm run ci is green end-to-end locally after these changes (validate → coverage gate → build gate → smokes → 460 Storybook tests).

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 52s —— View job

---

Review: PR #1793 — follow-up commit 5834cd2

• Read the follow-up diff (git diff 13caa111..HEAD)
• Verify each of the four prior notes is actually addressed
• Check the new vitest import in the shared helper can't reach non-test consumers
• Post review

Verdict: all four notes are addressed, and notes 1–3 are addressed well — the fixes are structural rather than cosmetic. Nothing blocking. One note (4) I'd call an over-correction: the comment now asserts a mechanism more confidently than the evidence supports, and it doesn't quite close. Plus two small comment-accuracy nits in the same paragraph that was just rewritten for accuracy.

Note 1 — settle window derived from the duration ✅

HEADER_ANIM_MS exported (ViewHeader.tsx:62), TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 (ViewHeader.test.tsx:20). The invariant is now structural — bumping the header duration carries the settle window with it. Exporting a constant purely for a test is a fair trade here, and it sits directly under the existing "keep the CSS in sync with this value" comment, so the reason it's exported is legible at the definition site.

Residual (cosmetic): the helper's own fallback comment — renderWithMantine.tsx:62-66, "Mantine's default header transitions run ~300ms" — conflates a ViewHeader-specific duration with a Mantine default. Mantine has no "default header transition" (its Transition default duration is 250ms); 300ms is HEADER_ANIM_MS. Since the whole point of note 1 was decoupling those two ideas, the fallback's justification reads oddly. "≈500ms clears a typical few-hundred-ms transition plus rAF slack; callers that know their duration should pass it" says the same thing without the false attribution. Fix this →

Note 2 — shared scaffolding ✅

settleTransitions now lives in src/test/renderWithMantine.tsx next to renderWithMantineTransitions, with an optional ms; ViewHeader.test.tsx imports it (its only consumer — I grepped) and AGENTS.md points at the exported helper. Two things I checked that make this land cleanly:

• eslint.config.js:26-32 already turns off react-refresh/only-export-components for src/test/**, so exporting a non-component from this .tsx doesn't trip the lint rule.
• src/test/ is outside the coverage include whitelist in vite.config.ts, so the new if (vi.isFakeTimers()) branch and the ms default-parameter branch aren't gated — same as before the move (it lived in a test file, also unmeasured). Coverage-neutral.

The makeWrapper(env, colorScheme) dedup from the first commit is unchanged and still correct — RTL's rerender reuses the wrapper captured at initial render, so the fresh per-call wrapper identity can't remount the three rerender-driven trees.

Note 3 — fake-timer guard ✅

renderWithMantine.tsx:75-82 throws with an actionable message naming both escapes (useRealTimers() or advance manually). Correct, and the two vi.useFakeTimers() tests in this file (ViewHeader.test.tsx:278, :424) don't call it. Two observations, neither a problem:

• vi is now a static import in the helper. Safe: src/test/setup.ts:2 already imports from vitest in the same folder, and grep confirms every importer of renderWithMantine is a *.test.tsx (no *.stories.tsx, nothing in the app graph), so the vitest import can't reach the Storybook browser project or a vite build.
• I couldn't verify vi.isFakeTimers() against installed types — this checkout has no node_modules. It's a documented vitest 4 API and your local npm run ci typechecks the file, so I'm treating it as fine; CI (build job currently pending) will confirm.

Note 4 — the diagnosis went one step too far (worth softening)

renderWithMantine.tsx:67-74 now asserts, flatly, two things that don't fully compose:

1. Mantine's useTransition cancels its rAF + setTimeout on unmount, "so the hazard isn't a timer surviving unmount", and
2. the hazard is the out-of-act setStatus scheduled by Mantine's inner rAF, which "resolves after happy-dom tears down window and throws … from react-dom's dispatchSetState → resolveUpdatePriority."

resolveUpdatePriority is called synchronously inside dispatchSetState (React 19's requestUpdateLane reads window.event). So the throwing frame is whatever calls setStatus — i.e. the rAF callback itself, executing post-teardown. If that rAF were genuinely cancelled at unmount, the callback couldn't run and there'd be nothing to throw. So claim 1 and claim 2 can't both be complete: either something isn't actually cancelled (happy-dom's cancelAnimationFrame, an rAF re-armed after cleanup read the ref, or a tree that outlives cleanup()), or the throw arrives via a scheduler-flushed continuation rather than dispatchSetState proper.

I proposed this framing in my earlier note, so this is me walking back my own suggestion rather than yours: on a second look it explains the stack but not the survival, and it shouldn't be stated as settled. Your fix is unaffected either way — an act-wrapped drain against a mounted tree flushes queued rAF callbacks, the terminal timeout, and the React work they schedule, whichever of those is the actual escapee. I'd just say that: record the observation (a post-teardown dispatchSetState), the known fact (Mantine's cleanup exists), and that the precise escape route isn't pinned down — so the next reader doesn't build on a mechanism that doesn't close. Fix this →

setup.ts — right call, but check the new premise

Correcting the respectReducedMotion claim was the important part and it's accurate; leaving the theme alone was right. But the rewrite opens with a premise inherited from the old comment that I suspect is now stale: "happy-dom doesn't implement matchMedia, so anything reading it … would otherwise throw." happy-dom has implemented window.matchMedia/MediaQueryList for several major versions (this repo is on ^20.9.0), and Mantine's useMediaQuery guards a missing matchMedia rather than throwing — AGENTS.md itself lists "useMediaQuery fallbacks" among the happy-dom-inherent v8 ignore paths, which implies the fallback is reachable, not fatal. If that's right, this block isn't a polyfill for a missing API — it's a deliberate override of happy-dom's implementation to pin prefers-reduced-motion: reduce and force every other query false, which is a materially different (and more interesting) reason for it to exist. I can't confirm against node_modules here, so treat it as "worth a 30-second check" rather than a finding. Fix this →

Nit

MantineRenderOptions isn't exported, so a caller can't name the options type when building one in a variable. Trivial, and no current caller needs it.

Summary

Notes 1–3 are properly fixed and I'd merge on those. The remaining items are all comment-accuracy in test infra — zero runtime/behavioral risk, but this PR's value is largely the trail it leaves for the next person hitting #1760, so the comments are the deliverable and worth getting right. build check is still pending as I write this.
· v2/fix-1786-transition-timer-leak

[cliffhall]

All round-2 items addressed in a60c0d1 (test-infra comments only, no behavior change) — thanks for walking your own note 4 back, that's the right call.

Note 1 residual (DEFAULT_SETTLE_MS comment) — Reworded to drop the false attribution. It no longer claims a "Mantine default header transition" (Mantine's Transition default is 250ms; 300ms was HEADER_ANIM_MS); it now justifies ≈500ms generically as "a typical few-hundred-millisecond transition plus the two-frame rAF scheduling slack," and keeps the "callers who know their duration should pass it" advice.

Note 4 (over-confident mechanism) — Softened. The settleTransitions doc now separates observed (an uncaught post-teardown dispatchSetState throwing from resolveUpdatePriority, which — as you note — runs synchronously inside dispatchSetState, so the throwing frame is the rAF callback itself executing post-teardown), known (useTransition has an unmount cleanup), and explicitly states 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 — and that the act-wrapped drain fixes it regardless of which.

setup.ts premise — Verified against node_modules: happy-dom is 20.9.0 and ships matchMedia + a full match-media/MediaQueryList implementation. So the block is a deliberate override, not a polyfill for a missing API. Reworded to say exactly that (pins prefers-reduced-motion: reduce, reports false for every other query to preserve the historical absent-matchMedia layout behavior), keeping the correct respectReducedMotion/env="test" clarification.

Nit — MantineRenderOptions is now exported.

npm run ci green end-to-end locally after these changes.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 33s —— View job

---

Review: PR #1793 — round-3 (a60c0d1c, comment tightening)

• Read the full PR diff (git diff origin/v2/main...HEAD)
• Verify each round-2 item landed (DEFAULT_SETTLE_MS wording, note-4 softening, setup.ts premise, exported type)
• Re-verify the settle window against the component's real timer chain
• Check the fake-timer tests can't hit the new guard
• Post review

Verdict: round-2 items all landed correctly, and the note-4 walk-back is recorded honestly (observed / known / not-pinned-down) rather than papered over. Nothing blocking. One real finding, in the same file this commit was rewriting for accuracy.

1. setup.ts:75 still says the opposite of setup.ts:60 (worth fixing)

The block comment was corrected — but the inner comment on the value: property wasn't:

// clients/web/src/test/setup.ts:60
// happy-dom (v20) *does* implement `matchMedia`/`MediaQueryList`, so this isn't
// a polyfill for a missing API — it's a deliberate **override** …

// clients/web/src/test/setup.ts:75  ← two lines below the block above
// Partial `matchMedia` mock: happy-dom doesn't implement it, and the members
// below are all the app touches. …

Two comments 15 lines apart now make contradictory factual claims about the same API. That's worse than the original stale comment was — a reader who stops at the second one draws exactly the wrong conclusion, and there's no way to tell which is current. It also matters beyond tidiness: that comment is the AGENTS.md-mandated justification for the as unknown as MediaQueryList double cast on line 88, and its opening clause is now the false premise. The cast's real justification is the second sentence (the stub omits most of MediaQueryList), which stands on its own — so dropping the first clause both fixes the contradiction and leaves the double cast properly justified. Fix this →

2. Round-2 items — all landed ✅

Item	Status
DEFAULT_SETTLE_MS false "Mantine default header transition" attribution	Fixed — now justified generically (renderWithMantine.tsx:55-58), with the "callers who know their duration should pass it" advice kept
Note 4 over-confident mechanism	Softened well (:61-78) — states the observed failure, the known useTransition cleanup, and explicitly that the escape route (happy-dom cancelAnimationFrame, an rAF re-armed post-cleanup, or a scheduler-flushed continuation) isn't pinned down, plus why the fix is route-independent
setup.ts respectReducedMotion premise	Corrected, theme untouched as advised — except the inner comment, finding 1
MantineRenderOptions not exported	Exported (:11)

3. Re-verified the settle window actually bounds the JS chain

Worth spelling out since finding 1 is cosmetic and this is the part that has to be right:

• The four header Transitions all use duration={HEADER_ANIM_MS} / exitDuration={HEADER_ANIM_MS} (ViewHeader.tsx:319-419), no enterDelay/exitDelay — so the longest JS chain is one rAF pair + a 300ms setTimeout. TRANSITION_SETTLE_MS = 300 + 200 strictly exceeds it. ✅
• The 150ms stagger is genuinely CSS-only: App.css:325 is animation: inspector-fade-slide-in 300ms ease 150ms both. Total CSS time is 450ms — also inside the 500ms window, though only the JS side needs to be. ✅
• The second animation constant in the file, TOGGLE_ANIM_MS = 200 (:215, the monitorToggle slide), is not covered by the HEADER_ANIM_MS-derived formula — but it can't bite: connectedProps (ViewHeader.test.tsx:33-44) has no monitorToggle, so that Transition never mounts in the three real-timer tests, and 200 < 300 means the derived window would cover it anyway. ✅
• The new fake-timer guard can't fire spuriously: the two vi.useFakeTimers() tests (:278, :424) don't call settleTransitions, and both restore in a finally, so a failure inside one can't leak fake timers into a later settling test. ✅
• settleTransitions is imported (not copied) and is the sole consumer; no test in clients/web/src hand-rolls a MantineProvider any more — the only remaining match is a comment in ContentViewer.test.tsx:221. ✅

4. Nit — makeWrapper's doc leads with the wrong branch

renderWithMantine.tsx:15-22: the header line says "Build a MantineProvider wrapper for the given env", then the inherited paragraph explains why env="test" is right — but this function also builds the env="default" wrappers, whose whole point is the opposite. The env="test" rationale reads more naturally on renderWithMantine itself (mirroring the env="default" rationale that already sits on renderWithMantineTransitions), leaving makeWrapper with its one-liner. Pure readability; no behavior. Fix this →

5. Follow-up worth an issue (out of scope here)

setup.ts:80 is matches: /prefers-reduced-motion/.test(query), so the stub also answers true to (prefers-reduced-motion: no-preference) — it claims both preferences simultaneously. Harmless today (Mantine queries only the reduce form, and per your own correction respectReducedMotion: false means nothing acts on it), but now that this block is documented as a deliberate override rather than a shim, the sloppy match is a latent trap for the first test that queries no-preference. Matching /prefers-reduced-motion:\s*reduce/ would make the override say what the comment says it says. Not for this PR.

Verification caveat

This checkout has no node_modules, so I could not confirm vi.isFakeTimers() against the installed vitest types, nor the happy-dom v20 matchMedia claim at source. Both are load-bearing for the code and the comment respectively; I'm relying on your green local npm run ci (which typechecks the helper) and your stated node_modules check for the happy-dom version. Everything else above was verified against the files in this branch.

Summary

Merge-ready on substance — the fix, the derivation, the guard, and the shared-helper placement are all correct and I found nothing that changes behavior. Finding 1 is the one I'd fix first: a commit whose stated purpose is comment accuracy shouldn't leave two adjacent comments asserting opposite facts, especially when one of them is doing double duty as a double-cast justification. Findings 4 and 5 are optional.
· v2/fix-1786-transition-timer-leak

[cliffhall]

All round-3 items handled in d56e342 (test-infra comments + one regex tighten; no runtime behavior change to the app).

Finding 1 (contradictory adjacent comments) — Fixed. The inner value: comment no longer says "happy-dom doesn't implement it"; that false clause is gone. It keeps the part that actually justifies the as unknown as MediaQueryList double cast — the stub implements only the members the app touches and omits the rest of the interface, which a single cast can't bridge. So the block comment and the cast justification now agree.

Finding 5 (the no-preference trap) — Folded into this PR rather than deferred, per handling everything here. The matches regex is now /prefers-reduced-motion:\s*reduce/, so the override stops answering true to (prefers-reduced-motion: no-preference). npm run ci stays green, confirming nothing depended on the old broad match.

Nit 4 (makeWrapper doc led with the wrong branch) — Moved the env="test" / #1760 rationale onto renderWithMantine (mirroring the env="default" rationale already on renderWithMantineTransitions); makeWrapper keeps a one-line description that points at both helpers.

Re: your verification caveat — confirmed locally: happy-dom is 20.9.0 with a full match-media/MediaQueryList implementation, and vi.isFakeTimers() typechecks under the installed vitest 4 (the green npm run ci includes the helper's typecheck).

npm run ci green end-to-end (validate → coverage gate → build gate → smokes → 460 Storybook tests).

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 17s —— View job

---

Review: PR #1793 — round 4 (d56e3429)

• Read the full PR diff vs origin/v2/main
• Verify the three round-3 items landed (inner matchMedia comment, no-preference regex, makeWrapper doc)
• Check the tightened regex against every prefers-reduced-motion consumer
• Re-verify the settle window, the fake-timer guard, and that no other file opens real transitions
• Post review

Verdict: merge it. Every round-3 item landed, and I found no new defect — no behavioral, correctness, or coverage issue in the diff. The two items below are forward-looking design suggestions about the helper's durability, not problems with this change; neither should hold the PR.

Round-3 items — all confirmed

Item	Status
Contradictory adjacent matchMedia comments (finding 1)	Fixed (setup.ts:75-80). The false "happy-dom doesn't implement it" clause is gone; what remains — the stub omits most of MediaQueryList — is the part that actually justifies the as unknown as MediaQueryList double cast on :91, so the AGENTS.md double-cast rule is still satisfied and the block comment on :60 no longer contradicts it.
no-preference trap (finding 5)	Tightened to /prefers-reduced-motion:\s*reduce/ (:83). Verified safe: the only prefers-reduced-motion consumers in the repo are this stub and App.css:336 (a CSS @media, which the JS stub can't affect); the only JS matchMedia caller is hostContext.ts:30 (prefers-color-scheme: dark), false before and after. The \s* also covers the unspaced (prefers-reduced-motion:reduce) form.
makeWrapper doc led with the wrong branch (nit 4)	Moved — makeWrapper:15-16 is now a neutral one-liner pointing at both helpers, and the env="test"/#1760 rationale sits on renderWithMantine:27-33, mirroring the env="default" rationale on renderWithMantineTransitions:42-48.

Also re-verified from the tree (not taken on trust):

• renderWithMantineTransitions has exactly one consumer file — ViewHeader.test.tsx — and all three of its call sites now settle (:274, :370, :420). So the leak class is closed everywhere it currently exists, not just in the test that was failing.
• env="default" is genuinely required by those three tests, so this couldn't have been fixed by just switching them to the safe helper: data-anim is set from props, not from transition status (ViewHeader.tsx:327/361/385/401), and the render props discard Mantine's interpolated styles — the Transitions are keep-alive only (:348-352). Under env="test" the duration collapses to 0 and the exiting cell unmounts synchronously, so the mid-flight "out" assertion has nothing to observe.
• The settle window still strictly bounds the JS chain: all four header transitions use duration/exitDuration={HEADER_ANIM_MS} with no enterDelay/exitDelay, so the longest chain is one rAF pair + a 300ms timeout, and 300 + 200 clears it. TOGGLE_ANIM_MS = 200 (:215) is uncovered by the formula but can't bite — connectedProps has no monitorToggle, so that Transition never mounts, and 200 < 300 anyway.
• The fake-timer guard can't fire spuriously: the two vi.useFakeTimers() tests (:278, :424) don't call settleTransitions, and both restore in a finally.
• Coverage-neutral: src/test/** isn't in the include whitelist (vite.config.ts:180-195), so the new vi.isFakeTimers() branch and the ms default parameter aren't gated — same as when the helper lived in a test file. And HEADER_ANIM_MS's new export doesn't trip react-refresh/only-export-components: reactRefresh.configs.vite (eslint.config.js:19) sets allowConstantExport.
• The vi static import can't escape the test graph: the only importers of renderWithMantine are *.test.tsx files (the setup.ts hit is a comment), so nothing reaches the Storybook browser project or a vite build.
• Versions match the comments' premises: happy-dom ^20.9.0, react ^19.2.4, vitest ^4.1.0, @mantine/core ^8.3.17, and nothing in clients/web/src or core sets respectReducedMotion — confirming the setup.ts:64-67 correction.

1. The settle contract is duration-only; a future caller with a delay under-settles (optional)

settleTransitions(HEADER_ANIM_MS + 200) is right for this component precisely because ViewHeader sets no enterDelay/exitDelay — a fact the PR body states but the helper's doc doesn't. renderWithMantine.tsx:57-60 tells the next caller to pass "duration + slack", which silently under-settles a Transition with a delay, or a component that chains two transitions. One clause — "duration plus any enterDelay/exitDelay plus rAF slack" — makes the formula complete at the place the next person reads it. Cheap, and it's the same class of fix as note 1 from round 1 (make the invariant legible where it's used, not only where it was derived). Fix this →

2. Settling is still a convention AGENTS.md has to police (follow-up, not this PR)

The failure mode this PR fixes was "the author forgot the concurrent enter had no DOM signal." After the PR, the defense against a recurrence is a paragraph in AGENTS.md plus a comment on the helper — i.e. the same class of defense that failed the first time. Since renderWithMantineTransitions already owns the wrapper, it could own the settle too: register a settle on test completion (vitest's onTestFinished, or an afterEach the helper installs), skipping it when fake timers are active. Then a real-transitions test can't leak by omission, and the three explicit calls in ViewHeader.test.tsx become redundant rather than load-bearing.

The load-bearing constraint if anyone tries this: the settle must run before the global cleanup() in setup.ts:94-97 unmounts the tree — that ordering is the whole reason the current act-wrapped drain works. I can't verify onTestFinished's ordering relative to afterEach from this checkout (no node_modules), so that needs confirming before it's worth doing; if it runs after cleanup(), the idea is dead and the convention stands. Worth an issue, not a change here.

Nit

ViewHeader.tsx:57-62's comment explains the CSS-sync duty of HEADER_ANIM_MS but not that it's now exported because ViewHeader.test.tsx derives its settle window from it — which is exactly the kind of "why is this exported?" a future tidy-up removes. Half a sentence at the definition site would pin it.

Verification caveat

This checkout has no node_modules, so vi.isFakeTimers() (vitest 4), happy-dom 20's matchMedia implementation, and Mantine's useTransition internals were checked against versions in package.json and documented behavior, not installed source. All are consistent with your local green npm run ci. Everything else above was verified against files on this branch. CI run 30210638880 is still in progress as I write this.

Summary

Three rounds in, the substance has been right since 5834cd24 and this commit finished the comment trail — the code, the derived settle window, the fake-timer guard, the shared-helper placement, and the tightened matchMedia override all check out, and no other test in the suite opens real transitions without settling. Both notes above are optional; note 2 is a follow-up issue at most.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Round-4 items handled in 0ce9dae — including note 2, implemented in-PR (I have node_modules, so I could verify the ordering you flagged as load-bearing).

Note 2 (settling was a convention AGENTS.md had to police) — renderWithMantineTransitions now owns the settle. It 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. The three explicit settleTransitions() calls in ViewHeader.test.tsx are gone; each render passes settleMs: HEADER_ANIM_MS + 200 instead (kept duration-derived, so your round-1 note-1 concern doesn't regress).

The ordering you correctly called load-bearing — the settle must run before setup.ts's cleanup() unmounts — I verified empirically two ways before committing: (1) a probe confirming a helper-module top-level afterEach runs after the test body, and (2) a probe rendering a marker and asserting the tree is still mounted inside that afterEach (document.body.textContent still contains it). Both pass. The mechanism is the same LIFO principle RTL's own auto-cleanup relies on: setup.ts is a setupFile, so its cleanup() afterEach registers first; the helper's afterEach registers at import/collection time (second), so it runs first. The auto-settle skips under vi.isFakeTimers() (no transitions test uses fake timers), and settleTransitions stays exported for the rare manual mid-body case. ViewHeader ran clean across repeated runs.

Note 1 (settle formula was duration-only) — The helper doc and the new settleMs option doc now state the window must exceed the longest JS timer chain — duration plus any enterDelay/exitDelay — plus rAF slack.

Nit — ViewHeader.test.tsx now says why HEADER_ANIM_MS is exported: the settle window must track the animation it settles.

npm run ci green end-to-end (the new afterEach is registered suite-wide but is a no-op — reads a null flag — for every non-transitions test, so the coverage gate and 3.5k-test unit suite pass unchanged).

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 44s —— View job

---

Review: PR #1793 — round 5 (0ce9dae8, helper-owned auto-settle)

• Read the round-4 diff (git diff d56e3429..HEAD) and the full PR diff
• Verify the new helper-registered afterEach ordering claim vs setup.ts cleanup
• Check the fake-timer skip, the arming lifecycle, and cross-test leakage
• Verify ViewHeader.test.tsx still asserts mid-flight state without explicit settles
• Post review

Verdict: this is the right final shape — moving the settle into the helper converts a convention into a mechanism, which is exactly what the original failure (an author forgetting a concurrent enter had no DOM signal) called for. Nothing blocking. Two notes worth acting on: one is a doc claim I think is now backwards, the other a durability guard for the ordering the whole thing rests on.

What I verified

• All three settleTransitions() call sites are gone and each renderWithMantineTransitions call now passes settleMs: TRANSITION_SETTLE_MS (ViewHeader.test.tsx:245, :318, :395). TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 is unchanged, so round-1 note 1 doesn't regress, and the test file now records why HEADER_ANIM_MS is exported (:17-18).
• renderWithMantineTransitions is still single-consumer (ViewHeader.test.tsx), so arming can't currently interact with anything else.
• The env="test" path is untouched: armedSettleMs stays null for every renderWithMantine test, so the afterEach is a null-flag read — the suite-wide registration is genuinely free, as you said.
• The fake-timer tests can't collide: both (:277, :419) use renderWithMantine, both restore in a finally, and neither arms the settle.
• RTL auto-cleanup isn't a third hook in the mix — the unit project has no globals: true (vite.config.ts:285-288), so @testing-library/react never registers its own afterEach(cleanup). That makes the ordering story exactly two hooks (setup.ts:94 and the helper's), which is what the new comment asserts. Good that the premise actually holds.
• Coverage-neutral: src/test/** isn't in the include whitelist, so the new ms !== null && !vi.isFakeTimers() branches aren't gated — same as when this lived in a test file.
• MantineTransitionsRenderOptions is exported alongside MantineRenderOptions, and the settleMs doc now carries the complete formula (duration + any enterDelay/exitDelay + rAF slack), closing round-4 note 1.

1. The AGENTS.md fake-timer claim looks backwards (worth fixing)

AGENTS.md now says a renderWithMantineTransitions test must not use fake timers because "the auto-settle skips under fake timers, so the leak would return." I don't think that follows. Under vi.useFakeTimers() Mantine's setTimeout and requestAnimationFrame are installed on the fake clock; vi.useRealTimers() (or teardown) discards that clock rather than draining it, so those callbacks never execute at all — which means there's nothing left to fire post-teardown. Fake timers plausibly prevent this leak class rather than expose it.

The real reason such a test is a problem is different and worth stating instead: the settle silently becomes a no-op, so anything the test genuinely left pending on the real clock (e.g. it flipped back to real timers mid-body) is unprotected, and the test is now relying on which clock happened to be installed at teardown. Since the whole value of this PR is the trail it leaves, I'd rather the stated reason be the one that holds. Fix this →

Related, same paragraph: the afterEach skips silently while the manual settleTransitions() throws an explanatory message. That asymmetry means the one future test most likely to get this wrong gets no signal, and the comment (renderWithMantine.tsx:119-122) frames the silence as deliberate ("keeps the guard from throwing during teardown"). A console.warn in that branch keeps the diagnostic without failing teardown — cheaper than the cross-file debugging session this PR was paying down.

2. The load-bearing hook ordering is unpinned and undefended

The auto-settle only works because the helper's afterEach runs before setup.ts:94's cleanup(). That's true today via after-hook LIFO, and your two probes confirm it empirically — but nothing in the repo holds it: grep sequence clients/web/vite.config.ts has no hits, so the behavior is inherited from Vitest's default. If a future config (or a Vitest default change) sets sequence.hooks: 'list', cleanup runs first and every settle drains against an unmounted tree; the #1760 leak comes back intermittently, in an unrelated file, with no failing test — the exact "a green run is non-evidence" trap this PR opened with.

Two cheap guards, complementary:

• Assert the invariant where it lives. Capture the render's container in renderWithMantineTransitions and, in the afterEach, check container.isConnected before settling; if it's already detached, throw "settle ran after cleanup() — afterEach ordering changed". That turns a silent regression into a self-explaining failure. (Honest limitation: under sequence.hooks: 'parallel' this catches the case where cleanup's hook is entered first, but not a race where the settle's first await yields and the synchronous cleanup() unmounts underneath it.)
• Pin it explicitly — sequence: { hooks: "stack" } on the unit project, with a one-line comment saying the auto-settle depends on it. Since it's load-bearing, explicit beats inherited.

Fix this →

3. Minor: arming is last-write-wins

armedSettleMs = settleMs (:68) overwrites. A test that renders two real-transitions trees — a long-animation component then a short one — settles for the shorter window and under-settles the first. armedSettleMs = Math.max(armedSettleMs ?? 0, settleMs) is a one-line fix that makes the flag mean "the longest window this test needs". No current test does this; it's a footgun, not a bug. Fix this →

Nit

The settle is now unconditional for every real-transitions render, including the two ViewHeader tests that already waitFor their exit to unmount — so all three pay the full 500ms. That's the same ~1.5s as before, so no regression, but it's worth knowing that settleMs: 0 is a working opt-out (ms !== null still holds, so you get the act flush without the wait) if a future test proves it drove everything to completion.

Verification caveat

This checkout has no node_modules and my WebFetch permission was declined, so I could not run the suite, read Vitest 4's sequence.hooks default at source, or confirm Sinon/Vitest's toFake list for requestAnimationFrame. Note 1 and note 2 both rest on those behaviors — note 1 is my reading of how fake clocks are discarded (worth a 2-minute check against your installed vitest before you act on it), and note 2 stands regardless of what the default is, since the point is that it isn't pinned. Everything under "What I verified" was checked against files on this branch.

Summary

Round 4 finished the job the earlier rounds set up: the settle is now owned by the helper that creates the hazard, ViewHeader.test.tsx reads cleanly, and the derived-duration invariant survived the move. I'd merge. Note 1 is a factual correction to a doc claim; note 2 is the guard I'd want on an invariant that's now enforced by hook-ordering luck rather than by anything the repo states; note 3 and the nit are optional.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Round-5 items handled in 905cbfe — including both guards from note 2, plus a mechanism correction I found while verifying.

Note 2 (load-bearing ordering unpinned/undefended) — Added both guards you suggested:

1. Runtime self-check. renderWithMantineTransitions captures the render container; the auto-settle afterEach throws a clear, named error if it's already detached (i.e. cleanup() ran first). That converts a silent ordering regression into a loud, self-explaining failure.
2. Pinned sequence.hooks: "stack" on the unit project with a comment.

While wiring these up I verified the actual ordering (I have node_modules), and it turns out the premise we were both reasoning from was off: the helper's afterEach runs before setup.ts's cleanup() in every sequence.hooks mode — stack, list, and parallel. I proved it by flipping the mode and confirming the container.isConnected guard never trips in any of the three (if cleanup ran first, it would). The real reason is that setupFile hooks are outer relative to the helper's import-registered inner hook, and vitest runs inner afterEach before outer regardless of sequence.hooks. So I corrected the comments: the ordering isn't "pinned by stack" (that would've been the same misleading-trail problem from earlier rounds) — stack is documented as defense-in-depth and the container.isConnected check is the real, mechanism-independent enforcement. Full 3533-test unit suite passes under stack.

Note 1 (fake-timer claim backwards) — You're right, and thanks for the correction. AGENTS.md no longer says the leak "would return" under fake timers; it now states the actual hazard (the auto-settle silently no-ops, so anything left on the real clock is unprotected and the test depends on which clock was installed at teardown), and notes that fake timers park Mantine's setTimeout/rAF on a clock that's discarded rather than drained. I also closed the asymmetry you flagged: the auto-settle now console.warns on the fake-timer skip (the manual settleTransitions throws; silence was the worst option for the one future test most likely to get this wrong).

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

Nit — Documented settleMs: 0 as a working opt-out (the act flush still runs, just no wait) for a test that provably drove every transition to completion itself.

npm run ci green end-to-end.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 11s —— View job

---

Review: PR #1793 — round 6 (905cbfe5)

• Read the round-5 diff (git diff 0ce9dae8..HEAD) and the full PR diff
• Verify the container.isConnected self-check and the sequence.hooks: "stack" pin
• Verify the corrected fake-timer doc + the console.warn asymmetry fix
• Verify Math.max arming and the settleMs: 0 opt-out
• Post review

Verdict: all three round-5 items landed, and the mechanism correction you found while wiring them up is the right instinct — documenting stack as defense-in-depth rather than as the guarantee is honest. Nothing blocking. One real finding: the self-check can't actually detect the failure mode it's advertised as catching in parallel mode, which makes both the "verified across stack/list/parallel" claim and the "defense-in-depth" framing weaker than stated. One line fixes it.

Round-5 items — confirmed

Item	Status
Note 2a — runtime self-check	Added (renderWithMantine.tsx:154-165). armedContainer captured at render (:77), checked before settling, with a self-explaining error naming the ordering assumption and the pin.
Note 2b — pin the ordering	sequence: { hooks: "stack" } on the unit project (vite.config.ts:306) with the rationale above it.
Note 1 — fake-timer claim	Corrected in AGENTS.md — now states the actual hazard (the settle silently no-ops, so real-clock work is unprotected and the test depends on which clock was installed at teardown) and explicitly notes fake timers park Mantine's timers on a discarded clock. Accurate.
Note 1b — silent-skip asymmetry	Closed: console.warn on the auto-settle skip (:146-153), manual settleTransitions still throws (:115-121).
Note 3 — last-write-wins arming	Math.max(armedSettleMs ?? 0, settleMs) (:72) with a comment saying why.
Nit — settleMs: 0 opt-out	Documented (:19-20), and it works: Math.max(null ?? 0, 0) === 0, ms === null is false, so the act flush runs with a zero-delay wait.

Also re-verified from the tree: renderWithMantineTransitions still has exactly one consumer file, all three call sites pass settleMs: TRANSITION_SETTLE_MS, no explicit settleTransitions() calls remain, TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 is intact, the two vi.useFakeTimers() tests (:277, :419) use renderWithMantine and restore in a finally, and src/test/** stays outside the coverage include so the new branches aren't gated.

1. The self-check is blind in parallel mode — which is also why the mode probe isn't evidence there

renderWithMantine.tsx:157 reads container.isConnected synchronously, before the first await:

if (container && !container.isConnected) { throw … }   // sync
await settleTransitions(ms);                            // first yield

Under sequence.hooks: "parallel", Vitest starts same-level afterEach callbacks concurrently. The helper's body therefore gets to run its sync check first (registration order for entry), passes it because nothing has unmounted yet, then yields at the await — at which point setup.ts's fully-synchronous cleanup() runs to completion and detaches the tree. The settle then drains against an unmounted tree and returns quietly.

So in parallel mode, "the guard never trips" is exactly what the lost race looks like, not evidence the ordering held. That undercuts two claims added in this commit:

• vite.config.ts:299-302 / AGENTS.md: "That ordering already holds in every sequence.hooks mode here … (verified across stack/list/parallel)" — for parallel the probe can't distinguish "ordering held" from "guard blind". (list is a real signal: cleanup registered first would run first and the guard would trip, so that one does tell you setupFile hooks are outer.)
• renderWithMantine.tsx:132-135: "The container.isConnected check below is the actual enforcement … regardless of what guarantees it." It's enforcement against sequential mis-ordering only.

The fix is one line — re-assert after the settle, where a concurrent cleanup() is observable:

await settleTransitions(ms);
if (container && !container.isConnected) throw new Error(/* …drained after unmount… */);

That makes the check mechanism-independent for real (catches list and the parallel race), and it's what lets the stack pin honestly be called defense-in-depth. Given this PR's whole thesis is "a green run is non-evidence," a guard that can't fail in the racy mode is worth closing. Fix this →

2. Nits

• armedContainer is last-write-wins while armedSettleMs is now max-wins (:72 vs :77). Harmless in practice — cleanup() detaches every mounted container at once, so checking the last one is a valid proxy for "cleanup ran" — but the asymmetry will read as an oversight next time someone edits this. A word on :91-92 saying why the most recent container suffices would settle it.
• The skip warning fires during teardown, after the test's result line is printed, so it appears detached from the test that caused it. Interpolating expect.getState().currentTestName into the message would make it directly actionable — cheap, and this warning exists precisely for someone who doesn't yet know what they did wrong.
• settleMs: 0 still costs a macrotask tick + an act flush per render, which is the point; worth knowing it isn't literally free.

Verification caveat

This checkout has no node_modules, so I could not run the suite or read Vitest 4 at source. Two things rest on that: (a) finding 1's premise that sequence.hooks: "parallel" starts same-level after-hooks concurrently (documented behavior, and consistent with the round-4 caveat we both flagged) — worth a 2-minute confirmation before you act on it; and (b) that sequence is an accepted project-level key rather than root-only, which would make the pin a silent no-op. Your green npm run ci typechecks vite.config.ts, so (b) is probably fine, but I couldn't confirm it against the installed types. Everything under "Round-5 items — confirmed" was verified against files on this branch. CI run 30211731234 is in progress as I write this.

Summary

Substance has been right since 5834cd24; rounds 4–5 turned the convention into a mechanism and this round added the guards. The one thing I'd change before merge is finding 1 — not because the fix is wrong (it isn't; stack is what actually runs, and under stack the ordering is correct and the settle does its job), but because the newly-added comments assert an enforcement strength the check doesn't have in the one mode where it matters. Post-settle re-check + a one-clause correction to the three comments and this is done.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Round-6 handled in 7cd00be — I took the post-await check you suggested, and in wiring it up nailed down the actual mechanism, which turns out to differ from what both of us were assuming.

Finding 1 (self-check blind in parallel) — Added the post-await re-check (both checks are now one assertConnectedForSettle(container, "before"|"after") helper). But I could then test your premise directly, and it doesn't hold here: setup.ts's cleanup() is a setupFile (outer) hook, not a same-level one. Vitest runs inner afterEach hooks to completion before outer ones, in every sequence.hooks mode — so cleanup() never races this settle. Two probes nail it down:

• ViewHeader's real-transitions tests pass under stack, list, AND parallel with neither check tripping. If the parallel race were real, the post-check would fire under parallel — it doesn't.
• A deliberately-adversarial probe — a same-suite afterEach(cleanup) registered in the test file (which is same-level as the helper) — trips the guard under both stack and parallel (the pre-check: "tree was unmounted before the settle — cleanup() ran entirely first"). So the checks are live, not dead code; they just can't be triggered by the outer cleanup the suite actually uses.

So sequence.hooks isn't load-bearing here after all (the inner-before-outer rule is what guarantees it), and I corrected all three comment sites (renderWithMantine.tsx, vite.config.ts, AGENTS.md) to say that rather than repeat the "parallel race" framing. The "stack" pin stays as documentation-of-intent / defense-in-depth, and the two container.isConnected checks are the real guard against a future regression that makes cleanup run before this settle finishes (e.g. someone moving cleanup to a same-level hook — exactly the case the probe shows the checks catch).

Nits — Documented why last-write-wins on armedContainer is fine (cleanup() detaches every mounted container at once, so the last one is a faithful "cleanup ran" proxy). The fake-timer skip console.warn now interpolates expect.getState().currentTestName, so it's actionable even though it fires at teardown.

On your verification caveat (b): sequence is accepted as a project-level key — the pin takes effect (the same-suite probe behaves identically under the explicit "stack" and Vitest's default, and typecheck/npm run ci are green). npm run ci green end-to-end.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 33s —— View job

---

Review: PR #1793 — round 7 (7cd00be0)

• Read the round-6 diff (git diff 905cbfe5..HEAD) and the full PR diff
• Verify the assertConnectedForSettle pre/post checks and the corrected mechanism claims
• Verify the currentTestName warning + the armedContainer last-write-wins rationale
• Re-check the arming lifecycle against how unmount() is used elsewhere in this suite
• Post review

Verdict: the post-await re-check landed, and the mechanism correction you found while wiring it is the right outcome — "cleanup() is an outer setupFile hook, so inner-before-outer holds in every mode; stack is documentation-of-intent" is both a better story than the one I proposed and honest about which probe proves what. Your list probe is the discriminating evidence (FIFO at the same level would trip the pre-check), so I accept the premise. Nothing blocking. One real finding — the guard's predicate is broader than its diagnosis — plus two nits.

Round-6 items — confirmed

Item	Status
Post-await re-check	Added — both checks funnel through assertConnectedForSettle(container, "before"|"after") (renderWithMantine.tsx:164-168, :175-189), with per-position wording ("entirely first" / "concurrently mid-settle").
Correct the "parallel race" framing at all three sites	Done, and consistently: renderWithMantine.tsx:129-144, vite.config.ts:293-305, AGENTS.md all now say the same thing — outer-setupFile ordering is the guarantee, stack is defense-in-depth only, the two isConnected checks are the enforcement. No site still claims sequence.hooks is load-bearing.
Adversarial probe is reported, not just the green one	Good call — recording that a same-suite afterEach(cleanup) does trip the pre-check is what makes these checks demonstrably live rather than unfalsifiable decoration. That's the piece my round-6 note was actually reaching for.
armedContainer last-write-wins rationale	Documented (:91-94) — cleanup detaches every mounted container at once, so the last is a faithful proxy. Correct.
currentTestName in the skip warning	Interpolated (:157).

Also re-verified from the tree: renderWithMantineTransitions still has exactly one consumer file, all three call sites pass settleMs: TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 (ViewHeader.test.tsx:20, :243, :316, :389), no explicit settleTransitions() calls remain, the two vi.useFakeTimers() tests use renderWithMantine and can't arm the settle, settleMs: 0 still works (Math.max(null ?? 0, 0) === 0, ms === null false → act flush, no wait), and src/test/** stays outside the coverage include so the new branches aren't gated.

1. The guard says "cleanup ran" but only knows "detached" — a test that unmounts itself gets a misdiagnosis (worth fixing)

assertConnectedForSettle fires on !container.isConnected, and the error asserts a specific cause: "setup.ts's cleanup() ran entirely first … the afterEach ordering it depends on broke." But detachment has a second, entirely legitimate cause: the test called the unmount() that renderWithMantineTransitions returns.

That isn't hypothetical for this codebase — unmount() is a routine pattern here (30+ test files use it). So the first future real-transitions test that asserts unmount behavior (e.g. "no throw when the header unmounts mid-crossfade" — squarely in this helper's problem domain) fails in afterEach with an error accusing the harness of an ordering regression that didn't happen. Debugging that costs exactly what this PR has been spending six rounds paying down.

The clean fix is to make arming reflect reality rather than widen the message: wrap the returned unmount so it disarms (clears armedSettleMs/armedContainer). That's also the correct behavior, not just quieter — once the test has unmounted, there's nothing left to settle against a live tree, and Mantine's useTransition unmount cleanup is the applicable mechanism. Minimum viable alternative: name the second cause in the message ("…or the test unmounted the tree itself, in which case disarm instead of settling"). Fix this →

2. Nits

• Arming is module-level, so it isn't concurrency-safe. With describe.concurrent / test.concurrent in a real-transitions file, two tests share armedSettleMs/armedContainer: the max-wins accumulation crosses test boundaries and the afterEach may check a container belonging to a different in-flight test — which would make the guard fire spuriously (the other test's tree got unmounted). Nothing does this today and I wouldn't add machinery for it; one clause on the state comment (:88-94) saying "assumes sequential tests — don't use with test.concurrent" would keep the next person from discovering it the hard way.
• expect.getState().currentTestName can be undefined, in which case the warning reads skipped for "undefined". A ?? "(unknown test)" keeps the message from looking broken in the one path that exists to be read by a confused author.
• Manual settleTransitions() doesn't disarm, so a test that settles mid-body still pays the full auto-settle at teardown. Correct-but-slow; harmless while there are no such tests.

3. On the documentation weight (optional, and a judgement call I'd leave to you)

The AGENTS.md bullet is now a single ~450-word paragraph covering the option, the settle contract, the fake-timer hazard, the hook-ordering mechanism, and the self-checks. Everything in it is accurate and hard-won — but the helper's own comments (renderWithMantine.tsx:98-144) now carry the same derivation at more depth, and AGENTS.md's job is the rule ("use renderWithMantine; use the transitions variant only for mid-flight assertions and pass a duration-derived settleMs"). Trimming the mechanism half to a pointer at the helper would make the rule findable again without losing the trail. Not a defect; just the point at which more prose stops buying more safety.

Verification caveat

This checkout has no node_modules, so I could not run the suite, read Vitest 4's setup-file hook registration at source, or confirm sequence is honored as a project-level key. The comments' central claim (setupFile afterEach is outer relative to an import-registered hook, in all three modes) rests on your probes; the "list" result is genuinely discriminating, so I'm treating it as established. Everything under "Round-6 items — confirmed" was verified against files on this branch. CI run 30212190994 is in progress as I write this.

Summary

Six rounds in, the mechanism is now stronger than the story that motivated it: the settle is owned by the helper that creates the hazard, the window is derived from the animation it settles, the fake-timer paths both signal, and the ordering invariant has a check that has been shown to fire when broken. I'd merge. Finding 1 is the only one I'd act on before doing so — a guard whose failure message names a cause it can't distinguish is the same category of misleading trail this PR spent rounds 2–4 removing from the comments.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Round-10 handled in c0a1383 — thanks for tracing finding 1 across all ten commits; that regression was mine (the round-7 trim) and you're right it's the worst place for it.

Finding 1 (AGENTS.md fake-timer clause regressed) — Restored the round-5 framing that the round-7 doc trim silently reverted. It no longer says "silently dropping the protection": "silently" was made false by round 5's console.warn, and "dropping the protection" reinstated the claim round 5 walked back. It now reads that the auto-settle no-ops under fake timers — it warns, but anything left on the real clock is then unprotected, so the test depends on which clock was installed at teardown. That matches the helper it points at.

Finding 3 (warning wording) — Matched the console.warn string to the same corrected framing, so AGENTS.md and the runtime warning now say the same thing (they're both read by someone who doesn't yet know the mechanism).

Finding 2 (drain-time throw could discard the liveness error) — Closed. The drain error is now captured too, and the throw order prefers the liveness error over the drain error — so the explanatory error always wins, even if act surfaces something during the settling render (the substitution round 9 prevented, one step further in). Restructured as capture-then-throw rather than throw-in-finally to avoid no-unsafe-finally. Verified: a bare mid-body cleanup() still throws the liveness error after the best-effort drain has run; eslint clean on the file; ViewHeader unchanged.

npm run ci green end-to-end.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 16s —— View job

---

Review: PR #1793 — round 11 (c0a13834)

• Read the round-10 diff (git diff e9bce8c2..HEAD) and the full PR diff
• Verify the AGENTS.md fake-timer clause + matching console.warn wording
• Verify the liveness-error-wins throw ordering (round-10 finding 2)
• Re-check the arming/guard lifecycle end to end
• Post review

Verdict: merge it. Round-10's finding landed, findings 2 and 3 were taken too, and I found no code defect. The two items below are both comment/diagnosability polish on a path nobody should hit — neither should hold the PR, and I'd be comfortable if you merged as-is.

Round-10 items — confirmed

Item	Status
Finding 1 — AGENTS.md fake-timer clause regressed	Fixed (AGENTS.md:208). Now reads "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." Both defects gone: "silently" (falsified by the round-5 console.warn) and "dropping the protection" (the framing round 5 walked back). Matches the helper it defers to.
Finding 3 — warning wording out of step	Matched (renderWithMantine.tsx:187-189): "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." AGENTS.md and the runtime warning now say the same thing.
Finding 2 — a drain-time throw could discard the liveness error	Closed (:204-224). captureLiveness() records, the drain runs in its own try, and the throw order prefers the liveness error. The no-unsafe-finally avoidance (capture-then-throw rather than throw-in-finally) is the right call.

Re-verified end to end against the tree:

• Lifecycle: resetArming() still precedes every early return and the drain, so nothing leaks between tests even if the settle throws. Empty liveness set → some() is false → no throw, so a test that unmounted its only tree drains cleanly. settleMs: 0 still reaches the act flush (Math.max(null ?? 0, 0) === 0; ms === null false).
• The drain's own fake-timer throw is unreachable from the afterEach — the vi.isFakeTimers() skip returns before it — so drainError really can only be an act-surfaced error, as the new comment claims.
• Consumers: renderWithMantineTransitions still has exactly one consumer file; all three call sites pass settleMs: TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 (ViewHeader.test.tsx:21, :243, :316, :389). The four header Transitions all use duration/exitDuration={HEADER_ANIM_MS} with no enterDelay/exitDelay (ViewHeader.tsx:322-397), so the window still strictly bounds the JS chain.
• TOGGLE_ANIM_MS = 200 still can't bite: the only two tests that pass monitorToggle (:137, :174) use renderWithMantine, so that Transition never mounts under real timers — and 200 < 300 regardless.
• Fake-timer tests can't arm: :277 and :419 both render via renderWithMantine and restore in a finally.
• The vi static import can't escape the test graph: the only importers of renderWithMantine are *.test.tsx files (src/test/setup.ts's hit is a comment), so nothing reaches the Storybook browser project or a vite build. No test hand-rolls a MantineProvider (only match is a comment at ContentViewer.test.tsx:221).
• Coverage-neutral: the include whitelist (vite.config.ts:180-195 — components/hooks/theme/lib/utils/server + core/*) excludes src/test/**, so the new try/catch branches aren't gated. sequence is inside the unit project's test object (:305), so the pin is real, not a silently-ignored root key.

1. The discarded drainError is now a total loss (optional)

:223-224 throws the liveness error and drops drainError on the floor. Round 10 asked that a drain throw not substitute for the explanatory error; the fix substitutes in the other direction with no residue — so an act-surfaced component error during the settle vanishes entirely on that path. Attaching it ({ cause: drainError }) or a one-line console.error before the throw keeps both facts without changing which error fails the test. Reaching this needs a liveness failure and a drain throw simultaneously, so this is completeness, not a bug. Fix this →

2. The guard's error message routes the reader to the pin that rounds 6–7 established isn't load-bearing (optional)

:242-244 tells whoever trips the check to "see the ordering note here and the sequence.hooks: "stack" pin in vite.config.ts" — but the file's own comments (:163-164) and vite.config.ts:293-304 both say the pin is defense-in-depth and not what guarantees the ordering. So the one person who ever reads this message is pointed first at the thing that can't be the cause. The actual reachable cause is named 20 lines up (:167 — cleanup moved to a same-level hook, the case your round-6 probe showed does trip this); folding that into the message instead of the pin pointer would route them correctly. Same comment-consistency class as round 10's finding, one step deeper into the diagnostic path. Fix this →

Nit

livenessError ??= error (:209) is belt-and-braces given captureLiveness("after") only runs when livenessError === undefined. Harmless, and arguably the safer form if that guard ever moves.

Verification caveat

This checkout has no node_modules, so I could not run the suite or confirm vi.isFakeTimers() / project-level sequence against installed types. The hook-ordering claims rest on your rounds 5–7 probes (the list result and the same-suite afterEach(cleanup) probe remain the discriminating evidence). Everything in the confirmation section was verified against files on this branch. CI run 30213547440 is in progress as I write this.

Summary

Ten rounds in, this is finished work and the trail is consistent: helper-owned settle, duration-derived window, per-container arming so multi-render and self-unmount compose, both fake-timer paths signalling the same corrected framing, liveness checks demonstrated to fire when broken, and a drain that can't be skipped by its own guard nor have its explanatory error stolen. Both notes above are optional polish on an error path that only fires if someone breaks the harness.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Round-11 handled in 702c541 — both optional items taken.

Finding 1 (discarded drainError) — When both a liveness error and a drain throw occur, the drain error is now attached as the liveness error's cause (guarded on livenessError instanceof Error), so an act-surfaced component error from the settling render is preserved rather than lost — without changing which error fails the test.

Finding 2 (message routed to the non-causal pin) — Reworded the guard's error to name the actually-reachable cause first: cleanup having been moved to a same-level afterEach instead of the setup.ts setupFile (outer) hook, which is what your round-6 probe showed does trip this. The sequence.hooks pin is demoted to a parenthetical labelled defense-in-depth, so a maintainer who trips the check isn't sent to investigate a setting the file itself says isn't causal.

On the nit: left livenessError ??= error as the nullish-assign — agreed it's the safer form if the === undefined guard on the post-check ever moves.

Verified: a bare mid-body cleanup() throws the reworded message; eslint clean; ViewHeader unchanged. npm run ci green end-to-end.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 1s —— View job

---

Review: PR #1793 — round 12 (702c5416)

• Read the round-11 diff (git diff c0a13834..HEAD) and the full PR diff
• Verify the cause attachment + reworded guard message
• Re-check the arming/guard lifecycle and consumers end to end
• Post review

Verdict: merge it. Both round-11 items landed and I found no code defect — the runtime behavior of this PR has been settled for several rounds and nothing in this commit touches it. One small note, and it's the same category the trail has been converging on: the reworded message is clearer but slightly less hedged than the check can support. Optional; I'd be comfortable merging as-is.

Round-11 items — confirmed

Item	Status
Finding 1 — drainError was a total loss	Fixed (renderWithMantine.tsx:225-230). The drain error is attached as the liveness error's cause, guarded on instanceof Error, and the liveness error still wins — so which error fails the test is unchanged. assertLiveContainersConnected constructs its Error without a cause, so the assignment can't clobber anything.
Finding 2 — message routed to the non-causal pin	Reworded (:243-257). The sequence.hooks pin is now a parenthetical explicitly labelled defense-in-depth, and the same-level-afterEach case is named as the likely cause — consistent with :155-172 and vite.config.ts:293-304.
Nit — livenessError ??= error	Left as the nullish-assign, as discussed. Agreed.

Re-verified end to end against the tree:

• Lifecycle intact: resetArming() still precedes every early return and the drain, so nothing leaks between tests even if the settle throws. Empty liveness set → some() is false → no throw, so a test that unmounted its only tree drains cleanly. settleMs: 0 still reaches the act flush (Math.max(null ?? 0, 0) === 0; ms === null false).
• Consumers unchanged: renderWithMantineTransitions still has exactly one consumer file, and all three call sites pass settleMs: TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 (ViewHeader.test.tsx:21, :243, :316, :389). No settleTransitions() call sites remain; no test hand-rolls a MantineProvider.
• Window still bounds the JS chain: the four header Transitions use duration/exitDuration={HEADER_ANIM_MS} with no enterDelay/exitDelay, so 300 + 200 strictly exceeds one rAF pair + the terminal 300ms timeout. TOGGLE_ANIM_MS = 200 is still unreachable under real timers (the two tests that pass monitorToggle use renderWithMantine) and is under 300 anyway.
• Fake-timer tests can't arm (ViewHeader.test.tsx:277, :419 both render via renderWithMantine, both restore in a finally), and the drain's own fake-timer throw stays unreachable from the afterEach behind the vi.isFakeTimers() skip — so drainError really can only be an act-surfaced error, as the comment claims.
• Coverage-neutral: src/test/** is outside the include whitelist (vite.config.ts:180-195), so the new instanceof Error branch isn't gated. sequence is inside the unit project's test object (:305), so the pin is real rather than a silently-ignored root key.

1. The reworded message dropped the hedge on a cause the check can't observe (optional)

Round 11 turned

Expected cause: setup.ts's cleanup() ran entirely first

into

— setup.ts's cleanup() ran entirely first, so the settle drained nothing…

The check observes exactly one fact: a container the test left mounted is no longer connected. It cannot distinguish setup.ts's cleanup() from a bare mid-body cleanup() (the only route that's actually reachable today — the one your round-8 probe exercises) or from manual DOM surgery. So the sentence now opens by asserting the hypothetical cause as fact, promotes the other hypothetical (cleanup moved to a same-level hook) to "the likely cause", and leaves the reachable one to the closing sentence. The old Expected cause: prefix carried exactly the hedge that's now missing.

This is minor and it's within the message this commit improved — but it's the same pattern rounds 2–4 spent removing from the comments (asserting a mechanism the evidence doesn't pin down), so it's worth not reintroducing at the one place a maintainer reads when the harness breaks. Stating the observation first, then the candidates, costs a clause. Fix this →

2. Nits

• The cause only pays off if the reporter prints it. Vitest surfaces error.cause in its diff output, so this should be visible — but I can't confirm against installed types here, and if it ever isn't, the preserved drain error is invisible rather than lost. If you want belt-and-braces, a console.error(drainError) immediately before the throw guarantees it's readable regardless of reporter behavior. Not worth another round on its own.
• livenessError instanceof Error is unreachable-false today (the only producer throws an Error), so the else branch silently drops drainError. Correct as defensive code; just noting the drop is silent if that ever changes.

Verification caveat

This checkout has no node_modules, so I could not run the suite or confirm vi.isFakeTimers(), project-level sequence, or Vitest's cause rendering against installed versions. The hook-ordering claims rest on your rounds 5–7 probes (the list result and the same-suite afterEach(cleanup) probe remain the discriminating evidence). Everything in the confirmation section was verified against files on this branch.

Summary

Eleven rounds in, this is done: helper-owned settle, a window derived from the animation it settles, per-container arming so multi-render and self-unmount compose, both fake-timer paths signalling the same corrected framing, liveness checks demonstrated to fire when broken, a drain that can't be skipped by its own guard, and now an explanatory error that can't be stolen or swallow the drain's. The one note above is a clause of hedging in an error string that only fires if someone breaks the harness — take it or merge without it.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Round-12 handled in fd71da2.

Finding 1 (message asserted a cause the check can't observe) — Reworded so it states the observation first — a still-mounted armed tree was detached before/while the settle ran, so the settle drained nothing and #1760 is reopened — then lists candidate causes without asserting one: (a) a bare mid-body cleanup() in the test (the reachable case today, with the returned unmount() as the fix), or (b) cleanup() moved off setup.ts's setupFile hook to a same-level afterEach. The sequence.hooks pin stays a parenthetical labelled defense-in-depth. That drops the "assert a mechanism the evidence doesn't pin down" pattern the earlier rounds removed from the comments.

Nits 2a/2b — When both a liveness error and a drain error fire, the drain error is now console.error'd in addition to being attached as cause, so it's readable regardless of whether the reporter renders error.cause, and it's no longer silently dropped even in the (currently unreachable) case where the liveness error isn't an Error.

Verified: a bare mid-body cleanup() throws the reworded observation-first message; eslint clean; ViewHeader unchanged. npm run ci green end-to-end.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 38s —— View job

---

Review: PR #1793 — round 13 (fd71da2e)

• Read the round-12 diff (git diff 702c5416..HEAD) and the full PR diff
• Verify the observation-first guard message
• Verify the console.error + cause preservation of the drain error
• Re-check the arming/guard lifecycle and consumers
• Post review

Verdict: merge it. Both round-12 items landed, the diff is confined to one file (renderWithMantine.tsx, +24/−14) and touches nothing that runs on a passing test — no code defect. Two items below, both inside the strings and comments this commit rewrote; the first is the same "asserts more than the code guarantees" class the trail has been closing, so I'd take it if you're already touching the file, and merge without it otherwise.

Round-12 items — confirmed

Item	Status
Finding 1 — message asserted a cause the check can't observe	Fixed (:252-267). Now states the observation ("a still-mounted armed tree was detached before the settle started / while the settle was in flight"), then lists candidates (a) bare mid-body cleanup() — the reachable case, with the returned unmount() as the fix — and (b) cleanup() moved off the setupFile hook, with the sequence.hooks pin demoted to a parenthetical labelled defense-in-depth. No single cause asserted; matches :155-172 and vite.config.ts:293-304.
Nit 2a — cause only pays off if the reporter prints it	Closed (:232-236): console.error alongside the cause, so the drain error is readable regardless of reporter behavior.
Nit 2b — silent drop when livenessError isn't an Error	Closed: the log now sits outside the instanceof check, so the drain error survives on that (currently unreachable) path too.

Re-verified end to end against the tree:

• Lifecycle intact: resetArming() still precedes every early return and the drain, so nothing leaks between tests even if the settle throws. Empty liveness set → some() is false → no throw, so a test that unmounted its only tree drains cleanly. settleMs: 0 still reaches the act flush (Math.max(null ?? 0, 0) === 0; ms === null false).
• Consumers unchanged: renderWithMantineTransitions still has exactly one consumer file, all three call sites pass settleMs: TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 (ViewHeader.test.tsx:243, :316, :389), no settleTransitions() call sites remain, and the AGENTS.md bullet still matches the helper (fake-timer clause carries the round-10 correction; the unmount() clause says "drops that tree from the settle's liveness check, while still draining").
• The new console.error can't fire on a green run: it needs a liveness failure and a drain throw simultaneously, and the drain's own fake-timer throw is unreachable from the afterEach behind the vi.isFakeTimers() skip. So no new console noise in the suite (AGENTS.md's "suppress expected error output" rule isn't engaged).
• Coverage-neutral: src/test/** is outside the include whitelist (vite.config.ts:180-195), so the new branch isn't gated.

1. The log message asserts the cause attachment that the line above makes conditional (optional)

renderWithMantine.tsx:231-236:

if (livenessError instanceof Error) livenessError.cause = drainError;   // conditional
console.error(
  "…the drain also threw (reported as the liveness error's cause):",    // unconditional
  drainError,
);

Nit 2b was fixed by hoisting the log out of the instanceof guard — correct — but the message text still claims the cause was attached. On the non-Error path (the one the hoist exists to serve) the reader is told to look for a cause that isn't there. Unreachable today (assertLiveContainersConnected only throws an Error), so this is wording, not behavior — but it's the same pattern round 12 removed from the guard string one commit ago, in the same file, and the defensive branch exists precisely for the case where that producer changes. Dropping the parenthetical (or moving it inside the instanceof branch) makes the message true on both paths. Fix this →

2. Nits

• The comment block at :220-224 is now a stale duplicate of the one it introduced at :227-230. Two of its three sentences are restated near-verbatim 5 lines down, and the inner copy is the more accurate one (it mentions the log and the instanceof conditional; the outer says flatly "Preserve the drain error as its cause"). Trimming the outer to just the priority rationale — "prefer the liveness error; a drain-time throw is a downstream symptom" — leaves one description of the mechanism instead of two that differ. This is the "append prose" drift you and I agreed to avoid a few rounds back; cheapest place to correct it is while finding 1 is open.
• "so the settle drained nothing" is exact for when === "before" but overstated for "after" — a tree detached mid-settle was live for part of the drain. One word ("drained against a dead tree", which the function's own comment at :245 already uses) covers both.
• "see the ordering note here" inside a thrown string reads oddly out of context; the reader has the stack, so it resolves, but naming renderWithMantine.tsx would make it self-contained.

Verification caveat

This checkout has no node_modules, so I could not run the suite or confirm vi.isFakeTimers(), project-level sequence, or Vitest's cause rendering against installed versions. The hook-ordering claims rest on your rounds 5–7 probes (the "list" result and the same-suite afterEach(cleanup) probe remain the discriminating evidence). Everything in the confirmation section was verified against files on this branch.

Summary

Twelve rounds in and the last three have been polishing strings on an error path that only fires if someone breaks the harness — which is a good sign the substance is done. The mechanism stands: helper-owned settle, window derived from the animation it settles, per-container arming so multi-render and self-unmount compose, both fake-timer paths signalling the same framing, liveness checks demonstrated to fire when broken, a drain that can't be skipped by its own guard, and an explanatory error that can neither be stolen nor swallow the drain's. Take finding 1 or don't — either way this is ready.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Round-13 handled in 6596a6d — all string/comment polish.

• Finding 1 — Dropped the "reported as the liveness error's cause" clause from the console.error, since the cause attachment is conditional on instanceof Error; the log now just reports that the drain also threw, true on both paths (including the non-Error path the hoisted log exists to cover).
• Nit (duplicated comment) — Trimmed the outer comment to just the priority rationale ("prefer the liveness error; a drain throw is a downstream symptom"); the inner comment — which mentions the log and the instanceof conditional — is the accurate single description now.
• Nit ("drained nothing") — Guard message now says "drained against a dead tree", exact for both the before-settle and during-settle detach (a mid-settle detach was live for part of the drain).
• Nit ("see the ordering note here") — Now names renderWithMantine.tsx, so the thrown message is self-contained out of context.

eslint clean; ViewHeader unchanged; npm run ci green end-to-end.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 4m 23s —— View job

---

Review: PR #1793 — round 14 (6596a6d5)

• Read the round-13 diff (git diff fd71da2e..HEAD) and the full PR diff
• Verify the four polish items landed
• Re-check the arming/guard lifecycle and consumers
• Post review

Verdict: merge it. The round-13 diff is confined to comments and one thrown string in renderWithMantine.tsx (+14/−17) — nothing that executes on a passing test — and all four items landed correctly. No code defect. One new note below on a factual claim in a comment this PR added (not this commit), and one nit; neither should hold the PR.

Round-13 items — confirmed

Item	Status
Finding 1 — log claimed a cause the line above only conditionally sets	Fixed (:230-233). The message is now just "the drain also threw:", true on both the instanceof Error and non-Error paths; the inner comment (:224-228) states the conditional attachment explicitly.
Nit — duplicated comment block	Trimmed (:220-221 is now the priority rationale only); the inner comment is the single description of the preservation mechanism.
Nit — "drained nothing" overstated for the "after" case	Now "drained against a dead tree" (:256), which matches the function's own doc at :242 and is exact for both positions.
Nit — "see the ordering note here"	Self-contained: names renderWithMantine.tsx (:262-263).

Re-verified end to end against the tree (not taken on trust):

• Lifecycle intact. resetArming() still precedes every early return and the drain (:176), so state can't leak between tests even if the settle throws. Empty liveness set → some() is false → no throw, so a test that unmounted its only tree drains cleanly. settleMs: 0 still reaches the act flush (Math.max(null ?? 0, 0) === 0; ms === null false).
• Consumers unchanged. renderWithMantineTransitions still has exactly one consumer file; all three call sites pass settleMs: TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 (ViewHeader.test.tsx:21, :245, :318, :395); the two vi.useFakeTimers() tests (:277, :419) render via renderWithMantine so they can't arm; no settleTransitions() call sites remain.
• The vi/afterEach module scope still can't escape the test graph. The only non-test importer of renderWithMantine is src/test/setup.ts — and that's a comment, not an import. No *.stories.tsx imports it, so the top-level afterEach and the vitest import never reach the Storybook browser project or a vite build.
• New console output can't fire on a green run. The console.error needs a liveness failure and a drain throw simultaneously; the drain's own fake-timer throw is unreachable from the afterEach behind the vi.isFakeTimers() skip. So no new noise in the suite, and AGENTS.md's "suppress expected error output" rule isn't engaged.
• Coverage-neutral. src/test/** is outside the include whitelist (vite.config.ts:180-195 — components/hooks/theme/lib/utils/server + core/*), so none of the new branches are gated. HEADER_ANIM_MS's export doesn't change ViewHeader.tsx's coverage, and reactRefresh.configs.vite's allowConstantExport keeps it lint-clean.

1. The sequence.hooks comment asserts a Vitest default that I believe changed (worth a 30-second check)

vite.config.ts:293-294: "Run after-hooks LIFO (reverse registration) instead of Vitest's default "parallel"."

I'm fairly sure sequence.hooks defaulted to "parallel" only through Vitest 1.x/2.x and was changed to "stack" in Vitest 3 — this repo is on vitest ^4.1.0. If that's right, the pin isn't changing anything, and the comment's framing ("instead of the default") misdescribes what the line does: it would be documenting the default rather than overriding it, which is still a fine reason to keep it but a different one from what's written.

I can't confirm — this checkout has no node_modules — so treat this as a check, not a finding. You have the install: grep -rn "hooks" node_modules/vitest/dist/chunks/*.js around the config defaults, or just delete the line and see whether the same-suite afterEach(cleanup) probe behaves identically. Worth resolving because it's the one remaining comment in this PR asserting third-party behavior I couldn't verify, and the whole trail has been about not leaving claims the next reader will build on. Fix this →

2. Nit — the post-settle check is unreachable under the pin it sits next to

renderWithMantine.tsx:168-170 justifies the post-settle check as catching "a same-level 'parallel' race the pre-check can't see" — but with sequence.hooks: "stack" pinned on the unit project, same-level hooks run sequentially, so that race can't occur in this repo's configuration. It's correct as defensive code (the comment does frame it as guarding a future regression), and it costs nothing; just noting that the two additions land in slightly opposed positions — the pin removes the only mode in which the second check has a job. If finding 1 resolves to "the default is already stack", one clause covering both ("the post-check matters only if someone unpins this to parallel") would tie them together.

Verification caveat

This checkout has no node_modules, so I could not run the suite or confirm vi.isFakeTimers(), Vitest 4's sequence.hooks default, project-level sequence acceptance, or Vitest's cause rendering against installed versions. Finding 1 rests entirely on that gap — flagged as a check, not a claim. The hook-ordering mechanism rests on your rounds 5–7 probes (the "list" result and the same-suite afterEach(cleanup) probe remain the discriminating evidence). Everything in the confirmation section was verified against files on this branch.

Summary

Thirteen rounds in, and the last four have been narrowing to wording inside an error path that only fires if someone breaks the harness — which is the signal that the substance is finished. What ships: a helper-owned settle armed by the render that creates the hazard, a window derived from the animation it settles, per-container arming so multi-render and self-unmount compose, both fake-timer paths signalling the same corrected framing, liveness checks demonstrated to fire when broken, a drain that can neither be skipped by its own guard nor lose its explanatory error, a matchMedia override that no longer claims two contradictory preferences, and an AGENTS.md bullet that's a rule with the mechanism on the helper. Finding 1 is a 30-second check on a third-party default; the nit is optional. Ready to merge.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Round-14 handled in 661c963 — finding 1 was a good catch and you were right.

Finding 1 (sequence.hooks default) — Verified against the installed vitest 4.1.0: resolved.sequence.hooks ??= "stack" (in the coverage chunk), so the runtime default is "stack". The default: "parallel" in vitest's own CLI help-text is stale. Corrected the vite.config.ts comment: the pin doesn't override a "parallel" default — it makes Vitest 4's existing "stack" default explicit (documents intent + guards a future default change), which is a truer reason to keep the line. (My earlier "default parallel" belief came from that stale help-text; the mechanism conclusion is unaffected, since my probes flipped sequence.hooks explicitly and showed cleanup-is-outer holds in all three modes regardless.)

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

ViewHeader unchanged; npm run ci green end-to-end.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 31s —— View job

---

Review: PR #1793 — round 15 (661c963c)

• Read the round-14 diff (git diff 6596a6d5..HEAD) and the full PR diff
• Verify the sequence.hooks default correction + the pin/post-check tie-in
• Re-check the arming/guard lifecycle and consumers
• Post review

Verdict: merge it. Round-14 is a 2-file, comments-only commit (renderWithMantine.tsx +5/−3, vite.config.ts +14/−12) — nothing that executes on a passing test. Both items landed. No code defect in the diff. One note below on an implicit config dependency of the mechanism that this PR documents everything else about, and one wording nit; neither should hold the PR.

Round-14 items — confirmed

Item	Status
Finding 1 — sequence.hooks default claim	Corrected (vite.config.ts:293-296). The comment now says the pin makes Vitest 4's existing "stack" default explicit (documenting intent + guarding a future default change) rather than overriding a "parallel" default, and cites the resolution site (resolved.sequence.hooks ??= "stack") plus the stale help-text as the source of the old belief. That's a truer reason to keep the line, and it doesn't disturb the mechanism conclusion — your probes flipped the mode explicitly, so the outer-cleanup finding stands independent of what the default is. I still can't verify against installed vitest (no node_modules here); accepting your quoted source line.
Nit 2 — pin and post-settle check sat in opposed positions	Tied together (renderWithMantine.tsx:169-172): the post-check's race is now scoped to "only if sequence.hooks is changed to parallel", with the "stack"/"list" sequential-hook fact stated. The two additions now reference each other.

Re-verified end to end against the tree:

• Lifecycle intact. resetArming() still precedes every early return and the drain (:178); empty liveness set → some() is false → no throw, so a test that unmounted its only tree drains cleanly; settleMs: 0 still reaches the act flush (Math.max(null ?? 0, 0) === 0; ms === null false).
• Consumers unchanged. renderWithMantineTransitions still has exactly one consumer file, and all three call sites pass settleMs: TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 (ViewHeader.test.tsx:21, :243, :316, :389). No settleTransitions() call sites remain.
• Window still bounds the JS chain. All four header Transitions use duration/exitDuration={HEADER_ANIM_MS} (ViewHeader.tsx:322/356/381/396) with no enterDelay/exitDelay, so 300 + 200 strictly exceeds one rAF pair + the terminal 300ms timeout. TOGGLE_ANIM_MS = 200 (:439) is still unreachable under real timers (the tests passing monitorToggle use renderWithMantine) and is under 300 anyway.
• setup.ts and AGENTS.md are unchanged this round and still consistent: the matchMedia block is described as an override (not a polyfill), the reduce-anchored regex is in place, the respectReducedMotion/env="test" clarification stands, and the AGENTS.md bullet keeps the round-10 fake-timer wording ("it warns, but anything the test left pending on the real clock is then unprotected").
• Coverage-neutral. src/test/** is outside the include whitelist (vite.config.ts:180-195), and this commit adds no code anyway.

1. The auto-settle silently depends on isolate: true, which is nowhere stated (worth a line)

The settle is registered by a module-level afterEach in renderWithMantine.tsx:175. That hook attaches to whichever suite is collecting when the module first evaluates — i.e. the importing test file. It works today because nothing in the config sets isolate (I grepped clients/web/vite.config.ts, package.json, and vitest.shared.mts — no hits), so Vitest's default isolate: true gives every test file a fresh module registry and the hook re-registers per file.

Set isolate: false — a routine speed optimization someone will eventually try — and the module is evaluated once per worker. The afterEach then registers onto only the first file that imported it, and every other file's real-transitions renders arm armedSettleMs/liveArmedContainers with no hook to consume them. Consequences, in order of nastiness:

1. The drain silently never runs for those files → test(web): flaky coverage gate — Mantine Transition timer leaks 'window is not defined' after teardown (#1760 class) #1786 is back, with no guard trip (the liveness checks live inside the hook that isn't registered).
2. The module state leaks across files in that worker, so a stale container from file B can be consumed by a later test in file A and fail its isConnected check — a guard trip attributed to the wrong file.

Both are exactly the "green run is non-evidence / misattributed failure" shape this PR opened with, and this is the last load-bearing implicit config assumption left undocumented after rounds 5–7 nailed down the ordering one. Cheapest fix is a clause where the arming state is described (:105-115): "registration is per test-file module evaluation, so this assumes isolate: true (the default) — under isolate: false the hook would attach to only the first importing file." The structural alternative is arming via onTestFinished inside renderWithMantineTransitions (per-test, immune to module caching) — but that changes the ordering guarantee the file spent three rounds establishing, so I would not do it here; document the assumption instead. Fix this →

2. Nit — the new clause attributes the pre-check's blindness to the wrong cause

renderWithMantine.tsx:169-172: "…the post-settle check catches it detaching the tree concurrently while this hook awaits — a race that can only arise if the unit project's sequence.hooks is changed from "stack" to "parallel" … which is exactly why the pre-check can't see it."

The trailing clause doesn't follow. The pre-check can't see a concurrent detach because it runs synchronously before the await — that's a property of where it sits in the function, not of the hook mode. As written, a reader could conclude that flipping back to "stack" restores the pre-check's visibility. Also, "can only arise if sequence.hooks is parallel" is necessary-but-not-sufficient: it also needs cleanup moved to a same-level hook (stated earlier in the same sentence, so the compound is there — just easy to lose). Splitting it — "the pre-check runs before the await, so it can't observe a detach that happens mid-settle; that concurrent case additionally requires sequence.hooks: "parallel", since same-level hooks run sequentially under "stack"/"list"" — says both facts without the false attribution. Fix this →

Verification caveat

This checkout has no node_modules, so I could not run the suite or confirm Vitest 4's sequence.hooks default, vi.isFakeTimers(), project-level sequence acceptance, or cause rendering against installed versions. Round-14's central claim rests on your quoted resolved.sequence.hooks ??= "stack"; the hook-ordering mechanism rests on your rounds 5–7 probes (the "list" result and the same-suite afterEach(cleanup) probe remain the discriminating evidence). Finding 1 is derived from the absence of any isolate setting in the config files plus how module-level hooks bind at collection time — worth confirming with a isolate: false probe if you act on it beyond a comment.

Summary

Fourteen rounds in and the last five have been narrowing to wording inside an error path that only fires if someone breaks the harness — the substance has been settled since 5834cd24. What ships: a helper-owned settle armed by the render that creates the hazard, a window derived from the animation it settles, per-container arming so multi-render and self-unmount compose, both fake-timer paths signalling the same corrected framing, liveness checks demonstrated to fire when broken, a drain that can neither be skipped by its own guard nor lose its explanatory error, a matchMedia override that no longer claims two contradictory preferences, a sequence.hooks pin that now describes itself accurately, and an AGENTS.md bullet that's a rule with the mechanism on the helper. Finding 1 is the one I'd add a line for — it's the last implicit config dependency the file doesn't mention, and unlike the ordering one its failure mode is silent. The nit is optional.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Round-15 handled in a99df04 — finding 1 was a real one, and I reproduced it before documenting.

Finding 1 (implicit isolate: true dependency) — Confirmed empirically: vitest --no-isolate --pool=threads --no-file-parallelism over two real-transitions files leaves one file's arming unconsumed — its bare cleanup() does not trip the guard, exactly the silent leak you predicted (the module-level afterEach binds to only the first-collected file). Documented it on the arming-state comment: the mechanism assumes isolate: true (Vitest's default, which nothing here overrides), spelled out the isolate: false failure mode (drain silently never runs → #1760/#1786 back with no guard trip; state leaks across files), noted the probe, and pointed at onTestFinished-inside-the-helper as the structural alternative if isolation is ever disabled — with the caveat that its ordering vs cleanup() would need re-establishing. Comment-only, as you advised; didn't restructure the hook.

Nit 2 (pre-check blindness attributed to the wrong cause) — Reworded. The pre-check can't see a mid-settle detach because it runs synchronously before the await (a property of where it sits, not the hook mode); the concurrent case additionally needs sequence.hooks: "parallel" since same-level hooks run sequentially under "stack"/"list". The false "which is why the pre-check can't see it" attribution is gone.

ViewHeader unchanged; npm run ci green end-to-end.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 3m 7s —— View job

---

Review: PR #1793 — round 16 (a99df04d)

• Read the round-15 diff (git diff 661c963c..HEAD) and the full PR diff
• Verify the isolate: true documentation + the pre-check-blindness rewording
• Re-check the arming/guard lifecycle, consumers, and config placement
• Look for a new failure class (not just re-confirm the trail)
• Post review

Verdict: merge it. No findings. Round-15 is a single-file, comments-only commit (renderWithMantine.tsx +21/−6) with nothing that executes on a passing test, and both items landed correctly. I went looking for a new class of problem rather than re-walking the trail — the one candidate I found doesn't apply (details below). I'd stop the review loop here: sixteen rounds is well past the point of diminishing returns, and the remaining surface is prose in an error path that only fires if someone breaks the harness.

Round-15 items — confirmed

Item	Status
Finding 1 — undocumented isolate: true dependency	Documented (renderWithMantine.tsx:113-130), and better than I asked for: it states the assumption, the isolate: false failure mode (hook binds to the first importing file → other files arm with nothing to consume them → drain silently never runs, plus cross-file state leakage), the probe that reproduces it (vitest --no-isolate --pool=threads --no-file-parallelism over two real-transitions files), and the structural escape (onTestFinished) with the caveat that its ordering vs cleanup() would need re-establishing. Recording the repro is what makes this a fact rather than a caution. I re-grepped clients/web/vite.config.ts, vitest.shared.mts, and clients/web/package.json for isolate / pool / fileParallelism / poolOptions — zero hits, so "nothing overrides it" is accurate.
Nit 2 — pre-check blindness attributed to the hook mode	Reworded (:184-189). The two facts are now separated correctly: the pre-check can't see a mid-settle detach because it runs synchronously before the await (structural), and the concurrent case additionally requires sequence.hooks: "parallel" (same-level hooks run sequentially under "stack"/"list"). The false "which is exactly why the pre-check can't see it" is gone.

Re-verified against the tree

• Consumers: renderWithMantineTransitions still has exactly one consumer file, and all three call sites pass settleMs: TRANSITION_SETTLE_MS = HEADER_ANIM_MS + 200 (ViewHeader.test.tsx:21, :243, :316, :389). No settleTransitions() call sites. Of the 121 files importing the helper, the only two non-*.test.tsx hits are the helper itself and a comment in setup.ts:69 — so the module-level afterEach and the vitest import can't reach the Storybook browser project, the node-env integration project (zero importers under src/test/integration), or a vite build.
• Lifecycle: resetArming() still precedes every early return and the drain (:195), so nothing leaks between tests even if the settle throws. Empty liveness set → some() is false → no throw. settleMs: 0 still reaches the act flush (Math.max(null ?? 0, 0) === 0; ms === null false).
• Window still bounds the JS chain: HEADER_ANIM_MS = 300 is the duration/exitDuration on all four header Transitions with no enterDelay/exitDelay, so 300 + 200 strictly exceeds one rAF pair + the terminal timeout.
• Fake-timer tests can't arm: :277 and :419 both render via renderWithMantine and restore in a finally (:307, :447), so a failure inside one can't leak a fake clock into a settling test.
• sequence placement: inside the unit project's test object (vite.config.ts:306), not the root — so the pin is real, and its comment now correctly describes it as making Vitest 4's existing "stack" default explicit rather than overriding a "parallel" one.
• Coverage-neutral: src/test/** is outside the include whitelist (vite.config.ts:180-195), so none of this is gated — and this commit adds no code anyway.

The new candidate I chased, and why it's a non-issue

ViewHeader.test.tsx:25-32 mocks @mantine/hooks and replaces useMediaQuery wholesale — and the three real-transitions tests set mediaQueryMock.value = true. Since Mantine's useReducedMotion is useMediaQuery('(prefers-reduced-motion: reduce)'), that looked like a second, file-local path to "reduced motion is on" in exactly the tests that depend on real timers — one nobody had noted, and independent of the setup.ts matchMedia override this PR spent rounds correcting.

It's inert, for the reason this PR itself established: useTransition computes reduceMotion = theme.respectReducedMotion ? shouldReduceMotion : false, Mantine 8 defaults respectReducedMotion to false, and src/theme/theme.ts doesn't override it — so no reduced-motion signal from either path can collapse the duration. The mock also uses importActual + spread, so useReducedMotion itself is real. Worth recording that this was checked: it means "don't flip respectReducedMotion" (round 1's advice) is load-bearing for two independent reasons in this file, not one.

Nit (genuinely not worth a round)

renderWithMantine.tsx:189-191 carries a line-wrap seam from this commit's edit — "…Neither fires in the current outer-cleanup / setup. A container the test unmounted itself isn't in / the set, so…" — two short lines mid-paragraph. Prettier doesn't reflow comments, so format:check passes and nothing is wrong; it just reads as an editing artifact. Fold it into the next comment change in this file, if there ever is one.

Verification caveat

This checkout has no node_modules (root and clients/web both absent), so I could not run the suite or read Vitest 4 / Mantine 8 at source. Specifically unverified here: resolved.sequence.hooks ??= "stack" (round 14), vi.isFakeTimers(), project-level sequence acceptance, and the isolate: false probe result — all rest on your local runs, and the isolate one is now the strongest of them because you recorded the exact command. The hook-ordering mechanism rests on your rounds 5–7 probes (the "list" result and the same-suite afterEach(cleanup) probe remain the discriminating evidence). Everything in the confirmation sections above was verified against files on this branch. CI run 30215167004 is in progress as I write this — worth confirming green before merge, since I couldn't run anything locally.

Summary

The change does what #1786 asked and then some: a helper-owned settle armed by the render that creates the hazard, a window derived from the animation it settles, per-container arming so multi-render and self-unmount compose, both fake-timer paths signalling the same corrected framing, liveness checks demonstrated to fire when broken, a drain that can neither be skipped by its own guard nor lose its explanatory error, a matchMedia override that no longer claims two contradictory preferences, a sequence.hooks pin that describes itself accurately, a documented (and reproduced) isolate: true dependency, and an AGENTS.md bullet that's a rule with the mechanism on the helper. Nothing left that I'd act on. Ship it once CI is green.
· v2/fix-1786-transition-timer-leak

[cliffhall]

Thanks — agreed, stopping the review loop here. Round-16 returned a merge verdict with no findings (the useMediaQuery-mock reduced-motion path you chased is inert for the same respectReducedMotion: false reason, which is a nice confirmation that "don't flip respectReducedMotion" is load-bearing twice over), and the one remaining nit is a cosmetic comment line-wrap that prettier passes — not worth another round, so I'm leaving it to fold into a future edit as you suggested.

Confirming the thing you couldn't check locally: GitHub CI is green on the head commit (a99df04d, workflow run 30215167004 → success), matching my local npm run ci.

Fifteen rounds of review turned a three-line flaky-gate fix into properly-owned test infrastructure — appreciate the depth. Ready to merge.

[cliffhall]

Local smoke test results

Branch v2/fix-1786-transition-timer-leak @ a99df04d, clean tree, macOS (darwin 24.6.0).

Check	Result
ViewHeader.test.tsx × 6 runs	20/20 passed every run
Full web unit suite (vitest run --project=unit) × 3 runs	252 files / 3533 tests passed each pass — no window is not defined, no unhandled errors
npm run coverage (per-file ≥90 gate, all four clients)	exit 0; zero occurrences of window is not defined in the log
npm run smoke	smoke:launcher, smoke:cli, smoke:tui, smoke:web (200 + injected __INSPECTOR_API_TOKEN__), smoke:web:browser (booted, rendered "Add Servers", no uncaught errors) — all OK

Caveat on the evidence: this failure class is timer-timing dependent, so green runs are weaker evidence than red ones (as the PR description itself notes). Three clean full-suite passes plus one clean instrumented coverage run is a reasonable signal, not proof.

Not re-run in this pass: verify:build-gate and the Storybook play-function tests (the PR author reports a green npm run ci end-to-end).

🤖 Generated with Claude Code