perf(dashboard): de-duplicate overlapping vitest projects - #1700
Merged
Conversation
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Vitest configuration adds a ChangesVitest FUSION_DASHBOARD_DEEP Gating
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 3 individual chapters for you:
Chapters generated by Stage for commit 91957d4 on Jun 21, 2026 9:09am UTC. |
The default `vitest run` (no --project) path — used by `pnpm test` via test-changed's `vitest --changed` scoping — ran every dashboard test file in up to 3 overlapping projects: the `dashboard-app-quality` umbrella, a curated shard, and the broad `dashboard-app`/`dashboard-api` lanes. A scoped run selected 1899 executions for 829 unique files (2.3x redundant work). - Remove the dead `dashboard-app-quality` umbrella project: it re-ran the exact union of its eight curated shards. It is absent from test-inventory-spec.json, dashboardQualityProjectGlobs, and every script; package-config's contract test already asserts test:deep must not use it. - Gate the broad `dashboard-app`/`dashboard-api` includes behind FUSION_DASHBOARD_DEEP so they are empty in the default run (no longer duplicating the curated shards + backfill) but remain selectable via --project. The explicit deep escape hatches (test:deep/test:app/test:api/ test:build) set the flag to opt back in. Default scoped run now selects 829 executions for 829 files (1x). Verified: curated-gate inventory guard intact (828 files, 829 executed, 1 skip-listed), package-config contract test green (31/31), build-output.test.ts still reachable via test:build, deep hatch repopulates with the flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/app/components/WorkflowSwitcher.tsx (1)
194-196:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve a keyboard path to workflow edit/create actions.
Line 194 closes the menu on
Tabwhile focus stays on the trigger, but edit/create controls now exist only inside the dropdown (Line 264+ and Line 284+). Keyboard-only users can’t reliably reach those actions.Suggested direction
case "Tab": - setIsOpen(false); + if (isOpen) { + const firstMenuControl = dropdownRef.current?.querySelector<HTMLButtonElement>( + ".workflow-switcher-option, .workflow-switcher-edit, .workflow-switcher-create", + ); + if (firstMenuControl) { + event.preventDefault(); + firstMenuControl.focus(); + return; + } + } + setIsOpen(false); break;Also applies to: 264-295
🤖 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/WorkflowSwitcher.tsx` around lines 194 - 196, The Tab key handler in the WorkflowSwitcher component closes the dropdown immediately, preventing keyboard-only users from reaching the edit and create controls which now exist only inside the dropdown. Instead of closing the menu on Tab, allow the default Tab behavior to proceed so focus can move forward through the dropdown content to reach those actions. Remove or modify the case statement for Tab so it doesn't call setIsOpen(false), allowing natural keyboard navigation to continue through the dropdown controls.
🧹 Nitpick comments (2)
packages/dashboard/vitest.config.ts (1)
330-337: ⚡ Quick winAdd an FNXC-formatted requirement header for the deep-lane policy.
Line 330 documents a behavior-defining test-lane policy; please encode it with the repo’s FNXC convention so this requirement remains traceable in future edits.
Suggested update
+/* +FNXC:DashboardTesting 2026-06-21-00:00: +Deep lanes (`dashboard-app`/`dashboard-api`) are opt-in via FUSION_DASHBOARD_DEEP=1 to avoid duplicate execution in default scoped runs. +*/ // The broad `dashboard-app` / `dashboard-api` lanes fully duplicate the curated // shards + backfill projects, which already partition app/ and src/ exactly // once. They exist ONLY as the explicit deep escape hatchesAs per coding guidelines
**/*.{ts,tsx,js,jsx}: “Add FNXC_LOG comments (format:FNXC:Area-of-product yyyy-MM-dd-hh:mm:) …”; based on learnings, follow FNXC format directly (no literalFNXC_LOGtoken in comment text).🤖 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/vitest.config.ts` around lines 330 - 337, The comment block documenting the deep-lane policy (starting with "// The broad `dashboard-app` / `dashboard-api` lanes") needs to be prefixed with an FNXC-formatted requirement header to make it traceable. Add a new comment line at the beginning of this comment block following the format FNXC:Area-of-product yyyy-MM-dd-hh:mm: (without the literal `FNXC_LOG` token), identifying the relevant product area and using the current timestamp, then keep the existing explanatory comments intact below it.Sources: Coding guidelines, Learnings
packages/engine/src/__tests__/reliability-interactions/merge-runner-spawn-enoent-prevention.test.ts (1)
87-115: ⚡ Quick winAdd FNXC rationale comment above the FN-6817 regression test.
Line 87 introduces requirement-specific fixture-root behavior; please add an FNXC-formatted header so the reason for this invariant remains discoverable.
As per coding guidelines
**/*.{ts,tsx,js,jsx}: “Add FNXC_LOG comments (format:FNXC:Area-of-product yyyy-MM-dd-hh:mm:) …”; based on learnings, use the FNXC format directly rather than adding a literalFNXC_LOGtoken.🤖 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/engine/src/__tests__/reliability-interactions/merge-runner-spawn-enoent-prevention.test.ts` around lines 87 - 115, The test function for "FN-6817: roots the shared reliability fixture under the Vitest worker root" is missing a FNXC-formatted header comment that explains the rationale for this requirement-specific fixture-root behavior. Add an FNXC comment directly above the test function declaration using the format FNXC:Area-of-product yyyy-MM-dd-hh:mm: to document why this invariant behavior is necessary for the reliability interactions test.Sources: Coding guidelines, Learnings
🤖 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/desktop/src/local-runtime.ts`:
- Around line 53-72: The issue is that if initialization fails after creating
the CentralCore and ProjectEngineManager but before returning the cleanup
function (for example, if app.listen throws an error), the engines and central
core are never properly torn down. Fix this by wrapping the initialization logic
in a try-finally block within the function that contains CentralCore
initialization, ProjectEngineManager creation, and app.listen call. In the
finally block, ensure that engineManager.stopAll() and centralCore.close() are
always called regardless of success or failure. Additionally, ensure that
store.close() is called unconditionally in both the startup error catch handler
and the stopInternal cleanup path to guarantee proper resource cleanup across
all failure scenarios.
In `@packages/desktop/src/local-server.ts`:
- Around line 63-79: The cleanup function is defined only after createServer and
listen are called, so if either throws an error, the already-started
engineManager and centralCore will never be torn down. Additionally, if cleanup
throws in either the catch or stop paths, it prevents store.close() from
running. To fix this, define and populate the cleanup logic before calling
createServer and listen so it can handle early startup failures, and restructure
the error handling to ensure store.close() is always called even if cleanup
throws, likely by using a try-finally block or separating the cleanup execution
from store.close() so both are guaranteed to run.
- Around line 52-55: The dynamic imports of TaskStore, CentralCore,
createServer, and ProjectEngineManager on lines 52-55 must be converted to
static imports at the module level. Move these four import statements to the top
of the local-server.ts file using standard static import syntax (import { ... }
from "...") instead of dynamic await import() calls. Since these imports are
used immediately without lazy-loading justification, there is no valid reason to
keep them as runtime dynamic imports, and they must be statically analyzable per
the repo's requirements for `@fusion/`* packages.
In `@packages/desktop/src/main.ts`:
- Around line 241-249: In the onStartLocalRuntime function, replace the
createdWindow.webContents.reload() call with a pattern that loads a clean
renderer entrypoint instead. The issue is that reload() preserves the current
URL and query parameters, which keeps the UI bound to the remote context even
after switching to local mode. Follow the same loading pattern used in the
resetLaunchModeAndReload function to ensure a fresh start for the local runtime
context.
In `@packages/desktop/src/menu.ts`:
- Around line 18-22: The runMenuAction function has a synchronous error handling
issue where action() is called directly before Promise wrapping, allowing sync
errors to bypass the catch handler. To fix this, modify the Promise.resolve()
call to delay the action() execution by chaining .then(() => action()) instead
of passing action() directly as an argument to Promise.resolve(). This ensures
both synchronous and asynchronous errors thrown by action() are properly caught
by the catch handler.
---
Outside diff comments:
In `@packages/dashboard/app/components/WorkflowSwitcher.tsx`:
- Around line 194-196: The Tab key handler in the WorkflowSwitcher component
closes the dropdown immediately, preventing keyboard-only users from reaching
the edit and create controls which now exist only inside the dropdown. Instead
of closing the menu on Tab, allow the default Tab behavior to proceed so focus
can move forward through the dropdown content to reach those actions. Remove or
modify the case statement for Tab so it doesn't call setIsOpen(false), allowing
natural keyboard navigation to continue through the dropdown controls.
---
Nitpick comments:
In `@packages/dashboard/vitest.config.ts`:
- Around line 330-337: The comment block documenting the deep-lane policy
(starting with "// The broad `dashboard-app` / `dashboard-api` lanes") needs to
be prefixed with an FNXC-formatted requirement header to make it traceable. Add
a new comment line at the beginning of this comment block following the format
FNXC:Area-of-product yyyy-MM-dd-hh:mm: (without the literal `FNXC_LOG` token),
identifying the relevant product area and using the current timestamp, then keep
the existing explanatory comments intact below it.
In
`@packages/engine/src/__tests__/reliability-interactions/merge-runner-spawn-enoent-prevention.test.ts`:
- Around line 87-115: The test function for "FN-6817: roots the shared
reliability fixture under the Vitest worker root" is missing a FNXC-formatted
header comment that explains the rationale for this requirement-specific
fixture-root behavior. Add an FNXC comment directly above the test function
declaration using the format FNXC:Area-of-product yyyy-MM-dd-hh:mm: to document
why this invariant behavior is necessary for the reliability interactions test.
🪄 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: c13d4af4-816d-46dc-a360-ca174c160414
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (59)
.changeset/local-starts-engine.mdREADME.mddocs/cli-reference.mddocs/contributing.mddocs/dashboard-guide.mdpackages/cli/README.mdpackages/cli/STANDALONE.mdpackages/cli/skill/fusion/references/cli-commands.mdpackages/cli/skill/fusion/references/fusion-capabilities.mdpackages/cli/src/bin.tspackages/cli/src/commands/__tests__/dashboard.test.tspackages/cli/src/commands/__tests__/desktop.test.tspackages/cli/src/commands/dashboard.tspackages/cli/src/commands/desktop.tspackages/dashboard/app/App.tsxpackages/dashboard/app/__tests__/mobile-nav-bar-css.test.tspackages/dashboard/app/api/legacy.tspackages/dashboard/app/components/Board.tsxpackages/dashboard/app/components/EngineUnavailableBanner.csspackages/dashboard/app/components/EngineUnavailableBanner.tsxpackages/dashboard/app/components/GoalsView.csspackages/dashboard/app/components/Header.csspackages/dashboard/app/components/Header.tsxpackages/dashboard/app/components/Lane.csspackages/dashboard/app/components/LeftSidebarNav.csspackages/dashboard/app/components/ListView.tsxpackages/dashboard/app/components/WorkflowSwitcher.csspackages/dashboard/app/components/WorkflowSwitcher.tsxpackages/dashboard/app/components/__tests__/App.test.tsxpackages/dashboard/app/components/__tests__/Board.test.tsxpackages/dashboard/app/components/__tests__/GoalsView.test.tsxpackages/dashboard/app/components/__tests__/Header.test.tsxpackages/dashboard/app/components/__tests__/ListView.test.tsxpackages/dashboard/app/components/__tests__/WorkflowSwitcher.test.tsxpackages/dashboard/app/components/command-center/__tests__/charts.test.tsxpackages/dashboard/app/components/command-center/areas/__tests__/areas.test.tsxpackages/dashboard/app/components/command-center/charts/LineChart.tsxpackages/dashboard/app/components/command-center/charts/charts.csspackages/dashboard/package.jsonpackages/dashboard/src/__tests__/server.test.tspackages/dashboard/src/server.tspackages/dashboard/vitest.config.tspackages/desktop/package.jsonpackages/desktop/src/__tests__/local-runtime.test.tspackages/desktop/src/__tests__/local-server.test.tspackages/desktop/src/__tests__/menu.test.tspackages/desktop/src/local-runtime.tspackages/desktop/src/local-server.tspackages/desktop/src/main.tspackages/desktop/src/menu.tspackages/engine/src/__tests__/reliability-interactions/_helpers.tspackages/engine/src/__tests__/reliability-interactions/merge-runner-spawn-enoent-prevention.test.tspackages/i18n/locales/en/app.jsonpackages/i18n/locales/es/app.jsonpackages/i18n/locales/fr/app.jsonpackages/i18n/locales/ko/app.jsonpackages/i18n/locales/zh-CN/app.jsonpackages/i18n/locales/zh-TW/app.jsonscripts/start-local.mjs
💤 Files with no reviewable changes (1)
- packages/dashboard/app/components/Lane.css
gsxdsm
force-pushed
the
perf/dashboard-test-project-dedup
branch
from
June 21, 2026 07:11
d014e88 to
d92b325
Compare
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Selecting a gated deep lane explicitly (`vitest run --project dashboard-app` or `--project dashboard-api`) without FUSION_DASHBOARD_DEEP=1 matches zero files and exits green in milliseconds — a silent no-op that reads as a passing run. Emit a loud console.warn so a manual invocation isn't mistaken for coverage. Exact token match avoids firing on the curated `dashboard-app-*` shard projects; the deep scripts set the flag, so it never fires through the intended entry points. Argv parser validated against space/equals forms, both lanes, every curated shard, and the default no-`--project` run (10/10). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 25, 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.
Problem
pnpm testwas running every dashboard test file multiple times. The defaultvitest runpath (no--project) — whichpnpm testhits via test-changed'svitest --changedscoping — executed each file in up to 3 overlapping projects:dashboard-app-qualityumbrella (re-runs the exact union of its 8 curated shards),dashboard-app/dashboard-apilanes.Measured on a real diff: a scoped run selected 1899 executions for 829 unique files — a 2.3× redundancy with zero added coverage.
App.test.tsxalone ran in 3 projects.Change
dashboard-app-qualityumbrella project. It re-ran the exact union of its eight curated shards and is referenced by nothing — absent fromtest-inventory-spec.jsonanddashboardQualityProjectGlobs, used by no script, andpackage-config's contract test already assertstest:deepmust not use it.dashboard-app/dashboard-apiincludes behindFUSION_DASHBOARD_DEEP. They're empty in the default run (so they stop duplicating the curated shards + backfill) but remain selectable via--project. The explicit deep escape hatches —test:deep/test:app/test:api/test:build— set the flag to opt back in.Net: the default scoped run now selects 829 executions for 829 files (1×).
Verification
App.test.tsx3→1)check-test-inventory --dashboard-curated)package-configcontract testFUSION_DASHBOARD_DEEP=1hatchbuild-output.test.tsstill reachable fortest:buildNo coverage lost, no contract broken — purely the redundant re-execution removed.
🤖 Generated with Claude Code
Summary by CodeRabbit