Conversation
📝 WalkthroughWalkthroughRefactors CI/test pipeline into separate static/vitest/coverage jobs, decomposes server runtime modules (auto-import, data persistence, routing), reorganizes dashboard section rendering and command-palette logic into modular renderers and preload registries, adds CLI orchestration/timing scripts, updates Playwright/Vitest wiring and many tests, and removes archived review docs. ChangesCI & Test Infrastructure
Server: Auto-import / Runner / Messages
Server: Data Runtime & File Coordination
HTTP Router & Route Groups
Runtime Utilities & Versioning
Dashboard Rendering & Command Palette
Tests: fixtures, contracts, integration, unit
Documentation & Cleanup
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as HTTP Router
participant Usage as Usage Routes
participant Locks as File Locks
participant IO as File I/O
Client->>Router: POST /api/upload
activate Router
Router->>Usage: dispatch /upload handler
activate Usage
Usage->>Router: readMutationBody
Usage->>Locks: withSettingsAndDataMutationLock()
activate Locks
Locks-->>Usage: lock acquired
Usage->>Usage: normalizeIncomingUsagePayload
Usage->>IO: writeJsonAtomic(mergedData)
activate IO
IO-->>Usage: write complete
deactivate IO
Usage->>Locks: release lock
Locks-->>Usage: lock released
deactivate Locks
Usage-->>Router: return summary (days/totalCost)
deactivate Usage
Router-->>Client: 200 JSON
deactivate Router
sequenceDiagram
participant CI as CI runner
participant Static as static job
participant Vitest as vitest matrix
participant Coverage as coverage job
participant E2E as e2e job (Playwright)
CI->>Static: run test:static
Static-->>CI: artifact & status
CI->>Vitest: run per-project vitest jobs
Vitest-->>CI: per-project JUnit artifacts + timing reports
CI->>Coverage: run test:vitest:coverage
Coverage-->>CI: coverage reports + JUnit
CI->>E2E: run Playwright (workers=2)
E2E-->>CI: playwright-report & test-results
CI->>CI: ci-required checks needs.*.result → fail if any failed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/frontend/requests-over-time.test.tsx (1)
100-164:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert every non-zero moving-average series, not just a subset.
This test name says it covers all non-zero model moving-average lines, but the current assertion only checks three of them. A regression could drop one of the other averages and still pass.
♻️ Suggested fix
expect(lineNames).toEqual( expect.arrayContaining([ + 'GPT-5.4 7-day avg', + 'Claude Sonnet 4.5 7-day avg', + 'Claude Opus 4.7 7-day avg', + 'Gemini 2.5 Pro 7-day avg', + 'GPT-4.1 7-day avg', + 'Claude Haiku 4.5 7-day avg', 'Claude Opus 4.7', 'Claude Opus 4.7 7-day avg', 'Claude Haiku 4.5 7-day avg', ]), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/requests-over-time.test.tsx` around lines 100 - 164, The test only asserts a subset of non-zero moving-average lines; update the assertion in tests/frontend/requests-over-time.test.tsx so it verifies every non-zero model moving-average series is present: after rendering RequestsOverTime and expanding, derive the expected moving-average names from the provided data (use the model keys from the RequestChartDataPoint objects, filter out models with only zero values like "Unused Model", and append " 7-day avg" to each non-zero model), then assert that the collected lineNames (from getAllByTestId('request-line') -> data-name) contains all of those expected names instead of only the three currently checked.tests/frontend/filter-bar-presets.test.tsx (1)
24-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore coverage for the quick-preset order.
Removing the timeout is fine, but this hunk also drops the assertion that the quick presets render in the shared
7D/30D/Month/Year/Allorder. That sequence is a visible contract, so please keep it covered here or in another focused test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/filter-bar-presets.test.tsx` around lines 24 - 58, The test removed an assertion that verifies the quick-preset order (7D/30D/Month/Year/All), so re-add a check in the test (the one using resolveDashboardPresetRange, renderFilterBar, rerender, FilterBar, buildFilterBarProps) that asserts the rendered preset buttons appear in that exact sequence; locate the initial render created by renderFilterBar(...) and add an assertion (e.g., inspect the ordered button elements by role or label) to confirm the labels ['7D','30D','Month','Year','All'] are rendered in that order so the visible contract remains covered.tests/frontend/metric-ratio-locale.test.tsx (1)
125-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the coverage expectation timezone-independent.
new Date().getDate()still depends on the runner’s local timezone, so this can flap even with a fixed UTC timestamp. UsegetUTCDate()or a fixed constant for the denominator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/metric-ratio-locale.test.tsx` around lines 125 - 145, The test's coverage calculation uses new Date().getDate(), which is timezone-dependent and can cause flakiness; update the coverage denominator to be timezone-independent by using getUTCDate() (e.g., new Date(...).getUTCDate()) or a fixed constant/day value so the expected string in the coverage variable is stable; adjust the coverage variable calculation where it’s declared (referencing coverage and the Intl.NumberFormat usage, and the MonthMetrics render that asserts it) so the expectation no longer depends on the runner's local timezone.src/components/dashboard/sections/dashboard-section-renderer-types.d.ts (1)
1-28: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDrop the semicolons in this declaration file.
This new
.d.tsstill uses semicolons throughout, which conflicts with the repo’ssrc/TypeScript style. A quick formatter pass would make it consistent with the rest of the codebase.As per coding guidelines,
src/**/*.{ts,tsx}: frontend code should use TypeScript + React with 2-space indentation, single quotes, trailing commas where the formatter leaves them, and no semicolons insrc/files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/sections/dashboard-section-renderer-types.d.ts` around lines 1 - 28, Remove all semicolons from this declaration file and reformat it to match repo style: use 2-space indentation, single quotes, and trailing commas as your formatter dictates. Specifically update the declarations for DashboardSectionRenderOptions, DashboardSectionRenderContext, and DashboardSectionRenderer to remove trailing semicolons after property/type lines, then run the repo formatter/Prettier or ESLint autofix to ensure consistent spacing and commas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 40: The sentence wrongly suggests keeping Playwright specs in
tests/e2e/fixtures.ts; update the wording to clarify that tests/e2e/fixtures.ts
is a shared helper for fixtures and setup, and that specs belong in the
tests/e2e spec files while state should be reset via resetAppState(...) or
prepared with prepareDashboard(...); replace the current line with a concise
instruction like “Keep Playwright fixtures and shared helpers in
tests/e2e/fixtures.ts; place test specs in your tests/e2e spec files and reset
state using resetAppState(...) or prepareDashboard(...).”
In `@docs/testing.md`:
- Around line 138-152: The docs list contains a duplicated runbook entry for the
same command; remove the redundant line for "PLAYWRIGHT_TEST_PORT=3016 npm run
verify:full:parallel" so only one occurrence remains (keep the clearer
description and delete the other), updating the list that includes items like
`npm run verify`, `npm run test:vitest`, and `PLAYWRIGHT_TEST_PORT=3016 npm run
verify:full:parallel` to eliminate the duplicate entry.
In `@package.json`:
- Line 32: The npm script "test:architecture" (and any other scripts in
package.json that pass --outputFile.junit) currently sets an output path but
doesn't enable the JUnit reporter; update each script that uses
--outputFile.junit to also include --reporter=junit (e.g., change the
"test:architecture" script invocation to add --reporter=junit), and apply the
same change to the other scripts that set --outputFile.junit so Vitest actually
writes the XML artifacts.
In `@scripts/run-vitest-project-timings.js`:
- Around line 201-203: The script currently calls
fs.mkdirSync(options.reportDir, { recursive: true }) (near the creation of the
results Map) even when running with --dry-run; change the control flow so
filesystem writes are skipped in dry-run mode: guard the directory creation and
any subsequent file writes in the block that handles report generation (the code
around options.reportDir, the results Map, and the code between lines ~216-243
that writes reports) with a check like if (!options.dryRun) or equivalent, so no
directories or files are created when options.dryRun is true; ensure the results
Map and in-memory operations still run so timing logic can be exercised without
side effects.
In `@server/auto-import-runtime/toktrack-runners.js`:
- Around line 34-37: The getLocalToktrackDisplayCommand currently checks
processObject.env.TTDASH_TOKTRACK_LOCAL_BIN but still builds commands using the
hardcoded toktrackLocalBin elsewhere; update getLocalToktrackDisplayCommand, the
probe command construction (probe runner) and the exec command builder to use
the override when present (processObject.env.TTDASH_TOKTRACK_LOCAL_BIN) instead
of toktrackLocalBin; specifically, reference and prefer the override value when
composing the command strings in getLocalToktrackDisplayCommand and the
functions that build/execute probe and exec commands so the custom binary path
is honored.
In `@server/data-runtime/file-io.js`:
- Around line 45-97: The temp filename for atomic writes in writeJsonAtomic and
writeJsonAtomicAsync is collision-prone because it uses only processObject.pid
and Date.now(); update the tempPath construction in both functions (symbols:
writeJsonAtomic, writeJsonAtomicAsync, tempPath) to append a random/UUID suffix
(e.g., crypto.randomBytes or a UUID v4) so each temp file is unique per write,
ensure the random value is safe for filenames, and keep the existing pid and
timestamp for readability.
In `@server/data-runtime/file-locks.js`:
- Around line 27-35: The validation currently treats secureDirMode and
secureFileMode as optional but later code assumes they exist; make them
consistently required or provide safe defaults in the factory: update the
options validation block (the code pushing into invalidOptions for
secureDirMode/secureFileMode) to either enforce presence (reject when
null/undefined) or assign explicit defaults (e.g., 0o700 for secureDirMode and
0o600 for secureFileMode) before returning from the factory, and ensure the
lock-creation code that uses secureDirMode/secureFileMode reads the
validated/filled values so runtime lock operations never encounter undefined
modes.
In `@server/data-runtime/import-merge.js`:
- Around line 218-226: The branch handling !current currently returns raw
importedData when skippedDays === 0, allowing stale/missing totals to persist;
instead always normalize the daily array and recompute totals: use the same
normalization that produces validImportedDaily (apply it to importedData.daily)
and then return an object with daily set to that normalized array and totals =
computeUsageTotals(normalizedDaily) (referencing variables current, skippedDays,
importedData, validImportedDaily, and computeUsageTotals to locate the relevant
logic).
- Around line 128-139: computeUsageTotals currently reduces over daily and adds
raw field values which will perform string concatenation if any field is a
string (e.g., "5"); fix by coercing every summed field to a numeric value and
supplying an explicit initial accumulator: for each property referenced in
computeUsageTotals (inputTokens, outputTokens, cacheCreationTokens,
cacheReadTokens, thinkingTokens, totalCost, totalTokens, requestCount) convert
day.<prop> to a number with a safe fallback (e.g., Number(day.<prop>) || 0 or
parseFloat(day.<prop>) || 0) before adding, and initialize totals with all
properties set to 0 so reduce never starts with undefined.
In `@server/routes/static-routes.js`:
- Around line 43-56: The HTML response path in sendStaticFile currently replaces
global securityHeaders with prepareHtmlResponse(data).headers causing defaults
like CSP/HSTS to be lost; change the header merge so securityHeaders are
preserved and per-HTML headers override or extend them (e.g., compute
responseSecurityHeaders = isHtml ? {...securityHeaders,
...prepareHtmlResponse(data.toString('utf8')).headers} : securityHeaders) and
then use responseSecurityHeaders in res.writeHead; keep prepareHtmlResponse
usage and responseBody selection as-is but ensure the header merge uses both
sources.
In `@server/routes/usage-routes.js`:
- Around line 31-45: normalizeIncomingUsagePayload currently may return the
original payload and the route persists it without verifying it matches the
expected usage object shape; update the `/usage/import` route (and the other
usage persistence site around the normalize call at lines ~100-118) to validate
the normalized result before persisting: after calling
normalizeIncomingUsagePayload(payload, invalidMessage) check that the returned
value is a well-formed usage object (required fields/types you use elsewhere —
e.g., timestamp, userId, event/type, amount — or reuse an existing validator
function if available), throw a clear Error if validation fails, and only write
to storage when validation passes so invalid payloads are rejected rather than
persisted.
In `@src/components/charts/CostByWeekday.tsx`:
- Around line 35-37: The weekendCost calculation currently hard-codes indices
(index === 5 || index === 6) which breaks if the series order changes; update
the weekendCost computation in CostByWeekday to filter entries by an explicit
weekday identifier (e.g., an entry.weekday / entry.day string of
"Saturday"/"Sunday", or by parsing entry.date and using getDay() to match 0/6)
rather than by array index, then reduce the filtered entries as before so the
subtitle always reflects the true weekend share.
In `@tests/e2e/fixtures.ts`:
- Around line 111-118: The stopServer function currently force-kills
serverProcess with serverProcess.kill('SIGKILL') but returns without awaiting
its exit, causing port contention; update stopServer to, after sending SIGKILL,
wait for the process 'exit' event using the same Promise.race pattern used
earlier (e.g., new Promise<boolean>(resolve => serverProcess.once('exit', () =>
resolve(true))) raced against a timeout) so the function only returns once the
process has actually exited or the timeout elapses; reference serverProcess and
the existing Promise.race pattern to implement this wait.
In `@tests/frontend/dashboard-language-regressions.test.tsx`:
- Around line 94-96: Element.prototype.scrollIntoView is stubbed in the test
setup (beforeAll) but never restored, causing cross-test leakage; capture the
original function before stubbing (e.g. const originalScrollIntoView =
Element.prototype.scrollIntoView) and then restore it in an afterAll/teardown
block by setting Element.prototype.scrollIntoView = originalScrollIntoView so
tests that call initI18n or other helpers run with the original behavior.
In `@tests/unit/background-runtime.test.ts`:
- Around line 79-125: The object passed to createBackgroundRuntime contains a
duplicate fs property (fs: fsMock appears twice), causing a lint/CI failure;
remove the earlier duplicate assignment so the factory object only defines fs
once (retain a single fs: fsMock entry), e.g., delete the first fs: fsMock near
the top of the argument object and keep the final fs: fsMock, ensuring
createBackgroundRuntime receives one fs property.
In `@tests/unit/dashboard-preferences.test.ts`:
- Around line 205-212: The test is incorrectly treating the string "false" as
truthy; update the behavior so normalizeDashboardSectionVisibility rejects
non-boolean values (does not coerce strings) and change the test assertions
accordingly: for the input today: 'false' expect that the normalized object does
not include the today key (or that today is undefined) rather than expecting it
to be true; ensure normalizeDashboardSectionVisibility strips or preserves only
boolean-valued keys and that the test asserts absence of malformed keys instead
of coercion.
In `@tests/unit/data-runtime-contract.test.ts`:
- Around line 96-118: The test uses hard-coded /tmp/ttdash/* paths which can
leak state and fail on non-Unix runners; change the test to create per-test
temporary directories (e.g., using os.tmpdir()+fs.mkdtempSync or a test helper
that creates and cleans temp dirs) and set TTDASH_CACHE_DIR, TTDASH_CONFIG_DIR,
and TTDASH_DATA_DIR to those temp paths when calling createRuntime; assert
runtime.appPaths, runtime.paths, and runtime.readSettings() against those temp
locations (referencing createRuntime, runtime.appPaths, runtime.paths,
runtime.readSettings) and ensure the temp dirs are removed in test teardown.
In `@tests/unit/report-test-timings.test.ts`:
- Around line 95-109: The test fails on Windows because options.junitPath
generated by timingScript.parseArgs uses path.resolve which can contain
backslashes; update the assertion to normalize separators before matching (e.g.,
convert options.junitPath backslashes to forward slashes) so the regex check is
platform-independent—locate the test in tests/unit/report-test-timings.test.ts
around the it(...) block and change the expect(options.junitPath).toMatch(...)
line to normalize options.junitPath (replace all "\\" with "/" or use a small
helper like replace(new RegExp(path.sep === '\\\\' ? '\\\\' : path.sep, 'g'),
'/')) before applying the regex while leaving other assertions unchanged.
---
Outside diff comments:
In `@src/components/dashboard/sections/dashboard-section-renderer-types.d.ts`:
- Around line 1-28: Remove all semicolons from this declaration file and
reformat it to match repo style: use 2-space indentation, single quotes, and
trailing commas as your formatter dictates. Specifically update the declarations
for DashboardSectionRenderOptions, DashboardSectionRenderContext, and
DashboardSectionRenderer to remove trailing semicolons after property/type
lines, then run the repo formatter/Prettier or ESLint autofix to ensure
consistent spacing and commas.
In `@tests/frontend/filter-bar-presets.test.tsx`:
- Around line 24-58: The test removed an assertion that verifies the
quick-preset order (7D/30D/Month/Year/All), so re-add a check in the test (the
one using resolveDashboardPresetRange, renderFilterBar, rerender, FilterBar,
buildFilterBarProps) that asserts the rendered preset buttons appear in that
exact sequence; locate the initial render created by renderFilterBar(...) and
add an assertion (e.g., inspect the ordered button elements by role or label) to
confirm the labels ['7D','30D','Month','Year','All'] are rendered in that order
so the visible contract remains covered.
In `@tests/frontend/metric-ratio-locale.test.tsx`:
- Around line 125-145: The test's coverage calculation uses new
Date().getDate(), which is timezone-dependent and can cause flakiness; update
the coverage denominator to be timezone-independent by using getUTCDate() (e.g.,
new Date(...).getUTCDate()) or a fixed constant/day value so the expected string
in the coverage variable is stable; adjust the coverage variable calculation
where it’s declared (referencing coverage and the Intl.NumberFormat usage, and
the MonthMetrics render that asserts it) so the expectation no longer depends on
the runner's local timezone.
In `@tests/frontend/requests-over-time.test.tsx`:
- Around line 100-164: The test only asserts a subset of non-zero moving-average
lines; update the assertion in tests/frontend/requests-over-time.test.tsx so it
verifies every non-zero model moving-average series is present: after rendering
RequestsOverTime and expanding, derive the expected moving-average names from
the provided data (use the model keys from the RequestChartDataPoint objects,
filter out models with only zero values like "Unused Model", and append " 7-day
avg" to each non-zero model), then assert that the collected lineNames (from
getAllByTestId('request-line') -> data-name) contains all of those expected
names instead of only the three currently checked.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 687fefc0-a9ea-4375-b172-52549e3126ba
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (128)
.github/workflows/ci.yml.gitignoreAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdREADME.mdRELEASING.mddocs/architecture.mddocs/review/README.mddocs/review/architecture-review.mddocs/review/code-review.mddocs/review/dashboard-review.mddocs/review/fixed-findings.mddocs/review/performance-review.mddocs/review/security-review.mddocs/review/server-review.mddocs/review/test-review.mddocs/testing.mdpackage.jsonplaywright.config.tsscripts/report-test-timings.jsscripts/run-parallel-gate.jsscripts/run-vitest-project-timings.jsscripts/start-test-server.jsserver/auto-import-runtime.jsserver/auto-import-runtime/command-runner.jsserver/auto-import-runtime/import-executor.jsserver/auto-import-runtime/latest-version.jsserver/auto-import-runtime/messages.jsserver/auto-import-runtime/toktrack-runners.jsserver/background-runtime.jsserver/data-runtime.jsserver/data-runtime/app-paths.jsserver/data-runtime/file-io.jsserver/data-runtime/file-locks.jsserver/data-runtime/import-merge.jsserver/http-router.jsserver/routes/auto-import-routes.jsserver/routes/http-route-utils.jsserver/routes/report-routes.jsserver/routes/runtime-routes.jsserver/routes/settings-routes.jsserver/routes/static-routes.jsserver/routes/usage-routes.jsserver/runtime-formatters.jsserver/startup-runtime.jsshared/toktrack-version.d.tsshared/toktrack-version.jssrc/components/charts/CostByWeekday.tsxsrc/components/dashboard/DashboardSections.tsxsrc/components/dashboard/sections/dashboard-analysis-sections.tsxsrc/components/dashboard/sections/dashboard-comparison-table-sections.tsxsrc/components/dashboard/sections/dashboard-forecast-sections.tsxsrc/components/dashboard/sections/dashboard-overview-sections.tsxsrc/components/dashboard/sections/dashboard-section-lazy-components.tssrc/components/dashboard/sections/dashboard-section-metadata.tssrc/components/dashboard/sections/dashboard-section-renderer-types.d.tssrc/components/dashboard/sections/dashboard-section-renderers.tssrc/components/features/command-palette/CommandPalette.commands.tsxsrc/components/features/command-palette/CommandPalette.tsxsrc/hooks/use-dashboard-controller-derived-state.tssrc/hooks/use-dashboard-controller.tssrc/types/dashboard-controller.d.tssrc/types/dashboard-view-model.d.tssrc/types/index.tstests/architecture/dashboard-sections-contract.test.tstests/architecture/import-patterns.tstests/architecture/server-auto-import-runtime-boundaries.test.tstests/architecture/server-data-runtime-boundaries.test.tstests/architecture/server-http-boundaries.test.tstests/architecture/server-runtime-state-contract.test.tstests/architecture/shared-declaration-contract.test.tstests/e2e/command-palette.spec.tstests/e2e/dashboard-forecast-filters.spec.tstests/e2e/dashboard-load-upload.spec.tstests/e2e/dashboard-reporting.spec.tstests/e2e/dashboard-settings-backups.spec.tstests/e2e/fixtures.tstests/e2e/helpers.tstests/frontend/auto-import-modal.test.tsxtests/frontend/chart-card.test.tsxtests/frontend/chart-legend-integration.test.tsxtests/frontend/command-palette-action-groups.test.tsxtests/frontend/dashboard-controller-actions.test.tsxtests/frontend/dashboard-controller-test-helpers.tstests/frontend/dashboard-error-state.test.tsxtests/frontend/dashboard-language-regressions.test.tsxtests/frontend/dashboard-sections-rendering.test.tsxtests/frontend/drill-down-modal-regressions.test.tsxtests/frontend/expandable-card.test.tsxtests/frontend/filter-bar-presets.test.tsxtests/frontend/header-links.test.tsxtests/frontend/info-heading.test.tsxtests/frontend/lazy-dashboard-charts.test.tsxtests/frontend/metric-ratio-locale.test.tsxtests/frontend/provider-cost-forecast.test.tsxtests/frontend/requests-over-time.test.tsxtests/frontend/toast.test.tsxtests/integration/app-runtime.test.tstests/integration/data-runtime-persistence.test.tstests/integration/server-api-imports.test.tstests/integration/server-background-concurrency.test.tstests/integration/server-test-helpers.tstests/unit/app-settings-contract.test.tstests/unit/background-runtime.test.tstests/unit/command-palette-commands.test.tstests/unit/csv-export.test.tstests/unit/dashboard-preferences.test.tstests/unit/data-runtime-contract.test.tstests/unit/http-router-mutations.test.tstests/unit/playwright-config.test.tstests/unit/process-utils.test.tstests/unit/report-test-timings.test.tstests/unit/run-parallel-gate.test.tstests/unit/run-vitest-project-timings.test.tstests/unit/server-helpers-auto-import-executor.test.tstests/unit/server-helpers-file-locks.test.tstests/unit/server-helpers-runner-core.test.tstests/unit/server-helpers-runner-process.test.tstests/unit/server-helpers.shared.tstests/unit/startup-runtime.test.tstests/unit/test-pipeline-scripts.test.tstests/unit/toktrack-version.test.tstests/unit/vitest-coverage-config.test.tsvitest.config.tsvitest.setup.frontend.tsvitest.setup.node.tsvitest.setup.ts
💤 Files with no reviewable changes (11)
- docs/review/README.md
- docs/review/server-review.md
- tests/unit/server-helpers-runner-process.test.ts
- docs/review/dashboard-review.md
- docs/review/performance-review.md
- tests/frontend/command-palette-action-groups.test.tsx
- docs/review/test-review.md
- docs/review/code-review.md
- docs/review/architecture-review.md
- docs/review/security-review.md
- docs/review/fixed-findings.md
Comment on lines
+205
to
+212
| const visibility = normalizeDashboardSectionVisibility({ | ||
| metrics: false, | ||
| today: 'false', | ||
| unknown: false, | ||
| }) | ||
| expect(visibility.metrics).toBe(false) | ||
| expect(visibility.today).toBe(true) | ||
| expect(visibility).not.toHaveProperty('unknown') |
There was a problem hiding this comment.
Don't treat the string "false" as visible.
This assertion currently encodes truthy coercion for malformed persisted data, which can reopen a section the user explicitly hid. The normalization path should reject non-boolean values or preserve false, not pass them through truthiness.
Suggested fix
- expect(visibility.today).toBe(true)
+ expect(visibility.today).toBe(false)If invalid keys are stripped instead of coerced, assert that today is absent instead.
📝 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
| const visibility = normalizeDashboardSectionVisibility({ | |
| metrics: false, | |
| today: 'false', | |
| unknown: false, | |
| }) | |
| expect(visibility.metrics).toBe(false) | |
| expect(visibility.today).toBe(true) | |
| expect(visibility).not.toHaveProperty('unknown') | |
| const visibility = normalizeDashboardSectionVisibility({ | |
| metrics: false, | |
| today: 'false', | |
| unknown: false, | |
| }) | |
| expect(visibility.metrics).toBe(false) | |
| expect(visibility.today).toBe(false) | |
| expect(visibility).not.toHaveProperty('unknown') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/dashboard-preferences.test.ts` around lines 205 - 212, The test is
incorrectly treating the string "false" as truthy; update the behavior so
normalizeDashboardSectionVisibility rejects non-boolean values (does not coerce
strings) and change the test assertions accordingly: for the input today:
'false' expect that the normalized object does not include the today key (or
that today is undefined) rather than expecting it to be true; ensure
normalizeDashboardSectionVisibility strips or preserves only boolean-valued keys
and that the test asserts absence of malformed keys instead of coercion.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const authHeaders = readApiAuthHeaders(authSessionPath) | ||
| if (authHeaders) { | ||
| const apiResponse = await fetch(new URL('/api/usage', baseURL), { | ||
| headers: authHeaders, |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/data-transforms.ts (1)
140-147:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep
weekdayDatashape consistent for empty and non-empty datasets.
buildDashboardChartTransformsnow emitsweekdayIndexfor populated data, but the empty-data branch still omits it. That creates a branch-dependent output shape and forces downstream fallback logic unnecessarily.Suggested patch
if (sorted.length === 0) { return { costChartData: [], modelCostChartData: [], tokenChartData: [], requestChartData: [], - weekdayData: createWeekdayLabels(locale).map((day) => ({ day, cost: 0 })), + weekdayData: createWeekdayLabels(locale).map((day, weekdayIndex) => ({ + day, + cost: 0, + weekdayIndex, + })), } }Also applies to: 294-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/data-transforms.ts` around lines 140 - 147, The empty-data branch in buildDashboardChartTransforms returns weekdayData items without the weekdayIndex property, causing inconsistent output shape; update that branch (and the other empty-data branch later) so weekdayData is built using createWeekdayLabels(locale).map((day, weekdayIndex) => ({ day, cost: 0, weekdayIndex })) so every weekday object includes weekdayIndex (matching the populated-data shape) and keep other returned chart arrays the same.tests/unit/server-helpers-file-locks.test.ts (1)
519-532:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuarantee spy cleanup when these assertions fail.
These cases restore
rename/unlink/Date.nowonly after the expectations run. If one assertion fails, the mocked fs/clock behavior leaks into later lock/write tests and turns a single regression into a cascade. Wrap the assertion body intry/finallyso cleanup always runs.🧹 Suggested pattern
- await expect(writeJsonAtomicAsync(targetFile, { ok: true })).rejects.toBe(renameError) - - expect(renameSpy).toHaveBeenCalled() - const tempPath = unlinkSpy.mock.calls[0]?.[0] as string - expectAtomicTempPath(tempPath, targetFile, 1700000000000) - expect(existsSync(tempPath)).toBe(false) - - renameSpy.mockRestore() - unlinkSpy.mockRestore() - nowSpy.mockRestore() + try { + await expect(writeJsonAtomicAsync(targetFile, { ok: true })).rejects.toBe(renameError) + + expect(renameSpy).toHaveBeenCalled() + const tempPath = unlinkSpy.mock.calls[0]?.[0] as string + expectAtomicTempPath(tempPath, targetFile, 1700000000000) + expect(existsSync(tempPath)).toBe(false) + } finally { + renameSpy.mockRestore() + unlinkSpy.mockRestore() + nowSpy.mockRestore() + }Also applies to: 637-649
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/server-helpers-file-locks.test.ts` around lines 519 - 532, The test installs spies on fsPromises.rename, fsPromises.unlink and Date.now before running assertions, but restores them only after expectations, which can leak mocks if an assertion throws; update the test around the writeJsonAtomicAsync call (the block that sets up renameSpy, unlinkSpy, nowSpy and calls expect(...).rejects.toBe(renameError)) to wrap the assertion and subsequent checks in a try/finally and move renameSpy.mockRestore(), unlinkSpy.mockRestore(), and nowSpy.mockRestore() into the finally so mocks are always restored; apply the same try/finally pattern to the analogous block that uses the same spies (the other test around writeJsonAtomicAsync mentioned in the comment).
♻️ Duplicate comments (2)
server/routes/usage-routes.js (1)
35-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject uploads with a partial
totalsshape.
isUsageData()only verifies thattotalsis an object. A payload like{ daily: [{ date: '2026-04-27' }], totals: {} }still passes, then the upload route returnstotalCost: undefinedand persists a shape that does not matchEMPTY_USAGE_RESPONSE. Tighten the guard to require the numeric totals fields you rely on, or replace the payload with a fully normalized object before writing it.🔒 Suggested hardening
function isUsageData(value) { return ( isPlainObject(value) && Array.isArray(value.daily) && value.daily.every((day) => isPlainObject(day) && typeof day.date === 'string') && - isPlainObject(value.totals) + isPlainObject(value.totals) && + typeof value.totals.totalCost === 'number' && + typeof value.totals.totalTokens === 'number' && + typeof value.totals.requestCount === 'number' ); }Also applies to: 122-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/usage-routes.js` around lines 35 - 59, The current isUsageData() only checks totals is an object, allowing empty totals like { totals: {} } which leads to undefined totalCost; update isUsageData to assert the expected numeric totals fields (e.g., ensure value.totals.totalCost is a number and any other totals your code uses such as totalUsage or totalCount) so malformed payloads are rejected, and/or ensure normalizeIncomingUsagePayload calls dataRuntime.normalizeIncomingData and requires it to return a fully normalized shape (treat null/partial objects as invalid) before accepting nextPayload; apply the same tightened validation where similar checks occur (the other usage-check block around lines 122-133).tests/unit/run-vitest-project-timings.test.ts (1)
48-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
reportDirbefore matching.Line 59 hardcodes POSIX separators, so this assertion will fail on Windows where
path.resolve()returns backslashes.♻️ Suggested fix
- expect(options.reportDir).toMatch(/\.cache\/test-timings$/) + expect(options.reportDir.replaceAll('\\', '/')).toMatch(/\.cache\/test-timings$/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/run-vitest-project-timings.test.ts` around lines 48 - 61, The test assertion for options.reportDir uses a POSIX-style regex and will fail on Windows; update the test in run-vitest-project-timings.test.ts to normalize the resolved path before matching by converting backslashes to forward slashes (or use path.normalize/path.sep-aware comparison) after calling timingRunner.parseArgs, e.g., operate on options.reportDir to produce a normalized string and then assert it matches the expected ".cache/test-timings" suffix; reference timingRunner.parseArgs and options.reportDir when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/run-vitest-project-timings.js`:
- Around line 224-235: The code currently ignores spawnSync launch errors for
the vitest and budget processes by only checking testResult.status and
budgetResult.status; update the logic after runCommand calls (for testResult
from runCommand(vitestCommand.command, vitestCommand.args, spawnSyncImpl) and
for budgetResult from runCommand(budgetCommand.command, budgetCommand.args,
spawnSyncImpl)) to explicitly check if testResult.error or budgetResult.error is
set, log the error (including its message) via the existing logger or console
with context (e.g., "failed to launch vitest" / "failed to launch budget check")
and then return a non-zero exit code, before falling back to the existing status
checks and returns.
In `@server/auto-import-runtime/toktrack-runners.js`:
- Around line 151-155: The local `--version` invocation is using the probe
timeout, making toktrackLocalRunnerVersionCheckTimeoutMs ineffective; update the
timeout passed to runToktrack in the parseToktrackVersionOutput call so it uses
getToktrackRunnerTimeouts(localRunner).versionCheckMs (instead of .probeMs) when
invoking runToktrack for the local version check (the call that awaits
runToktrack(localRunner, ['--version'], { timeoutMs: ... })). This ensures the
local version check honors the version-check timeout config.
In `@tests/e2e/fixtures.ts`:
- Around line 181-186: Before spawning the test server, remove any persisted
runtime artifacts for that worker so stale session-auth.json or app state cannot
leak into the new run: obtain the runtime directory from
buildWorkerServer(workerInfo.parallelIndex) (use the returned object's
runtimeDir field or the env variable it sets, e.g.,
workerServer.env.WORKER_RUNTIME), recursively delete or empty that directory
(all files including session-auth.json) and then proceed to
spawn(process.execPath, ['scripts/start-test-server.js'], { cwd: process.cwd(),
env: workerServer.env, }); ensure the cleanup happens synchronously/awaited
before createOutputBuffer(serverProcess) is called.
- Around line 89-136: The readiness loop in waitForServerReady can hang the full
timeout if the child process fails to spawn because there is no handler for the
serverProcess 'error' event; add an immediate failure path by creating an
errorPromise that listens for serverProcess.once('error', err => reject(new
Error(`Playwright server spawn error: ${err.message}\n${readOutput()}`))) and
then await Promise.race([errorPromise, existing readiness loop promise]) (or
integrate the reject into the existing async flow) so that spawn errors reject
immediately instead of waiting for startupTimeoutMs; reference
waitForServerReady, serverProcess, readOutput, and startupTimeoutMs when making
the change.
In `@tests/frontend/dashboard-language-regressions.test.tsx`:
- Around line 212-224: Extract the command-palette-specific test block from
tests/frontend/dashboard-language-regressions.test.tsx and place it in a new
focused test file (e.g., tests/frontend/command-palette-language.test.tsx); keep
the test function body (the it(...) block that renders CommandPalette, triggers
fireEvent.keyDown and asserts screen queries) intact but update imports so
renderWithAppProviders, buildCommandPaletteProps, CommandPalette, fireEvent and
screen are imported from the same test helpers/modules used in the original
file; remove the original it(...) block from
dashboard-language-regressions.test.tsx and ensure any shared setup/teardown
(beforeEach/afterEach) used by this test is copied or referenced in the new file
so the test still runs in isolation.
In `@tests/frontend/lazy-dashboard-charts.test.tsx`:
- Around line 240-325: The current single suite mixes unrelated component tests
(CostByWeekday, ModelMix, TokensOverTime, PeriodComparison); split it into four
focused test files by moving each describe/it blocks into their own test file
named for the component and keep only that component's tests per file (e.g.,
tests for CostByWeekday -> new CostByWeekday.test.tsx, ModelMix ->
ModelMix.test.tsx, etc.). Ensure each new file imports the shared helpers
(renderWithTooltip, initI18n, createDailyUsage, weekdayData, tokenData) and
either calls initI18n('en') in a beforeAll or uses a shared test-setup helper;
remove the moved tests from the original catch-all file so it only contains any
remaining related tests. Also keep assertions and test IDs unchanged (e.g.,
references to CostByWeekday, ModelMix, TokensOverTime, PeriodComparison,
getByTestId keys like 'chart-bar'/'chart-area'/'chart-line'/'composed-chart') so
behavior stays identical.
In `@tests/unit/http-router-mutations.test.ts`:
- Around line 415-652: The PR has mixed unrelated tests into the mutation suite
— move the auto-import SSE tests (those referencing '/api/auto-import/stream',
acquireAutoImportLease, performAutoImport, toAutoImportErrorEvent,
createAutoImportMessageEvent, formatAutoImportMessageEvent), the
runtime/host/auth tests (those exercising '/api/runtime', validateRequestHost,
validateApiRequest, getRuntimeSnapshot), and the PDF report tests (those using
'/api/report/pdf', readData, readBody, generatePdfReport) into focused spec
files (e.g., auto-import.stream.test.ts, runtime.routes.test.ts,
report.pdf.test.ts); copy the relevant it blocks and test setup that uses
createRouter, request, requestRaw and their overrides into the new files, update
imports/describe titles, and leave the original mutation test file containing
only true mutation-related tests so each file tests a single concern.
In `@tests/unit/run-vitest-project-timings.test.ts`:
- Around line 131-149: The test creates a spy via vi.spyOn(fs, 'mkdirSync') but
only calls mkdirSync.mockRestore() at the end, risking spy leakage on assertion
failure; update the test for 'prints dry-run commands without spawning Vitest'
to always restore the spy by wrapping the test body in a try/finally that calls
mkdirSync.mockRestore() (or move the spy/restore into an afterEach) so mkdirSync
is reliably restored after calling
timingRunner.run(['--projects=unit','--repeat=2','--dry-run'], { stdout, stderr
}, spawnSyncImpl).
In `@tests/unit/server-helpers-runner-core.test.ts`:
- Around line 361-362: The test's assertion mismatches the real onStderr
contract: runCommandWithSpawn forwards the raw stderr chunk (including newline)
to onStderr and only trims to decide whether to call the callback. Update the
expectation in the test that emits to child.stderr (the Buffer.from('runner
warning\n') case) so expect(stderrLines) includes the trailing newline (e.g.,
'runner warning\n') instead of the trimmed string; adjust any other assertions
that assume trimming. Ensure this refers to the stderr emit in the test and the
onStderr handler used to collect stderrLines.
---
Outside diff comments:
In `@src/lib/data-transforms.ts`:
- Around line 140-147: The empty-data branch in buildDashboardChartTransforms
returns weekdayData items without the weekdayIndex property, causing
inconsistent output shape; update that branch (and the other empty-data branch
later) so weekdayData is built using createWeekdayLabels(locale).map((day,
weekdayIndex) => ({ day, cost: 0, weekdayIndex })) so every weekday object
includes weekdayIndex (matching the populated-data shape) and keep other
returned chart arrays the same.
In `@tests/unit/server-helpers-file-locks.test.ts`:
- Around line 519-532: The test installs spies on fsPromises.rename,
fsPromises.unlink and Date.now before running assertions, but restores them only
after expectations, which can leak mocks if an assertion throws; update the test
around the writeJsonAtomicAsync call (the block that sets up renameSpy,
unlinkSpy, nowSpy and calls expect(...).rejects.toBe(renameError)) to wrap the
assertion and subsequent checks in a try/finally and move
renameSpy.mockRestore(), unlinkSpy.mockRestore(), and nowSpy.mockRestore() into
the finally so mocks are always restored; apply the same try/finally pattern to
the analogous block that uses the same spies (the other test around
writeJsonAtomicAsync mentioned in the comment).
---
Duplicate comments:
In `@server/routes/usage-routes.js`:
- Around line 35-59: The current isUsageData() only checks totals is an object,
allowing empty totals like { totals: {} } which leads to undefined totalCost;
update isUsageData to assert the expected numeric totals fields (e.g., ensure
value.totals.totalCost is a number and any other totals your code uses such as
totalUsage or totalCount) so malformed payloads are rejected, and/or ensure
normalizeIncomingUsagePayload calls dataRuntime.normalizeIncomingData and
requires it to return a fully normalized shape (treat null/partial objects as
invalid) before accepting nextPayload; apply the same tightened validation where
similar checks occur (the other usage-check block around lines 122-133).
In `@tests/unit/run-vitest-project-timings.test.ts`:
- Around line 48-61: The test assertion for options.reportDir uses a POSIX-style
regex and will fail on Windows; update the test in
run-vitest-project-timings.test.ts to normalize the resolved path before
matching by converting backslashes to forward slashes (or use
path.normalize/path.sep-aware comparison) after calling timingRunner.parseArgs,
e.g., operate on options.reportDir to produce a normalized string and then
assert it matches the expected ".cache/test-timings" suffix; reference
timingRunner.parseArgs and options.reportDir when making this change.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6cd4a2a-64a8-4397-b7d8-709584a508b0
📒 Files selected for processing (35)
.github/workflows/ci.yml.github/workflows/release.ymlAGENTS.mddocs/testing.mdpackage.jsonscripts/run-vitest-project-timings.jsscripts/verify-main-ci.jsserver/auto-import-runtime/toktrack-runners.jsserver/data-runtime/file-io.jsserver/data-runtime/file-locks.jsserver/data-runtime/import-merge.jsserver/routes/static-routes.jsserver/routes/usage-routes.jssrc/components/cards/MonthMetrics.tsxsrc/components/charts/CostByWeekday.tsxsrc/lib/data-transforms.tssrc/types/index.tstests/e2e/dashboard-settings-backups.spec.tstests/e2e/fixtures.tstests/e2e/helpers.tstests/frontend/dashboard-language-regressions.test.tsxtests/frontend/filter-bar-presets.test.tsxtests/frontend/lazy-dashboard-charts.test.tsxtests/frontend/metric-ratio-locale.test.tsxtests/frontend/requests-over-time.test.tsxtests/unit/background-runtime.test.tstests/unit/dashboard-preferences.test.tstests/unit/data-runtime-contract.test.tstests/unit/http-router-mutations.test.tstests/unit/http-router-static.test.tstests/unit/report-test-timings.test.tstests/unit/run-vitest-project-timings.test.tstests/unit/server-helpers-file-locks.test.tstests/unit/server-helpers-runner-core.test.tstests/unit/test-pipeline-scripts.test.ts
Comment on lines
+224
to
+235
| const startedAt = Date.now(); | ||
| const testResult = runCommand(vitestCommand.command, vitestCommand.args, spawnSyncImpl); | ||
| const durationMs = Date.now() - startedAt; | ||
|
|
||
| if (testResult.status !== 0) { | ||
| return testResult.status || 1; | ||
| } | ||
|
|
||
| const budgetResult = runCommand(budgetCommand.command, budgetCommand.args, spawnSyncImpl); | ||
|
|
||
| if (budgetResult.status !== 0) { | ||
| return budgetResult.status || 1; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
fd -n "run-vitest-project-timings.js" -x wc -l {}Repository: roastcodes/ttdash
Length of output: 292
🏁 Script executed:
# Read the relevant section of the file
cat -n scripts/run-vitest-project-timings.js | sed -n '220,240p'Repository: roastcodes/ttdash
Length of output: 912
🏁 Script executed:
# Find the runCommand function definition
rg "function runCommand|const runCommand" scripts/run-vitest-project-timings.js -A 10Repository: roastcodes/ttdash
Length of output: 328
🏁 Script executed:
# Check the entire runCommand function and surrounding code for error handling
cat -n scripts/run-vitest-project-timings.js | sed -n '1,50p'Repository: roastcodes/ttdash
Length of output: 1619
🏁 Script executed:
# Look for any imports and see what spawnSync is
rg "import|require" scripts/run-vitest-project-timings.js | head -20Repository: roastcodes/ttdash
Length of output: 196
🏁 Script executed:
# Check the full function signature of the run function to understand streams usage
rg "function run" scripts/run-vitest-project-timings.js -A 50 | head -80Repository: roastcodes/ttdash
Length of output: 1724
🌐 Web query:
Node.js spawnSync error property launch failure behavior
💡 Result:
In Node.js child_process.spawnSync, when a launch failure occurs (e.g., the executable cannot be found, typically resulting in an ENOENT error), the function does not throw an exception. Instead, it returns an object with an 'error' property populated with the Error instance detailing the failure (e.g., { error: Error: spawnSync doesnotexist ENOENT ..., status: null, signal: null, output: null, pid: 0, stdout: null, stderr: null }). Other properties like status, stdout, etc., are null or default values since the process never successfully launched. This contrasts with successful spawns where 'error' is undefined/null, and the object contains the process exit status, output, etc. Developers must check result.error to detect and handle launch failures, as shown in official documentation and practical examples. Example: const { spawnSync } = require('child_process'); const result = spawnSync('doesnotexist'); console.log(result); // Output includes error property with ENOENT details if (result.error) { console.error('Launch failed:', result.error); } Note: On Windows, ENOENT often occurs with batch scripts (e.g., 'npm' vs 'npm.cmd'). Use shell: true or specify extensions for cross-platform compatibility. This behavior is consistent across recent Node.js versions (v18+ to v25+).
Citations:
- 1: https://nodejs.org/docs/latest-v18.x/api/child_process.html
- 2:
spawnSyncthrows error instead of defining property oven-sh/bun#8729 - 3: https://stackoverflow.com/questions/37125619/spawnsyncnpm-install-gives-error-spawnsync-npm-enoent
- 4: child_process.spawnSync() fails with ENOENT under Windows nodejs/node-v0.x-archive#25330
- 5: https://nodejs.org/api/child_process.html
- 6: https://nodejs.org/api/child%5Fprocess.html
Surface child-process launch errors before returning.
When spawnSync fails to launch npx or the budget-check node process, status is null and the current code silently returns 1. This turns an actionable setup problem (e.g., executable not found) into an opaque CI/local failure with no diagnostic output.
Add explicit checks for the error property on both testResult and budgetResult to log the actual failure reason before exiting:
Suggested fix
const startedAt = Date.now();
const testResult = runCommand(vitestCommand.command, vitestCommand.args, spawnSyncImpl);
const durationMs = Date.now() - startedAt;
+ if (testResult.error) {
+ streams.stderr.write(
+ `Failed to start ${vitestCommand.command}: ${
+ testResult.error instanceof Error ? testResult.error.message : String(testResult.error)
+ }\n`,
+ );
+ return 1;
+ }
+
if (testResult.status !== 0) {
return testResult.status || 1;
}
const budgetResult = runCommand(budgetCommand.command, budgetCommand.args, spawnSyncImpl);
+ if (budgetResult.error) {
+ streams.stderr.write(
+ `Failed to start ${path.basename(budgetCommand.args[0])}: ${
+ budgetResult.error instanceof Error ? budgetResult.error.message : String(budgetResult.error)
+ }\n`,
+ );
+ return 1;
+ }
+
if (budgetResult.status !== 0) {
return budgetResult.status || 1;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/run-vitest-project-timings.js` around lines 224 - 235, The code
currently ignores spawnSync launch errors for the vitest and budget processes by
only checking testResult.status and budgetResult.status; update the logic after
runCommand calls (for testResult from runCommand(vitestCommand.command,
vitestCommand.args, spawnSyncImpl) and for budgetResult from
runCommand(budgetCommand.command, budgetCommand.args, spawnSyncImpl)) to
explicitly check if testResult.error or budgetResult.error is set, log the error
(including its message) via the existing logger or console with context (e.g.,
"failed to launch vitest" / "failed to launch budget check") and then return a
non-zero exit code, before falling back to the existing status checks and
returns.
Comment on lines
+151
to
+155
| try { | ||
| const localVersion = parseToktrackVersionOutput( | ||
| await runToktrack(localRunner, ['--version'], { | ||
| timeoutMs: getToktrackRunnerTimeouts(localRunner).probeMs, | ||
| }), |
There was a problem hiding this comment.
Use the version-check timeout for the local --version call.
Line 154 is doing the actual local version check, but it still uses probeMs. That makes toktrackLocalRunnerVersionCheckTimeoutMs a dead config knob and can cause local resolution to time out under the wrong threshold when probe and version-check budgets diverge.
Proposed fix
const localVersion = parseToktrackVersionOutput(
await runToktrack(localRunner, ['--version'], {
- timeoutMs: getToktrackRunnerTimeouts(localRunner).probeMs,
+ timeoutMs: getToktrackRunnerTimeouts(localRunner).versionCheckMs,
}),
);📝 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
| try { | |
| const localVersion = parseToktrackVersionOutput( | |
| await runToktrack(localRunner, ['--version'], { | |
| timeoutMs: getToktrackRunnerTimeouts(localRunner).probeMs, | |
| }), | |
| try { | |
| const localVersion = parseToktrackVersionOutput( | |
| await runToktrack(localRunner, ['--version'], { | |
| timeoutMs: getToktrackRunnerTimeouts(localRunner).versionCheckMs, | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/auto-import-runtime/toktrack-runners.js` around lines 151 - 155, The
local `--version` invocation is using the probe timeout, making
toktrackLocalRunnerVersionCheckTimeoutMs ineffective; update the timeout passed
to runToktrack in the parseToktrackVersionOutput call so it uses
getToktrackRunnerTimeouts(localRunner).versionCheckMs (instead of .probeMs) when
invoking runToktrack for the local version check (the call that awaits
runToktrack(localRunner, ['--version'], { timeoutMs: ... })). This ensures the
local version check honors the version-check timeout config.
Comment on lines
+89
to
+136
| async function waitForServerReady( | ||
| serverProcess: ChildProcessWithoutNullStreams, | ||
| baseURL: string, | ||
| authSessionPath: string, | ||
| readOutput: () => string, | ||
| ) { | ||
| const deadline = Date.now() + startupTimeoutMs | ||
| let lastError = '' | ||
|
|
||
| while (Date.now() < deadline) { | ||
| if (serverProcess.exitCode !== null) { | ||
| throw new Error( | ||
| `Playwright test server exited with code ${serverProcess.exitCode}.\n${readOutput()}`, | ||
| ) | ||
| } | ||
|
|
||
| try { | ||
| const response = await fetch(baseURL, { | ||
| redirect: 'manual', | ||
| signal: AbortSignal.timeout(1_000), | ||
| }) | ||
|
|
||
| if (response.status < 400 && fs.existsSync(authSessionPath)) { | ||
| const authHeaders = readApiAuthHeaders(authSessionPath) | ||
| if (authHeaders) { | ||
| const apiResponse = await fetch(new URL('/api/usage', baseURL), { | ||
| headers: authHeaders, | ||
| signal: AbortSignal.timeout(1_000), | ||
| }) | ||
|
|
||
| if (apiResponse.status === 200) { | ||
| return | ||
| } | ||
|
|
||
| lastError = `GET /api/usage returned ${apiResponse.status}` | ||
| } | ||
| } | ||
| } catch (error) { | ||
| lastError = error instanceof Error ? error.message : String(error) | ||
| } | ||
|
|
||
| await sleep(250) | ||
| } | ||
|
|
||
| throw new Error( | ||
| `Timed out waiting for Playwright test server at ${baseURL}. Last error: ${lastError}\n${readOutput()}`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "fixtures.ts" -path "*/e2e/*" 2>/dev/null | head -20Repository: roastcodes/ttdash
Length of output: 43
🏁 Script executed:
git ls-files | grep -E "fixtures\.ts|e2e" | head -20Repository: roastcodes/ttdash
Length of output: 306
🏁 Script executed:
cat -n tests/e2e/fixtures.ts | sed -n '80,150p'Repository: roastcodes/ttdash
Length of output: 2439
🏁 Script executed:
cat -n tests/e2e/fixtures.ts | head -90Repository: roastcodes/ttdash
Length of output: 2896
🏁 Script executed:
rg "spawn\(" tests/e2e/fixtures.ts -B 3 -A 3Repository: roastcodes/ttdash
Length of output: 347
🏁 Script executed:
rg "spawn\(" tests/e2e/fixtures.ts -B 5 -A 15Repository: roastcodes/ttdash
Length of output: 941
🏁 Script executed:
cat -n tests/e2e/fixtures.ts | sed -n '140,200p'Repository: roastcodes/ttdash
Length of output: 2168
🏁 Script executed:
cat > /tmp/verify_spawn_error.md << 'EOF'
According to Node.js documentation:
- child_process.spawn() returns a ChildProcess immediately
- If the process cannot be spawned (e.g., command not found),
an 'error' event is emitted on the ChildProcess object
- Without an error listener, this error event may cause issues
- The exitCode property reflects when the child process exits
The issue: If spawn() fails to start the child, the error event
is emitted asynchronously. The waitForServerReady function only checks
exitCode and loops for 120 seconds, masking the actual spawn error.
EOF
cat /tmp/verify_spawn_error.mdRepository: roastcodes/ttdash
Length of output: 607
🏁 Script executed:
cat -n tests/e2e/fixtures.ts | sed -n '178,220p'Repository: roastcodes/ttdash
Length of output: 1965
Handle spawn errors in waitForServerReady to fail fast.
The spawn() call at line 182 has no error handler. If the child process fails to start (e.g., script not found), the 'error' event is emitted but unhandled. Meanwhile, waitForServerReady() only checks exitCode and loops for the full 120s timeout, masking the actual spawn error with a misleading readiness failure message.
Add an error listener to catch spawn failures immediately:
Suggested fix
async function waitForServerReady(
serverProcess: ChildProcessWithoutNullStreams,
baseURL: string,
authSessionPath: string,
readOutput: () => string,
) {
const deadline = Date.now() + startupTimeoutMs
let lastError = ''
+ let spawnError: Error | null = null
+
+ serverProcess.once('error', (error) => {
+ spawnError = error
+ })
while (Date.now() < deadline) {
+ if (spawnError) {
+ throw new Error(
+ `Failed to start Playwright test server: ${spawnError.message}\n${readOutput()}`,
+ )
+ }
+
if (serverProcess.exitCode !== null) {
throw new Error(
`Playwright test server exited with code ${serverProcess.exitCode}.\n${readOutput()}`,
)
}📝 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
| async function waitForServerReady( | |
| serverProcess: ChildProcessWithoutNullStreams, | |
| baseURL: string, | |
| authSessionPath: string, | |
| readOutput: () => string, | |
| ) { | |
| const deadline = Date.now() + startupTimeoutMs | |
| let lastError = '' | |
| while (Date.now() < deadline) { | |
| if (serverProcess.exitCode !== null) { | |
| throw new Error( | |
| `Playwright test server exited with code ${serverProcess.exitCode}.\n${readOutput()}`, | |
| ) | |
| } | |
| try { | |
| const response = await fetch(baseURL, { | |
| redirect: 'manual', | |
| signal: AbortSignal.timeout(1_000), | |
| }) | |
| if (response.status < 400 && fs.existsSync(authSessionPath)) { | |
| const authHeaders = readApiAuthHeaders(authSessionPath) | |
| if (authHeaders) { | |
| const apiResponse = await fetch(new URL('/api/usage', baseURL), { | |
| headers: authHeaders, | |
| signal: AbortSignal.timeout(1_000), | |
| }) | |
| if (apiResponse.status === 200) { | |
| return | |
| } | |
| lastError = `GET /api/usage returned ${apiResponse.status}` | |
| } | |
| } | |
| } catch (error) { | |
| lastError = error instanceof Error ? error.message : String(error) | |
| } | |
| await sleep(250) | |
| } | |
| throw new Error( | |
| `Timed out waiting for Playwright test server at ${baseURL}. Last error: ${lastError}\n${readOutput()}`, | |
| ) | |
| } | |
| async function waitForServerReady( | |
| serverProcess: ChildProcessWithoutNullStreams, | |
| baseURL: string, | |
| authSessionPath: string, | |
| readOutput: () => string, | |
| ) { | |
| const deadline = Date.now() + startupTimeoutMs | |
| let lastError = '' | |
| let spawnError: Error | null = null | |
| serverProcess.once('error', (error) => { | |
| spawnError = error | |
| }) | |
| while (Date.now() < deadline) { | |
| if (spawnError) { | |
| throw new Error( | |
| `Failed to start Playwright test server: ${spawnError.message}\n${readOutput()}`, | |
| ) | |
| } | |
| if (serverProcess.exitCode !== null) { | |
| throw new Error( | |
| `Playwright test server exited with code ${serverProcess.exitCode}.\n${readOutput()}`, | |
| ) | |
| } | |
| try { | |
| const response = await fetch(baseURL, { | |
| redirect: 'manual', | |
| signal: AbortSignal.timeout(1_000), | |
| }) | |
| if (response.status < 400 && fs.existsSync(authSessionPath)) { | |
| const authHeaders = readApiAuthHeaders(authSessionPath) | |
| if (authHeaders) { | |
| const apiResponse = await fetch(new URL('/api/usage', baseURL), { | |
| headers: authHeaders, | |
| signal: AbortSignal.timeout(1_000), | |
| }) | |
| if (apiResponse.status === 200) { | |
| return | |
| } | |
| lastError = `GET /api/usage returned ${apiResponse.status}` | |
| } | |
| } | |
| } catch (error) { | |
| lastError = error instanceof Error ? error.message : String(error) | |
| } | |
| await sleep(250) | |
| } | |
| throw new Error( | |
| `Timed out waiting for Playwright test server at ${baseURL}. Last error: ${lastError}\n${readOutput()}`, | |
| ) | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 115-115: File data in outbound network request
Outbound network request depends on file data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/fixtures.ts` around lines 89 - 136, The readiness loop in
waitForServerReady can hang the full timeout if the child process fails to spawn
because there is no handler for the serverProcess 'error' event; add an
immediate failure path by creating an errorPromise that listens for
serverProcess.once('error', err => reject(new Error(`Playwright server spawn
error: ${err.message}\n${readOutput()}`))) and then await
Promise.race([errorPromise, existing readiness loop promise]) (or integrate the
reject into the existing async flow) so that spawn errors reject immediately
instead of waiting for startupTimeoutMs; reference waitForServerReady,
serverProcess, readOutput, and startupTimeoutMs when making the change.
Comment on lines
+181
to
+186
| const workerServer = buildWorkerServer(workerInfo.parallelIndex) | ||
| const serverProcess = spawn(process.execPath, ['scripts/start-test-server.js'], { | ||
| cwd: process.cwd(), | ||
| env: workerServer.env, | ||
| }) | ||
| const readOutput = createOutputBuffer(serverProcess) |
There was a problem hiding this comment.
Clear the worker runtime directory before booting the server.
Line 37 points every rerun for a given worker index at the same runtime tree, and Line 111 only requires the auth session file to exist before using it. A stale session-auth.json or persisted app state from a previous aborted run can make the next worker look ready too early and leak state across runs.
Suggested fix
async ({}, runFixture, workerInfo) => {
const workerServer = buildWorkerServer(workerInfo.parallelIndex)
+ fs.rmSync(workerServer.runtimeRoot, { recursive: true, force: true })
const serverProcess = spawn(process.execPath, ['scripts/start-test-server.js'], {
cwd: process.cwd(),
env: workerServer.env,
})📝 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
| const workerServer = buildWorkerServer(workerInfo.parallelIndex) | |
| const serverProcess = spawn(process.execPath, ['scripts/start-test-server.js'], { | |
| cwd: process.cwd(), | |
| env: workerServer.env, | |
| }) | |
| const readOutput = createOutputBuffer(serverProcess) | |
| const workerServer = buildWorkerServer(workerInfo.parallelIndex) | |
| fs.rmSync(workerServer.runtimeRoot, { recursive: true, force: true }) | |
| const serverProcess = spawn(process.execPath, ['scripts/start-test-server.js'], { | |
| cwd: process.cwd(), | |
| env: workerServer.env, | |
| }) | |
| const readOutput = createOutputBuffer(serverProcess) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/fixtures.ts` around lines 181 - 186, Before spawning the test
server, remove any persisted runtime artifacts for that worker so stale
session-auth.json or app state cannot leak into the new run: obtain the runtime
directory from buildWorkerServer(workerInfo.parallelIndex) (use the returned
object's runtimeDir field or the env variable it sets, e.g.,
workerServer.env.WORKER_RUNTIME), recursively delete or empty that directory
(all files including session-auth.json) and then proceed to
spawn(process.execPath, ['scripts/start-test-server.js'], { cwd: process.cwd(),
env: workerServer.env, }); ensure the cleanup happens synchronously/awaited
before createOutputBuffer(serverProcess) is called.
Comment on lines
+212
to
+224
| it('localizes command palette action groups while keeping representative command ids renderable', async () => { | ||
| renderWithAppProviders(<CommandPalette {...buildCommandPaletteProps()} />) | ||
|
|
||
| fireEvent.keyDown(document, { key: 'k', metaKey: true }) | ||
|
|
||
| expect(await screen.findByRole('dialog', { name: 'Command Palette' })).toBeInTheDocument() | ||
| expect(screen.getByText('Daten laden')).toBeInTheDocument() | ||
| expect(screen.getByText('Exporte')).toBeInTheDocument() | ||
| expect(screen.getByText('Wartung')).toBeInTheDocument() | ||
| expect(screen.getByTestId('command-auto-import')).toBeInTheDocument() | ||
| expect(screen.getByTestId('command-csv')).toBeInTheDocument() | ||
| expect(screen.getByTestId('command-settings-open')).toBeInTheDocument() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move this command-palette regression into a focused test file.
This adds keyboard + command-palette-specific behavior into an already broad dashboard language regression suite. Please split this into a dedicated focused file (for example, tests/frontend/command-palette-language.test.tsx) to keep concerns isolated and maintainable.
As per coding guidelines: "Prefer focused *.test.ts or *.test.tsx files over broad catch-all regression suites" and "Split test files once they mix unrelated concerns like content, navigation, localization, motion, or keyboard behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/frontend/dashboard-language-regressions.test.tsx` around lines 212 -
224, Extract the command-palette-specific test block from
tests/frontend/dashboard-language-regressions.test.tsx and place it in a new
focused test file (e.g., tests/frontend/command-palette-language.test.tsx); keep
the test function body (the it(...) block that renders CommandPalette, triggers
fireEvent.keyDown and asserts screen queries) intact but update imports so
renderWithAppProviders, buildCommandPaletteProps, CommandPalette, fireEvent and
screen are imported from the same test helpers/modules used in the original
file; remove the original it(...) block from
dashboard-language-regressions.test.tsx and ensure any shared setup/teardown
(beforeEach/afterEach) used by this test is copied or referenced in the new file
so the test still runs in isolation.
Comment on lines
+240
to
+325
| describe('lazy dashboard charts', () => { | ||
| beforeAll(async () => { | ||
| await initI18n('en') | ||
| }) | ||
|
|
||
| it('renders CostByWeekday peak and low bars without a browser-only chart dependency', () => { | ||
| renderWithTooltip(<CostByWeekday data={weekdayData} />) | ||
|
|
||
| expect(screen.getByText('Cost by weekday')).toBeInTheDocument() | ||
| expect(screen.getByText('Peak: Tu · Low: Sa · Weekend 19%')).toBeInTheDocument() | ||
| expect(screen.getByTestId('chart-bar')).toHaveAttribute('data-name', 'Avg cost') | ||
|
|
||
| const fills = screen.getAllByTestId('bar-cell').map((cell) => cell.getAttribute('data-fill')) | ||
| expect(fills).toHaveLength(7) | ||
| expect(fills.some((fill) => fill?.includes('weekdayPeak'))).toBe(true) | ||
| expect(fills.some((fill) => fill?.includes('weekdayLow'))).toBe(true) | ||
| }) | ||
|
|
||
| it('uses localized day labels as a defensive weekend fallback without weekday indices', () => { | ||
| renderWithTooltip(<CostByWeekday data={legacyWeekdayData} />) | ||
|
|
||
| expect(screen.getByText('Peak: Mar · Low: Sáb. · Weekend 19%')).toBeInTheDocument() | ||
| }) | ||
|
|
||
| it('renders ModelMix for enough model history and skips underspecified input', () => { | ||
| const modelMixData = [ | ||
| createDailyUsage('2026-04-01', { claudeCost: 3, gptCost: 7 }), | ||
| createDailyUsage('2026-04-02', { claudeCost: 5, gptCost: 5 }), | ||
| createDailyUsage('2026-04-03', { claudeCost: 8, gptCost: 2 }), | ||
| ] | ||
| const view = renderWithTooltip(<ModelMix data={modelMixData.slice(0, 2)} />) | ||
|
|
||
| expect(view.container).toBeEmptyDOMElement() | ||
| view.unmount() | ||
|
|
||
| renderWithTooltip(<ModelMix data={modelMixData} />) | ||
|
|
||
| expect(screen.getByText('Model mix')).toBeInTheDocument() | ||
| expect(screen.getByText('Cost share by model over time')).toBeInTheDocument() | ||
| expect(screen.getAllByTestId('chart-area')).toHaveLength(2) | ||
| }) | ||
|
|
||
| it('renders token totals, moving averages, and drilldown clicks in TokensOverTime', () => { | ||
| const onClickDay = vi.fn() | ||
|
|
||
| renderWithTooltip(<TokensOverTime data={tokenData} onClickDay={onClickDay} />) | ||
|
|
||
| expect(screen.getByText('Tokens over time')).toBeInTheDocument() | ||
| expect(screen.getByText('Cache tokens')).toBeInTheDocument() | ||
| expect(screen.getByText('Input / Output tokens')).toBeInTheDocument() | ||
| expect(screen.getByText('Thinking tokens')).toBeInTheDocument() | ||
| expect(screen.getByText('Total tokens (all types)')).toBeInTheDocument() | ||
| expect(screen.getAllByTestId('chart-area')).toHaveLength(6) | ||
| expect(screen.getAllByTestId('chart-line')).toHaveLength(6) | ||
|
|
||
| fireEvent.click(screen.getAllByTestId('composed-chart')[0]) | ||
|
|
||
| expect(onClickDay).toHaveBeenCalledWith('2026-04-01') | ||
| }) | ||
|
|
||
| it('renders PeriodComparison empty and populated states with preset switching', () => { | ||
| const comparisonData = Array.from({ length: 14 }, (_, index) => { | ||
| const day = String(index + 1).padStart(2, '0') | ||
| return createDailyUsage(`2026-04-${day}`, { | ||
| claudeCost: index + 1, | ||
| gptCost: index + 2, | ||
| }) | ||
| }) | ||
| const view = renderWithTooltip(<PeriodComparison data={comparisonData.slice(0, 3)} />) | ||
|
|
||
| expect(screen.getByText('Not enough data for a comparison')).toBeInTheDocument() | ||
| expect(screen.getByText('At least 7 days required (currently: 3)')).toBeInTheDocument() | ||
| view.unmount() | ||
|
|
||
| renderWithTooltip(<PeriodComparison data={comparisonData} />) | ||
|
|
||
| expect(screen.getByText('Period comparison')).toBeInTheDocument() | ||
| expect(screen.getByText('Last week')).toBeInTheDocument() | ||
| expect(screen.getByText('This week')).toBeInTheDocument() | ||
|
|
||
| fireEvent.click(screen.getByRole('button', { name: 'Month' })) | ||
|
|
||
| expect(screen.getByText('Last month')).toBeInTheDocument() | ||
| expect(screen.getByText('This month')).toBeInTheDocument() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Split this catch-all chart suite into focused per-component test files.
This single suite mixes unrelated component behaviors (CostByWeekday, ModelMix, TokensOverTime, PeriodComparison), which hurts isolation and makes failures harder to triage.
As per coding guidelines: "Prefer focused *.test.ts or *.test.tsx files over broad catch-all regression suites" and "Split test files once they mix unrelated concerns like content, navigation, localization, motion, or keyboard behavior." Based on learnings: "When improving coverage, prefer critical branch-heavy runtime modules like src/lib/api.ts, src/hooks/use-usage-data.ts, and src/hooks/use-dashboard-controller.ts over adding another broad dashboard catch-all test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/frontend/lazy-dashboard-charts.test.tsx` around lines 240 - 325, The
current single suite mixes unrelated component tests (CostByWeekday, ModelMix,
TokensOverTime, PeriodComparison); split it into four focused test files by
moving each describe/it blocks into their own test file named for the component
and keep only that component's tests per file (e.g., tests for CostByWeekday ->
new CostByWeekday.test.tsx, ModelMix -> ModelMix.test.tsx, etc.). Ensure each
new file imports the shared helpers (renderWithTooltip, initI18n,
createDailyUsage, weekdayData, tokenData) and either calls initI18n('en') in a
beforeAll or uses a shared test-setup helper; remove the moved tests from the
original catch-all file so it only contains any remaining related tests. Also
keep assertions and test IDs unchanged (e.g., references to CostByWeekday,
ModelMix, TokensOverTime, PeriodComparison, getByTestId keys like
'chart-bar'/'chart-area'/'chart-line'/'composed-chart') so behavior stays
identical.
Comment on lines
+415
to
+652
| it('streams auto-import success events and releases the acquired lease', async () => { | ||
| const lease = { release: vi.fn() } | ||
| const closeImport = vi.fn() | ||
| const performAutoImport = vi.fn(async ({ onCheck, onOutput, onProgress, signalOnClose }) => { | ||
| signalOnClose(closeImport) | ||
| onCheck({ tool: 'toktrack', status: 'found' }) | ||
| onProgress({ key: 'startingLocalImport', vars: {} }) | ||
| onOutput('runner warning') | ||
| return { days: 2, totalCost: 3.5 } | ||
| }) | ||
| const { router } = createRouter({ | ||
| autoImportRuntimeOverrides: { | ||
| acquireAutoImportLease: vi.fn(() => lease), | ||
| performAutoImport, | ||
| }, | ||
| }) | ||
|
|
||
| const { res } = await requestRaw(router, '/api/auto-import/stream', 'POST') | ||
|
|
||
| expect(res.status).toBe(200) | ||
| expect(res.headers['Content-Type']).toBe('text/event-stream') | ||
| expect(res.body).toContain('event: check') | ||
| expect(res.body).toContain('"status":"found"') | ||
| expect(res.body).toContain('event: progress') | ||
| expect(res.body).toContain('event: stderr') | ||
| expect(res.body).toContain('runner warning') | ||
| expect(res.body).toContain('event: success') | ||
| expect(res.body).toContain('"totalCost":3.5') | ||
| expect(res.body).toContain('event: done') | ||
| expect(performAutoImport).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| source: 'auto-import', | ||
| lease, | ||
| }), | ||
| ) | ||
| expect(lease.release).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('maps concurrent auto-import starts to a localized conflict response', async () => { | ||
| const { router } = createRouter({ | ||
| autoImportRuntimeOverrides: { | ||
| acquireAutoImportLease: vi.fn(() => { | ||
| throw Object.assign(new Error('already running'), { messageKey: 'autoImportRunning' }) | ||
| }), | ||
| createAutoImportMessageEvent: vi.fn((key: string) => ({ key })), | ||
| formatAutoImportMessageEvent: vi.fn(() => 'An auto-import is already running.'), | ||
| }, | ||
| }) | ||
|
|
||
| const { res, body } = await request(router, '/api/auto-import/stream', 'POST') | ||
|
|
||
| expect(res.status).toBe(409) | ||
| expect(body).toEqual({ message: 'An auto-import is already running.' }) | ||
| }) | ||
|
|
||
| it('streams structured auto-import errors and releases the lease after failures', async () => { | ||
| const lease = { release: vi.fn() } | ||
| const { router } = createRouter({ | ||
| autoImportRuntimeOverrides: { | ||
| acquireAutoImportLease: vi.fn(() => lease), | ||
| performAutoImport: vi.fn(async () => { | ||
| throw new Error('toktrack failed') | ||
| }), | ||
| toAutoImportErrorEvent: vi.fn((error: Error) => ({ | ||
| message: error.message, | ||
| })), | ||
| }, | ||
| }) | ||
|
|
||
| const { res } = await requestRaw(router, '/api/auto-import/stream', 'POST') | ||
|
|
||
| expect(res.status).toBe(200) | ||
| expect(res.body).toContain('event: error') | ||
| expect(res.body).toContain('"message":"toktrack failed"') | ||
| expect(res.body).toContain('event: done') | ||
| expect(lease.release).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('rejects untrusted hosts before API auth is evaluated', async () => { | ||
| const validateApiRequest = vi.fn(() => ({ | ||
| status: 401, | ||
| message: 'Authentication required', | ||
| })) | ||
| const { router } = createRouter({ | ||
| httpUtilsOverrides: { | ||
| validateRequestHost: vi.fn(() => ({ | ||
| status: 403, | ||
| message: 'Untrusted host header', | ||
| })), | ||
| }, | ||
| remoteAuthOverrides: { | ||
| validateApiRequest, | ||
| }, | ||
| }) | ||
|
|
||
| const { res, body } = await request(router, '/api/runtime', 'GET') | ||
|
|
||
| expect(res.status).toBe(403) | ||
| expect(body).toEqual({ message: 'Untrusted host header' }) | ||
| expect(validateApiRequest).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('forwards API authentication failures with challenge headers', async () => { | ||
| const { router } = createRouter({ | ||
| remoteAuthOverrides: { | ||
| validateApiRequest: vi.fn(() => ({ | ||
| headers: { 'WWW-Authenticate': 'Bearer realm="TTDash API"' }, | ||
| status: 401, | ||
| message: 'Authentication required', | ||
| })), | ||
| }, | ||
| }) | ||
|
|
||
| const { res, body } = await request(router, '/api/runtime', 'GET') | ||
|
|
||
| expect(res.status).toBe(401) | ||
| expect(res.headers['WWW-Authenticate']).toBe('Bearer realm="TTDash API"') | ||
| expect(body).toEqual({ message: 'Authentication required' }) | ||
| }) | ||
|
|
||
| it('serves runtime snapshots only through GET', async () => { | ||
| const snapshot = { | ||
| id: 'runtime-2', | ||
| mode: 'background', | ||
| port: 3020, | ||
| url: 'http://localhost:3020', | ||
| } | ||
| const { getRuntimeSnapshot, router } = createRouter({ | ||
| getRuntimeSnapshot: vi.fn(() => snapshot), | ||
| }) | ||
|
|
||
| const runtimeResponse = await request(router, '/api/runtime', 'GET') | ||
| const rejectedMethod = await request(router, '/api/runtime', 'POST') | ||
|
|
||
| expect(runtimeResponse.res.status).toBe(200) | ||
| expect(runtimeResponse.body).toEqual(snapshot) | ||
| expect(getRuntimeSnapshot).toHaveBeenCalledTimes(1) | ||
| expect(rejectedMethod.res.status).toBe(405) | ||
| expect(rejectedMethod.body).toEqual({ message: 'Method Not Allowed' }) | ||
| }) | ||
|
|
||
| it('keeps unknown API endpoints distinct from stale API prefixes', async () => { | ||
| const { router } = createRouter({ | ||
| httpUtilsOverrides: { | ||
| // Simulate a configured API prefix with no handler separately from a stale /api URL. | ||
| resolveApiPath: (pathname: string) => (pathname === '/custom/unknown' ? '/unknown' : null), | ||
| }, | ||
| }) | ||
|
|
||
| const unknownEndpoint = await request(router, '/custom/unknown', 'GET') | ||
| const stalePrefix = await request(router, '/api/not-configured', 'GET') | ||
|
|
||
| expect(unknownEndpoint.res.status).toBe(404) | ||
| expect(unknownEndpoint.body).toEqual({ message: 'API endpoint not found' }) | ||
| expect(stalePrefix.res.status).toBe(404) | ||
| expect(stalePrefix.body).toEqual({ message: 'Not Found' }) | ||
| }) | ||
|
|
||
| it('rejects PDF reports without usage data before reading the request body', async () => { | ||
| const readBody = vi.fn(async () => ({ title: 'Usage' })) | ||
| const { router } = createRouter({ | ||
| dataRuntimeOverrides: { | ||
| readData: vi.fn(() => null), | ||
| }, | ||
| readBody, | ||
| }) | ||
|
|
||
| const { res, body } = await request(router, '/api/report/pdf', 'POST') | ||
|
|
||
| expect(res.status).toBe(400) | ||
| expect(body).toEqual({ message: 'No data available for the report.' }) | ||
| expect(readBody).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('maps PDF body and generator failures to client or service errors', async () => { | ||
| const usageData = { daily: [{ date: '2026-04-27' }], totals: { totalCost: 1 } } | ||
| const invalidBodyRouter = createRouter({ | ||
| dataRuntimeOverrides: { | ||
| readData: vi.fn(() => usageData), | ||
| }, | ||
| readBody: vi.fn(async () => { | ||
| throw new Error('broken report JSON') | ||
| }), | ||
| }).router | ||
| const oversizedRouter = createRouter({ | ||
| dataRuntimeOverrides: { | ||
| readData: vi.fn(() => usageData), | ||
| }, | ||
| readBody: vi.fn(async () => { | ||
| throw Object.assign(new Error('too large'), { code: 'PAYLOAD_TOO_LARGE' }) | ||
| }), | ||
| }).router | ||
| const typstMissingRouter = createRouter({ | ||
| dataRuntimeOverrides: { | ||
| readData: vi.fn(() => usageData), | ||
| }, | ||
| generatePdfReport: vi.fn(async () => { | ||
| throw Object.assign(new Error('Typst not found'), { code: 'TYPST_MISSING' }) | ||
| }), | ||
| readBody: vi.fn(async () => ({ locale: 'de-CH' })), | ||
| }).router | ||
|
|
||
| const invalidBody = await request(invalidBodyRouter, '/api/report/pdf', 'POST') | ||
| const oversized = await request(oversizedRouter, '/api/report/pdf', 'POST') | ||
| const typstMissing = await request(typstMissingRouter, '/api/report/pdf', 'POST') | ||
|
|
||
| expect(invalidBody.res.status).toBe(400) | ||
| expect(invalidBody.body).toEqual({ message: 'Invalid report request' }) | ||
| expect(oversized.res.status).toBe(413) | ||
| expect(oversized.body).toEqual({ message: 'Report request too large' }) | ||
| expect(typstMissing.res.status).toBe(503) | ||
| expect(typstMissing.body).toEqual({ message: 'Typst not found' }) | ||
| }) | ||
|
|
||
| it('sends generated PDF buffers with attachment headers', async () => { | ||
| const usageData = { daily: [{ date: '2026-04-27' }], totals: { totalCost: 1 } } | ||
| const generatePdfReport = vi.fn(async () => ({ | ||
| buffer: Buffer.from('%PDF-1.4'), | ||
| filename: 'ttdash-report.pdf', | ||
| })) | ||
| const { router } = createRouter({ | ||
| dataRuntimeOverrides: { | ||
| readData: vi.fn(() => usageData), | ||
| }, | ||
| generatePdfReport, | ||
| readBody: vi.fn(async () => ({ locale: 'de-CH' })), | ||
| }) | ||
|
|
||
| const { res } = await requestRaw(router, '/api/report/pdf', 'POST') | ||
|
|
||
| expect(res.status).toBe(200) | ||
| expect(res.headers['Content-Type']).toBe('application/pdf') | ||
| expect(res.headers['Content-Disposition']).toBe( | ||
| 'attachment; filename="ttdash-report.pdf"; filename*=UTF-8\'\'ttdash-report.pdf', | ||
| ) | ||
| expect(res.body).toBe('%PDF-1.4') | ||
| expect(generatePdfReport).toHaveBeenCalledWith(usageData.daily, { locale: 'de-CH' }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Split the streaming/auth/runtime/report coverage out of this mutation suite.
This file now mixes mutation failures with SSE auto-import behavior, host/auth gating, runtime snapshot routing, endpoint resolution, and PDF download handling. That makes failures harder to localize and drifts away from the file’s stated scope. Please move these new cases into focused router/route test files.
As per coding guidelines, "Prefer focused *.test.ts or *.test.tsx files over broad catch-all regression suites" and "Split test files once they mix unrelated concerns like content, navigation, localization, motion, or keyboard behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/http-router-mutations.test.ts` around lines 415 - 652, The PR has
mixed unrelated tests into the mutation suite — move the auto-import SSE tests
(those referencing '/api/auto-import/stream', acquireAutoImportLease,
performAutoImport, toAutoImportErrorEvent, createAutoImportMessageEvent,
formatAutoImportMessageEvent), the runtime/host/auth tests (those exercising
'/api/runtime', validateRequestHost, validateApiRequest, getRuntimeSnapshot),
and the PDF report tests (those using '/api/report/pdf', readData, readBody,
generatePdfReport) into focused spec files (e.g., auto-import.stream.test.ts,
runtime.routes.test.ts, report.pdf.test.ts); copy the relevant it blocks and
test setup that uses createRouter, request, requestRaw and their overrides into
the new files, update imports/describe titles, and leave the original mutation
test file containing only true mutation-related tests so each file tests a
single concern.
Comment on lines
+131
to
+149
| it('prints dry-run commands without spawning Vitest', () => { | ||
| const stdout = { write: vi.fn() } | ||
| const stderr = { write: vi.fn() } | ||
| const spawnSyncImpl = vi.fn(() => ({ status: 0 })) | ||
| const mkdirSync = vi.spyOn(fs, 'mkdirSync') | ||
|
|
||
| const status = timingRunner.run( | ||
| ['--projects=unit', '--repeat=2', '--dry-run'], | ||
| { stdout, stderr }, | ||
| spawnSyncImpl, | ||
| ) | ||
|
|
||
| expect(status).toBe(0) | ||
| expect(spawnSyncImpl).not.toHaveBeenCalled() | ||
| expect(mkdirSync).not.toHaveBeenCalled() | ||
| expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-1.junit.xml') | ||
| expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-2.junit.xml') | ||
|
|
||
| mkdirSync.mockRestore() |
There was a problem hiding this comment.
Always restore the mkdirSync spy.
If an assertion fails before Line 149, the spy leaks into later tests. Wrap the body in try/finally or restore mocks in an afterEach.
♻️ Suggested fix
it('prints dry-run commands without spawning Vitest', () => {
const stdout = { write: vi.fn() }
const stderr = { write: vi.fn() }
const spawnSyncImpl = vi.fn(() => ({ status: 0 }))
const mkdirSync = vi.spyOn(fs, 'mkdirSync')
-
- const status = timingRunner.run(
- ['--projects=unit', '--repeat=2', '--dry-run'],
- { stdout, stderr },
- spawnSyncImpl,
- )
-
- expect(status).toBe(0)
- expect(spawnSyncImpl).not.toHaveBeenCalled()
- expect(mkdirSync).not.toHaveBeenCalled()
- expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-1.junit.xml')
- expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-2.junit.xml')
-
- mkdirSync.mockRestore()
+ try {
+ const status = timingRunner.run(
+ ['--projects=unit', '--repeat=2', '--dry-run'],
+ { stdout, stderr },
+ spawnSyncImpl,
+ )
+
+ expect(status).toBe(0)
+ expect(spawnSyncImpl).not.toHaveBeenCalled()
+ expect(mkdirSync).not.toHaveBeenCalled()
+ expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-1.junit.xml')
+ expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-2.junit.xml')
+ } finally {
+ mkdirSync.mockRestore()
+ }
})📝 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
| it('prints dry-run commands without spawning Vitest', () => { | |
| const stdout = { write: vi.fn() } | |
| const stderr = { write: vi.fn() } | |
| const spawnSyncImpl = vi.fn(() => ({ status: 0 })) | |
| const mkdirSync = vi.spyOn(fs, 'mkdirSync') | |
| const status = timingRunner.run( | |
| ['--projects=unit', '--repeat=2', '--dry-run'], | |
| { stdout, stderr }, | |
| spawnSyncImpl, | |
| ) | |
| expect(status).toBe(0) | |
| expect(spawnSyncImpl).not.toHaveBeenCalled() | |
| expect(mkdirSync).not.toHaveBeenCalled() | |
| expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-1.junit.xml') | |
| expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-2.junit.xml') | |
| mkdirSync.mockRestore() | |
| it('prints dry-run commands without spawning Vitest', () => { | |
| const stdout = { write: vi.fn() } | |
| const stderr = { write: vi.fn() } | |
| const spawnSyncImpl = vi.fn(() => ({ status: 0 })) | |
| const mkdirSync = vi.spyOn(fs, 'mkdirSync') | |
| try { | |
| const status = timingRunner.run( | |
| ['--projects=unit', '--repeat=2', '--dry-run'], | |
| { stdout, stderr }, | |
| spawnSyncImpl, | |
| ) | |
| expect(status).toBe(0) | |
| expect(spawnSyncImpl).not.toHaveBeenCalled() | |
| expect(mkdirSync).not.toHaveBeenCalled() | |
| expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-1.junit.xml') | |
| expect(stdout.write.mock.calls.join('\n')).toContain('vitest-unit.timing-run-2.junit.xml') | |
| } finally { | |
| mkdirSync.mockRestore() | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/run-vitest-project-timings.test.ts` around lines 131 - 149, The
test creates a spy via vi.spyOn(fs, 'mkdirSync') but only calls
mkdirSync.mockRestore() at the end, risking spy leakage on assertion failure;
update the test for 'prints dry-run commands without spawning Vitest' to always
restore the spy by wrapping the test body in a try/finally that calls
mkdirSync.mockRestore() (or move the spy/restore into an afterEach) so mkdirSync
is reliably restored after calling
timingRunner.run(['--projects=unit','--repeat=2','--dry-run'], { stdout, stderr
}, spawnSyncImpl).
Comment on lines
+361
to
+362
| child.stderr.emit('data', Buffer.from('runner warning\n')) | ||
| expect(stderrLines).toEqual(['runner warning']) |
There was a problem hiding this comment.
Match the real onStderr contract in this assertion.
runCommandWithSpawn forwards the raw stderr chunk to onStderr; it only uses trim() to decide whether to call the callback. This expectation should therefore include the trailing newline, otherwise the test is asserting behavior the production code does not have.
Proposed fix
- expect(stderrLines).toEqual(['runner warning'])
+ expect(stderrLines).toEqual(['runner warning\n'])📝 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
| child.stderr.emit('data', Buffer.from('runner warning\n')) | |
| expect(stderrLines).toEqual(['runner warning']) | |
| child.stderr.emit('data', Buffer.from('runner warning\n')) | |
| expect(stderrLines).toEqual(['runner warning\n']) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/server-helpers-runner-core.test.ts` around lines 361 - 362, The
test's assertion mismatches the real onStderr contract: runCommandWithSpawn
forwards the raw stderr chunk (including newline) to onStderr and only trims to
decide whether to call the callback. Update the expectation in the test that
emits to child.stderr (the Buffer.from('runner warning\n') case) so
expect(stderrLines) includes the trailing newline (e.g., 'runner warning\n')
instead of the trimmed string; adjust any other assertions that assume trimming.
Ensure this refers to the stderr emit in the test and the onStderr handler used
to collect stderrLines.
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
2 participants
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 by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation