fix(web): swallow vitest browser-mode birpc-teardown race that flakes CI #1091
No reviewers
Labels
No labels
area:agents
area:dashboard
area:database
area:design
area:design-review
area:flows
area:infra
area:meta
area:security
area:sessions
area:webhook
area:workdir
security
type:bug
type:chore
type:meta
type:user-story
No milestone
No project
No assignees
4 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
charles/claude-hooks!1091
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "code-lead/1080"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Closes #1080. Patches the
[birpc] rpc is closed, cannot call "resolveManualMock"rejection that flakesapps/webCI even when every test passes, and re-enables the three test files that were excluded as a coverage tax.Diagnosis
@vitest/browser@4.1.5registers manual-mock factories without a try/catch aroundrpc.resolveManualMock()(seeindex.js:3231). When Playwright'sRouteHandler— whose lifecycle is the page, not the runner — invokes the factory after the runner has closed its birpc channel, the rejection escapes_handleInternalas an unhandled error and the runner exits 1.The upstream pre-mocker branch (
index.js:3211) already catches the same RPC call and returns{}on failure; the post-mocker branch just forgot. The issue body's sync-factory suggestion is a red herring —@vitest/mocker/node.js:239classifies every factory mock asmockType: "manual", so sync and async go through the identical RPC.Fix
apps/web/vitest.mock-rpc-patch.tsmonkey-patchesManualMockedModule.fromJSONat vitest-config load so every factory it wraps treats[birpc] rpc is closedrejections as "return empty exports" — the same graceful degradation upstream already applies on its other branch.@vitest/mockeris deduped (single physical copy innode_modules/.bun), so one Node-side patch covers every factory the runner registers later.Other changes:
apps/web/vitest.config.ts— side-effect imports the patch and drops the threeexcludeentries forapp-shell.test.tsx,nav-sections.test.tsx,sidebar-nav.test.tsx.apps/web/package.json— declares@vitest/mocker@4.1.5as a direct devDep so the patch canimport { ManualMockedModule }cleanly. The package is already in the install tree as a transitive dep ofvitest/@vitest/browser; pinning it here just makes TS resolve it.apps/web/vitest.mock-rpc-patch.test.ts— Node-only unit test (run viabun test apps/web/vitest.mock-rpc-patch.test.ts) covering the three branches: rpc-closed →{}, other errors → still throw (with originalcausepreserved), happy-path → exports untouched.Why not the other options
5.0.0-beta.2); no earlier patch fixes this race.mockType: "manual".unhandledRejectionhandler — already attempted in #1080's description and did not catch (rejection propagates through Playwright internals).docs/specs/vitest-browser-mode.md; would regress test fidelity.Test plan
bun test apps/web/vitest.mock-rpc-patch.test.ts— patch unit tests (4 pass, 0 fail).bun x tsc --noEmit(apps/web) — clean.bun x @biomejs/biome@^2 check apps/web— clean (pre-existing warnings only).apps/webto confirm the flake is gone.🤖 Generated with Claude Code
Follow-up commit
fcf7d39: the birpc-closed patch landed and worked (CI run #1902 reached vitest summary20 passed / 603 passed (605)— none of the failures were the original race). The remaining hard failure onapp-shell.test.tsxwas a different teardown error:The drawer-close test was clicking
nav-board, whichrouterStubs.StubLinkrendered as a plain<a href="/board">. The click navigated the Playwright iframe out from under vitest. Fix:StubLinknow callspreventDefault()(matching real<Link>from tanstack-router, which routes throughnavigate()rather than letting the browser load). Element-leveladdEventListenerlisteners still observedefaultPrevented === falsebecause React's delegatedonClickfires last, soavatar-menu.test.tsx's existing assertion continues to hold.Blocked on log access. Run #1905 (head
fcf7d39) —qa-1still red after the StubLinkpreventDefaultfix. Step grid from the run page:bun x turbo run testEvery Forgejo log endpoint I tried returns
dbfs open "actions_log/": file does not exist(/actions/runs/1905/jobs/{0,5,8420}/attempt/1/logs,/api/v1/repos/.../actions/{jobs,tasks}/{7349,8420}/logs,/actions/runs/1905/jobs/.../logs/full, …). The job's embeddedstepsLogis[]. Web UI is cookie-gated and doesn't accept the token, andapt-getis locked so I can't bring up Chromium locally to reproduce (missinglibglib-2.0.so.0, no root). No logs ⇒ no way to identify which test case is now bailing the runner at 46s.What I can rule out from the static diff against the previous-known-bad run (#1902, "20 passed / 603 passed (605)"):
avatar-menu.test.tsx"click does not prevent default navigation" still holds: element-leveladdEventListenerfires at the anchor's native target phase, before React's root-delegatedonClickin the bubble —e.defaultPrevented === falsethere.app-shell.test.tsx"tapping a nav link closes the drawer" still hascloseMenu()fire inonClickCapturebefore ourpreventDefault, so the drawer should still close.sidebar-nav,nav-sections) appears to click an<a href>rendered byStubLink— only buttons.So either the second
(605 − 603 = 2)failure from #1902 was a separate test the comment didn't name, orfcf7d39regressed something unrelated. Can't tell which without the Test-step output.Suggested unblock: re-run the job once log retention is healthy, or have an operator surface the failing test name (e.g. paste the last 50 lines of the
Teststep) so a targeted fix is possible. Stopping rather than guess-pushing.fcf7d399b9cd926c0d5acd926c0d5ae824972d6be824972d6b83bf44cb9883bf44cb980159343e84Review — REQUEST_CHANGES
One blocker: CI is red on HEAD
0159343(run #1905, Test step, 46 s). The fix itself is well-crafted — this is purely a "get CI green first" gate.What's good
Diagnosis is correct. The PR description nails the upstream split: the pre-mocker branch at line ~3211 already has a
try/catchthat returns{}, the post-mocker branch at line ~3231 is the one missing it. Confirming this via a quick check of the@vitest/browser/dist/index.jssource is the right call, and the conclusion that sync vs. async factories makes no difference (both route throughmockType: "manual"in@vitest/mocker/dist/node.js:239) is accurate.Monkey-patch is well-crafted.
__chBirpcSafe) is clean — safe to re-import the file.bind(ManualMockedModule)preserves the receiver for the original call.message.includes("rpc is closed")is specific enough; it matches the full upstream message and won't swallow unrelated rejections.createHelpfulErrorstill wraps them and the original.causeis preserved — confirmed by the unit test.StubLinkfix is correct. CallingcallerOnClick?.(e)first thenif (!e.defaultPrevented) e.preventDefault()matches tanstack-router's behaviour, doesn't breakavatar-menu.test.tsx'sdefaultPrevented === falseassertion (element-leveladdEventListenerfires at the anchor's native target phase before React's root-delegated bubble), and the comment explains all of this precisely.Unit tests cover all branches (idempotency marker, rpc-closed →
{}, other error → still throws with cause, happy path).@vitest/mockerpin — declaring the transitive dep directly so TS resolves it cleanly is the right approach; no resolution ambiguity.Blocker
CI run #1905 is still failing. From the code-lead's comment the log content is inaccessible (
dbfs open "actions_log/": file does not exist), so the specific failing test isn't known. Before this can merge the failure needs to be identified and fixed (or ruled out as infra noise and re-run clean).Suggested diagnostic paths:
actions_logerror is transient/infra noise the next run may produce readable logs.bun x vitest --reporter=verboseinapps/webagainst the 3 re-enabled files pinpoints which test assertion is bailing the runner in 46 s.app-shell.test.tsx,nav-sections.test.tsx,sidebar-nav.test.tsxin isolation (vitest run --reporter=verbose <file>) to isolate the failing file before digging into the specific case.Once CI is green this is ready to merge — the approach is sound and the coverage tax is worth removing.
CI log surfaced (run #1929, head
0159343e)Operator pasted the Test step output. The single failing scope:
Test count:
599 passed (600). Same iframe-navigation pattern as the drawer-close test you already fixed infcf7d39— different test, same root cause. TheStubLinkpreventDefaultdid its job for the link the drawer test clicked; this one is a separate<a href="/board">path that bypassesrouterStubs.Link.Diagnostic hints
The sidebar-rail render mounts the full
<AppShell>. Candidates for an un-stubbed<a href="/board">:<Link to="/board">directly, or via a sub-component that importsLinkfrom somewherevi.mock("@tanstack/react-router", …)doesn't intercept (e.g.@tanstack/react-router-coreor@tanstack/router-core)?NavSectionsrail variant — the nav items themselves. Rail variant might render<a>differently than the drawer variant.Collapse navigationbutton — does it render as a link?<Link>import — search forfrom "@tanstack/router-core"or any other path the globalvi.mockdoesn't cover.Suggested next pass
In
apps/web/src/lib/test-router-stub.tsx, thevi.mockmocks@tanstack/react-router. Grep for other modules that re-export or wrapLink:Two viable fixes:
vi.mocklist invitest.setup.tsxwithrouterStubs(preserves test fidelity).preventDefaulton every anchor click whosehrefstarts with/. Acts as a backstop for any future un-stubbed<Link>.Going with A is correct because it stays test-scoped and surfaces future mismatches at mock time, but B as a safety net (in
vitest.setup.tsx) would prevent class of regression entirely.Re-dispatching with the failing test name + log should unblock the loop; the fix is bounded to one file in
apps/web/src/components/.Review — fix(web): swallow vitest browser-mode birpc-teardown race
Overview
The PR tackles two separate root causes of CI failures in
apps/web:vitest.mock-rpc-patch.tsmonkey-patchesManualMockedModule.fromJSONsorpc is closedrejections during teardown degrade gracefully to{}instead of escaping as unhandled rejections.test-router-stub.tsxnow callspreventDefault()on clicks, stopping the Playwright iframe from navigating away from the vitest session URL.Both diagnoses are correct and the implementations are solid. The problem is that CI is still red and one of the three re-enabled test files is still failing — see below.
✅ What's well done
__chBirpcSafemarker), narrowly-scoped catch (only therpc is closedsubstring), non-rpc errors re-thrown untouched, rich inline documentation explaining the upstream bug. This is exactly the right shape for a monkey-patch workaround.{}, other errors → still throw (with.causepreserved), happy-path → exports unchanged.@vitest/mockeras a direct devDep is the correct way to give TS a clean resolution target for an otherwise-transitive package.StubLinkfix matches production behaviour. Real tanstack-router<Link>routes throughnavigate()and prevents browser navigation; the comment correctly explains whyavatar-menu.test.tsx'sdefaultPrevented === falseassertion continues to hold (React delegatedonClickfires after nativeaddEventListener).❌ Blocker — CI is still failing
CI run #1929 (head
0159343e) reports 599 passed (600) with one failure:This is the same root cause as the drawer-close failure fixed by the
StubLinkpreventDefault— a different code path (sidebar-rail render) is navigating the Playwright iframe to/boardvia an un-stubbed<a href="/board">. TheStubLinkfix only covers links that flow throughrouterStubs.StubLink; this path bypasses it (likely the brand strip / logo or aNavSectionsitem rendered via a<Link>import that isn't covered by the currentvi.mock("@tanstack/react-router", …)).claude-desktop's comment #16858 already identifies the likely candidates and suggests two viable fixes (A: add the missing module to thevi.mocklist; B: global anchor-click backstop invitest.setup.tsx). That comment has not been acted on yet.The removal of the three
excludeentries is therefore premature.app-shell.test.tsxis still broken. Until all three re-enabled files are confirmed green in CI, those exclusions should stay in place (or the test should be fixed in this PR).⚠️ Minor observations (non-blocking)
1.
vitest.mock-rpc-patch.test.tsplacement riskThe file lives at
apps/web/vitest.mock-rpc-patch.test.ts(root, not undersrc/). Theexcludelist doesn't cover it. Depending on vitest'sincludeglob, the browser runner might pick it up — but it imports@vitest/mocker(Node-side API) and is explicitly intended to run underbun test. Ifincludeissrc/**/*.test.{ts,tsx}it's fine; if it's**/*.test.{ts,tsx}it would be wrong. Worth adding a comment or confirming the include pattern guards it.2.
safeFactorytype castFactoryArgisParameters<FromJSON>[1]which likely acceptsundefined | (() => …). The cast side-steps the nullability. Harmless in practice (we're only called whenfactoryis defined) but afactory?.()null-guard would be cleaner.Required before merge
app-shell.test.tsx > sidebar rail. Follow the diagnostic in comment #16858 — find the un-stubbed<Link>import and either add it to the mock list or add a global anchor-click backstop. TheStubLinkpatch alone is not sufficient.includescope sovitest.mock-rpc-patch.test.tsis clearly not mistakenly picked up by the browser runner.The birpc patch and the
StubLinkfix are both correct and can land — the PR just needs the remaining iframe-navigation path closed out.Review —
fix(web): swallow vitest browser-mode birpc-teardown raceCI ✅ green on
a2cb147. Three previously-excluded test files restored. Approved.Overview
Two independent root causes were identified and fixed:
@vitest/browser@4.1.5's post-mocker factory branch lacks the try/catch that the pre-mocker branch already has. A PlaywrightRouteHandlertied to the page outlives the runner's birpc socket, causing a post-teardown rejection that fails the run even when every assertion passed.<Link>(or any un-stubbed<a href="/">) was navigating the Playwright iframe to the link'shref, killing the orchestrator's iframe channel mid-suite.Both are now addressed with a targeted shim + a document-level backstop. The diagnosis in the PR body and code comments is thorough and accurate.
vitest.mock-rpc-patch.ts— core patch__chBirpcSafemarker prevents double-patching across repeated config loads. ✓original.bind(ManualMockedModule)correctly preserves the static method's receiver before wrapping. ✓@vitest/mockeris deduped to a single physical copy, so one Node-side wrap covers every factory registered later. ✓Minor observation:
message.includes("rpc is closed")is a string-match on an internal birpc message. If birpc changes the text in a future version the shim silently becomes a no-op (the error would resurface). Not blocking — it's the right pragmatic choice — but worth adding the exact birpc version in the comment (it's already implied byvitest 4.1.5 + @vitest/browser-playwright 4.1.5in the file header, which is sufficient).vitest.mock-rpc-patch.test.ts— test placementPlacing the test at
apps/web/vitest.mock-rpc-patch.test.ts(not undersrc/) is intentional and correct: theinclude: ["src/**/*.{test,spec}.{ts,tsx}"]pattern invitest.config.tskeeps it out of the browser runner. It imports Node-only@vitest/mockerand would fail in Chromium. Running it viabun testis the right call and the PR description is clear about this. The four test branches (marker, rpc-closed →{}, non-rpc error → rethrown with.cause, happy-path passthrough) give complete coverage of the shim's logic.test-router-stub.tsxcallerOnClick?.(e)fires first so caller-provided handlers seedefaultPrevented === falsebefore the stub's ownpreventDefault. ✓if (!e.defaultPrevented) e.preventDefault()— won't double-prevent if the caller already did. ✓element.addEventListenerlisteners fire before React's synthetic handler. Element-level listeners (avatar-menu contract) continue to observedefaultPrevented === false. ✓vitest.setup.tsx— document-level backstoptypeof document !== "undefined"for Node contexts. ✓target?.closest("a[href]")handles clicks on child elements inside an anchor. ✓href?.startsWith("/")— correct scope; hash-only#foolinks are left alone (they don't navigate the origin). ✓defaultPrevented === falsecontract. ✓if (ev.defaultPrevented) return— cooperates withStubLink.handleClick; if React already prevented it, the backstop is a no-op. ✓This is a good defence-in-depth layer for any un-stubbed anchor that slips through
vi.mock.package.json/bun.lockPinning
@vitest/mockerto exact4.1.5(no^) is correct — the patch is written against this specific version's class shape and should not silently upgrade. Already a transitive dep; this just makes TS resolve it cleanly.Acceptance criteria (closes #1080)
[birpc] rpc is closedrejection no longer fails the runapp-shell.test.tsx,nav-sections.test.tsx,sidebar-nav.test.tsxre-enabledapps/webPRs to confirm flake is gone (ongoing, not a merge blocker)