From 73026e490f16be817fd359822185833d8f52de3c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 10:42:35 -0400 Subject: [PATCH 1/2] web: disable Mantine transitions in tests to kill post-teardown timer flake (#1760) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- .../groups/ViewHeader/ViewHeader.test.tsx | 19 ++++++++--- clients/web/src/test/renderWithMantine.tsx | 32 +++++++++++++++++-- 2 files changed, 43 insertions(+), 8 deletions(-) diff --git a/clients/web/src/components/groups/ViewHeader/ViewHeader.test.tsx b/clients/web/src/components/groups/ViewHeader/ViewHeader.test.tsx index 705857f6e..04d18b565 100644 --- a/clients/web/src/components/groups/ViewHeader/ViewHeader.test.tsx +++ b/clients/web/src/components/groups/ViewHeader/ViewHeader.test.tsx @@ -3,6 +3,7 @@ import userEvent from "@testing-library/user-event"; import { act, renderWithMantine, + renderWithMantineTransitions, screen, waitFor, } from "../../../test/renderWithMantine"; @@ -12,11 +13,13 @@ import { theme } from "../../../theme/theme"; import { ViewHeader } from "./ViewHeader"; // Render under a forced-dark MantineProvider so `useComputedColorScheme` -// returns "dark" (the light/dark icon + logo branches). +// returns "dark" (the light/dark icon + logo branches). `env="test"` disables +// timer-driven transitions, matching the shared `renderWithMantine` wrapper and +// avoiding post-teardown `window is not defined` races (#1760). function renderDark(ui: React.ReactElement) { return render(ui, { wrapper: ({ children }) => ( - + {children} ), @@ -240,7 +243,9 @@ describe("ViewHeader", () => { it("crossfades on disconnect: tab bar exits while the title enters, then the bar unmounts (#1450)", async () => { mediaQueryMock.value = true; - const { rerender } = renderWithMantine( + // Asserts the mid-flight exit ("out") state, so it needs real transitions; + // it drives them to completion below (waitFor unmount). + const { rerender } = renderWithMantineTransitions( , ); expect(screen.getAllByRole("radio").length).toBeGreaterThan(0); @@ -309,7 +314,9 @@ describe("ViewHeader", () => { it("animates the server name and disconnect controls in on connect, out on disconnect (#1450)", async () => { mediaQueryMock.value = true; - const { rerender } = renderWithMantine( + // Asserts the mid-flight exit ("out") state, so it needs real transitions; + // it drives them to completion below (waitFor unmount). + const { rerender } = renderWithMantineTransitions( , ); // Connected: the server name and the Disconnect control are in their @@ -368,8 +375,10 @@ describe("ViewHeader", () => { it("crossfades the title out and the connected header in on connect (#1450)", async () => { mediaQueryMock.value = true; + // Asserts the mid-flight exit ("out") state, so it needs real transitions; + // it drives them to completion below (findByText for the entering header). // Start disconnected: the title cell is the entering one. - const { rerender } = renderWithMantine( + const { rerender } = renderWithMantineTransitions( + + {children} + + ); +} + +// Opt-in wrapper that keeps Mantine's timer-driven transitions enabled, for the +// few tests that assert on transition/animation state that only exists mid-flight +// (e.g. a `data-anim="out"` cell during an exit crossfade). Such a test MUST +// drive the transition to completion (`await waitFor`/`findBy`) so its timer +// resolves before teardown; otherwise it reintroduces the #1760 leak. +function TransitionsWrapper({ children }: { children: ReactNode }) { + return ( + {children} ); @@ -15,7 +34,14 @@ export function renderWithMantine( ui: ReactElement, options?: Omit, ) { - return render(ui, { wrapper: MantineWrapper, ...options }); + return render(ui, { wrapper: TestEnvWrapper, ...options }); +} + +export function renderWithMantineTransitions( + ui: ReactElement, + options?: Omit, +) { + return render(ui, { wrapper: TransitionsWrapper, ...options }); } export * from "@testing-library/react"; From 2b611e043b3054c8821b4e7bc45ce8a13561cbac Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 24 Jul 2026 10:45:35 -0400 Subject: [PATCH 2/2] docs: note the renderWithMantine env=test invariant in AGENTS.md (#1760) 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) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 6a90172d3..1437936c8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -200,7 +200,7 @@ gh project item-edit --project-id PVT_kwDOCt2Azc4BJVxt --id "$ITEM_ID" --field-i - **TUI** (`clients/tui`): the gate covers the **non-React logic** only — `logger.ts`, `components/tabsConfig.ts`, and `utils/*` (server resolution lives in `core/` and is measured by the web suite). The Ink components, `App.tsx`, and `hooks/` are an **interim exclusion** in `clients/tui/vitest.config.ts` pending the renderer-based follow-up (#1501). When adding new **non-React** logic under `clients/tui/src`, it falls under the gate automatically — add tests for it. - Run `npm run test:integration` (also from `clients/web/`) for the InspectorClient + transport + auth integration suite. It runs under a separate `integration` vitest project in node env (no happy-dom) with 30s timeouts. The script builds `test-servers/` first via `tsc -p ../../test-servers --noCheck` so the stdio MCP test server can be spawned as a real subprocess. CI does not run `test:integration` as its own step — the integration project is covered by the CI `coverage` gate, whose web `test:coverage` runs `--project=unit --project=integration --coverage`. - Test files live alongside the source as `.test.tsx` (or `.test.ts` for non-React modules). Integration tests live under `clients/web/src/test/integration/`, mirroring the `core/` source layout (`mcp/`, `mcp/node/`, `mcp/remote/`, `auth/`, `auth/node/`, `storage/`). Any test file under that folder is automatically picked up by the `integration` vitest project (node env, 30s timeouts) via the folder glob in `vite.config.ts` — placement is the manifest, there is no enumeration to keep in sync. Tests outside the folder run in the `unit` project (happy-dom). When adding a new test for, e.g., `core/mcp/remote/foo.ts`, put it at `src/test/integration/mcp/remote/foo.test.ts`. -- Use `renderWithMantine` from `src/test/renderWithMantine.tsx` to render components — it wraps in `MantineProvider` with the project theme +- Use `renderWithMantine` from `src/test/renderWithMantine.tsx` to render components — it wraps in `MantineProvider` with the project theme. It sets `env="test"` so Mantine renders transitions synchronously (no internal `setTimeout`); this prevents a `Transition`/`Modal` timer from firing after happy-dom tears down `window` at end-of-run and failing the whole run with an uncaught `ReferenceError: window is not defined` (#1760). **Always render through `renderWithMantine`; do not hand-roll a bare `MantineProvider` in a test** (that reintroduces the leak class). Only when a test must assert *mid-flight* transition state (e.g. a `data-anim="out"` cell during an exit crossfade) use `renderWithMantineTransitions` (real transitions) — and that test MUST drive the transition to completion (`await waitFor`/`findBy`) so its timer resolves before teardown. ### Responding to Code Reviews - When asked to respond to a code review of a PR,