Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the monolithic server into a composed app runtime and lifecycle, extracts shared settings/dashboard-preferences, decomposes the dashboard controller into focused hooks/components, adds new server runtimes (data/auto-import/background/startup) and HTTP/auth/CSP guards, splits Settings/FilterBar UI, and enforces import rules via dependency-cruiser. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Process
participant App as createAppRuntime()
participant Lifecycle as ServerLifecycle
participant Router as HttpRouter
participant Data as DataRuntime
participant Auth as ServerAuth
participant AutoImport as AutoImportRuntime
CLI->>App: require('./server/app-runtime').createAppRuntime()
App->>Data: createDataRuntime()
App->>Auth: createServerAuth()
App->>AutoImport: createAutoImportRuntime()
App->>Lifecycle: createServerLifecycle({ router, httpUtils, dataRuntime, ... })
Lifecycle->>Router: wire handler -> handleServerRequest
Lifecycle->>CLI: bind/listen on port
Lifecycle->>AutoImport: optionally runStartupAutoLoad()
Router->>Auth: validateApiRequest(req)
Router->>Data: read/write usage & settings under locks
Router->>AutoImport: acquire lease / stream progress (auto-import endpoint)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (16)
src/components/features/request-quality/RequestQuality.tsx (1)
20-51: Add a safe fallback for unexpected metric IDs.The switch currently has no fallback; an unknown
item.idcan produceundefinedentries and break the subsequent render map.🧩 Suggested hardening
const qualityMetrics = requestQualityData.qualityMetrics.map((item) => { switch (item.id) { case 'tokensPerRequest': return { ...item, label: t('requestQuality.tokensPerRequest'), value: metrics.hasRequestData ? formatTokens(item.value) : t('common.notAvailable'), hint: t('requestQuality.tokensHint'), } case 'costPerRequest': return { ...item, label: t('requestQuality.costPerRequest'), value: metrics.hasRequestData ? formatCurrency(item.value) : t('common.notAvailable'), hint: t('requestQuality.costHint'), } case 'cachePerRequest': return { ...item, label: t('requestQuality.cachePerRequest'), value: metrics.hasRequestData ? formatTokens(item.value) : t('common.notAvailable'), hint: t('requestQuality.cacheHint'), } case 'thinkingPerRequest': return { ...item, label: t('requestQuality.thinkingPerRequest'), value: metrics.hasRequestData ? formatTokens(item.value) : t('common.notAvailable'), hint: t('requestQuality.thinkingHint'), } + default: + return { + ...item, + label: item.id, + value: t('common.notAvailable'), + hint: '', + } } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/request-quality/RequestQuality.tsx` around lines 20 - 51, The switch in RequestQuality.tsx inside the qualityMetrics = requestQualityData.qualityMetrics.map(...) lacks a default branch, which can yield undefined for unknown item.id values and break rendering; add a default case that returns a safe fallback (e.g., return the original item or return {...item, label: t('common.notAvailable'), value: t('common.notAvailable')}) so every map iteration always returns a defined object for any item.id.playwright.config.ts (1)
10-10: Consider limiting single-worker mode to CI only.Line 10 serializes all local runs too, which can slow iteration. Keeping
workers: 1only in CI preserves stability while improving local turnaround.💡 Suggested tweak
export default defineConfig({ testDir: './tests/e2e', fullyParallel: false, - workers: 1, + workers: process.env.CI ? 1 : undefined, timeout: 30_000,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright.config.ts` at line 10, The current config forces workers: 1 for all runs; change it so single-worker is applied only in CI by making the workers value conditional on process.env.CI (e.g., set workers to 1 when CI is truthy, otherwise leave undefined or use default), updating the export in playwright.config.ts (the object passed to defineConfig/exportDefault) so local runs are not serialized while CI remains deterministic..dependency-cruiser.cjs (1)
25-31: Avoid a permanent orphan exception for the controller type contract.Whitelisting
src/hooks/use-dashboard-controller-types.tsturns off the dead-code guard for that file entirely. If this is a shared contract, move it undersrc/types/or make it reachable through a real entrypoint instead of exempting it fromno-orphans-src.Based on learnings: Applies to
src/types/**/*.{ts,tsx}: Put TypeScript type definitions insrc/types/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.dependency-cruiser.cjs around lines 25 - 31, The dependency-cruiser rule currently whitelists 'src/hooks/use-dashboard-controller-types.ts' (in pathNot array), which disables orphan checks for that contract; remove that specific exception and either move the shared type file into the canonical types area (e.g., under src/types/) or make it reachable via a real entrypoint (export/import it from an entry module) so the file is no longer orphaned; update references/imports to the new location or add the minimal re-export entrypoint, and then delete the 'src/hooks/use-dashboard-controller-types.ts' entry from the pathNot list in the configuration.docs/architecture.md (1)
145-146: Keep the documented type location aligned with repo guidance.This points contributors to a
.d.tscontract undersrc/lib/, but the repo guidance says TypeScript type definitions belong insrc/types/. Either move that contract or update the docs/guidelines so new type files do not follow two different placement rules.Based on learnings: Applies to
src/types/**/*.{ts,tsx}: Put TypeScript type definitions insrc/types/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture.md` around lines 145 - 146, The doc currently points to `src/lib/dashboard-view-model.d.ts` but repository guidance mandates placing TypeScript definitions under `src/types/`; update the architecture doc to reference `src/types/dashboard-view-model.d.ts` (or move the contract file from `src/lib/dashboard-view-model.d.ts` into `src/types/` and adjust any imports), and ensure the docs include the rule "Apply to src/types/**/*.{ts,tsx}: Put TypeScript type definitions in src/types/" so future contributors follow the single placement convention.scripts/verify-package.js (1)
104-105: Make auth URL parsing scheme-agnostic for future-proofing.The parser currently matches only
http://. Acceptinghttps://as well prevents silent auth-header extraction failures if startup output format changes.💡 Suggested patch
- const match = output.match(/Local Auth URL:\s+(http:\/\/[^\s]+)/); + const match = output.match(/Local Auth URL:\s+(https?:\/\/[^\s]+)/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-package.js` around lines 104 - 105, The regex that extracts the Local Auth URL uses a hard-coded "http://" scheme causing failures for "https://"; update the call to output.match in scripts/verify-package.js so it accepts either scheme (e.g., use a pattern like /(Local Auth URL:\s+(https?:\/\/[^\s]+))/ or otherwise make the scheme optional) and keep the rest of the downstream logic that uses the captured URL (the match variable) unchanged.src/components/layout/FilterBarQuickControls.tsx (1)
34-40: Prefer shared preset metadata over another local preset list.The PR already centralizes preset semantics in the shared dashboard-preferences contract, but this component recreates the visible
7d/30d/month/year/allsurface locally. That makes the quick controls another place that can drift when a preset is renamed, hidden, or reordered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/FilterBarQuickControls.tsx` around lines 34 - 40, Replace the locally defined presets array in FilterBarQuickControls with the shared preset metadata exported by the dashboard-preferences contract: remove the hardcoded presets variable and import the central presets (and/or its DashboardDatePreset type) from the shared contract, then use that imported array wherever presets is referenced so the component consumes the single source-of-truth for keys/labels/order/visibility; ensure type compatibility (replace the local satisfies Array<{ key: DashboardDatePreset; label: string }> with the imported type) and update any references in the component to the new imported identifier.src/lib/calculations.ts (1)
34-41: Delegation simplifies code but may duplicate computation.Both
computeModelCostsandcomputeProviderMetricsnow independently callsummarizeUsageBreakdowns. If both are invoked on the same dataset (e.g., in the same render), the aggregation runs twice.Consider whether callers (like
use-computed-metrics.ts) should callsummarizeUsageBreakdownsdirectly and destructure both results, rather than calling these wrapper functions separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/calculations.ts` around lines 34 - 41, Both computeModelCosts and computeProviderMetrics call summarizeUsageBreakdowns separately which duplicates work; update callers (e.g., use-computed-metrics.ts) to call summarizeUsageBreakdowns(data) once and destructure the returned { modelCosts, providerMetrics } so both values are reused, then remove or deprecate the thin wrappers computeModelCosts/computeProviderMetrics (or keep them but no longer call them from the hot path). Reference summarizeUsageBreakdowns, computeModelCosts, computeProviderMetrics and the caller use-computed-metrics.ts when making this change.src/components/layout/FilterBarDateRange.tsx (1)
72-73: Consider memoizingtodayvalue.
localToday()is called on every render. While the impact is minimal for a simple date string, memoizing it would be more consistent with the other memoized values in this component.🔧 Suggested refactor
- const today = localToday() - const [focusedDate, setFocusedDate] = useState<string | null>(value ?? today) + const today = useMemo(() => localToday(), []) + const [focusedDate, setFocusedDate] = useState<string | null>(value ?? localToday())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/FilterBarDateRange.tsx` around lines 72 - 73, The component calls localToday() on every render, so memoize the computed today value and use that memoized value to initialize focusedDate; specifically replace the direct call to localToday() with a memoized value (e.g., useMemo(() => localToday(), [])) and keep the useState initialization for focusedDate (value ?? today) and related setters (setFocusedDate) unchanged so the component behavior remains the same.src/hooks/use-dashboard-controller-derived-state.ts (1)
10-23: Avoid deriving this exported controller contract from hookReturnTypes.
DashboardControllerDerivedStateis now part of the controller boundary, butfiltersandcomputedare tied directly to whateveruseDashboardFilters()anduseComputedMetrics()happen to return today. That makes small internal hook shape changes cascade through the controller API. Prefer explicit shared types in the controller-types layer here instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-dashboard-controller-derived-state.ts` around lines 10 - 23, DashboardControllerDerivedState currently depends on hook shapes via ReturnType<typeof useDashboardFilters> and ReturnType<typeof useComputedMetrics>, which leaks internal hook types into the controller API; replace those ReturnType references with explicit shared types exported from your controller-types layer (e.g., DashboardFilters and ComputedMetrics or similarly named interfaces) and import/use those types in place of ReturnType<typeof useDashboardFilters> and ReturnType<typeof useComputedMetrics>; ensure any other hook-derived fields (like forecastState if needed) use explicit exported types instead of ReturnType to prevent internal hook shape changes from changing the controller contract.server/data-runtime.js (1)
430-433: Consider optimizing equivalence check for large datasets.The
JSON.stringifycomparison works correctly but performs full serialization on both objects. For entries with manymodelBreakdowns, a field-by-field comparison with early exit on mismatch would be more efficient.♻️ Optional: Field-by-field comparison with early exit
function areUsageDaysEquivalent(left, right) { - return ( - JSON.stringify(canonicalizeUsageDay(left)) === JSON.stringify(canonicalizeUsageDay(right)) - ); + const l = canonicalizeUsageDay(left); + const r = canonicalizeUsageDay(right); + + if (l.date !== r.date || l.totalCost !== r.totalCost || l.totalTokens !== r.totalTokens) { + return false; + } + if (l.modelBreakdowns.length !== r.modelBreakdowns.length) { + return false; + } + + return JSON.stringify(l.modelBreakdowns) === JSON.stringify(r.modelBreakdowns); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/data-runtime.js` around lines 430 - 433, The current areUsageDaysEquivalent uses JSON.stringify(canonicalizeUsageDay(...)) which serializes entire objects; replace it with a field-by-field comparison: call canonicalizeUsageDay for left and right into local vars, compare top-level scalar fields first (date/ids/etc.) and return false on mismatch, then compare modelBreakdowns lengths and iterate through entries comparing keys and values with early returns on any mismatch (and compare nested primitives similarly) so the function exits as soon as a difference is found; keep the function name areUsageDaysEquivalent and continue to use canonicalizeUsageDay to normalize inputs.src/components/features/settings/settings-modal-helpers.ts (2)
95-117: Verify drag-and-drop semantics match user expectations.The
reorderSettingsSectionsfunction moves the source item to insert before the target when dragging forward (e.g., moving item 0 to position 2 results in inserting at index 1 after removal). This is standard drag-and-drop "insert before drop target" behavior, but the docstring says "moving the source section to the target slot."Consider clarifying the docstring to explicitly state "inserts before the target section" to avoid confusion during maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/settings/settings-modal-helpers.ts` around lines 95 - 117, The docstring for reorderSettingsSections is ambiguous about drop semantics; update the comment above the function reorderSettingsSections to state that the function removes the source item and inserts it before the target section (i.e., "insert before the target section" behavior when dragging forward), so maintainers know the function implements an "insert before" drag-and-drop semantics rather than replacing or appending to the target slot.
11-19: Potential precision issue withtoFixed(2)thenNumber()conversion.The function uses
toFixed(2)which returns a string, then wraps it inNumber(). This is correct, but note that floating-point arithmetic can still cause precision issues. For currency values, consider documenting that this is intentional for display purposes only, or use a decimal library for financial calculations if precision is critical.// Example: parseSettingsNumberInput('0.1') + parseSettingsNumberInput('0.2') may not equal 0.3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/settings/settings-modal-helpers.ts` around lines 11 - 19, The parseSettingsNumberInput function currently uses parsed.toFixed(2) wrapped in Number which can mask floating-point precision issues; replace that string-based rounding with numeric rounding (e.g., use Math.round(parsed * 100) / 100) to avoid unnecessary string conversions and keep a numeric result in parseSettingsNumberInput, and add a short comment above parseSettingsNumberInput stating this is display/formatting rounding only and that a decimal library (like decimal.js or Big.js) should be used if exact financial precision is required.src/components/features/settings/SettingsModalSections.tsx (2)
756-768: Number input value binding may cause UX issue.The input
value={config.subscriptionPrice}binds a parsed number, butparseSettingsNumberInputrounds to 2 decimal places and clamps to non-negative. This means if a user types "1.234", on blur/change the value snaps to "1.23" which is expected. However, while typing, the controlled input might feel sluggish or jump around since every keystroke triggers theonChangewhich parses and updates.Consider using an uncontrolled input with
defaultValueand parsing on blur, or use a local string state that only parses/commits on blur to improve typing UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/settings/SettingsModalSections.tsx` around lines 756 - 768, The controlled number input currently uses value={config.subscriptionPrice} and immediately parses/commits each keystroke via parseSettingsNumberInput and viewModel.onProviderChange, causing input jumps; change the input to use a local string state (e.g., subscriptionPriceLocal) initialized from config.subscriptionPrice (or use defaultValue for an uncontrolled input), update that local string on every onChange, and only call parseSettingsNumberInput and viewModel.onProviderChange(provider, { subscriptionPrice: ... }) on blur (onBlur) or Enter to commit the parsed/clamped number; keep the disabled prop and className logic as-is and reference parseSettingsNumberInput, viewModel.onProviderChange, config.subscriptionPrice and provider when applying the change.
346-378: Consider adding keyboard support for drag-and-drop.The drag-and-drop implementation works for mouse users, but the
draggableattribute alone doesn't provide keyboard accessibility. The up/down buttons (lines 400-427) provide an accessible alternative, which is good. However, the grip handle visual might mislead keyboard users into thinking they can drag with keyboard.Consider either:
- Hiding the grip icon from screen readers with
aria-hidden="true"(already implicitly hidden since it's decorative), or- Adding a visually-hidden label explaining to use arrow buttons for keyboard reordering.
This is a minor accessibility enhancement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/settings/SettingsModalSections.tsx` around lines 346 - 378, The grip handle in SettingsModalSections's draggable row can mislead keyboard users into expecting keyboard drag support; update the grip icon element (the visual handle inside the draggable container in SettingsModalSections) to be hidden from assistive tech by adding aria-hidden="true", or alternatively add a visually-hidden label (e.g., a sr-only span or aria-label on the handle) that states "Use the up and down buttons to reorder items" so screen-reader users know to use the existing up/down buttons tied to the viewModel reordering methods.server/http-router.js (2)
102-134: Mixing sync try/catch with async callback inserveFile.The outer
try/catch(lines 125-133) wrapsfs.readFilewhich uses a callback pattern. This catch block will only handle synchronous exceptions thrown before the callback is invoked (e.g., during argument validation), not errors that occur during file reading. While this provides some defense againstERR_INVALID_ARG_VALUE, the pattern is unusual and may confuse maintainers. Consider adding a comment explaining the intent, or refactor to usefs.promises.readFilewith proper async/await for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/http-router.js` around lines 102 - 134, The try/catch around the callback-based fs.readFile in serveFile is ineffective for async errors; refactor serveFile to use fs.promises.readFile with async/await (or remove the outer try/catch and handle all errors inside the readFile callback) so ERR_INVALID_ARG_VALUE is caught reliably. Specifically, make serveFile async, await fs.promises.readFile(reqPath) in a try/catch, fall back to reading path.join(staticRoot, 'index.html') on ENOENT, and call sendStaticFile or writeStaticErrorResponse based on caught errors; this ensures all file-read errors (including ERR_INVALID_ARG_VALUE) are handled in the same catch block.
519-524: Path traversal check has edge case for root directory.The condition allows
filePath === path.resolve(staticRoot, 'index.html')as a special case, but doesn't explicitly blockfilePath === resolvedStaticRoot(the directory itself). WhileserveFilewould likely fail gracefully when trying to read a directory, consider tightening the check to be explicit:if ( !filePath.startsWith(resolvedStaticRoot + path.sep) && - filePath !== path.resolve(staticRoot, 'index.html') + filePath !== resolvedStaticRoot + path.sep + 'index.html' ) {Alternatively, verify that
serveFilehandles the directory case correctly (EISDIR error).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/http-router.js` around lines 519 - 524, The current path traversal guard in the HTTP router special-cases index.html but doesn't explicitly forbid serving the root directory itself; update the condition (that uses filePath, resolvedStaticRoot, path.sep, staticRoot, and path.resolve) to also reject filePath === resolvedStaticRoot (or otherwise ensure filePath is a regular file) before calling serveFile, or add an explicit fs.stat check to confirm the path is not a directory and return 403 if stat indicates a directory or if stat fails with EISDIR, so directories cannot be served.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/review/dashboard-review.md`:
- Around line 16-61: Update docs/review/dashboard-review.md so resolved items
are clearly marked as historical by adding a short banner at the top and/or
appending a status line to each finding (e.g., after headings M-01, M-02, N-01,
N-02) indicating "Resolved — see docs/review/fixed-findings.md" or current
status/date; ensure the banner references the fixed-findings.md file and the
specific finding IDs (M-01, M-02, N-01, N-02) so readers don’t treat them as
open issues.
In `@server/cli.js`:
- Around line 143-145: The port parsing currently uses Number.parseInt on
parsed.values.port (stored as parsedPort) which allows inputs like "3000abc";
update the validation to require the entire port string be numeric before
conversion—e.g., check parsed.values.port matches /^\d+$/ (or use String/Number
round-trip) and only then convert to integer and validate range; if the check
fails call exitWithHelp as before. Ensure you reference parsed.values.port and
parsedPort in the change so the new check replaces the Number.parseInt-based
acceptance.
In `@server/process-utils.js`:
- Around line 18-22: The formatDateTime function can throw when given invalid
date inputs; update formatDateTime to first construct a Date from value (e.g.,
const d = new Date(value)), check if d.getTime() is NaN (or otherwise invalid),
and if so return a safe fallback (empty string or the original value) instead of
calling Intl.DateTimeFormat; only call Intl.DateTimeFormat.format when the date
is valid to avoid RangeError bubbling up from formatDateTime.
In `@server/security-headers.js`:
- Around line 51-55: The current head detection uses html.includes('<head>') and
replace('<head>', ...) which misses variants like <head data-...>; update the
logic in the function that injects the nonce metaTag to search for the opening
<head> with a case-insensitive regex (e.g. match /<head\b[^>]*>/i) and replace
the first match with the matched tag plus the injected metaTag (instead of
prepending to the document). Ensure you use the same unique symbols metaTag and
html and only replace the first occurrence so injected markup lands inside any
<head ...> variant.
In `@server/startup-runtime.js`:
- Around line 104-105: Printed API URLs and curl examples currently hardcode
"/api/usage" which fails when runtime apiPrefix differs; update the log output
and example strings to use the runtime apiPrefix variable (e.g., replace
occurrences of "/api/usage" with `${apiPrefix}/usage`) where the server prints
the API URL (the log calls that use url and port) and in the curl example
outputs so all displayed endpoints are built from apiPrefix, url and port rather
than hardcoded paths.
In `@shared/dashboard-preferences.d.ts`:
- Around line 52-53: Update the TypeScript declaration for
parseDashboardPreferencesConfig to accept the second optional parameter that the
JS implementation supports: add an options parameter (e.g., { validDatePresets?:
string[]; validViewModes?: string[]; validSectionIds?: string[] }) so callers
can pass custom validation options; keep the return type as
DashboardPreferencesConfig and reference the existing function name
parseDashboardPreferencesConfig when making the change.
In `@shared/dashboard-preferences.js`:
- Around line 22-50: validateSectionDefinitions currently only validates
individual entries and allowed ids but permits duplicates or missing ids; update
validateSectionDefinitions to ensure the resulting entries’ id set exactly
equals the provided validSectionIds (no duplicates, no omissions). After mapping
entries, compute the set of ids found and if its size differs from
validSectionIds.length or it does not contain every id in validSectionIds, throw
a descriptive Error; also detect duplicate ids during processing (e.g., by
checking if an id is already seen) and throw on duplicate to fail fast before
DASHBOARD_SECTION_DEFINITION_MAP would be built/overwritten.
In `@src/components/features/drill-down/DrillDownModal.tsx`:
- Around line 84-97: The model breakdown still uses hardcoded locale keys (e.g.,
"common.cacheRead"/"common.cacheWrite"); update the per-model token cards to
call the canonical getTokenSegmentLabelKey(id) instead of using literal strings
so both the DrillDownModal and model breakdown use the same labels; if needed,
export getTokenSegmentLabelKey from DrillDownModal.tsx (or move it to a shared
util) and import it into the per-model token card component, ensuring the
function accepts the same DrillDownTokenSegmentId type used by the model cards.
In `@src/components/features/settings/SettingsModal.tsx`:
- Line 264: The save flow in handleSave (use-settings-modal-draft.ts) must catch
failures from the await onSaveSettings() call: wrap the await onSaveSettings()
in a try-catch-finally block inside handleSave, in the catch call the
component’s existing error surface (e.g., invoke any provided
onSaveError/onError handler or set a local saveError state) and ensure
settingsBusy is cleared in finally; if no error callback exists, at minimum log
the error and set an error state so the UI can show a message.
In `@src/components/features/settings/use-settings-modal-draft.ts`:
- Around line 194-208: The handleSave function currently awaits onSaveSettings
but lacks error handling and user feedback; wrap the await in a try-catch, call
the underlying saveSettings wrapper (onSaveSettings) inside the try, on success
call applyDefaultFilters(updatedSettings.defaultFilters) and
addToast(t('toasts.settingsSaved'), 'success') and then close the modal via
onOpenChange(false), and on failure catch the error and show
addToast(normalizeErrorMessage(error) ?? t('api.saveSettingsFailed'), 'error');
ensure you reference handleSave / onSaveSettings and include dependencies
saveSettings, applyDefaultFilters, addToast and normalizeErrorMessage in any
related hooks or callbacks accordingly.
In `@src/components/tables/RecentDays.tsx`:
- Around line 62-63: The sort-state updates in RecentDays.tsx
(sortKey/setSortKey and sortAsc/setSortAsc) are non-atomic: resolveNextSortState
called inside startTransition captures stale render-time values so rapid clicks
can lose toggles; fix by making updates atomic — either combine sortKey and
sortAsc into a single useState object (e.g., { key, asc }) and update it in one
setState call, or keep separate states but use functional updaters
(setSortKey(prev=>...) and setSortAsc(prev=>...)) inside
resolveNextSortState/startTransition so each update reads the latest committed
state and correctly flips direction on successive clicks.
In `@src/hooks/use-dashboard-controller-drill-down.ts`:
- Around line 17-20: The drill-down dialog can stay open when drillDownDate is
removed from filteredData causing drillDownDay to become null; add logic in the
use-dashboard-controller-drill-down hook to close/reset the dialog when that
happens (e.g., add an effect that watches drillDownDay and calls setOpen(false)
and clears drillDownDate when drillDownDay === null) so the open state cannot
remain true while drillDownDay is null; reference drillDownDay, drillDownDate,
filteredData and open/setOpen in the new effect to implement this fix.
In `@src/hooks/use-dashboard-controller-effects.ts`:
- Around line 39-43: The useEffect invokes i18n.changeLanguage(language) but
doesn't handle promise rejections; update the effect (around useEffect,
i18n.changeLanguage, i18n.resolvedLanguage, language) to await or attach a
.catch to the returned Promise and handle failures (e.g., log the error via
console or a logger and/or set an error state/notify the user) so language-load
failures aren't silently ignored; ensure the changeLanguage call still only runs
when i18n.resolvedLanguage !== language.
In `@src/hooks/use-dashboard-controller-shell-state.ts`:
- Around line 14-16: normalizeErrorMessage currently only extracts messages from
Error instances so non-Error shapes like plain strings or { message } objects
(used by settingsError/usageError) return null and hide the recovery UI; update
normalizeErrorMessage to accept unknown values by: if value is a non-empty
string return the trimmed string, if it's an object with a string "message"
property return that trimmed message, otherwise keep the existing Error handling
and return null for everything else (refer to normalizeErrorMessage and usages
loadError/settingsError/usageError to ensure all upstream unknowns are
normalized).
In `@src/lib/dashboard-preferences.ts`:
- Around line 1-17: The current facade re-exports many symbols from
'../../shared/dashboard-preferences.js' but omits the factory
createDefaultDashboardFilters, causing callers to bypass this adapter; update
the export list in src/lib/dashboard-preferences.ts to include
createDefaultDashboardFilters so callers can obtain fresh default filter objects
via the frontend adapter (i.e., add the createDefaultDashboardFilters symbol
alongside the existing exported names such as normalizeDashboardDefaultFilters
and DEFAULT_DASHBOARD_FILTERS).
---
Nitpick comments:
In @.dependency-cruiser.cjs:
- Around line 25-31: The dependency-cruiser rule currently whitelists
'src/hooks/use-dashboard-controller-types.ts' (in pathNot array), which disables
orphan checks for that contract; remove that specific exception and either move
the shared type file into the canonical types area (e.g., under src/types/) or
make it reachable via a real entrypoint (export/import it from an entry module)
so the file is no longer orphaned; update references/imports to the new location
or add the minimal re-export entrypoint, and then delete the
'src/hooks/use-dashboard-controller-types.ts' entry from the pathNot list in the
configuration.
In `@docs/architecture.md`:
- Around line 145-146: The doc currently points to
`src/lib/dashboard-view-model.d.ts` but repository guidance mandates placing
TypeScript definitions under `src/types/`; update the architecture doc to
reference `src/types/dashboard-view-model.d.ts` (or move the contract file from
`src/lib/dashboard-view-model.d.ts` into `src/types/` and adjust any imports),
and ensure the docs include the rule "Apply to src/types/**/*.{ts,tsx}: Put
TypeScript type definitions in src/types/" so future contributors follow the
single placement convention.
In `@playwright.config.ts`:
- Line 10: The current config forces workers: 1 for all runs; change it so
single-worker is applied only in CI by making the workers value conditional on
process.env.CI (e.g., set workers to 1 when CI is truthy, otherwise leave
undefined or use default), updating the export in playwright.config.ts (the
object passed to defineConfig/exportDefault) so local runs are not serialized
while CI remains deterministic.
In `@scripts/verify-package.js`:
- Around line 104-105: The regex that extracts the Local Auth URL uses a
hard-coded "http://" scheme causing failures for "https://"; update the call to
output.match in scripts/verify-package.js so it accepts either scheme (e.g., use
a pattern like /(Local Auth URL:\s+(https?:\/\/[^\s]+))/ or otherwise make the
scheme optional) and keep the rest of the downstream logic that uses the
captured URL (the match variable) unchanged.
In `@server/data-runtime.js`:
- Around line 430-433: The current areUsageDaysEquivalent uses
JSON.stringify(canonicalizeUsageDay(...)) which serializes entire objects;
replace it with a field-by-field comparison: call canonicalizeUsageDay for left
and right into local vars, compare top-level scalar fields first (date/ids/etc.)
and return false on mismatch, then compare modelBreakdowns lengths and iterate
through entries comparing keys and values with early returns on any mismatch
(and compare nested primitives similarly) so the function exits as soon as a
difference is found; keep the function name areUsageDaysEquivalent and continue
to use canonicalizeUsageDay to normalize inputs.
In `@server/http-router.js`:
- Around line 102-134: The try/catch around the callback-based fs.readFile in
serveFile is ineffective for async errors; refactor serveFile to use
fs.promises.readFile with async/await (or remove the outer try/catch and handle
all errors inside the readFile callback) so ERR_INVALID_ARG_VALUE is caught
reliably. Specifically, make serveFile async, await
fs.promises.readFile(reqPath) in a try/catch, fall back to reading
path.join(staticRoot, 'index.html') on ENOENT, and call sendStaticFile or
writeStaticErrorResponse based on caught errors; this ensures all file-read
errors (including ERR_INVALID_ARG_VALUE) are handled in the same catch block.
- Around line 519-524: The current path traversal guard in the HTTP router
special-cases index.html but doesn't explicitly forbid serving the root
directory itself; update the condition (that uses filePath, resolvedStaticRoot,
path.sep, staticRoot, and path.resolve) to also reject filePath ===
resolvedStaticRoot (or otherwise ensure filePath is a regular file) before
calling serveFile, or add an explicit fs.stat check to confirm the path is not a
directory and return 403 if stat indicates a directory or if stat fails with
EISDIR, so directories cannot be served.
In `@src/components/features/request-quality/RequestQuality.tsx`:
- Around line 20-51: The switch in RequestQuality.tsx inside the qualityMetrics
= requestQualityData.qualityMetrics.map(...) lacks a default branch, which can
yield undefined for unknown item.id values and break rendering; add a default
case that returns a safe fallback (e.g., return the original item or return
{...item, label: t('common.notAvailable'), value: t('common.notAvailable')}) so
every map iteration always returns a defined object for any item.id.
In `@src/components/features/settings/settings-modal-helpers.ts`:
- Around line 95-117: The docstring for reorderSettingsSections is ambiguous
about drop semantics; update the comment above the function
reorderSettingsSections to state that the function removes the source item and
inserts it before the target section (i.e., "insert before the target section"
behavior when dragging forward), so maintainers know the function implements an
"insert before" drag-and-drop semantics rather than replacing or appending to
the target slot.
- Around line 11-19: The parseSettingsNumberInput function currently uses
parsed.toFixed(2) wrapped in Number which can mask floating-point precision
issues; replace that string-based rounding with numeric rounding (e.g., use
Math.round(parsed * 100) / 100) to avoid unnecessary string conversions and keep
a numeric result in parseSettingsNumberInput, and add a short comment above
parseSettingsNumberInput stating this is display/formatting rounding only and
that a decimal library (like decimal.js or Big.js) should be used if exact
financial precision is required.
In `@src/components/features/settings/SettingsModalSections.tsx`:
- Around line 756-768: The controlled number input currently uses
value={config.subscriptionPrice} and immediately parses/commits each keystroke
via parseSettingsNumberInput and viewModel.onProviderChange, causing input
jumps; change the input to use a local string state (e.g.,
subscriptionPriceLocal) initialized from config.subscriptionPrice (or use
defaultValue for an uncontrolled input), update that local string on every
onChange, and only call parseSettingsNumberInput and
viewModel.onProviderChange(provider, { subscriptionPrice: ... }) on blur
(onBlur) or Enter to commit the parsed/clamped number; keep the disabled prop
and className logic as-is and reference parseSettingsNumberInput,
viewModel.onProviderChange, config.subscriptionPrice and provider when applying
the change.
- Around line 346-378: The grip handle in SettingsModalSections's draggable row
can mislead keyboard users into expecting keyboard drag support; update the grip
icon element (the visual handle inside the draggable container in
SettingsModalSections) to be hidden from assistive tech by adding
aria-hidden="true", or alternatively add a visually-hidden label (e.g., a
sr-only span or aria-label on the handle) that states "Use the up and down
buttons to reorder items" so screen-reader users know to use the existing
up/down buttons tied to the viewModel reordering methods.
In `@src/components/layout/FilterBarDateRange.tsx`:
- Around line 72-73: The component calls localToday() on every render, so
memoize the computed today value and use that memoized value to initialize
focusedDate; specifically replace the direct call to localToday() with a
memoized value (e.g., useMemo(() => localToday(), [])) and keep the useState
initialization for focusedDate (value ?? today) and related setters
(setFocusedDate) unchanged so the component behavior remains the same.
In `@src/components/layout/FilterBarQuickControls.tsx`:
- Around line 34-40: Replace the locally defined presets array in
FilterBarQuickControls with the shared preset metadata exported by the
dashboard-preferences contract: remove the hardcoded presets variable and import
the central presets (and/or its DashboardDatePreset type) from the shared
contract, then use that imported array wherever presets is referenced so the
component consumes the single source-of-truth for keys/labels/order/visibility;
ensure type compatibility (replace the local satisfies Array<{ key:
DashboardDatePreset; label: string }> with the imported type) and update any
references in the component to the new imported identifier.
In `@src/hooks/use-dashboard-controller-derived-state.ts`:
- Around line 10-23: DashboardControllerDerivedState currently depends on hook
shapes via ReturnType<typeof useDashboardFilters> and ReturnType<typeof
useComputedMetrics>, which leaks internal hook types into the controller API;
replace those ReturnType references with explicit shared types exported from
your controller-types layer (e.g., DashboardFilters and ComputedMetrics or
similarly named interfaces) and import/use those types in place of
ReturnType<typeof useDashboardFilters> and ReturnType<typeof
useComputedMetrics>; ensure any other hook-derived fields (like forecastState if
needed) use explicit exported types instead of ReturnType to prevent internal
hook shape changes from changing the controller contract.
In `@src/lib/calculations.ts`:
- Around line 34-41: Both computeModelCosts and computeProviderMetrics call
summarizeUsageBreakdowns separately which duplicates work; update callers (e.g.,
use-computed-metrics.ts) to call summarizeUsageBreakdowns(data) once and
destructure the returned { modelCosts, providerMetrics } so both values are
reused, then remove or deprecate the thin wrappers
computeModelCosts/computeProviderMetrics (or keep them but no longer call them
from the hot path). Reference summarizeUsageBreakdowns, computeModelCosts,
computeProviderMetrics and the caller use-computed-metrics.ts 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b71093af-17db-4aaa-88b6-74399ddabfe6
📒 Files selected for processing (178)
.dependency-cruiser.cjs.gitignoreCHANGELOG.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/start-test-server.jsscripts/verify-package.jsserver.jsserver/app-runtime.jsserver/auto-import-runtime.jsserver/background-runtime.jsserver/cli.jsserver/data-runtime.jsserver/http-request-guards.jsserver/http-router.jsserver/http-utils.jsserver/process-utils.jsserver/remote-auth.jsserver/runtime-state.jsserver/security-headers.jsserver/server-lifecycle.jsserver/startup-runtime.jsshared/app-settings.d.tsshared/app-settings.jsshared/dashboard-preferences.d.tsshared/dashboard-preferences.jsshared/locales/de/common.jsonshared/locales/en/common.jsonsrc/components/Dashboard.tsxsrc/components/dashboard/DashboardMotion.tsxsrc/components/dashboard/DashboardSections.tsxsrc/components/dashboard/dashboard-section-preloading.tssrc/components/features/command-palette/CommandPalette.tsxsrc/components/features/drill-down/DrillDownModal.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/features/request-quality/RequestQuality.tsxsrc/components/features/settings/SettingsModal.tsxsrc/components/features/settings/SettingsModalSections.tsxsrc/components/features/settings/settings-modal-helpers.tssrc/components/features/settings/use-settings-modal-draft.tssrc/components/features/settings/use-settings-modal-version-status.tssrc/components/layout/FilterBar.tsxsrc/components/layout/FilterBarChipFilters.tsxsrc/components/layout/FilterBarDateRange.tsxsrc/components/layout/FilterBarQuickControls.tsxsrc/components/layout/FilterBarStatus.tsxsrc/components/layout/Header.tsxsrc/components/tables/ModelEfficiency.tsxsrc/components/tables/ProviderEfficiency.tsxsrc/components/tables/RecentDays.tsxsrc/hooks/use-computed-metrics.tssrc/hooks/use-dashboard-controller-actions.tssrc/hooks/use-dashboard-controller-browser.tssrc/hooks/use-dashboard-controller-derived-state.tssrc/hooks/use-dashboard-controller-dialogs.tssrc/hooks/use-dashboard-controller-drill-down.tssrc/hooks/use-dashboard-controller-effects.tssrc/hooks/use-dashboard-controller-shell-state.tssrc/hooks/use-dashboard-controller-types.tssrc/hooks/use-dashboard-controller.tssrc/hooks/use-dashboard-filters.tssrc/hooks/use-provider-limits.tssrc/hooks/use-theme.tssrc/lib/app-settings.tssrc/lib/calculations.tssrc/lib/dashboard-aggregation.tssrc/lib/dashboard-filter-data.tssrc/lib/dashboard-preferences.tssrc/lib/dashboard-view-model.d.tssrc/lib/data-transforms.tssrc/lib/drill-down-data.tssrc/lib/filter-date-picker-data.tssrc/lib/heatmap-calendar-data.tssrc/lib/model-utils.tssrc/lib/provider-limits.tssrc/lib/request-quality-data.tssrc/lib/sortable-table-data.tssrc/lib/toktrack-version-status.tstests/architecture/dashboard-sections-contract.test.tstests/architecture/frontend-layers.test.tstests/architecture/hook-naming.test.tstests/architecture/server-entrypoint-contract.test.tstests/architecture/server-http-boundaries.test.tstests/architecture/server-runtime-state-contract.test.tstests/architecture/shared-ui-placement.test.tstests/architecture/source-graph.test.tstests/architecture/source-graph.tstests/architecture/unused-hooks.test.tstests/auth-test-helpers.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/dashboard.spec.tstests/e2e/helpers.tstests/frontend/command-palette-action-groups.test.tsxtests/frontend/dashboard-controller-actions.test.tsxtests/frontend/dashboard-controller-browser.test.tsxtests/frontend/dashboard-controller-drill-down.test.tsxtests/frontend/dashboard-controller-state.test.tsxtests/frontend/dashboard-controller-test-helpers.tstests/frontend/dashboard-error-state.test.tsxtests/frontend/dashboard-filter-visibility.test.tsxtests/frontend/dashboard-motion.test.tsxtests/frontend/drill-down-modal-content.test.tsxtests/frontend/filter-bar-accessibility.test.tsxtests/frontend/filter-bar-date-picker.test.tsxtests/frontend/filter-bar-presets.test.tsxtests/frontend/header-links.test.tsxtests/frontend/heatmap-calendar-accessibility.test.tsxtests/frontend/request-quality.test.tsxtests/frontend/settings-modal-backups.test.tsxtests/frontend/settings-modal-defaults.test.tsxtests/frontend/settings-modal-draft-state.test.tsxtests/frontend/settings-modal-provider-limits.test.tsxtests/frontend/settings-modal-sections.test.tsxtests/frontend/settings-modal-tabs.test.tsxtests/frontend/settings-modal-test-helpers.tsxtests/frontend/settings-modal-version-status.test.tsxtests/frontend/sortable-table-provider-model.test.tsxtests/frontend/sortable-table-recent-days.test.tsxtests/frontend/use-computed-metrics.test.tsxtests/frontend/use-dashboard-filters.test.tsxtests/integration/server-api-guards.test.tstests/integration/server-api-imports.test.tstests/integration/server-api-persistence.test.tstests/integration/server-api-recovery.test.tstests/integration/server-api-routing-runtime.test.tstests/integration/server-auto-import.test.tstests/integration/server-background-concurrency.test.tstests/integration/server-background-registry.test.tstests/integration/server-background-selection.test.tstests/integration/server-background.test.tstests/integration/server-local-auth.test.tstests/integration/server-remote-auth.test.tstests/integration/server-startup-cli.test.tstests/integration/server-test-helpers.tstests/unit/app-settings-contract.test.tstests/unit/background-runtime.test.tstests/unit/code-rabbit-phase1.test.tstests/unit/dashboard-aggregation.test.tstests/unit/dashboard-filter-data.test.tstests/unit/dashboard-preferences.test.tstests/unit/dashboard-section-preloading.test.tstests/unit/drill-down-data.test.tstests/unit/filter-date-picker-data.test.tstests/unit/heatmap-calendar-data.test.tstests/unit/http-request-guards.test.tstests/unit/http-utils.test.tstests/unit/model-colors.test.tstests/unit/recent-days-reveal.test.tstests/unit/remote-auth.test.tstests/unit/request-quality-data.test.tstests/unit/runtime-state.test.tstests/unit/security-headers.test.tstests/unit/server-cli.test.tstests/unit/server-helpers-file-locks.test.tstests/unit/server-helpers.shared.tstests/unit/server-lifecycle.test.tstests/unit/settings-modal-helpers.test.tstests/unit/sortable-table-data.test.tstests/unit/startup-runtime.test.tstests/unit/test-fixture-secrets.test.tstests/unit/toktrack-version-status.test.tstests/unit/vitest-coverage-config.test.tsvitest.config.ts
💤 Files with no reviewable changes (2)
- src/hooks/use-theme.ts
- src/hooks/use-provider-limits.ts
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/features/settings/SettingsModal.tsx (1)
136-143:⚠️ Potential issue | 🟠 MajorPrevent the dialog from closing while a save or import is still running.
Backdrop clicks and Escape can still call
onOpenChangewhilesettingsBusyordataBusyis true. If that pending operation then fails, the catch path insrc/components/features/settings/use-settings-modal-draft.ts:233-235can only toast, and the user has already dismissed the draft they were editing.Suggested guard
<DialogContent className="max-h-[88vh] max-w-5xl overflow-x-visible overflow-y-auto" onOpenAutoFocus={(event) => { event.preventDefault() titleRef.current?.focus() }} + onEscapeKeyDown={(event) => { + if (settingsBusy || dataBusy) { + event.preventDefault() + } + }} + onInteractOutside={(event) => { + if (settingsBusy || dataBusy) { + event.preventDefault() + } + }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/settings/SettingsModal.tsx` around lines 136 - 143, The dialog can be dismissed via backdrop/Escape while async operations (settingsBusy or dataBusy) run; update the Dialog's onOpenChange handler in SettingsModal.tsx to guard against closing when either settingsBusy or dataBusy is true: replace the inline onOpenChange={onOpenChange} with a local handler (e.g., handleDialogOpenChange) that checks if the requested open value is false and (settingsBusy || dataBusy) is true, then prevents the close (do not call onOpenChange(false)) and optionally surface a toast; otherwise call onOpenChange as normal. This ensures backdrop clicks/Escape won't dismiss the dialog while use-settings-modal-draft.ts's pending operations are running.
♻️ Duplicate comments (1)
shared/dashboard-preferences.js (1)
27-64:⚠️ Potential issue | 🟠 MajorStill reject duplicate
domIdvalues insectionDefinitions.The new validation now guarantees each section
idappears exactly once, but two entries can still share the samedomId. That leaves the shared metadata internally inconsistent and can send browser navigation to the wrong element oncesrc/hooks/use-dashboard-controller-browser.tscallsdocument.getElementById(...).🛡️ Suggested hardening
function validateSectionDefinitions(value, validSectionIds) { if (!Array.isArray(value)) { throw new Error('Invalid dashboard preferences: "sectionDefinitions" must be an array.') } const seenIds = new Set() + const seenDomIds = new Set() const sectionDefinitions = value.map((entry) => { if (!isPlainObject(entry)) { throw new Error( 'Invalid dashboard preferences: each "sectionDefinitions" entry must be an object.', @@ if (typeof domId !== 'string' || !domId.trim()) { throw new Error('Invalid dashboard preferences: sectionDefinitions require a domId.') } + if (seenDomIds.has(domId)) { + throw new Error(`Invalid dashboard preferences: duplicate section domId "${domId}".`) + } + seenDomIds.add(domId) if (typeof labelKey !== 'string' || !labelKey.trim()) { throw new Error('Invalid dashboard preferences: sectionDefinitions require a labelKey.') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/dashboard-preferences.js` around lines 27 - 64, The validation currently ensures unique section ids but not unique domId values, so update the sectionDefinitions validation loop to also reject duplicate domId values: create a seenDomIds Set alongside seenIds, check each entry's domId (after existing typeof and trim check) against seenDomIds and throw an Error if already present, add domId to seenDomIds, and optionally assert at the end that seenDomIds.size === validSectionIds.length; reference the symbols sectionDefinitions, domId, seenIds, validSectionIds (and the consumer use-dashboard-controller-browser.ts) so the duplicate DOM id case is caught and fails early.
🧹 Nitpick comments (3)
src/components/features/drill-down/DrillDownModal.tsx (1)
214-271: Consider memoizing per-card deltas to avoid repeatedgetDelta(...)calls.Several benchmark cards compute the same delta twice (primary + secondary). Caching each delta once per card would reduce repeated work and keep card fields tightly coupled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/drill-down/DrillDownModal.tsx` around lines 214 - 271, The benchmarkCards array repeatedly calls getDelta(...) for the same pair of values (used in both primary and secondary) — compute each card's delta once and reuse it (e.g., const delta = getDelta(...)) when building each card so primary = formatDeltaValue(delta, ...) and secondary = formatDeltaPercent(delta) or the averaged fallback; wrap the whole benchmarkCards construction in useMemo (or otherwise memoize) to avoid recomputing across renders. Update code around the benchmarkCards declaration and usages of getDelta, formatDeltaValue, formatDeltaPercent, and consider memoizing costPerMillion/previousCostPerMillion deltas as well (referencing symbols: benchmarkCards, getDelta, formatDeltaValue, formatDeltaPercent, useMemo).scripts/verify-package.js (1)
103-143: The smoke test is brittle and couples to runtime log formatting.
getLocalAuthHeaderFromOutput()parses the hardcoded log message "Local Auth URL:" to extract the bootstrap token. This creates fragility—if the server changes how it logs the bootstrap URL, the smoke test will break. Consider exposing the bootstrap URL or token through a test-accessible mechanism (e.g., environment variable, status file, or shared constant) rather than parsing log output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-package.js` around lines 103 - 143, The test is brittle because getLocalAuthHeaderFromOutput() depends on parsing the "Local Auth URL:" log line; instead change the handshake to read a test-accessible source: have the server export the bootstrap URL/token to a known environment variable or writable status file, update getLocalAuthHeaderFromOutput() (and waitForServer()) to first check that environment variable (e.g., process.env.TEST_BOOTSTRAP_URL) or read the status file for the bootstrap URL/token and only fall back to log parsing if those are absent; also update the server bootstrap to set that env var or write the status file so waitForServer()/getLocalAuthHeaderFromOutput() can reliably obtain the token without relying on log formatting.src/hooks/use-dashboard-controller.ts (1)
86-343: Add focused controller-composition tests for critical branches.This orchestration now fans out through many slices (
effects,derived,actions,dialogs,drillDown,shellState) and is a high-regression surface; a few focused tests on composed outputs/callback wiring would materially de-risk future changes.Based on learnings: Focus coverage improvements on critical branch-heavy runtime modules like
src/lib/api.ts,src/hooks/use-usage-data.ts, andsrc/hooks/use-dashboard-controller.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/use-dashboard-controller.ts` around lines 86 - 343, Add focused controller-composition unit tests for use-dashboard-controller that exercise the main orchestration and its critical branches: mount the hook (or call the exported controller factory) stubbing/mocking the collaborators useDashboardControllerEffects, useDashboardControllerDerivedState, useDashboardControllerActions, useDashboardControllerDrillDown and useDashboardControllerShellState to return controlled outputs and simulate branch conditions (hasData true/false, startDate/endDate present, errors like bootstrapSettingsError/settingsError/usageError). Assert composed outputs and callback wiring such as header.onExportCSV (calls downloadCSV and addToast via handleExportCSV), header.onDelete / commandPalette.onDelete (calls actions.onDelete), dialogs/autoImport drillDown.dialog presence, filterBar date/provider/model handlers (setStartDate/setEndDate/toggleProvider/toggleModel), settingsModal handlers (onSaveSettings/onExportSettings/onImportSettings), and report/command palette actions (report.onGenerate, onClearDateRange); include tests for both normal and error/empty branches to cover fan-out logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 38: Update the "test:e2e:ci" npm script to avoid hard-coding the CI
environment variable; either remove the "CI=1 " prefix so the script becomes
"playwright test" and rely on the CI system to set process.env.CI, or if you
must force CI locally replace the prefix with a cross-platform wrapper (e.g.,
use "cross-env CI=1 playwright test") and add "cross-env" to devDependencies;
edit the "test:e2e:ci" entry in package.json accordingly and run npm install
--save-dev cross-env if choosing the wrapper.
In `@server/data-runtime.js`:
- Around line 389-409: sortStrings and canonicalizeModelBreakdown are not
normalizing model names, so strings that differ only by surrounding whitespace
are treated as different; update sortStrings to trim each string before
filtering/uniquing/sorting (i.e., map values to value.trim() then filter
non-empty and use a Set on the trimmed values) and update
canonicalizeModelBreakdown to assign modelName as the trimmed string when
entry?.modelName is a string (e.g., modelName: typeof entry?.modelName ===
'string' ? entry.modelName.trim() : ''), ensuring comparisons and deduplication
use normalized names.
- Around line 221-233: The current logic in shouldReapFileMutationLock treats a
lock as stale based solely on lockAgeMs before checking owner.pid; reorder the
checks so you first handle the owner PID: if Number.isInteger(owner?.pid) then
return !isProcessRunning(owner.pid) (i.e. do not reap if the process is still
running), and only if there is no live PID proceed to compute
ownerCreatedAt/lockAgeMs and compare against fileMutationLockStaleMs to decide
reaping; update the block referencing ownerCreatedAt, lockAgeMs,
fileMutationLockStaleMs, owner.pid, and isProcessRunning accordingly.
In `@server/http-router.js`:
- Around line 123-131: The SPA fallback currently serves index.html for any
ENOENT, including missing assets; change the ENOENT branch in the error handling
around readStaticFile/sendStaticFile to only serve index.html for route-like
requests by detecting requests that expect HTML or lack a file extension:
inspect the request (e.g., req.headers.accept contains "text/html" or
path.extname(req.url) is empty or "/" ) and only then call
readStaticFile(indexPath) and sendStaticFile; otherwise return a 404 using
writeStaticErrorResponse(res, 404, 'Not Found') so missing assets still produce
proper 404s.
In `@server/startup-runtime.js`:
- Around line 108-126: The auth/URL messaging currently branches on
remoteBind/bindHost and must instead consult the serverAuth state; update the
sections that print "Local Auth: required", emit the Local Auth URL
(bootstrapUrl) and the bearer-token curl example to use
serverAuth.isLocalRequired() (or serverAuth.mode === 'none') as the determinant
(in combination with remoteBind and shouldOpenBrowser()), e.g. replace checks
that only use remoteBind/bindHost with conditions like !remoteBind &&
serverAuth.isLocalRequired() && !shouldOpenBrowser(), and similarly gate the
curl example using serverAuth.isLocalRequired() so messages reflect the actual
serverAuth.mode.
- Around line 36-40: The CI check currently only treats '1' as non-interactive;
update the condition around cliOptions.noOpen /
processObject.env.NO_OPEN_BROWSER / processObject.env.CI to treat any truthy CI
value as non-interactive by replacing the strict equality check
(processObject.env.CI === '1') with a truthy test (e.g.,
Boolean(processObject.env.CI) or processObject.env.CI && processObject.env.CI
!== '0') so that variables like CI='true' are handled; adjust the condition in
startup-runtime.js where cliOptions.noOpen, processObject.env.NO_OPEN_BROWSER
and processObject.env.CI are evaluated.
In `@src/components/features/settings/SettingsModalSections.tsx`:
- Around line 118-129: The language/motion controls and any remaining mutating
UI in SettingsModalSections must be disabled while settingsBusy is true to
prevent draft changes being lost; specifically, thread the settingsBusy
prop/state into the language and motion card controls and guard handlers like
viewModel.onLanguageChange, drag handlers, input onChange/onBlur, and the Button
variants/aria-pressed toggles so they no-op or set disabled when settingsBusy is
true (reset buttons already honor it); mirror the same disabling logic used by
the existing reset buttons throughout this file (including the other sections
you noted) so handleSave (use-settings-modal-draft.ts lines around 218-236)
cannot be interrupted by further local-draft mutations.
- Around line 756-766: The monthlyLimit input currently parses on every
keystroke (using parseSettingsNumberInput) which breaks partial decimal entry;
introduce a local string draft (e.g., monthlyLimitDraft) in the component like
subscriptionPrice does, initialize it from config.monthlyLimit, bind the input
value to monthlyLimitDraft, update the draft onChange without parsing, and only
call viewModel.onProviderChange(provider, { monthlyLimit: parsedNumber }) on
commit (onBlur or explicit save) using parseSettingsNumberInput; also allow the
draft to be empty or end with a dot while editing so users can type decimals
naturally.
In `@src/components/layout/FilterBarDateRange.tsx`:
- Around line 183-186: The computed top position for the calendar overlay
(derived from showAbove, rect, estimatedHeight, viewportHeight) can become
negative when the viewport is shorter than estimatedHeight; clamp the calculated
top value to a safe range before calling setOverlayStyle: ensure top is at least
a small padding (e.g., 12) and at most (viewportHeight - estimatedHeight - 12)
so the overlay never renders off-screen, then call setOverlayStyle({ top, left,
width }) with the clamped top.
In `@src/hooks/use-dashboard-controller-actions.ts`:
- Around line 211-216: The file input reset is currently skipped on upload
failure because the code returns inside the catch; move the input-reset logic
into a finally block so it always runs after attempting upload. Specifically,
for the try/catch around uploadUsageData(parsed) (and the analogous block at the
other occurrence), remove the early return in catch or keep the toast call but
ensure the reset of the input element (the same reset call currently after the
try/catch) is executed from a finally block so selecting the same file again
will trigger change whether upload succeeds or fails.
---
Outside diff comments:
In `@src/components/features/settings/SettingsModal.tsx`:
- Around line 136-143: The dialog can be dismissed via backdrop/Escape while
async operations (settingsBusy or dataBusy) run; update the Dialog's
onOpenChange handler in SettingsModal.tsx to guard against closing when either
settingsBusy or dataBusy is true: replace the inline onOpenChange={onOpenChange}
with a local handler (e.g., handleDialogOpenChange) that checks if the requested
open value is false and (settingsBusy || dataBusy) is true, then prevents the
close (do not call onOpenChange(false)) and optionally surface a toast;
otherwise call onOpenChange as normal. This ensures backdrop clicks/Escape won't
dismiss the dialog while use-settings-modal-draft.ts's pending operations are
running.
---
Duplicate comments:
In `@shared/dashboard-preferences.js`:
- Around line 27-64: The validation currently ensures unique section ids but not
unique domId values, so update the sectionDefinitions validation loop to also
reject duplicate domId values: create a seenDomIds Set alongside seenIds, check
each entry's domId (after existing typeof and trim check) against seenDomIds and
throw an Error if already present, add domId to seenDomIds, and optionally
assert at the end that seenDomIds.size === validSectionIds.length; reference the
symbols sectionDefinitions, domId, seenIds, validSectionIds (and the consumer
use-dashboard-controller-browser.ts) so the duplicate DOM id case is caught and
fails early.
---
Nitpick comments:
In `@scripts/verify-package.js`:
- Around line 103-143: The test is brittle because
getLocalAuthHeaderFromOutput() depends on parsing the "Local Auth URL:" log
line; instead change the handshake to read a test-accessible source: have the
server export the bootstrap URL/token to a known environment variable or
writable status file, update getLocalAuthHeaderFromOutput() (and
waitForServer()) to first check that environment variable (e.g.,
process.env.TEST_BOOTSTRAP_URL) or read the status file for the bootstrap
URL/token and only fall back to log parsing if those are absent; also update the
server bootstrap to set that env var or write the status file so
waitForServer()/getLocalAuthHeaderFromOutput() can reliably obtain the token
without relying on log formatting.
In `@src/components/features/drill-down/DrillDownModal.tsx`:
- Around line 214-271: The benchmarkCards array repeatedly calls getDelta(...)
for the same pair of values (used in both primary and secondary) — compute each
card's delta once and reuse it (e.g., const delta = getDelta(...)) when building
each card so primary = formatDeltaValue(delta, ...) and secondary =
formatDeltaPercent(delta) or the averaged fallback; wrap the whole
benchmarkCards construction in useMemo (or otherwise memoize) to avoid
recomputing across renders. Update code around the benchmarkCards declaration
and usages of getDelta, formatDeltaValue, formatDeltaPercent, and consider
memoizing costPerMillion/previousCostPerMillion deltas as well (referencing
symbols: benchmarkCards, getDelta, formatDeltaValue, formatDeltaPercent,
useMemo).
In `@src/hooks/use-dashboard-controller.ts`:
- Around line 86-343: Add focused controller-composition unit tests for
use-dashboard-controller that exercise the main orchestration and its critical
branches: mount the hook (or call the exported controller factory)
stubbing/mocking the collaborators useDashboardControllerEffects,
useDashboardControllerDerivedState, useDashboardControllerActions,
useDashboardControllerDrillDown and useDashboardControllerShellState to return
controlled outputs and simulate branch conditions (hasData true/false,
startDate/endDate present, errors like
bootstrapSettingsError/settingsError/usageError). Assert composed outputs and
callback wiring such as header.onExportCSV (calls downloadCSV and addToast via
handleExportCSV), header.onDelete / commandPalette.onDelete (calls
actions.onDelete), dialogs/autoImport drillDown.dialog presence, filterBar
date/provider/model handlers
(setStartDate/setEndDate/toggleProvider/toggleModel), settingsModal handlers
(onSaveSettings/onExportSettings/onImportSettings), and report/command palette
actions (report.onGenerate, onClearDateRange); include tests for both normal and
error/empty branches to cover fan-out logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: daaef9a1-7998-4835-99d9-378400c8cda0
📒 Files selected for processing (73)
.dependency-cruiser.cjsdocs/architecture.mddocs/review/dashboard-review.mddocs/review/fixed-findings.mdpackage.jsonplaywright.config.tsscripts/verify-package.jsserver/cli.jsserver/data-runtime.jsserver/http-router.jsserver/process-utils.jsserver/security-headers.jsserver/startup-runtime.jsshared/dashboard-preferences.d.tsshared/dashboard-preferences.jssrc/components/charts/CostByModelOverTime.tsxsrc/components/charts/CumulativeCost.tsxsrc/components/charts/CumulativeCostPerProvider.tsxsrc/components/dashboard/DashboardSections.tsxsrc/components/features/command-palette/CommandPalette.tsxsrc/components/features/drill-down/DrillDownModal.tsxsrc/components/features/forecast/CostForecast.tsxsrc/components/features/forecast/ForecastZoomDialog.tsxsrc/components/features/forecast/ProviderCostForecast.tsxsrc/components/features/request-quality/RequestQuality.tsxsrc/components/features/settings/SettingsModal.tsxsrc/components/features/settings/SettingsModalSections.tsxsrc/components/features/settings/settings-modal-helpers.tssrc/components/features/settings/use-settings-modal-draft.tssrc/components/layout/FilterBar.tsxsrc/components/layout/FilterBarDateRange.tsxsrc/components/layout/FilterBarQuickControls.tsxsrc/components/layout/Header.tsxsrc/components/tables/RecentDays.tsxsrc/hooks/use-dashboard-controller-actions.tssrc/hooks/use-dashboard-controller-browser.tssrc/hooks/use-dashboard-controller-derived-state.tssrc/hooks/use-dashboard-controller-drill-down.tssrc/hooks/use-dashboard-controller-effects.tssrc/hooks/use-dashboard-controller-shell-state.tssrc/hooks/use-dashboard-controller.tssrc/lib/calculations.tssrc/lib/dashboard-preferences.tssrc/lib/data-transforms.tssrc/types/dashboard-controller.d.tssrc/types/dashboard-view-model.d.tssrc/types/index.tstests/architecture/dashboard-sections-contract.test.tstests/frontend/dashboard-controller-browser.test.tsxtests/frontend/dashboard-controller-drill-down.test.tsxtests/frontend/dashboard-controller-effects.test.tsxtests/frontend/dashboard-controller-state.test.tsxtests/frontend/dashboard-controller-test-helpers.tstests/frontend/drill-down-modal-content.test.tsxtests/frontend/filter-bar-date-picker.test.tsxtests/frontend/filter-bar-presets.test.tsxtests/frontend/request-quality.test.tsxtests/frontend/settings-modal-draft-state.test.tsxtests/frontend/settings-modal-provider-limits.test.tsxtests/frontend/settings-modal-sections.test.tsxtests/frontend/settings-modal-tabs.test.tsxtests/frontend/settings-modal-test-helpers.tsxtests/frontend/settings-modal-version-status.test.tsxtests/frontend/sortable-table-recent-days.test.tsxtests/integration/server-api-imports.test.tstests/unit/dashboard-preferences.test.tstests/unit/http-router-static.test.tstests/unit/playwright-config.test.tstests/unit/process-utils.test.tstests/unit/security-headers.test.tstests/unit/server-cli.test.tstests/unit/settings-modal-helpers.test.tstests/unit/startup-runtime.test.ts
✅ Files skipped from review due to trivial changes (8)
- src/components/charts/CostByModelOverTime.tsx
- src/components/features/forecast/ForecastZoomDialog.tsx
- src/components/features/forecast/ProviderCostForecast.tsx
- src/components/features/forecast/CostForecast.tsx
- server/process-utils.js
- src/hooks/use-dashboard-controller-drill-down.ts
- shared/dashboard-preferences.d.ts
- server/security-headers.js
🚧 Files skipped from review as they are similar to previous changes (6)
- playwright.config.ts
- src/components/layout/FilterBarQuickControls.tsx
- src/hooks/use-dashboard-controller-effects.ts
- src/hooks/use-dashboard-controller-shell-state.ts
- src/components/layout/Header.tsx
- src/lib/calculations.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
shared/dashboard-preferences.js (1)
79-81: Redundant ternary: both branches are identical.The
instanceof Datecheck currently has no effect since both branches executenew Date(referenceDate). This can be simplified.♻️ Suggested simplification
function getReferenceDate(referenceDate = new Date()) { - const candidate = - referenceDate instanceof Date ? new Date(referenceDate) : new Date(referenceDate) + const candidate = new Date(referenceDate) if (!Number.isFinite(candidate.getTime())) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/dashboard-preferences.js` around lines 79 - 81, The ternary in getReferenceDate is redundant because both branches call new Date(referenceDate); simplify by replacing the conditional with a single assignment to candidate (e.g., const candidate = new Date(referenceDate)) and keep the rest of the function logic intact; update any uses of the candidate variable in getReferenceDate accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/data-runtime.js`:
- Around line 105-115: writeJsonAtomic currently creates tempPath and
writes/chmods/renames without cleaning up on thrown errors; modify
writeJsonAtomic (referencing function writeJsonAtomic and tempPath) to wrap the
write/chmod/rename sequence in a try/catch/finally, and on any error ensure the
temp file (tempPath) is removed if it exists (using fs.existsSync/unlinkSync or
their async equivalents) before rethrowing the original error so no
token-bearing *.tmp files are left behind; keep the existing behavior of
rethrowing the error after cleanup.
- Around line 693-703: The migration branch that moves legacyDataFile to
dataFile (the try { fs.renameSync(legacyDataFile, dataFile) ... } catch {
fs.copyFileSync(...) ... } block) doesn't reapply the secure file mode, so a
permissive legacy file can remain world-readable; after either fs.renameSync or
after the copy+unlink fallback, call the same permission-normalizing step used
elsewhere (e.g., fs.chmodSync on dataFile with the secure mode constant or
function used in this module) to enforce the intended restrictive permissions on
dataFile; ensure you apply it in both the rename success path and the copy
fallback path so both flows end up with correct modes.
In `@server/http-router.js`:
- Around line 272-280: The current PATCH /settings handler lumps request parsing
and persistence together so filesystem/lock/write errors (e.g., ENOSPC, EACCES,
lock failures thrown by updateSettings) surface as 400; refactor the handler by
first try/catch only the request parsing/validation (await readBody(req)) and
return 413/400 as now, then call updateSettings(body) in a separate try/catch
that maps unexpected server-side errors to a 500 json(res, 500, { message:
'Server error' }) while still preserving specific client errors if any; use the
helpers isPayloadTooLargeError, json, readBody, and updateSettings to locate the
code and apply the same two-stage pattern to the other mutation handlers
mentioned (the settings import, upload, and usage import handlers at the other
ranges) so write/lock failures return 500 instead of 400.
---
Nitpick comments:
In `@shared/dashboard-preferences.js`:
- Around line 79-81: The ternary in getReferenceDate is redundant because both
branches call new Date(referenceDate); simplify by replacing the conditional
with a single assignment to candidate (e.g., const candidate = new
Date(referenceDate)) and keep the rest of the function logic intact; update any
uses of the candidate variable in getReferenceDate accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29251dc1-da7c-4dfd-9c8a-f1e0f5ca2c4f
📒 Files selected for processing (27)
package.jsonscripts/verify-package.jsserver/data-runtime.jsserver/http-router.jsserver/remote-auth.jsserver/startup-runtime.jsshared/dashboard-preferences.jssrc/components/features/drill-down/DrillDownModal.tsxsrc/components/features/settings/SettingsModal.tsxsrc/components/features/settings/SettingsModalSections.tsxsrc/components/layout/FilterBarDateRange.tsxsrc/hooks/use-dashboard-controller-actions.tssrc/hooks/use-dashboard-controller-drill-down.tstests/e2e/dashboard-settings-backups.spec.tstests/frontend/dashboard-controller-actions.test.tsxtests/frontend/filter-bar-date-picker.test.tsxtests/frontend/settings-modal-provider-limits.test.tsxtests/frontend/settings-modal-sections.test.tsxtests/frontend/settings-modal-tabs.test.tsxtests/integration/server-api-imports.test.tstests/integration/server-remote-auth.test.tstests/unit/dashboard-preferences.test.tstests/unit/http-router-static.test.tstests/unit/playwright-config.test.tstests/unit/remote-auth.test.tstests/unit/server-helpers-file-locks.test.tstests/unit/startup-runtime.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/layout/FilterBarDateRange.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/verify-package.js
- src/components/features/settings/SettingsModalSections.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/http-router.js (1)
240-252:⚠️ Potential issue | 🟠 MajorCatch persistence failures on the DELETE handlers too.
DELETE /usageandDELETE /settingsstill await lock/unlink/write work without a server-error mapping.EACCES/ENOSPC, lock timeouts, or persisted-state failures will escape this handler, while the other mutation routes now return JSON 500s.🛠️ Suggested pattern
if (req.method === 'DELETE') { const validationError = validateMutationRequest(req); if (validationError) { return json(res, validationError.status, { message: validationError.message }); } - await withSettingsAndDataMutationLock(async () => { - await unlinkIfExists(dataFile); - await updateDataLoadState({ - lastLoadedAt: null, - lastLoadSource: null, - }); - }); + try { + await withSettingsAndDataMutationLock(async () => { + await unlinkIfExists(dataFile); + await updateDataLoadState({ + lastLoadedAt: null, + lastLoadSource: null, + }); + }); + } catch (error) { + if (isPersistedStateError(error, 'usage') || isPersistedStateError(error, 'settings')) { + return json(res, 500, { message: error.message }); + } + return writeMutationServerError(res); + } return json(res, 200, { success: true }); }Apply the same shape to
DELETE /settings.Also applies to: 277-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/http-router.js` around lines 240 - 252, The DELETE handlers (e.g., the block that calls validateMutationRequest, withSettingsAndDataMutationLock, unlinkIfExists, updateDataLoadState and then returns json(res,200,...)) lack error handling for persistence/lock failures; wrap the async mutation work inside a try/catch, catch any thrown errors (including EACCES/ENOSPC or lock timeouts) and return a JSON 500 response via json(res, 500, { message: err.message || 'Internal Server Error' }); apply the same pattern to the DELETE /settings handler so both deletion routes mirror the other mutation routes' server-error mapping.
🧹 Nitpick comments (2)
server/data-runtime.js (2)
132-145: Normalize parent directory mode in the async write path.
writeJsonAtomicAsync()only passesmodetomkdir(). Ifpath.dirname(filePath)already exists with broader permissions, that call leaves it unchanged, unlike the sync path'sensureDir(). Since this helper writessettings.json/data.json, I'd keep the async path aligned and reapplysecureDirModebefore the write.🛠️ Suggested adjustment
try { await fsPromises.mkdir(path.dirname(filePath), { recursive: true, mode: secureDirMode }); + if (!isWindows) { + await fsPromises.chmod(path.dirname(filePath), secureDirMode); + } tempPathCreated = true; await fsPromises.writeFile(tempPath, JSON.stringify(data, null, 2), { mode: secureFileMode, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/data-runtime.js` around lines 132 - 145, writeJsonAtomicAsync currently only passes mode to fsPromises.mkdir, which won’t change permissions of an existing parent directory; mirror the sync path by normalizing the parent dir permissions before writing: after ensuring the directory (path.dirname(filePath)) exists, call fsPromises.chmod on that directory to set secureDirMode (handle non-Windows as appropriate) so the parent folder has the intended restrictive permissions before creating tempPath and writing JSON.
677-686: MakeupdateDataLoadStateexplicitly locked or explicitly internal.Unlike
updateSettings(), this exported helper does an unlocked read-modify-write onsettings.json. That makes it easy for a future caller to race a concurrent settings update and clobber unrelated fields. I'd either take the lock in the public method or split this into locked/unlocked variants so the API shape makes the requirement obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/data-runtime.js` around lines 677 - 686, The current exported updateDataLoadState performs an unlocked read-modify-write (it calls readSettingsForWrite() and writeSettings()), which risks races; either take the settings lock inside this function or make the unlocked helper internal and expose a locked wrapper. Fix by renaming the current function to something like _updateDataLoadStateUnlocked (internal) and implement a new exported updateDataLoadState that acquires the same lock used by updateSettings (or delegates to updateSettings) before reading/writing, or alternatively add lock acquisition/release around the readSettingsForWrite()/writeSettings() calls so callers cannot accidentally clobber concurrent updates; reference updateDataLoadState, _updateDataLoadStateUnlocked (or the lock-acquire helper), readSettingsForWrite, writeSettings, and updateSettings when making the 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 `@server/http-router.js`:
- Around line 527-533: The toktrack version branch awaits
lookupLatestToktrackVersion() without handling failures; wrap the await in a
try/catch inside the apiPath === '/toktrack/version-status' handler and on error
return a JSON 5xx (e.g., 503 Service Unavailable) using the existing json(res,
...) helper, include a concise error message (and optionally error.message or
code) and log the error; update the code around lookupLatestToktrackVersion() to
catch network/npm errors and respond with json(res, 503, { message: 'Service
Unavailable', detail: <error info> }) instead of letting the exception bubble.
---
Duplicate comments:
In `@server/http-router.js`:
- Around line 240-252: The DELETE handlers (e.g., the block that calls
validateMutationRequest, withSettingsAndDataMutationLock, unlinkIfExists,
updateDataLoadState and then returns json(res,200,...)) lack error handling for
persistence/lock failures; wrap the async mutation work inside a try/catch,
catch any thrown errors (including EACCES/ENOSPC or lock timeouts) and return a
JSON 500 response via json(res, 500, { message: err.message || 'Internal Server
Error' }); apply the same pattern to the DELETE /settings handler so both
deletion routes mirror the other mutation routes' server-error mapping.
---
Nitpick comments:
In `@server/data-runtime.js`:
- Around line 132-145: writeJsonAtomicAsync currently only passes mode to
fsPromises.mkdir, which won’t change permissions of an existing parent
directory; mirror the sync path by normalizing the parent dir permissions before
writing: after ensuring the directory (path.dirname(filePath)) exists, call
fsPromises.chmod on that directory to set secureDirMode (handle non-Windows as
appropriate) so the parent folder has the intended restrictive permissions
before creating tempPath and writing JSON.
- Around line 677-686: The current exported updateDataLoadState performs an
unlocked read-modify-write (it calls readSettingsForWrite() and
writeSettings()), which risks races; either take the settings lock inside this
function or make the unlocked helper internal and expose a locked wrapper. Fix
by renaming the current function to something like _updateDataLoadStateUnlocked
(internal) and implement a new exported updateDataLoadState that acquires the
same lock used by updateSettings (or delegates to updateSettings) before
reading/writing, or alternatively add lock acquisition/release around the
readSettingsForWrite()/writeSettings() calls so callers cannot accidentally
clobber concurrent updates; reference updateDataLoadState,
_updateDataLoadStateUnlocked (or the lock-acquire helper), readSettingsForWrite,
writeSettings, and updateSettings when making the 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3268bdc5-bc9a-46db-96f1-a00631e7c5dd
📒 Files selected for processing (5)
server/data-runtime.jsserver/http-router.jsshared/dashboard-preferences.jstests/unit/http-router-mutations.test.tstests/unit/server-helpers-file-locks.test.ts
✅ Files skipped from review due to trivial changes (1)
- shared/dashboard-preferences.js
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/app-runtime.js`:
- Around line 115-144: The createBackgroundRuntime call is missing the
remoteAuthHeader prop so background code that expects remoteAuthHeader (used in
server/background-runtime.js at places like the remote-bind fallback) gets
undefined; update the composition where createBackgroundRuntime(...) is invoked
to pass remoteAuthHeader using the same value as authHeader (call
serverAuth.getAuthorizationHeader() for both authHeader and remoteAuthHeader) so
both authHeader and remoteAuthHeader are provided to the background runtime.
In `@server/data-runtime.js`:
- Around line 709-729: The migrateLegacyDataFile function currently races when
another process migrates legacyDataFile between the initial existsSync check and
renameSync/copyFileSync; update the try/catch logic around fs.renameSync and the
fallback fs.copyFileSync so that when an error is thrown (specifically handle
ENOENT or "no such file"), you check fs.existsSync(dataFile) and, if dataFile
now exists, treat the migration as already done and return normally (only apply
applySecureFileMode and log if dataFile exists); otherwise rethrow or continue
with the existing fallback behavior—refer to migrateLegacyDataFile,
legacyDataFile, dataFile, fs.renameSync and fs.copyFileSync to locate where to
add the ENOENT-aware existence check and short-circuit.
In `@server/http-router.js`:
- Around line 355-359: The current code releases the settingsFile lock before
calling readSettings(), so another writer can mutate settings and cause this
endpoint to return a different state; keep the post-import read inside the same
withFileMutationLock callback by calling readSettings() (or capturing the
written settings) before the lock callback completes and return that value via
json(res, 200, ...), ensuring writeSettings(importedSettings) and the subsequent
readSettings() occur atomically within the withFileMutationLock for
settingsFile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56976ea8-d244-4b53-8960-503c4e3d5654
📒 Files selected for processing (6)
server/app-runtime.jsserver/data-runtime.jsserver/http-router.jstests/unit/http-router-mutations.test.tstests/unit/server-helpers-file-locks.test.tstests/unit/server-helpers.shared.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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
Documentation