test: speed up slow dashboard test suites - #1763
Merged
Merged
Conversation
Reduce feedback-loop wall-time on the slowest dashboard test files (FN-5048 velocity), behavior-preserving — no assertions, timeouts, or worker counts changed. - SettingsModal.test.tsx (231 tests, ~60.5s in one sequential worker) split into 4 sibling files sharing SettingsModal.test-harness.tsx so the settings project parallelizes them across workers (~60.5s -> ~25s wall). Test bodies moved verbatim; static it() count preserved at 231. vi.mock factories stay per-file and reference the harness-exported mock fns; shared beforeEach/afterEach via installSettingsModalEnv(). - insights-routes.test.ts: lazily init the project-b store (only 1 of 24 tests needs it) instead of a second TaskStore.init() per test (tests 5.78s -> 3.66s). - evals-routes.test.ts: same lazy storeB pattern. - vitest.config.ts: point the settings-only project at the 4 new files; drop the bare "SettingsModal" entry from the component shards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a shared SettingsModal test harness, splits SettingsModal coverage into multiple sharded Vitest suites, and updates evals/insights route tests to lazily create the project-b task store during scoped cases. ChangesSettingsModal test coverage and sharding
Dashboard route test store setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
Ready to review this PR? Stage has broken it down into 4 individual chapters for you: 
Chapters generated by Stage for commit a1e7602 on Jun 25, 2026 6:04pm UTC. |
Contributor
Greptile SummaryThis PR speeds up the slowest dashboard test suites by splitting the 231-test
Confidence Score: 5/5All changes are test infrastructure only — no production code, assertions, or timeout values are touched. The split is behavior-preserving: 231 tests moved verbatim into four isolated files; the backfill exclusion that was missing in the prior review cycle is now correctly in place via No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Before
OldFile[SettingsModal.test.tsx 231 tests 1 worker]
end
subgraph After
Harness[SettingsModal.test-harness.tsx]
F1[SettingsModal.general.test.tsx]
F2[SettingsModal.models-auth.test.tsx]
F3[SettingsModal.scheduling-merge.test.tsx]
F4[SettingsModal.remote-notifications.test.tsx]
Harness --> F1
Harness --> F2
Harness --> F3
Harness --> F4
end
subgraph Config
Settings[dashboard-app-quality-settings]
Backfill[dashboard-app-quality-backfill excludes all 4 files]
F1 & F2 & F3 & F4 --> Settings
F1 & F2 & F3 & F4 -.->|excluded| Backfill
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph Before
OldFile[SettingsModal.test.tsx 231 tests 1 worker]
end
subgraph After
Harness[SettingsModal.test-harness.tsx]
F1[SettingsModal.general.test.tsx]
F2[SettingsModal.models-auth.test.tsx]
F3[SettingsModal.scheduling-merge.test.tsx]
F4[SettingsModal.remote-notifications.test.tsx]
Harness --> F1
Harness --> F2
Harness --> F3
Harness --> F4
end
subgraph Config
Settings[dashboard-app-quality-settings]
Backfill[dashboard-app-quality-backfill excludes all 4 files]
F1 & F2 & F3 & F4 --> Settings
F1 & F2 & F3 & F4 -.->|excluded| Backfill
end
Reviews (2): Last reviewed commit: "Address PR review feedback (#1763)" | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/dashboard/app/components/__tests__/SettingsModal.general.test.tsx`:
- Around line 694-721: The test file uses PersistSettingInput in the it.each
call but never imports it, causing a TypeScript error; add the missing type
import from SettingsModal.test-harness alongside the existing test harness
imports so the SettingsModal.general.test.tsx suite compiles cleanly.
In `@packages/dashboard/src/__tests__/evals-routes.test.ts`:
- Around line 38-46: Make getStoreB() return and reuse a shared initialization
promise so concurrent project-b lookups cannot observe storeB before init
completes. Update the helper around getStoreB and the related project-b test
setup so the first call creates the store and stores the init promise, and
subsequent calls await that same promise before returning the TaskStoreClass
instance.
In `@packages/dashboard/src/__tests__/insights-routes.test.ts`:
- Around line 89-97: Serialize the lazy initialization in getStoreB so
concurrent calls cannot observe a partially initialized TaskStore: instead of
assigning storeB before await storeB.init() completes, add a pending creation
promise in the test helper and have getStoreB dedupe and await that promise
until initialization finishes. Keep the behavior aligned with
getOrCreateProjectStore and update the affected assertions around getStoreB so
the mock matches the production contract for concurrent project-scoped requests.
🪄 Autofix (Beta)
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: 83c37df9-4e7b-45c1-9412-4d4936590bc7
📒 Files selected for processing (9)
packages/dashboard/app/components/__tests__/SettingsModal.general.test.tsxpackages/dashboard/app/components/__tests__/SettingsModal.models-auth.test.tsxpackages/dashboard/app/components/__tests__/SettingsModal.remote-notifications.test.tsxpackages/dashboard/app/components/__tests__/SettingsModal.scheduling-merge.test.tsxpackages/dashboard/app/components/__tests__/SettingsModal.test-harness.tsxpackages/dashboard/app/components/__tests__/SettingsModal.test.tsxpackages/dashboard/src/__tests__/evals-routes.test.tspackages/dashboard/src/__tests__/insights-routes.test.tspackages/dashboard/vitest.config.ts
Comment on lines
+694
to
+721
| it.each<PersistSettingInput>([ | ||
| { | ||
| section: "Project General", | ||
| label: "Completion Documentation Automation", | ||
| kind: "select", | ||
| value: "changeset", | ||
| scope: "project", | ||
| expectedKey: "completionDocumentationMode", | ||
| }, | ||
| { | ||
| section: "Project General", | ||
| label: "Auto-cleanup old chats", | ||
| kind: "select", | ||
| value: 14, | ||
| scope: "project", | ||
| expectedKey: "chatAutoCleanupDays", | ||
| }, | ||
| { | ||
| section: "Project General", | ||
| label: "Operational log retention", | ||
| kind: "select", | ||
| value: 7, | ||
| scope: "project", | ||
| expectedKey: "operationalLogRetentionDays", | ||
| }, | ||
| ])("persists $expectedKey through the expected settings scope", async (input) => { | ||
| await expectSettingPersists(input); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm PersistSettingInput is exported from the harness but not imported in the test file.
fd 'SettingsModal.test-harness.tsx' --exec rg -n 'export type PersistSettingInput' {}
fd 'SettingsModal.general.test.tsx' --exec rg -n 'PersistSettingInput' {}Repository: Runfusion/Fusion
Length of output: 233
PersistSettingInput is referenced but never imported.
The type PersistSettingInput is used at line 694 but is not imported in this file, causing a TypeScript compilation error. It is exported from SettingsModal.test-harness.
🐛 Add the missing type import
expectSettingPersists,
+ type PersistSettingInput,
installSettingsModalEnv,
} from "./SettingsModal.test-harness";🤖 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 `@packages/dashboard/app/components/__tests__/SettingsModal.general.test.tsx`
around lines 694 - 721, The test file uses PersistSettingInput in the it.each
call but never imports it, causing a TypeScript error; add the missing type
import from SettingsModal.test-harness alongside the existing test harness
imports so the SettingsModal.general.test.tsx suite compiles cleanly.
Comment on lines
+38
to
+46
| let rootB: string | null = null; | ||
| let storeB: TaskStore | null = null; | ||
| async function getStoreB(): Promise<TaskStore> { | ||
| if (!storeB) { | ||
| rootB = mkdtempSync(join(tmpdir(), "kb-evals-routes-b-")); | ||
| storeB = new TaskStoreClass(rootB, join(rootB, ".fusion-global-settings"), { inMemoryDb: true }); | ||
| await storeB.init(); | ||
| } | ||
| return storeB; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make getStoreB() return the shared init promise.
Line 41 makes storeB visible before await storeB.init() completes, so concurrent project-b lookups can observe an uninitialized store. That differs from the real resolver behavior, which deduplicates creation and resolves only after initialization finishes.
Suggested fix
let rootB: string | null = null;
let storeB: TaskStore | null = null;
+let storeBInit: Promise<TaskStore> | null = null;
async function getStoreB(): Promise<TaskStore> {
- if (!storeB) {
+ if (!storeBInit) {
rootB = mkdtempSync(join(tmpdir(), "kb-evals-routes-b-"));
storeB = new TaskStoreClass(rootB, join(rootB, ".fusion-global-settings"), { inMemoryDb: true });
- await storeB.init();
+ storeBInit = storeB.init().then(() => storeB!);
}
- return storeB;
+ return storeBInit;
}- rootB = null;
- storeB = null;
+ rootB = null;
+ storeB = null;
+ storeBInit = null;Also applies to: 52-53
🤖 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 `@packages/dashboard/src/__tests__/evals-routes.test.ts` around lines 38 - 46,
Make getStoreB() return and reuse a shared initialization promise so concurrent
project-b lookups cannot observe storeB before init completes. Update the helper
around getStoreB and the related project-b test setup so the first call creates
the store and stores the init promise, and subsequent calls await that same
promise before returning the TaskStoreClass instance.
Comment on lines
+89
to
+97
| let rootB: string | null = null; | ||
| let storeB: TaskStore | null = null; | ||
| async function getStoreB(): Promise<TaskStore> { | ||
| if (!storeB) { | ||
| rootB = mkdtempSync(join(tmpdir(), "kb-insights-routes-b-")); | ||
| storeB = new TaskStoreClass(rootB, join(rootB, ".fusion-global-settings"), { inMemoryDb: true }); | ||
| await storeB.init(); | ||
| } | ||
| return storeB; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Serialize the lazy store initialization.
Line 92 assigns storeB before init() finishes, so a second concurrent getStoreB() call can resolve with a half-initialized store. The real getOrCreateProjectStore() path deduplicates the pending creation promise, so this mock no longer matches the production contract for concurrent project-scoped requests.
Suggested fix
let rootB: string | null = null;
let storeB: TaskStore | null = null;
+let storeBInit: Promise<TaskStore> | null = null;
async function getStoreB(): Promise<TaskStore> {
- if (!storeB) {
+ if (!storeBInit) {
rootB = mkdtempSync(join(tmpdir(), "kb-insights-routes-b-"));
storeB = new TaskStoreClass(rootB, join(rootB, ".fusion-global-settings"), { inMemoryDb: true });
- await storeB.init();
+ storeBInit = storeB.init().then(() => storeB!);
}
- return storeB;
+ return storeBInit;
}- rootB = null;
- storeB = null;
+ rootB = null;
+ storeB = null;
+ storeBInit = null;Also applies to: 120-121
🤖 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 `@packages/dashboard/src/__tests__/insights-routes.test.ts` around lines 89 -
97, Serialize the lazy initialization in getStoreB so concurrent calls cannot
observe a partially initialized TaskStore: instead of assigning storeB before
await storeB.init() completes, add a pending creation promise in the test helper
and have getStoreB dedupe and await that promise until initialization finishes.
Keep the behavior aligned with getOrCreateProjectStore and update the affected
assertions around getStoreB so the mock matches the production contract for
concurrent project-scoped requests.
- Exclude the 4 SettingsModal split files from the app backfill project (greptile P1): the split dropped the bare "SettingsModal" entry from qualityAppComponentTests, which had excluded the curated file from backfill; without spreading qualityAppSettingsOnlyTests into backfillAppExclude the split files matched the backfill `app/**` glob and ran in two projects, doubling their wall-time. Now collected by exactly one project (dashboard-app-quality-settings). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Addressed in a1e7602: spread Verified with |
Contributor
|
The fix in a1e7602 is exactly right — spreading Tip: You can customize Greptile's behavior for this repo with |
This was referenced Jul 16, 2026
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.
What & why
Cuts feedback-loop wall-time on the slowest dashboard test files (FN-5048 velocity). Profiled the velocity baseline's top offenders; all changes are behavior-preserving — no assertions, timeouts, or worker/concurrency knobs changed.
Changes
SettingsModal.test.tsx(#1, ~60.5s)SettingsModal.test-harness.tsx; the settings project parallelizes them across workersinsights-routes.test.ts(#2, ~26.5s)TaskStore.init()per test; only 1 of 24 tests needs the project-b storestoreBon first useevals-routes.test.tsstoreBSettingsModal split mechanics
it()count preserved at 231.vi.mockfactories stay in each test file (they only apply per module) and reference the ~61 mock fns imported from the harness.beforeEach/afterEachregistered via the harness's exportedinstallSettingsModalEnv().vitest.config.ts:qualityAppSettingsOnlyTestsnow points at the 4 new files; bare"SettingsModal"dropped fromqualityAppComponentTests/isolatedQualityAppComponentTests.Verification
it()s — matches.check-test-inventory --dashboard-curatedpassed.insights-routes24/24 andevals-routes7/7 pass.Notes
test-timings.json,line-count-baseline.json,test-velocity-history.json); they refresh on the nextpnpm test:velocityrun.🤖 Generated with Claude Code
Summary by CodeRabbit