test(ui): drain React's scheduler before jsdom teardown (BLO-23426)

[kkroo]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work, and its master branch lands through a merge queue that has to pass General tests (workspaces-a).
• That lane runs the ui vitest project, where most component tests render with createRoot and unmount with flushSync in afterEach.
• React 19 schedules a passive-effect flush after every commit with passive flags, and that scheduler task reads window.event before anything else (react-dom-client.development.js:17920).
• When a jsdom file's environment is torn down before that setImmediate fires, it throws ReferenceError: window is not defined as an unhandled error, and vitest exits 1 on a run where every test passed.
• It has ejected merge-queue groups: test: de-flake the two assertions ejecting merge groups (#1787, #1952) #1987 (run 35903530339, 20 errors blamed on BuiltInBundlePanel.test.tsx) and fix(heartbeat): make the queued-run dispatch order total (BLO-28886) #1976 (run 35993984182, 4 errors blamed on ArtifactGroupCard.test.tsx). Each ejection costs about two hours of queue time.
• This pull request drains a few macrotasks in a setup-file afterAll, which vitest runs after every other hook, so React's pending task completes while jsdom is still installed.
• The benefit is that an all-green workspaces-a run stops going red on teardown ordering, whichever file happens to leave the task behind.

Linked Issues or Issue Description

• Fixes BLO-23426 — https://paperclip.blockcast.net/BLO/issues/BLO-23426. The issue's own "durable version" is teardown ordering rather than chasing one file, and that is what this does.
• Refs BLO-31595 and BLO-31438. Those are unowned setTimeouts that fire after teardown: a different mechanism, which this does not address.

What Changed

• ui/src/lib/drainReactScheduler.ts (new): drainReactScheduler() awaits three setImmediate turns. Node runs immediates in order, so this runs every scheduler task React queued before the call.
• ui/src/lib/drainReactScheduler.test.tsx (new): after a flushSync render and unmount of a component with a useEffect, followed by the drain, three more immediate turns must produce no further window.event reads. If React stops deferring the task the test still passes; it fails only when the drain leaves work behind.
• ui/vitest.setup.ts: for jsdom files only (typeof window !== "undefined"), afterAll(drainReactScheduler), with a comment citing the two run IDs.

Verification

• Mechanism, confirmed on unmodified origin/master with a throwaway probe test. After flushSync(() => root.unmount()) of a component with a useEffect, a window.event getter has not yet been read; one await new Promise(r => setImmediate(r)) later, it has. So a pending task that touches window exists at unmount, and one immediate turn runs it.
• Ordering, checked in @vitest/runner 4.1.8: afterAll hooks run in reverse registration order (sequence.hooks default stack, not overridden in ui/vitest.config.ts or the root config). The setup-file hook is registered first, so it runs after every test file's own afterEach / afterAll, and before environment teardown.
• Mutation: making drainReactScheduler a no-op fails the new test with expected 6 to be 4, because two pending window.event reads survive the no-op. The file was restored byte-identical afterwards.
• cd ui && npx vitest run at 50e488241: 373/374 files and 3073 tests passed, with 0 unhandled errors. The one red file was App.cases-routing.test.tsx: its beforeAll hit its own 180s App-import timeout (BLO-17053) while the devbox sat at load average 67 on 32 cores. Re-run alone it passes (4/4 together with the new test). An earlier full run at 629150f0a was 373/373 and 3075/3075, exit 0.
• cd ui && npx tsc --noEmit -p .: exit 0.
• The three files that have been blamed so far (ArtifactGroupCard, BuiltInBundlePanel, routine-sections/editable-sections) pass together, 16/16.
• Not verifiable locally: the race only loses on a contended runner. The evidence is the mechanism plus the ordering guarantee, not a reproduced red.

Risks

• Low risk. Three extra setImmediate turns per jsdom test file, which is microseconds. Node-environment files are unaffected because the hook is only registered when window exists.
• It can't hide a real failure. It only lets already-scheduled work run while the environment exists. Anything that throws during the drain still surfaces, now with window defined.

For core feature work, check ROADMAP.md first and discuss it in #dev before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See CONTRIBUTING.md.

Model Used

• Claude Opus 5.5 (claude-opus-5-5, 1M context) via Claude Code, with tool use (shell, file edit, test execution) and extended thinking.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots
• I have updated relevant documentation to reflect my changes
• I have considered and documented any risks above
• All Paperclip CI gates are green
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-23426
🔗 Paperclip issue: BLO-31595
🔗 Paperclip issue: BLO-31438

[allyblockcast]

✅ All checks passing — ready for Greptile review and maintainer approval.

— commitperclip

[kkroo]

@allyblockcast please publish one consolidated review for exact current head 50e488241e40a3645461674b082cb8f909d9abcc, and emit a standalone Reviewed head: 50e488241e40a3645461674b082cb8f909d9abcc line.

It's a teardown-ordering fix for BLO-23426: a jsdom afterAll in ui/vitest.setup.ts drains React's pending scheduler task, which reads window.event, before the environment is torn down. src/lib/drainReactScheduler.test.tsx fails if the drain is a no-op (expected 6 to be 4). The Verification section in the body covers the mechanism, the hook ordering, the mutation check, and the full ui suite.

🤖 Generated with Claude Code

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 50e4882

Looks good. A teardown-ordering fix that treats the cause rather than the blamed file, with a regression test that is mutation-checked rather than assumed. Nothing blocking.

Critical Issues (0)

Important Issues (0)

Suggestions (4)

• [native-codex] ui/vitest.setup.ts:51 — Vitest's default fakeTimers.toFake includes setImmediate, so a jsdom file that installs fake timers and never restores them would leave this hook awaiting an immediate that never fires, hanging to the 60s hookTimeout instead of failing fast. Not live today — the full ui suite is green at this head, and the reverse-registration ordering you rely on also means any file-level afterEach/afterAll calling vi.useRealTimers() runs before this hook, so the correct patterns are already protected. Only a file with no restore at all is exposed. Worth a defensive vi.useRealTimers() at the top of the drain, or a line in the comment noting the assumption, so the next person meets a clear cause rather than a 60s timeout.
• [code] ui/src/lib/drainReactScheduler.ts:12 — setImmediate is Node-only, but this module sits in ui/src/, the browser source tree that vite build compiles. Nothing in app code imports it so it tree-shakes out today, and the placement does match the repo's colocated-test convention. The risk is purely that the location invites a future app-side import, where setImmediate is undefined. A one-line "test-environment only, Node setImmediate" note on the export would close it without moving the file.
• [code] ui/src/lib/drainReactScheduler.ts:12 — the turn count is a bounded heuristic: three covers a task that re-posts itself once, and a hypothetical deeper re-post chain would slip through. The docblock already says as much, which is the right call over a drain-until-idle loop that could spin. Noting it only so the ceiling is on the record — if this flake ever recurs, the turn count is the first knob, not evidence the approach is wrong.
• [tests] ui/src/lib/drainReactScheduler.test.tsx:31 — the assertion is reads == afterDrain, so if a future React version stops deferring the passive flush, afterDrain is whatever it is and the test passes vacuously while silently no longer guarding anything. Your PR body calls this out deliberately, and the tradeoff is defensible — asserting afterDrain > 0 would couple the test to React internals and buy brittleness. Flagging only so the known ceiling of the guard is recorded alongside the guard.

Strengths

• Fixes the mechanism, not the blamed file. ArtifactGroupCard / BuiltInBundlePanel were symptoms of whichever file happened to leave the task behind; a setup-file drain covers all of them, which is why this should stop recurring under a new name.
• The mutation check is the part that makes this reviewable. "No-op the drain and the new test fails with expected 6 to be 4" is evidence the test would notice the fix being removed — as opposed to a test that merely passes alongside it.
• The ordering claim is verified against @vitest/runner 4.1.8 rather than assumed, and I confirmed it independently: sequence.hooks is unset in ui/vitest.config.ts, so the default stack applies and this hook runs after every test file's own hooks.
• Comments carry the two merge-group run IDs and the reverse-registration assumption. That is exactly what a future reader needs to tell a real regression from a re-tune, and it is rare.
• The typeof window !== "undefined" guard keeps node-environment files untouched, and the cost on jsdom files is three immediate turns.
• Scope discipline: BLO-31595/BLO-31438 are named as a different mechanism this explicitly does not address, rather than being folded in.

Recommended Action

1. No blocking changes requested.
2. Merge once the remaining required CI checks finish green.