Skip to content

web: disable Mantine transitions in tests to kill post-teardown timer flake (#1760)#1761

Merged
cliffhall merged 2 commits into
v2/mainfrom
v2/1760-mantine-test-env
Jul 24, 2026
Merged

web: disable Mantine transitions in tests to kill post-teardown timer flake (#1760)#1761
cliffhall merged 2 commits into
v2/mainfrom
v2/1760-mantine-test-env

Conversation

@cliffhall

Copy link
Copy Markdown
Member

Closes #1760

Problem

A coverage / CI build job can fail even when all tests pass, because Vitest exits non-zero on a single unhandled error:

⎯⎯ Uncaught Exception ⎯⎯
ReferenceError: window is not defined
 ❯ resolveUpdatePriority react-dom-client.development.js:1308
 ❯ dispatchSetState      react-dom-client.development.js:9126
 ❯ Timeout._onTimeout    @mantine/core/.../use-transition.mjs:61
This error originated in "ServerRemoveConfirmModal.test.tsx"

A Mantine Transition/Modal open/close setTimeout fires after happy-dom tears down window at the end of the run. Seen on PR #1759's CI (which was otherwise green on re-run).

Fix

Set env="test" on the shared renderWithMantine MantineProvider (and ViewHeader's local forced-dark provider). Under env="test" Mantine renders transitions synchronously — no setTimeout — so the whole class of post-teardown timer races is eliminated for every test that renders through the shared wrapper (Modals, Menus, Popovers, …).

The one wrinkle: tests that assert mid-transition state

Three ViewHeader tests (#1450) assert the exit/crossfade state (data-anim="out"), which only exists while a transition is in flight — so they genuinely need real transitions. For them, added an opt-in renderWithMantineTransitions wrapper (env="default") and routed those three through it. Each already drives the transition to completion (waitFor/findBy), so it doesn't reintroduce the leak. Tests that assert only the entered ("in") state stay on the default, leak-free wrapper.

Verification

  • Full unit suite: 249 files / 3457 tests pass, zero unhandled errors (previously 1).
  • ViewHeader test run 3× — clean, no window is not defined.
  • Full npm run ci green (validate, ≥90 coverage gate, smoke, 456 Storybook tests).

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

… flake (#1760)

A Mantine `Transition`/`Modal` open/close `setTimeout` can fire after happy-dom
tears down `window` at the end of a run, throwing an uncaught
`ReferenceError: window is not defined` that fails the whole `coverage` job even
when every assertion passed (seen on PR #1759's CI, originating in
ServerRemoveConfirmModal.test.tsx).

Set `env="test"` on the shared `renderWithMantine` MantineProvider so Mantine
renders transitions synchronously (no timer). This removes the leak class for
every test that renders through the shared wrapper (Modals, Menus, Popovers, …).
Also apply it to ViewHeader's local forced-dark provider.

The handful of tests that assert mid-flight transition state (ViewHeader's
`data-anim="out"` crossfade checks, #1450) genuinely need real transitions, so
add an opt-in `renderWithMantineTransitions` wrapper (env="default") and route
those three tests through it — each already drives the transition to completion
(waitFor/findBy) so it doesn't reintroduce the leak. Tests that assert only the
entered state stay on the default (leak-free) wrapper.

Full `npm run ci` green; unit run reports zero unhandled errors.

Closes #1760

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 24, 2026
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

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


Review: PR #1761 — disable Mantine transitions in tests

  • Read the changed files & diff
  • Analyze the fix approach
  • Check for correctness / edge cases
  • Post review

Verdict: LGTM — well-targeted, correct, and thoroughly documented. No changes requested.

Why the fix is sound

The root cause is a classic post-teardown timer race: a Mantine Transition/Modal setTimeout fires after happy-dom tears down window, and Vitest fails the whole run on that one uncaught ReferenceError even when every assertion passed. Setting env="test" on the shared provider makes Mantine render transitions synchronously (no internal timer), which removes the race at its source rather than papering over it (e.g. no blanket vi.useFakeTimers, no arbitrary flush delays).

I verified the fix actually covers the reported failure:

  • The crashing file, ServerRemoveConfirmModal.test.tsx, renders exclusively through the shared renderWithMantine (ServerRemoveConfirmModal.test.tsx:5,37…) → now gets env="test" for free. ✅
  • Only two test files bypass the shared wrapper with their own MantineProvider: ViewHeader.test.tsx (its renderDark helper is updated to env="test" in this PR) and ContentViewer.test.tsx (line 221 renders no transition-driven component, so no leak). Coverage of the shared path is complete. ✅

The opt-in escape hatch is handled correctly

The three #1450 ViewHeader tests genuinely need real transitions because they assert the mid-flight data-anim="out" state, which doesn't exist under synchronous rendering. renderWithMantineTransitions (env="default") is the right minimal opt-in, and each of the three drives its transition to completion before teardown, so none reintroduces the leak:

  • ViewHeader.test.tsx:275await waitFor(... radio count === 0) (bar unmounts)
  • ViewHeader.test.tsx:358await waitFor(... "my-mcp-server" removed)
  • ViewHeader.test.tsx:375+findByText for the entering header

The JSDoc on TransitionsWrapper explicitly states this contract ("MUST drive the transition to completion … otherwise it reintroduces the #1760 leak"), which is exactly the guardrail a future author needs.

Minor observations (non-blocking, no action needed)

  1. renderDark now diverges slightly from the two opt-in transition tests — it forces defaultColorScheme="dark" and env="test", while the crossfade tests use light + env="default". That's intentional (the dark tests assert only the entered "in" state, per ViewHeader.test.tsx:241), just worth being aware that there's no dark-scheme path that exercises real transitions. Not a gap for these tests.
  2. Discoverability of the invariant. The env="test" contract now lives in a JSDoc comment. If the suite grows, a lint rule or a short note in AGENTS.md under the testing section ("render through renderWithMantine; use renderWithMantineTransitions only when asserting mid-flight animation state") would keep new tests from hand-rolling a bare MantineProvider and quietly reintroducing the class of leak. Optional.

Nice, surgical fix that kills the whole class of post-teardown timer flake for every test on the shared wrapper.

Per review feedback: document that tests must render through
`renderWithMantine` (env="test", transition-free) rather than a hand-rolled
`MantineProvider`, and reserve `renderWithMantineTransitions` for asserting
mid-flight animation state (driving the transition to completion). Keeps future
tests from quietly reintroducing the post-teardown timer leak class.

Refs #1760

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

Copy link
Copy Markdown
Member Author

Thanks! LGTM noted. Addressed observation #2 in 2b611e04: added the renderWithMantine (env=test) vs renderWithMantineTransitions invariant to AGENTS.md under the testing rules, so new tests won't hand-roll a bare provider and reintroduce the leak. Observation #1 (renderDark using dark + env=test) is intentional as you noted — those tests assert only the entered state — so no change there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test teardown: Mantine Transition timer fires after happy-dom window teardown (window is not defined)

1 participant