Conversation
📝 WalkthroughWalkthroughConsolidates dashboard domain into a shared module, centralizes frontend dashboard controller, hardens server networking/HTTP/filesystem (bind-host opt-in, port probing, payload limits, secure perms), replaces SSE auto-import with fetch/stream parsing (POST), adds CSV helpers, expands i18n/accessibility, includes tests, docs, and a Windows smoke CI job. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Server startup
participant Runtime as server/runtime.js
participant OS as Network/OS
CLI->>Runtime: ensureBindHostAllowed(bindHost, ALLOW_REMOTE)
alt non-loopback & ALLOW_REMOTE not set
Runtime-->>CLI: throw REMOTE_BIND_REQUIRES_OPT_IN
CLI->>CLI: exit with guidance
else allowed
Runtime-->>CLI: allowed
end
CLI->>Runtime: listenOnAvailablePort(server, port, maxPort, bindHost)
loop try ports
Runtime->>OS: server.listen(currentPort, bindHost)
OS-->>Runtime: listening / EADDRINUSE / error
alt EADDRINUSE
Runtime->>Runtime: log and try next port
else other error
Runtime-->>CLI: throw error
end
end
Runtime-->>CLI: bound port returned
sequenceDiagram
participant Client as Browser/Tool
participant Server as server.js
participant Utils as server/http-utils.js
participant Reader as readBody(req)
Client->>Server: POST /api/some-mutation
Server->>Utils: resolveApiPath(pathname)
Server->>Utils: validateMutationRequest(req, { requiresJsonContentType })
alt validation fails
Utils-->>Server: { status: 403|415, message }
Server-->>Client: JSON error
else
Server->>Reader: readBody(req)
Reader->>Reader: enforce maxBodySize -> may reject PAYLOAD_TOO_LARGE
Reader-->>Server: parsed JSON
Server->>Server: handler executes -> respond
end
sequenceDiagram
participant UI as Browser UI
participant AutoImport as src/lib/auto-import.ts
participant Fetch as fetch(POST /api/auto-import/stream)
participant Server as server auto-import endpoint
UI->>AutoImport: startAutoImport()
AutoImport->>Fetch: POST /api/auto-import/stream
Fetch->>Server: request
Server->>Fetch: stream text/event-stream (event:/data:)
loop on chunks
Fetch->>AutoImport: chunk
AutoImport->>AutoImport: parse event frames (event:data)
alt key = progress
AutoImport-->>UI: onProgress({ key, vars, message })
else key = stderr
AutoImport-->>UI: onStderr(...)
else key = success
AutoImport-->>UI: onSuccess(...)
else key = error
AutoImport-->>UI: onError({ message })
end
end
AutoImport-->>UI: onDone() (guaranteed once)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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: 11
🧹 Nitpick comments (13)
src/components/features/drill-down/DrillDownModal.tsx (1)
124-138: Use stable IDs instead of localized labels as React keys.Using translated labels as keys is a bit fragile. Prefer a stable
idper segment.Suggested refactor
const tokenSegments = [ { + id: 'cacheRead', value: day.cacheReadTokens, color: 'hsl(160, 50%, 42%)', label: t('drillDown.tokenSegments.cacheRead'), }, { + id: 'cacheWrite', value: day.cacheCreationTokens, color: 'hsl(262, 60%, 55%)', label: t('drillDown.tokenSegments.cacheWrite'), }, - { value: day.inputTokens, color: 'hsl(340, 55%, 52%)', label: t('common.input') }, - { value: day.outputTokens, color: 'hsl(35, 80%, 52%)', label: t('common.output') }, - { value: day.thinkingTokens, color: 'hsl(12, 78%, 56%)', label: t('common.thinking') }, + { id: 'input', value: day.inputTokens, color: 'hsl(340, 55%, 52%)', label: t('common.input') }, + { id: 'output', value: day.outputTokens, color: 'hsl(35, 80%, 52%)', label: t('common.output') }, + { id: 'thinking', value: day.thinkingTokens, color: 'hsl(12, 78%, 56%)', label: t('common.thinking') }, ] as const ... - key={seg.label} + key={seg.id} ... - <span key={segment.label} className="flex items-center gap-1"> + <span key={segment.id} className="flex items-center gap-1">Also applies to: 243-246, 256-258
🤖 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 124 - 138, The segment arrays in DrillDownModal (e.g., tokenSegments) use translated labels as keys which is unstable; add a stable id field to each segment object (e.g., id: 'cacheRead', 'cacheWrite', 'input', 'output', 'thinking') and use that id as the React key wherever these arrays are mapped/rendered (replace usages that currently use label as key). Do the same refactor for the other segment arrays referenced around lines 243-246 and 256-258 so all map() calls use the stable id property instead of the localized label.tests/e2e/dashboard.spec.ts (1)
443-445: Use the existing localized pattern constant for consistency.This assertion is the only one still hardcoded to English text.
Proposed cleanup
- ).toContainText('Monthly view') + ).toContainText(monthlyViewPattern)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/dashboard.spec.ts` around lines 443 - 445, The assertion uses a hardcoded English string 'Monthly view'; replace it with the existing localized monthly-view pattern constant (the same pattern family as viewModeComboboxPattern — e.g., viewModeMonthlyPattern or the project's VIEW_MODE_MONTHLY constant) so the test uses the localized pattern rather than fixed English; update the locator assertion in tests/e2e/dashboard.spec.ts (the expect on freshPage.locator('#filters').getByRole(...)) to reference that monthly-view pattern constant.tests/frontend/phase4-correctness.test.tsx (1)
162-163: RedundantinitI18n('en')call.The
beforeEachhook on lines 51-62 already initializes i18n to'en', making this call redundant.♻️ Proposed fix
it('localizes drill-down labels in English', async () => { - await initI18n('en') - const day: DailyUsage = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/phase4-correctness.test.tsx` around lines 162 - 163, Remove the redundant initI18n('en') call inside the test "localizes drill-down labels in English" because i18n is already initialized to 'en' in the beforeEach hook (see initI18n usage in the beforeEach block and the test function name). Edit the test to rely on the beforeEach initialization and delete the extra initI18n('en') invocation so the test uses the shared setup.src/lib/model-utils.ts (1)
136-138: Unreachable fallback:dynamicColoralways returns a string.
dynamicColor(name)always returns a non-empty string, so?? MODEL_COLOR_DEFAULTwill never be reached. Consider removing the dead code.♻️ Proposed fix
export function getModelColor(name: string): string { - return MODEL_COLORS[name] ?? dynamicColor(name) ?? MODEL_COLOR_DEFAULT + return MODEL_COLORS[name] ?? dynamicColor(name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/model-utils.ts` around lines 136 - 138, The fallback MODEL_COLOR_DEFAULT in getModelColor is unreachable because dynamicColor(name) always returns a non-empty string; update getModelColor to remove the dead fallback or change the logic to only call dynamicColor when MODEL_COLORS[name] is undefined/empty and preserve MODEL_COLOR_DEFAULT as the final fallback if you want it reachable—specifically adjust the getModelColor function to first return MODEL_COLORS[name] if present, otherwise call dynamicColor(name) only when you intend it to override MODEL_COLOR_DEFAULT, or simply return MODEL_COLORS[name] ?? dynamicColor(name) (and remove MODEL_COLOR_DEFAULT) to eliminate unreachable code.src/lib/dashboard-preferences.ts (1)
9-9: Type assertions on lines 17–20 trust JSON structure matches expected types without import-time validation.The
ascasts assumedashboardPreferencesconforms to the TypeScript types. While the JSON structure currently matches the expected fields (verified:sectionDefinitionsentries havedomId,id, andlabelKey), there's no validation at import time if the JSON drifts. The file mitigates this somewhat through normalization functions that validate derived values, but adding a build-time schema check (e.g., with a JSON schema validator) would provide stronger guarantees.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/dashboard-preferences.ts` at line 9, The code blindly asserts the imported JSON (dashboardPreferences) with `as` casts for sectionDefinitions and related types, risking silent failures if the JSON shape drifts; replace the unchecked assertions by validating the imported dashboardPreferences at import/build time using a schema validator (e.g., Zod or AJV) and fail-fast on mismatch: create a schema that matches expected fields (sectionDefinitions entries with domId, id, labelKey, and any other expected fields), run dashboardPreferences through that schema in the module that currently uses the `as` casts, and use the validated/parsed result (or throw/exit with a clear error) instead of the raw `dashboardPreferences` value so functions like your normalization routines operate on a guaranteed shape.tests/frontend/de-analysis-terminology.test.tsx (1)
13-17: IntersectionObserver stub is missing required readonly properties.The stub class is missing
root,rootMargin, andthresholdsproperties required by theIntersectionObserverinterface. While this may work in practice due to the type assertion, consider adding these for completeness.♻️ Complete stub implementation
globalThis.IntersectionObserver = class IntersectionObserver { + readonly root: Element | Document | null = null + readonly rootMargin: string = '' + readonly thresholds: ReadonlyArray<number> = [] observe() {} unobserve() {} disconnect() {} + takeRecords(): IntersectionObserverEntry[] { return [] } } as typeof IntersectionObserver🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/de-analysis-terminology.test.tsx` around lines 13 - 17, The IntersectionObserver test stub is missing the readonly properties required by the interface; update the globalThis.IntersectionObserver stub class to include readonly properties root: Element | Document | null, rootMargin: string, and thresholds: ReadonlyArray<number> (or number[]) with sensible defaults (e.g., null, "", and [] respectively) so the stub fully implements the IntersectionObserver shape while keeping the existing observe/unobserve/disconnect methods.tests/unit/api.test.ts (1)
30-31: RedundantinitI18n('en')calls.Lines 31 and 60 call
initI18n('en'), butbeforeEachalready initializes i18n to English. These calls are unnecessary unless you intend to re-initialize for a specific reason.♻️ Suggested cleanup
it('uses the localized fallback when saving settings fails without a server message', async () => { - await initI18n('en') vi.stubGlobal(it('returns bootstrap defaults together with the localized error message', async () => { - await initI18n('en') vi.stubGlobal(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/api.test.ts` around lines 30 - 31, The test contains redundant explicit calls to initI18n('en') inside individual it blocks (e.g., the test "uses the localized fallback when saving settings fails without a server message") even though beforeEach already initializes i18n to English; remove those extra initI18n('en') calls from the test cases (and any other it blocks in this file) so initialization relies on the existing beforeEach setup, leaving only one central i18n initialization via beforeEach.src/components/LoadErrorState.tsx (1)
47-49: Potential duplicate key warning ifdetailscontains identical strings.Using
detailas the key assumes all detail strings are unique. If duplicate messages are possible, React will warn about duplicate keys.♻️ Use index-based key for safety
<ul className="mt-3 space-y-2 text-sm text-foreground"> - {details.map((detail) => ( - <li key={detail}>{detail}</li> + {details.map((detail, index) => ( + <li key={index}>{detail}</li> ))} </ul>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/LoadErrorState.tsx` around lines 47 - 49, In LoadErrorState's JSX where you render details.map((detail) => (<li key={detail}>{detail}</li>)), change the key to use the index (or a stable unique identifier) instead of the detail string to avoid duplicate key warnings; update the map callback signature to (detail, idx) => and set key={idx} (or key={someStableId}) so duplicate detail strings won't produce React key collisions.src/components/features/settings/SettingsModal.tsx (1)
326-339: Consider extracting supported languages to a constant.The hardcoded
(['de', 'en'] as const)array works well for now, but extracting it to a shared constant would improve maintainability if additional languages are added later.♻️ Extract to constant
+const SUPPORTED_LANGUAGES = ['de', 'en'] as const + // In the component: <div className="mt-4 flex flex-wrap gap-2"> - {(['de', 'en'] as const).map((nextLanguage) => ( + {SUPPORTED_LANGUAGES.map((nextLanguage) => (🤖 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 326 - 339, Extract the hardcoded language tuple (currently used as (['de', 'en'] as const) in the language selector map) into a shared constant (e.g., SUPPORTED_LANGUAGES) and import/use it in the component instead of the inline array; update the map to iterate SUPPORTED_LANGUAGES while keeping existing behavior around aria-pressed (languageDraft), setLanguageDraft, Button props and t(`app.languages.${nextLanguage}`) usages so adding languages later only requires changing the constant.src/components/charts/ChartCard.tsx (2)
212-217: Consider addingtype="button"to the export button for consistency.The expand button correctly has
type="button", but the CSV export button inside the dialog omits it. While likely not an issue in practice, adding it maintains consistency.♻️ Add type attribute
<button + type="button" onClick={handleExport} className="text-xs px-3 py-1.5 rounded-lg border border-border hover:bg-accent transition-all duration-200 text-muted-foreground hover:text-foreground" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/charts/ChartCard.tsx` around lines 212 - 217, The export CSV button in the ChartCard component is missing an explicit type attribute; update the button element that uses onClick={handleExport} (inside ChartCard) to include type="button" to match the expand button and avoid unintended form submissions, keeping the handler and classes unchanged.
36-47: Consider usingbuildCsvLinefor data rows for consistency.The header row now uses
buildCsvLine(keys), but data rows still manually map and join cells. For consistency and to ensure uniform escaping behavior:♻️ Optional: Use buildCsvLine for data rows too
export function buildChartCsv(chartData: Record<string, unknown>[]): string { if (chartData.length === 0) return '' const firstRow = chartData[0] if (!firstRow) return '' const keys = Object.keys(firstRow) return [ buildCsvLine(keys), - ...chartData.map((row) => keys.map((key) => stringifyCsvCell(row[key])).join(',')), + ...chartData.map((row) => buildCsvLine(keys.map((key) => row[key]))), ].join('\n') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/charts/ChartCard.tsx` around lines 36 - 47, The data rows in buildChartCsv are manually mapping and joining cells instead of reusing buildCsvLine like the header; update buildChartCsv so the data row generation uses buildCsvLine (e.g., replace the keys.map(...).join(',') with buildCsvLine(keys.map(key => row[key]))), or if buildCsvLine expects already-stringified values, pass stringifyCsvCell(row[key]) into buildCsvLine; this ensures consistent escaping/formatting between the header and each data row.server/runtime.js (1)
1-3: Consider additional loopback address formats.The current check covers common loopback addresses, but may miss edge cases like
::ffff:127.0.0.1(IPv4-mapped IPv6 loopback) or[::1](bracketed IPv6). Depending on howBIND_HOSTis configured, these could be valid loopback bindings that get incorrectly rejected.♻️ Extended loopback detection
function isLoopbackHost(host) { - return host === '127.0.0.1' || host === 'localhost' || host === '::1'; + const normalized = host.toLowerCase(); + return ( + normalized === '127.0.0.1' || + normalized === 'localhost' || + normalized === '::1' || + normalized === '[::1]' || + normalized === '::ffff:127.0.0.1' + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/runtime.js` around lines 1 - 3, The isLoopbackHost(host) function currently only checks '127.0.0.1', 'localhost', and '::1' and can miss forms like '::ffff:127.0.0.1' or bracketed IPv6 '[::1]'; update isLoopbackHost to normalize the input (trim whitespace, strip surrounding brackets) and then detect IPv4-mapped IPv6 and IPv6 loopback patterns (e.g., accept strings starting with '::ffff:' followed by '127.0.0.1' and the normalized '::1'), keeping the existing checks for '127.0.0.1' and 'localhost' so all common loopback formats are recognized.docs/architecture.md (1)
49-49: Trim the redundant “CLI interface” wording.“CLI” already implies an interface, so this reads cleaner as just “CLI”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture.md` at line 49, Update the phrasing that currently reads "CLI interface" to just "CLI" in the sentence referencing server.js as the public entrypoint and the published CLI interface; locate the text mentioning "server.js" and replace the redundant "CLI interface" wording with "CLI" so the sentence reads cleaner without changing meaning.
🤖 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.js`:
- Around line 290-295: The ensureDir function currently only tightens
permissions for newly created directories; change it so permissions are enforced
for both new and pre-existing dirs by calling fs.chmodSync(dirPath,
SECURE_DIR_MODE) whenever not IS_WINDOWS (remove the existed check), while still
keeping the recursive mkdirSync call and SECURE_DIR_MODE usage; ensure the chmod
call is applied after mkdirSync to cover both cases (and avoid Windows by
checking IS_WINDOWS).
In `@server/http-utils.js`:
- Around line 78-90: The resolveApiPath function currently falls back to
handling '/api' and '/api/*' which bypasses a custom apiPrefix; update
resolveApiPath to only accept paths that match the configured apiPrefix (i.e.,
if pathname === apiPrefix return '/' and if pathname.startsWith(apiPrefix + '/')
return pathname.slice(apiPrefix.length)) and remove or disable the branches that
check for '/api' and '/api/*'; for non-matching paths return null/undefined (or
keep existing non-match behavior) so requests to the default '/api' are not
accepted when apiPrefix is customized.
- Around line 8-12: The readBody promise currently only listens to 'data',
'end', and 'error' and can hang if the client disconnects; update the code in
readBody to also listen for the 'close' event on req, and in the close handler
check req.readableEnded (or !req.readableEnded) to detect an aborted/incomplete
read and reject the promise accordingly; ensure the cleanup function (which
removes onData, onEnd, onError) also removes the onClose listener and reference
the existing symbols (readBody, cleanup, onData, onEnd, onError) so both the
primary block and the similar block at lines 48-57 are updated to attach and
remove the close handler and settle the promise when the socket closes.
In `@shared/dashboard-domain.d.ts`:
- Around line 1-19: The .d.ts file imports types from ../src/types which aren’t
published; update the package so external consumers can resolve these symbols
(aggregateToDailyFormat, computeBusiestWeek, computeMetrics,
computeMovingAverage, computeWeekOverWeekChange, filterByDateRange,
filterByModels, filterByMonth, filterByProviders, getModelProvider,
normalizeModelName, sortByDate) by either (A) moving the types used by these
declarations into shared/ and updating the import in
shared/dashboard-domain.d.ts to the new local path, (B) creating a root-level
types/ module and changing the import to that published path, or (C) adding
src/types to the published "files" in package.json and ensuring the declaration
"types" entry points reference the published location; after relocating, update
the import in shared/dashboard-domain.d.ts to the new path and confirm
package.json includes the new types files so consumers can resolve them.
In `@shared/dashboard-domain.js`:
- Around line 52-58: parseClaudeName currently uses rest.split('-', 2) which
drops the family prefix and collapses multi-part numeric versions (e.g., "4-5")
by truncating after the first dash; fix by splitting all parts (const parts =
rest.split('-')), then treat parts[0] as the family/prefix and parts[1] as the
model name (if present) and join the remaining parts with '.' to form the full
version string (const version = parts.slice(2).join('.')); pass that full
version to formatVersion and use capitalize on the family and model parts when
building the returned string (refer to parseClaudeName, formatVersion, and
capitalize) so names like "claude-sonnet-4-5" become "Claude Sonnet 4.5" without
losing the family prefix or collapsing dotted versions.
In `@src/components/cards/PrimaryMetrics.tsx`:
- Around line 99-108: The translation currently receives ioRatio as a raw string
(from .toFixed(1)) which isn't locale-aware; update the code in
PrimaryMetrics.tsx where subtitle is built (the ioRatio ternary branch) to
format the numeric ratio with the app locale before passing it to t — use
getCurrentLocale() and Intl.NumberFormat (or Number.prototype.toLocaleString) to
produce a locale-aware string (e.g., format Number(ioRatio) with
getCurrentLocale()), then pass that formatted string as the ratio to t; keep
formatTokens(metrics.avgTokensPerRequest) unchanged.
In `@src/components/features/heatmap/HeatmapCalendar.tsx`:
- Around line 64-72: The Intl.DateTimeFormat memo (fullDateFormatter) and the
dayLabels useMemo are using an empty dependency array so they don't update when
language changes; update both useMemo hooks to include the current locale (from
getCurrentLocale()) in their dependency arrays so the formatters are recreated
when the locale changes, ensuring aria-labels and day labels reflect runtime
language switches.
In `@src/components/ui/expandable-card.tsx`:
- Line 34: Replace the current concatenation and toLowerCase usage for the
aria-label (the aria-label prop built from title and t('common.expand')) with a
single i18n call that accepts interpolation; for example call
t('common.expand_with_title', { title }) when title exists and
t('common.expand') otherwise (or provide a single key t('common.expand_label', {
title: title || '' }) that the translators can handle), and remove any
.toLowerCase() so the translation controls casing and word order; update the
aria-label assignment on the ExpandableCard/aria-label prop accordingly.
In `@src/hooks/use-dashboard-controller.ts`:
- Around line 359-383: The catch currently masks server-side or network errors
by always showing the generic "file read failed" toast; update handleFileChange
to separate JSON parsing errors from upload errors: first try/catch
JSON.parse(await file.text()) and show the existing file-read toast only for
parse/file-I/O failures, then call await uploadMutation.mutateAsync(parsed)
inside its own try/catch and, on upload failure, use
addToast(t('toasts.fileReadFailed') or a new key) with the backend error message
(error.message or error.response.data.message) so the user sees the server
validation/network error; reference handleFileChange,
uploadMutation.mutateAsync, and addToast when making these changes.
- Around line 390-396: handleDelete currently awaits
deleteMutation.mutateAsync() without error handling so rejections become
unhandled; wrap the mutateAsync call in try/catch inside handleDelete, on
success run queryClient.invalidateQueries({ queryKey: ['settings'] }),
setAnimationSeed, setDataSource(null) and addToast(t('toasts.dataDeleted'),
'info'), and on failure catch the error and call addToast with a failure message
(e.g. t('toasts.deleteFailed')) and optionally log the error; keep references to
handleDelete, deleteMutation.mutateAsync, queryClient.invalidateQueries,
setAnimationSeed, setDataSource, and addToast when applying the change.
In `@src/lib/auto-import.ts`:
- Around line 204-214: The loop exits without consuming the decoder's final
state or the remaining buffer, so before calling finish() / flushEvent you must
flush the TextDecoder and process any tail data: call decoder.decode(undefined,
{ stream: false }) (or decoder.decode() ) and append it to buffer, split the
combined buffer into lines, push those lines into dataLines (or otherwise feed
them through the same line-processing used earlier), then call
flushEvent(currentEvent, dataLines) and finish(); update the code around
reader.read() exit so the remaining decoded text is consumed and the last event
payload is not dropped.
---
Nitpick comments:
In `@docs/architecture.md`:
- Line 49: Update the phrasing that currently reads "CLI interface" to just
"CLI" in the sentence referencing server.js as the public entrypoint and the
published CLI interface; locate the text mentioning "server.js" and replace the
redundant "CLI interface" wording with "CLI" so the sentence reads cleaner
without changing meaning.
In `@server/runtime.js`:
- Around line 1-3: The isLoopbackHost(host) function currently only checks
'127.0.0.1', 'localhost', and '::1' and can miss forms like '::ffff:127.0.0.1'
or bracketed IPv6 '[::1]'; update isLoopbackHost to normalize the input (trim
whitespace, strip surrounding brackets) and then detect IPv4-mapped IPv6 and
IPv6 loopback patterns (e.g., accept strings starting with '::ffff:' followed by
'127.0.0.1' and the normalized '::1'), keeping the existing checks for
'127.0.0.1' and 'localhost' so all common loopback formats are recognized.
In `@src/components/charts/ChartCard.tsx`:
- Around line 212-217: The export CSV button in the ChartCard component is
missing an explicit type attribute; update the button element that uses
onClick={handleExport} (inside ChartCard) to include type="button" to match the
expand button and avoid unintended form submissions, keeping the handler and
classes unchanged.
- Around line 36-47: The data rows in buildChartCsv are manually mapping and
joining cells instead of reusing buildCsvLine like the header; update
buildChartCsv so the data row generation uses buildCsvLine (e.g., replace the
keys.map(...).join(',') with buildCsvLine(keys.map(key => row[key]))), or if
buildCsvLine expects already-stringified values, pass stringifyCsvCell(row[key])
into buildCsvLine; this ensures consistent escaping/formatting between the
header and each data row.
In `@src/components/features/drill-down/DrillDownModal.tsx`:
- Around line 124-138: The segment arrays in DrillDownModal (e.g.,
tokenSegments) use translated labels as keys which is unstable; add a stable id
field to each segment object (e.g., id: 'cacheRead', 'cacheWrite', 'input',
'output', 'thinking') and use that id as the React key wherever these arrays are
mapped/rendered (replace usages that currently use label as key). Do the same
refactor for the other segment arrays referenced around lines 243-246 and
256-258 so all map() calls use the stable id property instead of the localized
label.
In `@src/components/features/settings/SettingsModal.tsx`:
- Around line 326-339: Extract the hardcoded language tuple (currently used as
(['de', 'en'] as const) in the language selector map) into a shared constant
(e.g., SUPPORTED_LANGUAGES) and import/use it in the component instead of the
inline array; update the map to iterate SUPPORTED_LANGUAGES while keeping
existing behavior around aria-pressed (languageDraft), setLanguageDraft, Button
props and t(`app.languages.${nextLanguage}`) usages so adding languages later
only requires changing the constant.
In `@src/components/LoadErrorState.tsx`:
- Around line 47-49: In LoadErrorState's JSX where you render
details.map((detail) => (<li key={detail}>{detail}</li>)), change the key to use
the index (or a stable unique identifier) instead of the detail string to avoid
duplicate key warnings; update the map callback signature to (detail, idx) =>
and set key={idx} (or key={someStableId}) so duplicate detail strings won't
produce React key collisions.
In `@src/lib/dashboard-preferences.ts`:
- Line 9: The code blindly asserts the imported JSON (dashboardPreferences) with
`as` casts for sectionDefinitions and related types, risking silent failures if
the JSON shape drifts; replace the unchecked assertions by validating the
imported dashboardPreferences at import/build time using a schema validator
(e.g., Zod or AJV) and fail-fast on mismatch: create a schema that matches
expected fields (sectionDefinitions entries with domId, id, labelKey, and any
other expected fields), run dashboardPreferences through that schema in the
module that currently uses the `as` casts, and use the validated/parsed result
(or throw/exit with a clear error) instead of the raw `dashboardPreferences`
value so functions like your normalization routines operate on a guaranteed
shape.
In `@src/lib/model-utils.ts`:
- Around line 136-138: The fallback MODEL_COLOR_DEFAULT in getModelColor is
unreachable because dynamicColor(name) always returns a non-empty string; update
getModelColor to remove the dead fallback or change the logic to only call
dynamicColor when MODEL_COLORS[name] is undefined/empty and preserve
MODEL_COLOR_DEFAULT as the final fallback if you want it reachable—specifically
adjust the getModelColor function to first return MODEL_COLORS[name] if present,
otherwise call dynamicColor(name) only when you intend it to override
MODEL_COLOR_DEFAULT, or simply return MODEL_COLORS[name] ?? dynamicColor(name)
(and remove MODEL_COLOR_DEFAULT) to eliminate unreachable code.
In `@tests/e2e/dashboard.spec.ts`:
- Around line 443-445: The assertion uses a hardcoded English string 'Monthly
view'; replace it with the existing localized monthly-view pattern constant (the
same pattern family as viewModeComboboxPattern — e.g., viewModeMonthlyPattern or
the project's VIEW_MODE_MONTHLY constant) so the test uses the localized pattern
rather than fixed English; update the locator assertion in
tests/e2e/dashboard.spec.ts (the expect on
freshPage.locator('#filters').getByRole(...)) to reference that monthly-view
pattern constant.
In `@tests/frontend/de-analysis-terminology.test.tsx`:
- Around line 13-17: The IntersectionObserver test stub is missing the readonly
properties required by the interface; update the globalThis.IntersectionObserver
stub class to include readonly properties root: Element | Document | null,
rootMargin: string, and thresholds: ReadonlyArray<number> (or number[]) with
sensible defaults (e.g., null, "", and [] respectively) so the stub fully
implements the IntersectionObserver shape while keeping the existing
observe/unobserve/disconnect methods.
In `@tests/frontend/phase4-correctness.test.tsx`:
- Around line 162-163: Remove the redundant initI18n('en') call inside the test
"localizes drill-down labels in English" because i18n is already initialized to
'en' in the beforeEach hook (see initI18n usage in the beforeEach block and the
test function name). Edit the test to rely on the beforeEach initialization and
delete the extra initI18n('en') invocation so the test uses the shared setup.
In `@tests/unit/api.test.ts`:
- Around line 30-31: The test contains redundant explicit calls to
initI18n('en') inside individual it blocks (e.g., the test "uses the localized
fallback when saving settings fails without a server message") even though
beforeEach already initializes i18n to English; remove those extra
initI18n('en') calls from the test cases (and any other it blocks in this file)
so initialization relies on the existing beforeEach setup, leaving only one
central i18n initialization via beforeEach.
🪄 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: 6082617c-c5f4-4b14-9236-f92e0b61ab4e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (77)
.github/workflows/ci.ymlCHANGELOG.mdREADME.mdSECURITY.mddocs/architecture.mdpackage.jsonserver.jsserver/http-utils.jsserver/report/utils.jsserver/runtime.jsshared/dashboard-domain.d.tsshared/dashboard-domain.jsshared/dashboard-preferences.jsonsrc/App.tsxsrc/components/Dashboard.tsxsrc/components/LoadErrorState.tsxsrc/components/cards/PrimaryMetrics.tsxsrc/components/cards/TodayMetrics.tsxsrc/components/charts/ChartCard.tsxsrc/components/charts/CorrelationAnalysis.tsxsrc/components/charts/CustomTooltip.tsxsrc/components/charts/DistributionAnalysis.tsxsrc/components/dashboard/DashboardSections.tsxsrc/components/features/anomaly/AnomalyDetection.tsxsrc/components/features/cache-roi/CacheROI.tsxsrc/components/features/comparison/PeriodComparison.tsxsrc/components/features/drill-down/DrillDownModal.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/features/help/HelpPanel.tsxsrc/components/features/help/InfoHeading.tsxsrc/components/features/request-quality/RequestQuality.tsxsrc/components/features/risk/ConcentrationRisk.tsxsrc/components/features/settings/SettingsModal.tsxsrc/components/layout/FilterBar.tsxsrc/components/layout/Header.tsxsrc/components/tables/ModelEfficiency.tsxsrc/components/tables/ProviderEfficiency.tsxsrc/components/tables/RecentDays.tsxsrc/components/ui/dialog.tsxsrc/components/ui/expandable-card.tsxsrc/components/ui/formatted-value.tsxsrc/components/ui/section-header.tsxsrc/hooks/use-app-settings.tssrc/hooks/use-dashboard-controller.tssrc/lib/api.tssrc/lib/auto-import.tssrc/lib/calculations.tssrc/lib/csv-export.tssrc/lib/csv.tssrc/lib/dashboard-preferences.tssrc/lib/data-transforms.tssrc/lib/help-content.tssrc/lib/model-utils.tssrc/locales/de/common.jsonsrc/locales/en/common.jsonsrc/main.tsxtests/e2e/dashboard.spec.tstests/frontend/chart-card.test.tsxtests/frontend/custom-tooltip.test.tsxtests/frontend/dashboard-error-state.test.tsxtests/frontend/de-analysis-terminology.test.tsxtests/frontend/expandable-card.test.tsxtests/frontend/filter-bar.test.tsxtests/frontend/formatted-value.test.tsxtests/frontend/header-links.test.tsxtests/frontend/heatmap-calendar.test.tsxtests/frontend/help-panel.test.tsxtests/frontend/info-heading.test.tsxtests/frontend/phase4-correctness.test.tsxtests/frontend/settings-modal.test.tsxtests/integration/server.test.tstests/unit/api.test.tstests/unit/auto-import.test.tstests/unit/csv-export.test.tstests/unit/help-content.test.tstests/unit/server-helpers.test.tsvitest.config.ts
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
tests/unit/dashboard-preferences.test.ts (2)
12-13: Strengthen the happy-path assertion to exact config values.
toContain('monthly')can miss regressions (e.g., missingdaily/yearly). Prefer exact equality for arrays.As per coding guidelines: "Prefer focused `*.test.ts` or `*.test.tsx` coverage for data transforms, hooks, or complex UI behavior."Proposed test tightening
expect(parsed.sectionDefinitions).toEqual(DASHBOARD_SECTION_DEFINITIONS) - expect(parsed.viewModes).toContain('monthly') + expect(parsed.viewModes).toEqual(dashboardPreferences.viewModes) + expect(parsed.datePresets).toEqual(dashboardPreferences.datePresets)🤖 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 12 - 13, The test currently uses a loose assertion expect(parsed.viewModes).toContain('monthly'); tighten it to assert the exact expected array so regressions are caught: replace the toContain check with expect(parsed.viewModes).toEqual([...]) using the precise expected viewModes array (e.g., ['daily','weekly','monthly','yearly'] or the canonical constant) and ensure parsed.sectionDefinitions still equals DASHBOARD_SECTION_DEFINITIONS; update the test in dashboard-preferences.test.ts to assert exact equality on parsed.viewModes (referencing parsed.viewModes and DASHBOARD_SECTION_DEFINITIONS).
16-23: Split the drift test into one-invalid-field-per-case.This case currently invalidates multiple fields at once, which can hide which validator regressed. Use focused negative tests per field.
As per coding guidelines: "Prefer focused `*.test.ts` or `*.test.tsx` coverage for data transforms, hooks, or complex UI behavior."Proposed focused negative tests
- it('fails fast when the shared preferences JSON drifts from the expected shape', () => { + it('fails fast when viewModes contains unsupported values', () => { expect(() => parseDashboardPreferencesConfig({ - datePresets: ['all'], + datePresets: dashboardPreferences.datePresets, viewModes: ['daily', 'weekly'], - sectionDefinitions: [{ id: 'metrics', domId: 'metrics' }], + sectionDefinitions: dashboardPreferences.sectionDefinitions, }), ).toThrow('Invalid dashboard preferences') }) + + it('fails fast when sectionDefinitions entries are malformed', () => { + expect(() => + parseDashboardPreferencesConfig({ + datePresets: dashboardPreferences.datePresets, + viewModes: dashboardPreferences.viewModes, + sectionDefinitions: [{ id: 'metrics', domId: 'metrics' }], + }), + ).toThrow('Invalid dashboard preferences') + })🤖 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 16 - 23, The existing test for parseDashboardPreferencesConfig bundles multiple invalid fields into one case; split it into focused negative tests that each invalidate a single field so failures pinpoint the regressing validator. Create separate it blocks that call parseDashboardPreferencesConfig with only one malformed property per test (e.g., invalid datePresets, invalid viewModes, invalid sectionDefinitions), and assert each throws the same 'Invalid dashboard preferences' error (using expect(...).toThrow('Invalid dashboard preferences')) to ensure each validator is covered independently.tests/frontend/dashboard-error-state.test.tsx (1)
96-109: Extract repeatedusageDatafixtures to a helper to reduce drift.The same nested object is repeated in three tests; if the usage shape changes, these tests will be harder to maintain.
♻️ Suggested refactor
+function makeEmptyUsageData() { + return { + daily: [], + totals: { + inputTokens: 0, + outputTokens: 0, + cacheCreationTokens: 0, + cacheReadTokens: 0, + thinkingTokens: 0, + totalCost: 0, + totalTokens: 0, + requestCount: 0, + }, + } +} ... - usageHookMocks.useUsageData.mockReturnValue({ - data: { - daily: [], - totals: { - inputTokens: 0, - outputTokens: 0, - cacheCreationTokens: 0, - cacheReadTokens: 0, - thinkingTokens: 0, - totalCost: 0, - totalTokens: 0, - requestCount: 0, - }, - }, + usageHookMocks.useUsageData.mockReturnValue({ + data: makeEmptyUsageData(), isLoading: false, error: null, })Also applies to: 163-175, 203-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/dashboard-error-state.test.tsx` around lines 96 - 109, Extract the repeated nested usage object into a shared test fixture and replace the three inline mocks with it: create a const (e.g., usageDataFixture or makeUsageData()) that returns the object shape used by usageHookMocks.useUsageData.mockReturnValue and update the three tests (the blocks calling usageHookMocks.useUsageData.mockReturnValue) to use that fixture; ensure the fixture exports the same shape (daily and totals with the token/cost fields) so tests remain identical and update imports/requires in tests/frontend/dashboard-error-state.test.tsx to reference the fixture.src/components/cards/MonthMetrics.tsx (1)
120-125: Consider locale-aware formatting for consistency.The
thinkingSubtitleat line 123 still usestoFixed(1)directly, which doesn't respect locale. For consistency with the I/O ratio change, consider usingIntl.NumberFormathere as well. Similarly, coverage percentages at lines 160 and 180-181 usetoFixed(0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/cards/MonthMetrics.tsx` around lines 120 - 125, Replace direct toFixed calls with locale-aware formatting: use Intl.NumberFormat (or the existing percent formatter used for I/O ratio) to format the thinking percentage computed from agg.thinkingTokens/agg.totalTokens with one decimal place, and update the coverage percentage renderings that use toFixed(0) to use Intl.NumberFormat configured for zero fraction digits; change the logic that builds thinkingSubtitle (variable thinkingSubtitle) and the coverage percentage expressions at the two places that render coverage (the two occurrences using toFixed(0)) to call the locale-aware formatter so percentages respect user locale.src/lib/model-utils.ts (1)
136-138: Remove unusedMODEL_COLOR_DEFAULTfrom constants.ts.The
getModelColorfunction now falls back todynamicColor(name)instead of usingMODEL_COLOR_DEFAULT. This constant is no longer referenced anywhere in the codebase and can be removed fromsrc/lib/constants.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/model-utils.ts` around lines 136 - 138, Delete the unused constant MODEL_COLOR_DEFAULT from constants.ts and remove any exports/imports referencing it; since getModelColor(name) now falls back to dynamicColor(name) and no code uses MODEL_COLOR_DEFAULT, remove its declaration and any lingering references (search for MODEL_COLOR_DEFAULT) to avoid dead code and unused export warnings, ensuring getModelColor and MODEL_COLORS remain unchanged.
🤖 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.js`:
- Line 35: fetchRuntimeIdentity() currently probes the hardcoded '/api/runtime'
path which breaks when API_PREFIX is changed; update fetchRuntimeIdentity() to
build the probe URL using the API_PREFIX constant (API_PREFIX + '/runtime'),
ensuring you normalize slashes to avoid '//' or missing leading slash, and use
that composed path wherever fetchRuntimeIdentity() is used (background-instance
ownership checks, registry pruning, ttdash stop) so probes go to the correct
prefixed endpoint.
- Around line 765-768: When creating the background log you must enforce 0600
after open because openSync's mode is masked by umask; after calling
buildBackgroundLogFilePath() and opening the file into logFd, call
fs.fchmodSync(logFd, SECURE_FILE_MODE) (or fs.chmodSync(logFile,
SECURE_FILE_MODE) if you prefer path-based) to force the file permissions to
SECURE_FILE_MODE (0600) so the background log file created by the code that
opens logFile with logFd is hardened regardless of process umask.
In `@server/runtime.js`:
- Around line 1-11: isLoopbackHost currently only allows the literal 127.0.0.1
which rejects other valid loopback addresses (e.g. 127.0.0.2) and breaks
ensureBindHostAllowed; update isLoopbackHost to treat the full IPv4 loopback
range (127.0.0.0/8) as local by matching addresses that start with "127." in
addition to "localhost", "::1", and IPv4-mapped IPv6 loopback forms (e.g.
"::ffff:127.*.*.*"); locate the isLoopbackHost function and replace the
exact-equality check for '127.0.0.1' with a range/match check (startsWith /
regex) so any 127.x.x.x is accepted while preserving the other existing checks.
In `@shared/dashboard-domain.js`:
- Around line 312-336: computeMovingAverage currently skips undefined values
when updating sum but always divides by the fixed window size, producing wrong
averages when undefineds are present; in computeMovingAverage track a
definedCount (e.g., increment when currentValue !== undefined and decrement when
outgoingValue !== undefined) and use definedCount as the divisor when computing
result (if definedCount is 0 set result[index] = undefined), and update both sum
and definedCount when adding currentValue and when removing values at
values[index - window]; keep the existing behavior of emitting undefined for
indices < window - 1.
In `@src/lib/auto-import.ts`:
- Around line 226-236: The loop currently calls finish() unconditionally when
the reader returns streamDone, which treats an unexpected EOF as success; update
the logic in the while loop that reads from reader.read() to only call finish()
if a terminal success frame was actually received (e.g., check a flag such as
state.receivedFinalFrame or state.completed set by processLines/flushEvent when
a success frame is parsed); if that flag is false, do not call finish() and
instead invoke the server-connection-lost/error path (e.g., call the existing
serverConnectionLost handler or emit an error) after processing any remaining
buffer via processLines/flushEvent, ensuring reader.read, buffer, decoder,
processLines, flushEvent, state.currentEvent and state.dataLines are the
referenced points to modify.
---
Nitpick comments:
In `@src/components/cards/MonthMetrics.tsx`:
- Around line 120-125: Replace direct toFixed calls with locale-aware
formatting: use Intl.NumberFormat (or the existing percent formatter used for
I/O ratio) to format the thinking percentage computed from
agg.thinkingTokens/agg.totalTokens with one decimal place, and update the
coverage percentage renderings that use toFixed(0) to use Intl.NumberFormat
configured for zero fraction digits; change the logic that builds
thinkingSubtitle (variable thinkingSubtitle) and the coverage percentage
expressions at the two places that render coverage (the two occurrences using
toFixed(0)) to call the locale-aware formatter so percentages respect user
locale.
In `@src/lib/model-utils.ts`:
- Around line 136-138: Delete the unused constant MODEL_COLOR_DEFAULT from
constants.ts and remove any exports/imports referencing it; since
getModelColor(name) now falls back to dynamicColor(name) and no code uses
MODEL_COLOR_DEFAULT, remove its declaration and any lingering references (search
for MODEL_COLOR_DEFAULT) to avoid dead code and unused export warnings, ensuring
getModelColor and MODEL_COLORS remain unchanged.
In `@tests/frontend/dashboard-error-state.test.tsx`:
- Around line 96-109: Extract the repeated nested usage object into a shared
test fixture and replace the three inline mocks with it: create a const (e.g.,
usageDataFixture or makeUsageData()) that returns the object shape used by
usageHookMocks.useUsageData.mockReturnValue and update the three tests (the
blocks calling usageHookMocks.useUsageData.mockReturnValue) to use that fixture;
ensure the fixture exports the same shape (daily and totals with the token/cost
fields) so tests remain identical and update imports/requires in
tests/frontend/dashboard-error-state.test.tsx to reference the fixture.
In `@tests/unit/dashboard-preferences.test.ts`:
- Around line 12-13: The test currently uses a loose assertion
expect(parsed.viewModes).toContain('monthly'); tighten it to assert the exact
expected array so regressions are caught: replace the toContain check with
expect(parsed.viewModes).toEqual([...]) using the precise expected viewModes
array (e.g., ['daily','weekly','monthly','yearly'] or the canonical constant)
and ensure parsed.sectionDefinitions still equals DASHBOARD_SECTION_DEFINITIONS;
update the test in dashboard-preferences.test.ts to assert exact equality on
parsed.viewModes (referencing parsed.viewModes and
DASHBOARD_SECTION_DEFINITIONS).
- Around line 16-23: The existing test for parseDashboardPreferencesConfig
bundles multiple invalid fields into one case; split it into focused negative
tests that each invalidate a single field so failures pinpoint the regressing
validator. Create separate it blocks that call parseDashboardPreferencesConfig
with only one malformed property per test (e.g., invalid datePresets, invalid
viewModes, invalid sectionDefinitions), and assert each throws the same 'Invalid
dashboard preferences' error (using expect(...).toThrow('Invalid dashboard
preferences')) to ensure each validator is covered independently.
🪄 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: eae41fe7-c44d-43ee-88e7-204c779dea1c
📒 Files selected for processing (37)
docs/architecture.mdserver.jsserver/http-utils.jsserver/runtime.jsshared/dashboard-domain.d.tsshared/dashboard-domain.jsshared/dashboard-types.d.tssrc/components/LoadErrorState.tsxsrc/components/cards/MonthMetrics.tsxsrc/components/cards/PrimaryMetrics.tsxsrc/components/charts/ChartCard.tsxsrc/components/features/drill-down/DrillDownModal.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/features/settings/SettingsModal.tsxsrc/components/ui/expandable-card.tsxsrc/hooks/use-dashboard-controller.tssrc/lib/auto-import.tssrc/lib/dashboard-preferences.tssrc/lib/model-utils.tssrc/locales/de/common.jsonsrc/locales/en/common.jsontests/e2e/dashboard.spec.tstests/frontend/chart-card.test.tsxtests/frontend/dashboard-error-state.test.tsxtests/frontend/de-analysis-terminology.test.tsxtests/frontend/expandable-card.test.tsxtests/frontend/heatmap-calendar.test.tsxtests/frontend/metric-ratio-locale.test.tsxtests/frontend/phase4-correctness.test.tsxtests/integration/server.test.tstests/unit/api.test.tstests/unit/auto-import.test.tstests/unit/dashboard-preferences.test.tstests/unit/http-utils.test.tstests/unit/model-normalization.test.tstests/unit/report-utils.test.tstests/unit/server-helpers.test.ts
✅ Files skipped from review due to trivial changes (6)
- tests/frontend/expandable-card.test.tsx
- tests/unit/model-normalization.test.ts
- tests/frontend/de-analysis-terminology.test.tsx
- docs/architecture.md
- shared/dashboard-domain.d.ts
- src/hooks/use-dashboard-controller.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/frontend/chart-card.test.tsx
- tests/e2e/dashboard.spec.ts
- src/components/features/drill-down/DrillDownModal.tsx
- tests/frontend/phase4-correctness.test.tsx
- src/components/cards/PrimaryMetrics.tsx
- tests/unit/api.test.ts
- tests/unit/server-helpers.test.ts
- src/lib/dashboard-preferences.ts
- tests/frontend/heatmap-calendar.test.tsx
- src/components/LoadErrorState.tsx
- src/components/charts/ChartCard.tsx
- src/locales/en/common.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/auto-import.ts (1)
25-27: Type definition doesn't match the actual payload passed toonProgress.
ProgressMessagedeclares only{ message: string }, but line 128-131 spreads the fullProgressEventdata (includingkeyandvars) into the callback payload. The unit test at line 133-137 correctly expects all three fields.Consider extending the type to reflect the actual shape:
💡 Suggested fix
-export interface ProgressMessage { - message: string -} +export interface ProgressMessage extends ProgressEvent { + message: string +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auto-import.ts` around lines 25 - 27, The ProgressMessage type only defines { message: string } but the onProgress callback actually receives the full ProgressEvent payload (message, key, vars); update the ProgressMessage interface to include key: string and vars: Record<string, any> (or a more specific type if known) so it matches the shape used when spreading the ProgressEvent into onProgress and satisfies the unit test; adjust any related usages of ProgressMessage or onProgress signatures accordingly (e.g., where ProgressMessage is imported/used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/auto-import.ts`:
- Around line 25-27: The ProgressMessage type only defines { message: string }
but the onProgress callback actually receives the full ProgressEvent payload
(message, key, vars); update the ProgressMessage interface to include key:
string and vars: Record<string, any> (or a more specific type if known) so it
matches the shape used when spreading the ProgressEvent into onProgress and
satisfies the unit test; adjust any related usages of ProgressMessage or
onProgress signatures accordingly (e.g., where ProgressMessage is
imported/used).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8984fcd3-fa80-42ed-8b9f-0bf63d4ff365
📒 Files selected for processing (14)
server.jsserver/runtime.jsshared/dashboard-domain.jssrc/components/cards/MonthMetrics.tsxsrc/lib/auto-import.tssrc/lib/calculations.tssrc/lib/constants.tstests/frontend/dashboard-error-state.test.tsxtests/frontend/metric-ratio-locale.test.tsxtests/integration/server.test.tstests/unit/analytics.test.tstests/unit/auto-import.test.tstests/unit/dashboard-preferences.test.tstests/unit/server-helpers.test.ts
💤 Files with no reviewable changes (1)
- src/lib/constants.ts
✅ Files skipped from review due to trivial changes (3)
- tests/frontend/metric-ratio-locale.test.tsx
- tests/frontend/dashboard-error-state.test.tsx
- shared/dashboard-domain.js
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/dashboard-preferences.test.ts
- tests/unit/server-helpers.test.ts
- src/lib/calculations.ts
- server.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/auto-import.ts (1)
163-170: Consider validating event type instead of using a fallback type assertion.Line 168 casts
(type || 'message')toStreamEventType, but'message'isn't a validStreamEventType. If an event arrives without a type field, it will silently be ignored since'message'won't match any switch case indispatchEvent.This is likely fine since the backend always sends valid event types (per the server implementation), but the fallback is misleading.
💡 Optional: Make the fallback explicit
const flushEvent = (type: string, dataLines: string[]) => { if (dataLines.length === 0) { return } - const normalizedType = (type || 'message') as StreamEventType - dispatchEvent(normalizedType, dataLines.join('\n')) + if (!type) { + return // Ignore events without a type + } + dispatchEvent(type as StreamEventType, dataLines.join('\n')) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/auto-import.ts` around lines 163 - 170, The flushEvent function currently forces (type || 'message') into StreamEventType and calls dispatchEvent which can silently ignore invalid types; update flushEvent to validate that the incoming type is a known StreamEventType before casting: check the value against the allowed enum/union members (the StreamEventType set used by dispatchEvent) and only call dispatchEvent when it matches, otherwise log or ignore the event (or explicitly set a documented default valid type), ensuring you reference flushEvent, StreamEventType and dispatchEvent 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.
Nitpick comments:
In `@src/lib/auto-import.ts`:
- Around line 163-170: The flushEvent function currently forces (type ||
'message') into StreamEventType and calls dispatchEvent which can silently
ignore invalid types; update flushEvent to validate that the incoming type is a
known StreamEventType before casting: check the value against the allowed
enum/union members (the StreamEventType set used by dispatchEvent) and only call
dispatchEvent when it matches, otherwise log or ignore the event (or explicitly
set a documented default valid type), ensuring you reference flushEvent,
StreamEventType and dispatchEvent when making the change.
This was referenced Apr 14, 2026
This was referenced Apr 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation