perf(test): run Vitest in node by default, and fix the workflow guidance around it - #281
Merged
Merged
Conversation
The suite built a full jsdom for every one of its 140 test files. Only 37 of them ever touch a DOM, so the other 103 paid for one and threw it away: 719s of cumulative environment setup against 89s of actual test time. Flipping the default to `node` and letting the 37 opt back in with a `@vitest-environment` docblock takes the full run from 175s to 81s, back to back on the same machine, with no change to what any test asserts. The 37 were derived by running the suite under `--environment=node` and taking the files that failed, not by guessing from imports — 12 of them are `.ts` files (zustand stores, hooks, `platformUtils`) that a `.tsx`-only heuristic would have missed. `electron/media/audioPeaks.test.ts` already carried the same docblock the other way round to escape the global jsdom; that one is now redundant, and left alone. testTimeout goes to 15s in the same change because the two interact: with the machine loaded, 11 tests fail and 9 of those are purely "Test timed out in 5000ms" — ordinary component tests that pass in 200ms idle. A single jsdom file needs ~9.5s just to boot React, so a 5s budget was never a signal about the test. The one deliberately slow test (20 interleaved real disk writes) keeps its own longer override. The config comment also records the four things that look like speedups and are not — `--no-isolate`, `deps.optimizer.web`, `--pool=threads`, `--maxWorkers` — with what each measured, so the next person does not re-run that benchmark. `--no-isolate` is the tempting one: ~20% faster, but it shares one module registry per worker, which breaks `vi.mock`, and 29 test files depend on it.
Every agent-facing surface in the repo defined "done" as `npm run test` — the full suite. AGENTS.md, both harness reins and the git-workflow doc all said it, so an agent ran ~1670 tests after each edit and turned short tasks into long ones. They now say: targeted run while working (`npx vitest --run <path>`, or the new `npm run test:changed`), tsc and biome as the inner loop since they are seconds, and one full run at the end or left to CI. Also removes a tier of tests that does not exist. `npm run test:browser` and `vitest.browser.config.ts` were documented across four files and 80 lines of writing-tests.md, with a worked example and a timeouts section — but there is no script, no config, no `*.browser.test.ts` file and no CI job for any of it. An agent reading that runs a command that cannot work. What actually covers real codecs and GPU is the Rust suites under `crates/` and the manual checklist, so the table now points there. The remaining testing docs pick up the two rules this branch had to learn the hard way: the environment is node unless a file opts into jsdom, and anything gated on `process.platform` has to pin it, because CI is Linux-only and an unpinned Linux-only path is green there and red on every other machine.
`clearTimeout` sat on the line after the `await`, so only the success path ever reached it. When `fetch` rejects — a real network error, and the case the test file already drives — control jumped straight to `catch` and the 5s abort timer stayed armed, firing `controller.abort()` long after the function returned. `callDiscord` in the sibling `discord-bot-api.mjs` already had this right with a `try`/`finally`; this is the same shape. The new test pins it with fake timers on both the rejecting and the non-ok path: reverting the `finally` makes it fail with `expected 1 to be +0`.
Audit of every `void <call>` in src/ and electron/ after 2c4c426 fixed one of them, since `void` marks a promise as intentionally detached but does nothing about its rejection. 122 sites; most are fine and are left alone. The three in electron/ are the ones that can kill the app, because `installMainProcessErrorGuards` re-throws every unhandled rejection whose code is not EPIPE/ECONNRESET/ERR_STREAM_DESTROYED. One was the registry write fixed in 2c4c426; the other two are already correct — `document-service` voids a promise that is already `.catch()`ed, and `cliMain`'s chain ends in a `.catch` that exits non-zero. Neither gets ceremony added. In the renderer a rejection is console noise rather than a crash, so the fixes here are the sites where a rejection is not hypothetical: * `play()` and `requestFullscreen()` reject routinely (autoplay policy, a load interrupting a pending play). VirtualPreview and WebcamOverlay already caught theirs; Modals and NewEditorShell did not. Play state is driven by the element's own play/pause events in both, so a rejection leaves nothing to reconcile — same commented swallow as the existing two. * Bare `ipcRenderer.invoke` calls reject when the main handler throws: revealInFolder, startNewRecording, and the recording-prefs and selected-source reads. These log, because a failed IPC means something is genuinely broken. * The background duration probe in `useTimeline` ends in `saveDocument`, which throws on a failed write — the same shape as the registry bug. Not fixed here, deliberately: 12 sites where a user-initiated timeline mutation (removeRegion, removeClip, duplicateClip, insertClipAt, applyTimelineOp) can fail silently, because neither `useTimeline` nor `useSequentialTimelineOps` catches anything. Twelve scattered `console.warn`s would bury a real user-facing failure; they all route through two functions, so the fix belongs there with a toast — which is a UX change, not an audit cleanup. `handleSave` already does exactly that, so the pattern to copy is next door.
📝 WalkthroughWalkthroughThe pull request changes Vitest to Node by default, adds explicit jsdom annotations, documents targeted testing, handles rejected asynchronous operations, expands CI checks, and ensures Discord validation timers are cleared after failures. ChangesTesting and reliability updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5 tasks
Collaborator
Author
|
Follow-up for the twelve timeline-mutation sites this PR deliberately left alone: #282. They all route through |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.harness/docs/git-workflow.md:
- Around line 24-33: Update the native-helper manual smoke-test instruction in
the CI documentation to reference the repository’s actual helper paths,
electron/native/screencapturekit/ and electron/native/wgc-capture/, instead of
the incomplete electron/*-helper/ glob. Preserve the requirement to note manual
testing in the PR description.
In `@src/components/ai-edition/NewEditorShell.tsx`:
- Around line 686-688: Remove the startNewRecording invocation from
handleConfirmUnsaved when action === "record"; let handleNewRecording initiate
recording after the unsaved-changes confirmation resolves, while preserving the
existing confirmation flow for other actions.
In `@src/components/ai-edition/v4/RecStage.tsx`:
- Around line 79-84: Update updatePrefs so the recording preference patch is
persisted through setRecordingPrefs before committing it with setPrefsState.
Preserve the existing state on rejection, and retain the warning for failed
persistence so useScreenRecorder does not observe an uncommitted value.
In `@technical-documentation/testing/writing-tests.md`:
- Around line 21-24: Align the Vitest test-scope patterns in
technical-documentation/testing/writing-tests.md (lines 21-24) and
.harness/reins/openscreen-tester/agent.md (lines 12-13) with vitest.config.ts by
including both .test and .spec files and the js, mjs, cjs, ts, mts, cts, jsx,
and tsx extensions under the existing src, electron, and .github directories.
- Around line 40-42: Update the jsdom guidance in the test-environment section
so configuration is based on actual DOM or browser-global usage, not the
.test.tsx extension. State that tests rendering components, using renderHook, or
accessing browser globals require jsdom, while TS and TSX tests without those
behaviors remain in the Node environment.
In `@vitest.config.ts`:
- Around line 12-14: Correct the Vitest environment explanation in the
surrounding comment by identifying the legacy directive in
electron/media/audioPeaks.test.ts as `@vitest-environment` node, or remove the
parenthetical entirely; do not describe jsdom as escaping a global jsdom
environment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc6e724b-8c7b-404f-a262-ee186007e836
📒 Files selected for processing (53)
.github/scripts/discord-thread-validator.mjs.github/scripts/discord-thread-validator.test.mjs.harness/docs/git-workflow.md.harness/reins/openscreen-dev/agent.md.harness/reins/openscreen-tester/agent.mdAGENTS.mdelectron/ai-edition/document-service.test.tspackage.jsonsrc/components/ai-edition/CaptionsPane.gating.test.tsxsrc/components/ai-edition/ChatWelcome.test.tsxsrc/components/ai-edition/ColorField.test.tsxsrc/components/ai-edition/EditorEmptyState.test.tsxsrc/components/ai-edition/ExportDialog.tsxsrc/components/ai-edition/Modals.tsxsrc/components/ai-edition/NewEditorShell.tsxsrc/components/ai-edition/NewProjectModal.test.tsxsrc/components/ai-edition/Preview.test.tsxsrc/components/ai-edition/RightPanes.i18n.test.tsxsrc/components/ai-edition/TranscriptPane.gating.test.tsxsrc/components/ai-edition/TranscriptPane.keyboardCut.test.tsxsrc/components/ai-edition/TranscriptPane.sharedMedia.test.tsxsrc/components/ai-edition/TransportBar.test.tsxsrc/components/ai-edition/VirtualPreview.playback.test.tsxsrc/components/ai-edition/WebcamOverlay.test.tsxsrc/components/ai-edition/backgroundImageUpload.test.tsxsrc/components/ai-edition/v4/EditorTopBar.test.tsxsrc/components/ai-edition/v4/RecStage.tsxsrc/components/ai-edition/v4/SpeedControl.test.tsxsrc/components/ai-edition/v4/V4Timeline.geometry.test.tsxsrc/components/launch/LaunchWindow.test.tsxsrc/components/launch/NotesToolbar.test.tsxsrc/components/launch/NotesWindow.editable.test.tsxsrc/components/launch/NotesWindow.test.tsxsrc/components/launch/SourceSelector.test.tsxsrc/components/ui/gradient-editor.test.tsxsrc/hooks/recorderHandle.test.tssrc/hooks/useAudioPeaks.test.tssrc/hooks/useCameraDevices.test.tssrc/hooks/useScreenRecorder.nativeStopFailure.test.tsxsrc/hooks/useScreenRecorder.tssrc/lib/ai-edition/store/projectStore.test.tssrc/lib/ai-edition/store/transcriptionStore.test.tssrc/lib/ai-edition/store/useSequentialTimelineOps.test.tssrc/lib/ai-edition/store/useTimeline.test.tssrc/lib/ai-edition/store/useTimeline.tssrc/lib/ai-edition/timeline/duration.test.tssrc/lib/ai-edition/timeline/pointer-drag.test.tsxsrc/lib/captioning/transcribe.test.tssrc/native/hooks/useCompositorBackend.test.tssrc/native/hooks/useNativeCompositorView.test.tssrc/utils/platformUtils.test.tstechnical-documentation/testing/writing-tests.mdvitest.config.ts
Comment on lines
+24
to
+33
| CI runs on every PR to `main`, `feat/ai-edition` and `release/**`, and on every push to those: | ||
| - `npm run lint` (Biome) | ||
| - `npx tsc --noEmit` (TypeScript) | ||
| - `npx tsc --noEmit` (TypeScript, app code) | ||
| - `npx tsc -p tsconfig.test.json --noEmit` (TypeScript, test files — a separate gate at zero) | ||
| - `npm run test` (Vitest unit) | ||
| - `npm run test:browser` (Vitest + Playwright headless) | ||
| - `npm run docs:check` | ||
| - `npx vite build` (renderer build smoke) | ||
| - `cargo test` / `cargo check` for the compositor on macOS, Windows and Linux | ||
|
|
||
| All five must be green before merge. Native helper code is NOT covered by CI — manual smoke test is required for `electron/*-helper/` changes; note it in the PR description. | ||
| All must be green before merge. Native helper code is NOT covered by CI — manual smoke test is required for `electron/*-helper/` changes; note it in the PR description. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the repository's native-helper paths.
Line 33 names electron/*-helper/, but the documented helpers live under electron/native/screencapturekit/ and electron/native/wgc-capture/. The current glob does not cover those paths. A native change can therefore bypass the manual smoke-test instruction.
Proposed wording
-Native helper code is NOT covered by CI — manual smoke test is required for `electron/*-helper/` changes; note it in the PR description.
+Native helper code is NOT covered by CI — manual smoke test is required for changes under `electron/native/`; note it in the PR description.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CI runs on every PR to `main`, `feat/ai-edition` and `release/**`, and on every push to those: | |
| - `npm run lint` (Biome) | |
| - `npx tsc --noEmit` (TypeScript) | |
| - `npx tsc --noEmit` (TypeScript, app code) | |
| - `npx tsc -p tsconfig.test.json --noEmit` (TypeScript, test files — a separate gate at zero) | |
| - `npm run test` (Vitest unit) | |
| - `npm run test:browser` (Vitest + Playwright headless) | |
| - `npm run docs:check` | |
| - `npx vite build` (renderer build smoke) | |
| - `cargo test` / `cargo check` for the compositor on macOS, Windows and Linux | |
| All five must be green before merge. Native helper code is NOT covered by CI — manual smoke test is required for `electron/*-helper/` changes; note it in the PR description. | |
| All must be green before merge. Native helper code is NOT covered by CI — manual smoke test is required for `electron/*-helper/` changes; note it in the PR description. | |
| CI runs on every PR to `main`, `feat/ai-edition` and `release/**`, and on every push to those: | |
| - `npm run lint` (Biome) | |
| - `npx tsc --noEmit` (TypeScript, app code) | |
| - `npx tsc -p tsconfig.test.json --noEmit` (TypeScript, test files — a separate gate at zero) | |
| - `npm run test` (Vitest unit) | |
| - `npm run docs:check` | |
| - `npx vite build` (renderer build smoke) | |
| - `cargo test` / `cargo check` for the compositor on macOS, Windows and Linux | |
| All must be green before merge. Native helper code is NOT covered by CI — manual smoke test is required for changes under `electron/native/`; note it in the PR description. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.harness/docs/git-workflow.md around lines 24 - 33, Update the native-helper
manual smoke-test instruction in the CI documentation to reference the
repository’s actual helper paths, electron/native/screencapturekit/ and
electron/native/wgc-capture/, instead of the incomplete electron/*-helper/ glob.
Preserve the requirement to note manual testing in the PR description.
Source: Coding guidelines
Comment on lines
+686
to
+688
| void window.electronAPI?.startNewRecording?.().catch((err) => { | ||
| console.warn("[editor] failed to start a new recording:", err); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid starting the same recording twice after confirmation.
When the project is dirty, handleConfirmUnsaved calls startNewRecording, resolves the confirmation promise, and handleNewRecording calls startNewRecording again at Lines 699-701. This can start two recording flows.
Let handleNewRecording own the recording start after confirmation. Remove the action === "record" call from handleConfirmUnsaved.
Suggested fix
- const { action, resolve } = unsavedPrompt;
+ const { resolve } = unsavedPrompt;
...
- if (action === "record") {
- void window.electronAPI?.startNewRecording?.().catch((err) => {
- console.warn("[editor] failed to start a new recording:", err);
- });
- }
resolve(choice);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ai-edition/NewEditorShell.tsx` around lines 686 - 688, Remove
the startNewRecording invocation from handleConfirmUnsaved when action ===
"record"; let handleNewRecording initiate recording after the unsaved-changes
confirmation resolves, while preserving the existing confirmation flow for other
actions.
Comment on lines
79
to
+84
| const updatePrefs = (patch: Partial<RecordingPrefsState>) => { | ||
| setPrefsState((prev) => { | ||
| const next = { ...prev, ...patch }; | ||
| void window.electronAPI?.setRecordingPrefs?.(patch); | ||
| void window.electronAPI?.setRecordingPrefs?.(patch).catch((err) => { | ||
| console.warn("[rec-stage] failed to persist the recording prefs:", err); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not commit recording preferences before persistence succeeds.
setPrefsState applies the patch before setRecordingPrefs resolves. If the IPC call rejects, RecStage shows the new value, but useScreenRecorder reads the old main-process value for the next recording at Lines 205-220. Commit local state after persistence succeeds, or re-read or roll back the failed patch and report the failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ai-edition/v4/RecStage.tsx` around lines 79 - 84, Update
updatePrefs so the recording preference patch is persisted through
setRecordingPrefs before committing it with setPrefsState. Preserve the existing
state on rejection, and retain the warning for failed persistence so
useScreenRecorder does not observe an uncommitted value.
Comment on lines
21
to
24
| **Config:** `vitest.config.ts` | ||
| **Runs in:** jsdom (simulated DOM, no real browser) | ||
| **File pattern:** `src/**/*.test.ts` — anything that does **not** end in `.browser.test.ts` | ||
| **Runs in:** Node by default; jsdom only for files that ask for it | ||
| **File pattern:** `{src,electron,.github}/**/*.test.{ts,tsx}` | ||
| **CI command:** `npm run test` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep Vitest test-scope documentation aligned with vitest.config.ts.
The documentation lists only TypeScript *.test files, but the configured suite also includes .spec and JavaScript-family extensions. This can hide supported tests from contributors and the tester agent.
technical-documentation/testing/writing-tests.md#L21-L24: document{src,electron,.github}/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}..harness/reins/openscreen-tester/agent.md#L12-L13: expand the Vitest ownership pattern to the same extensions.
📍 Affects 2 files
technical-documentation/testing/writing-tests.md#L21-L24(this comment).harness/reins/openscreen-tester/agent.md#L12-L13
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@technical-documentation/testing/writing-tests.md` around lines 21 - 24, Align
the Vitest test-scope patterns in
technical-documentation/testing/writing-tests.md (lines 21-24) and
.harness/reins/openscreen-tester/agent.md (lines 12-13) with vitest.config.ts by
including both .test and .spec files and the js, mjs, cjs, ts, mts, cts, jsx,
and tsx extensions under the existing src, electron, and .github directories.
Source: Coding guidelines
Comment on lines
+40
to
+42
| That is also the fix when a new test dies on `document is not defined` or | ||
| `ReferenceError: window is not defined`. Every `*.test.tsx` needs it; a `*.test.ts` needs | ||
| it only if it renders a component, uses `renderHook`, or reaches for a browser global. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Make jsdom opt-in based on DOM usage, not the file extension.
Line 41 says every *.test.tsx file needs jsdom. This conflicts with the Node default and the conditional rule on Line 42. A TSX test that does not render a component or access browser globals should remain in the Node environment.
Proposed wording
-Every `*.test.tsx` needs it; a `*.test.ts` needs it only if it renders a component, uses `renderHook`, or reaches for a browser global.
+A test needs it only when it renders a component, uses `renderHook`, or reaches for a browser global.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| That is also the fix when a new test dies on `document is not defined` or | |
| `ReferenceError: window is not defined`. Every `*.test.tsx` needs it; a `*.test.ts` needs | |
| it only if it renders a component, uses `renderHook`, or reaches for a browser global. | |
| That is also the fix when a new test dies on `document is not defined` or | |
| `ReferenceError: window is not defined`. A test needs it only when it renders a component, uses `renderHook`, or reaches for a browser global. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@technical-documentation/testing/writing-tests.md` around lines 40 - 42,
Update the jsdom guidance in the test-environment section so configuration is
based on actual DOM or browser-global usage, not the .test.tsx extension. State
that tests rendering components, using renderHook, or accessing browser globals
require jsdom, while TS and TSX tests without those behaviors remain in the Node
environment.
Source: Coding guidelines
Comment on lines
+12
to
+14
| // defined`. (`electron/media/audioPeaks.test.ts` already used the same | ||
| // docblock the other way round, to escape the global jsdom; that one is now | ||
| // redundant but harmless.) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n '^// `@vitest-environment` (node|jsdom)$' electron/media/audioPeaks.test.tsRepository: getopenscreen/openscreen
Length of output: 192
Correct the Vitest environment explanation.
@vitest-environment jsdom enables jsdom. It cannot escape a global jsdom environment; the legacy directive used by electron/media/audioPeaks.test.ts is @vitest-environment node. Update the parenthetical to name node, or remove it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vitest.config.ts` around lines 12 - 14, Correct the Vitest environment
explanation in the surrounding comment by identifying the legacy directive in
electron/media/audioPeaks.test.ts as `@vitest-environment` node, or remove the
parenthetical entirely; do not describe jsdom as escaping a global jsdom
environment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Running the tests was slow enough to distort how work gets done in this repo, so this
looks at both halves of that: what the suite actually costs, and what the agent-facing
docs tell people to run.
The suite was not bloated — the environment was. It built a full jsdom for every one
of its 140 test files, and only 37 of them ever touch a DOM. The other 103 paid for one
and threw it away: 719s of cumulative environment setup against 89s of actual test
time. Flipping the default to
nodeand letting the 37 opt back in with a// @vitest-environment jsdomdocblock takes the full run from 175s to 81s, back toback on the same machine, with no change to what any test asserts. The median file is
78ms — there is nothing to delete here, so nothing was deleted.
The 37 were derived by running the suite under
--environment=nodeand taking the filesthat failed, not guessed from imports: 12 of them are
.tsfiles (zustand stores, hooks,platformUtils) that a.tsx-only heuristic would have missed.electron/media/audioPeaks.test.tsalready carried the same docblock the other wayround, to escape the global jsdom — the mechanism was already in the repo, this just
inverts which side needs it.
testTimeoutgoes to 15s in the same change because the two interact. With themachine loaded, 11 tests fail and 9 of those are purely
Test timed out in 5000ms—ordinary component tests that pass in 200ms idle. A single jsdom file needs ~9.5s just to
boot React, so a 5s budget was never a signal about the test. Same 6-CPU-burner stress
run: 11 failures before, 0 after.
Four things that look like speedups and are not are recorded in the config comment
with what each measured, so the next person doesn't re-run that benchmark:
--no-isolate(~20% faster but breaksvi.mock, which 29 test files depend on),deps.optimizer.web(slower),--pool=threads(>5min, killed),--maxWorkers=16(inside the noise, and machine-specific).
The docs half. Every agent-facing surface defined "done" as
npm run test— the fullsuite — so an agent ran ~1670 tests after each edit. AGENTS.md, both harness reins and
the git-workflow doc now say: targeted run while working (
npx vitest --run <path>, orthe new
npm run test:changed), tsc and biome as the inner loop, one full run at the endor left to CI. This also removes a documented test tier that does not exist:
npm run test:browserandvitest.browser.config.tshad 80 lines of docs across fourfiles, a worked example and a timeouts section — but no script, no config, no
*.browser.test.tsfile and no CI job.Two smaller fixes came out of chasing an intermittent
Errors 1 erroron otherwise greenruns. See the commits for detail — the audit one lists what it deliberately did not
change and why.
Related issue
Type of change
Release impact
Desktop impact
Screenshots / video
Not a visual change.
Testing
Measured back to back on the same machine (Windows, 8 cores), same tree, only the config
differing:
Final state on this branch:
The two behaviour fixes were each verified by ablation, not just by the suite going green:
validateThreadChannelcounting armed timers. Against the pre-fix file the rejecting path leaves 1 timer armed;
after, all four leave 0. The vitest regression test fails with
expected 1 to be +0whenthe
finallyis reverted.React components from a node script would prove nothing the
.catch()in the diff doesnot already show. Stated plainly rather than implied.
Notes for the reviewer
Two things worth knowing:
This branch was cut fresh from
mainafter the work was done, because the branch itstarted on was 108 commits behind. Re-deriving on the current tree mattered: the jsdom
set moved from 36 files to 37 (
useScreenRecorder.nativeStopFailure.test.tsxis new),and two fixes I had written turned out to be already on
main— themediaLinksRegistrydetached-write catch (2c4c426, same root cause, same diagnosis)and the
webm-seek-indexplatform pinning (main's version is better than mine, usingNodeJS.Platformandit.each(NON_LINUX)). Both were dropped in favour of main's.A pre-existing flake, untouched by this PR:
mediaLinksRegistry.test.ts > logs a refresh it cannot writepasses 3/3 alone and failsunder full-suite load — it asserts on a warning emitted by a detached promise, so it
races. It is main's own test and a different problem from the timeout class fixed here;
flagging rather than silently widening scope.
Summary by CodeRabbit
Bug Fixes
Testing
Documentation