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:
📝 WalkthroughWalkthroughAdded async atomic server file I/O and ordered per-file/cross-process mutation locks; centralized chart transforms; introduced drill‑down previous/next navigation with keyboard support; made animations motion‑aware and lazy‑loaded many dashboard components; expanded tests, JSDoc, ESLint docstring rules, and CI docstring linting. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client
participant Bootstrap as loadBootstrapSettings
participant Server as Server API
participant App as App Component
participant Cache as React Query
Browser->>Bootstrap: bootstrap()
Bootstrap->>Server: GET /api/settings
alt settings fetched
Server-->>Bootstrap: 200 + settings
Bootstrap->>App: {settings, loadedFromServer: true, fetchedAt}
else failure
Server-->>Bootstrap: error/non-OK
Bootstrap->>App: {settings: DEFAULT, loadedFromServer: false, fetchedAt: null}
end
App->>Cache: create QueryClient (no seeded side-effect)
App->>App: pass initialSettings + metadata to Dashboard
sequenceDiagram
participant User as User
participant Dashboard as Dashboard Component
participant Sequence as drillDownSequence
participant Modal as DrillDownModal
User->>Dashboard: open drill-down for date X
Dashboard->>Sequence: derive sorted drillDownSequence from filteredData
Sequence-->>Dashboard: [d1, d2, ...]
Dashboard->>Modal: render with currentIndex, hasPrevious/hasNext, onPrevious/onNext
alt navigate previous
User->>Modal: click Previous / ArrowLeft
Modal->>Dashboard: call onPrevious()
Dashboard->>Modal: re-render with index-1
else navigate next
User->>Modal: click Next / ArrowRight
Modal->>Dashboard: call onNext()
Dashboard->>Modal: re-render with index+1
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 13
🧹 Nitpick comments (1)
src/components/charts/ChartCard.tsx (1)
62-76: Remove the dead timing props fromChartReveal.
delayanddurationare now explicitly ignored, so callers still get an API that looks configurable even though reveal timing no longer does anything.🤖 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 62 - 76, ChartReveal's API still exposes dead timing props (delay, duration) that are ignored; remove them from the ChartRevealProps interface and from the ChartReveal parameter list, delete the no-op "void delay" and "void duration" lines, and update any callers or types that relied on those props to stop passing them (or adjust calls to only pass children and variant). Specifically edit ChartRevealProps to only include children and variant, and update the ChartReveal function signature to match that interface so the component no longer advertises unused timing options.
🤖 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 318-328: The current writeJsonAtomicAsync only protects the final
rename, allowing races during the read-modify-write window; wrap the entire
read/merge/write sequence for any file in a per-file mutex/queue so concurrent
requests serialize full operations. Implement a shared file lock map (keyed by
filePath) and acquire the lock before reading/parsing the existing file,
applying merges (e.g., where recordDataLoad() or clearDataLoadState() update
state, and in the /usage/import handlers that merge usage), then perform
writeJsonAtomicAsync and release the lock; update all call sites
(recordDataLoad, clearDataLoadState and the import/merge paths) to
acquire/release the same per-file lock so no two operations can interleave their
read/merge/write cycles. Ensure locks are cleaned up after release to avoid
leaks and handle errors by releasing the lock before propagating.
In `@src/components/charts/ChartLegend.tsx`:
- Line 17: The legend uses a non-unique key (`key={`${label}-${color}`}`) which
can collide; update the key in the ChartLegend component to use a stable unique
identifier from each legend item (e.g., an `id` property on the data object) or,
if no id exists, append the map index as a last-resort disambiguator (e.g.,
include `${index}`) so keys are guaranteed unique and React diffing remains
stable; modify the mapping that renders the div (where `label` and `color` are
used) to reference the unique field (or index) instead of only label and color.
In `@src/components/Dashboard.tsx`:
- Around line 244-248: The HelpPanel is conditionally mounted inside the hasData
branch causing stale helpOpen state to disappear when data is missing; move the
helpDialog (the Suspense wrapper rendering <HelpPanel open={helpOpen}
onOpenChange={setHelpOpen} />) out of the hasData conditional so it is always
mounted at the top-level alongside other global dialogs rendered by Header,
ensuring helpOpen (controller state) is preserved across fatal-load/no-data
routes and doesn't unexpectedly reappear later.
In `@src/components/dashboard/DashboardSections.tsx`:
- Around line 283-308: The Suspense boundaries around primary dashboard sections
(e.g., the Suspense wrappers that render ExpandableCard with CostForecast and
CacheROI) use fallback={null}, causing blank gaps and layout shift while lazy
chunks load; replace those null fallbacks with a lightweight, fixed-height
placeholder or skeleton (e.g., DashboardCardSkeleton or a fixed-height <div>
spinner) that matches ExpandableCard's body height and visual affordance, and
apply the same change to the other Suspense instances referenced (lines around
the other cards) so each Suspense uses a consistent skeleton/placeholder to
preserve layout and show loading state while CostForecast, CacheROI, and similar
lazy components load.
In `@src/components/features/cache-roi/CacheROI.tsx`:
- Around line 123-127: The UI currently treats negative savings as positive;
update the styling and width calculations so negative savings render as a loss:
switch text/color classes (replace uses of withCacheTextClass/emerald styling)
based on Math.sign(savings) (use a "loss" class like text-rose for negative,
keep "gain" for positive) for the displayed value spans (references: savings,
savingsPercent, FormattedValue, formatPercent) and for the comparison bar; also
clamp barWidth to the 0–100 range (e.g., Math.max(0, Math.min(100, barWidth)))
so it cannot exceed 100% or go below 0. Apply the same class-switch and clamping
logic to the other identical blocks that use withCacheTextClass and barWidth
(the later occurrences around the other display sections).
In `@src/components/features/drill-down/DrillDownModal.tsx`:
- Around line 137-142: The UI currently hardcodes "7d" labels while
previousSeven is actually the previous 7 periods (days/months/years) — update
labeling to be period-aware by deriving the unit and count from the existing
periodKind (or active unit/count) instead of hard-coding "7d"; e.g., add a small
helper that maps periodKind -> unit string (day/month/year) and formats the
benchmark label as `${count}${unitLabel}` (use the same count used to compute
previousSeven), then replace all occurrences that render "*Average7d" (including
where previousSeven is used and the other similar blocks referenced) to use this
helper so the label correctly reflects days/months/years.
In `@src/components/features/heatmap/HeatmapCalendar.tsx`:
- Around line 342-360: The rect elements are marked role="button" but do not
provide activation (no onClick/Enter/Space), which misleads screen reader users;
update the HeatmapCalendar cells to use a non-actionable composite pattern by
removing role="button" and the cursor-pointer visual affordance and instead use
role="gridcell" (or presentation) for the rect, keep the roving focus logic
(dayButtonRefs, focusedDate) and tabIndex as-is, and ensure handleCellKeyDown
only handles navigation (or update it to also handle Enter/Space if you choose
to add real activation). This change ensures accessibility: remove role="button"
from the rect, remove/replace the "cursor-pointer" class, and keep
aria-label/aria-current and onKeyDown for navigation only (or implement onClick
plus Enter/Space handling in handleCellKeyDown if you opt to make cells
actionable).
In `@src/hooks/use-app-settings.ts`:
- Around line 31-37: The hook currently sets initialDataUpdatedAt using
Date.now() when initialSettingsFresh is true, which uses hook mount time;
instead read a bootstrap fetch timestamp carried on the initial settings object
(e.g., initialSettings.fetchedAt or a similarly named bootstrap timestamp) and
pass that into useQuery's initialDataUpdatedAt; update the useQuery call in
use-app-settings.ts (the query config around initialDataUpdatedAt,
initialSettings, initialSettingsFresh and queryKey/useQuery) to prefer
initialSettings.fetchedAt (fallback to 0 or the existing fallback logic) so the
cache age reflects when the bootstrap data was actually fetched.
In `@src/lib/data-transforms.ts`:
- Around line 76-88: The weekday labels stay in the old locale because
createWeekdayLabels() calls getCurrentLocale() internally while
buildDashboardChartTransforms(data) is memoized only on data; change
createWeekdayLabels to accept a locale parameter (e.g., locale: string) and
update buildDashboardChartTransforms to accept and pass that locale into
createWeekdayLabels (and any other locale-using helpers), then update the caller
in use-computed-metrics.ts (the useMemo that calls
buildDashboardChartTransforms) to pass the current locale and include the locale
in the useMemo dependency array so weekdayData[].day refreshes on language
change.
In `@tests/frontend/drill-down-modal.test.tsx`:
- Around line 240-249: The test assertion is wrong: the simulated ArrowRight
keydown with shiftKey should trigger the same handler and increment onNext, so
update the expectation for onNext in tests/frontend/drill-down-modal.test.tsx
(referencing the onNext mock used in this test) from toHaveBeenCalledTimes(2) to
toHaveBeenCalledTimes(3); leave the onPrevious assertion unchanged.
In `@tests/frontend/motion-accessibility.test.tsx`:
- Around line 13-38: The test setup stubs global IntersectionObserver and
matchMedia in the beforeEach block but never restores them; add an afterEach
that calls vi.unstubAllGlobals() (e.g., add afterEach(() =>
vi.unstubAllGlobals())) so the stubbed globals are removed between tests and do
not leak into other specs; place this after the existing beforeEach/await
initI18n('en') setup in the same test suite (referencing the beforeEach that
stubs IntersectionObserver and matchMedia).
In `@tests/frontend/sortable-tables.test.tsx`:
- Around line 153-191: The test times out because RecentDays progressively
reveals rows using requestAnimationFrame; mock rAF in this test so callbacks run
synchronously (or use vi.useFakeTimers and advance timers) before asserting. In
the test that renders RecentDays (the it block that calls renderWithProviders
and fireEvent.click for "show all"), install a temporary
global.requestAnimationFrame = cb => { cb(0); return 0 } (or wrap with
vi.useFakeTimers() and vi.advanceTimersToNextTimer()/advanceTimersByTime to
drive frames), ensure you restore the original rAF after the test, then await
waitFor assertions for "Showing 35 of 35 days" and "show less".
In `@tests/frontend/toast.test.tsx`:
- Around line 19-22: The test setup enables fake timers in the beforeEach
(vi.useFakeTimers) which can leak into other tests; add a corresponding
afterEach hook that restores real timers by calling vi.useRealTimers() (e.g.,
afterEach(() => vi.useRealTimers())) so the timer state is reset after each test
and does not affect other suites; update tests/frontend/toast.test.tsx to
include this afterEach alongside the existing beforeEach/initI18n setup.
---
Nitpick comments:
In `@src/components/charts/ChartCard.tsx`:
- Around line 62-76: ChartReveal's API still exposes dead timing props (delay,
duration) that are ignored; remove them from the ChartRevealProps interface and
from the ChartReveal parameter list, delete the no-op "void delay" and "void
duration" lines, and update any callers or types that relied on those props to
stop passing them (or adjust calls to only pass children and variant).
Specifically edit ChartRevealProps to only include children and variant, and
update the ChartReveal function signature to match that interface so the
component no longer advertises unused timing options.
🪄 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: 20f49ac6-0ec2-49c2-a2d8-a9eeb9958566
📒 Files selected for processing (64)
CHANGELOG.mdserver.jsserver/report/index.jssrc/App.tsxsrc/components/Dashboard.tsxsrc/components/cards/MetricCard.tsxsrc/components/charts/ChartCard.tsxsrc/components/charts/ChartLegend.tsxsrc/components/charts/CorrelationAnalysis.tsxsrc/components/charts/CostByModel.tsxsrc/components/charts/CostByModelOverTime.tsxsrc/components/charts/CostOverTime.tsxsrc/components/charts/DistributionAnalysis.tsxsrc/components/charts/RequestCacheHitRateByModel.tsxsrc/components/charts/RequestsOverTime.tsxsrc/components/charts/chart-theme.tssrc/components/dashboard/DashboardSections.tsxsrc/components/features/animations/FadeIn.tsxsrc/components/features/cache-roi/CacheROI.tsxsrc/components/features/drill-down/DrillDownModal.tsxsrc/components/features/forecast/CostForecast.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/features/help/InfoButton.tsxsrc/components/features/limits/ProviderLimitsSection.tsxsrc/components/features/pdf-report/PDFReport.tsxsrc/components/features/request-quality/RequestQuality.tsxsrc/components/features/settings/SettingsModal.tsxsrc/components/layout/FilterBar.tsxsrc/components/layout/Header.tsxsrc/components/tables/RecentDays.tsxsrc/components/ui/card.tsxsrc/components/ui/dialog.tsxsrc/components/ui/expandable-card.tsxsrc/components/ui/select.tsxsrc/components/ui/toast.tsxsrc/components/ui/tooltip.tsxsrc/hooks/use-app-settings.tssrc/hooks/use-computed-metrics.tssrc/hooks/use-dashboard-controller.tssrc/hooks/use-dashboard-filters.tssrc/lib/api.tssrc/lib/data-transforms.tssrc/lib/motion.tssrc/locales/de/common.jsonsrc/locales/en/common.jsonsrc/main.tsxtests/e2e/dashboard.spec.tstests/frontend/chart-card.test.tsxtests/frontend/cost-by-model.test.tsxtests/frontend/cost-over-time.test.tsxtests/frontend/drill-down-modal.test.tsxtests/frontend/expandable-card.test.tsxtests/frontend/filter-bar.test.tsxtests/frontend/header-links.test.tsxtests/frontend/heatmap-calendar.test.tsxtests/frontend/info-heading.test.tsxtests/frontend/motion-accessibility.test.tsxtests/frontend/phase4-correctness.test.tsxtests/frontend/react-query-hooks.test.tsxtests/frontend/settings-modal.test.tsxtests/frontend/sortable-tables.test.tsxtests/frontend/toast.test.tsxtests/unit/api.test.tstests/unit/insights-localization.test.ts
tyl3r-ch
changed the title
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/layout/FilterBar.tsx (1)
461-483:⚠️ Potential issue | 🟠 MajorGive each day button a full accessible name.
Right now these buttons are exposed as just
"1","2","3", etc. Once focus starts moving around the picker, screen readers lose the date context. Add a full localizedaria-labelfor each day and mark today witharia-current="date".Suggested fix
const iso = toLocalDateStr(day) const isSelected = value === iso const isToday = iso === today + const dayLabel = formatDate(iso, 'long') return ( <button key={iso} ref={(node) => { if (node) dayButtonRefs.current.set(iso, node) else dayButtonRefs.current.delete(iso) }} type="button" tabIndex={focusedDate === iso ? 0 : -1} + aria-label={dayLabel} + aria-current={isToday ? 'date' : undefined} aria-pressed={isSelected} onFocus={() => setFocusedDate(iso)} onKeyDown={(event) => handleDayKeyDown(event, iso)} onClick={() => selectDate(iso)}src/components/features/heatmap/HeatmapCalendar.tsx (1)
301-361:⚠️ Potential issue | 🟠 Major
gridcellneeds a real grid ancestor.The cells now expose
role="gridcell", but the SVG never exposes arole="grid"/rowhierarchy. That leaves the ARIA tree invalid, so assistive tech won't get the keyboard model you're implementing here. Either add the missing grid structure around the weeks/days or switch the cells back to a non-grid role.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/heatmap/HeatmapCalendar.tsx` around lines 301 - 361, The ARIA tree is invalid because the SVG element and week/day groups don't expose the required grid/row roles while each cell uses role="gridcell"; fix by either (A) making the SVG a true grid: add role="grid" and appropriate aria-rowcount/aria-colcount to the <svg> (the element rendering months/days), wrap each week or day group (the <g> elements produced in the cells map or around cells grouped by week) with role="row" and move role="gridcell" to each <rect> so keyboard handling in handleCellKeyDown and focus via dayButtonRefs still works, or (B) revert to a non-grid pattern: remove role="gridcell" from the <rect> in the cells.map block and replace it with role="button" (or no role) and ensure keyboard handlers (handleCellKeyDown) and tabIndex logic remain intact; choose one approach and apply consistently to the SVG, the <g> groups and the <rect> elements referenced in the cells.map code.src/components/layout/Header.tsx (1)
148-163:⚠️ Potential issue | 🟠 MajorAdd explicit accessible names to the icon-only buttons.
On Line 148 and Line 198, these controls only expose
title. That is not a reliable accessible name, so screen readers can miss the help/theme actions. Addaria-labelalongside the existingtitle.♿ Proposed fix
<Button variant="ghost" size="icon" onClick={() => onHelpOpenChange(true)} + aria-label={t('header.help')} title={t('header.help')} > <CircleHelp className="h-4 w-4" /> </Button> <Button variant="ghost" size="icon" onClick={onToggleTheme} + aria-label={t('commandPalette.commands.themeDark.description')} title={t('commandPalette.commands.themeDark.description')} > {isDark ? <Sun className="h-4 w-4" /> : <Moon className="h-4 w-4" />} </Button>Also applies to: 198-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/Header.tsx` around lines 148 - 163, The icon-only Button components rendering CircleHelp and Sun/Moon lack reliable accessible names; update the Button usages that call onHelpOpenChange and onToggleTheme (the ones using t('header.help') and t('commandPalette.commands.themeDark.description')) to include an explicit aria-label prop matching the title text (e.g., aria-label={t('header.help')} and aria-label={t('commandPalette.commands.themeDark.description')}) so screen readers receive a proper name while keeping the existing title and behavior; ensure both the help button and the theme toggle (which checks isDark) receive the aria-label.
♻️ Duplicate comments (1)
tests/frontend/sortable-tables.test.tsx (1)
158-194:⚠️ Potential issue | 🟡 MinorThis never exercises the
requestAnimationFramebranch.
RecentDaysonly switches to the progressive path once the dataset exceedsDEFAULT_VISIBLE_ROWS + SHOW_ALL_BATCH_SIZE(150 rows). With 31 rows, this test only verifies the toggle text, not the batching logic added in this PR. Please bump the fixture above 150 rows and driverequestAnimationFrameexplicitly. As per coding guidelines, "Prefer focused*.test.tsor*.test.tsxcoverage for data transforms, hooks, or complex UI behavior".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/sortable-tables.test.tsx` around lines 158 - 194, The test never exercises the requestAnimationFrame progressive-path in RecentDays; increase the generated days fixture to exceed DEFAULT_VISIBLE_ROWS + SHOW_ALL_BATCH_SIZE (i.e., >150) so the component takes the batching branch, then explicitly flush the rAF callbacks after clicking "Show all" (e.g., mock window.requestAnimationFrame and invoke queued callbacks or use act to await a promise resolved via requestAnimationFrame) so the progressive reveal runs; update references to the RecentDays component, DEFAULT_VISIBLE_ROWS and SHOW_ALL_BATCH_SIZE in the test to locate where to change the array length and add the rAF flush.
🧹 Nitpick comments (4)
src/lib/data-transforms.ts (1)
294-296: Make the locale dependency explicit intoWeekdayData().The hook-side fix is good, but this wrapper still falls back to global locale state. Any caller that memoizes
toWeekdayData(data)only ondatacan accidentally reintroduce stale weekday labels. Accepting an optionallocalehere would keep the dependency visible.♻️ Proposed refactor
-export function toWeekdayData(data: DailyUsage[]): WeekdayData[] { - return buildDashboardChartTransforms(data).weekdayData +export function toWeekdayData( + data: DailyUsage[], + locale = getCurrentLocale(), +): WeekdayData[] { + return buildDashboardChartTransforms(data, locale).weekdayData }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/data-transforms.ts` around lines 294 - 296, The toWeekdayData function currently hides a global locale dependency by calling buildDashboardChartTransforms(data) without exposing locale; change toWeekdayData to accept an optional locale parameter (e.g., locale?: string) and pass it through to buildDashboardChartTransforms (call buildDashboardChartTransforms(data, locale)) so weekday labels are derived from the provided locale; update any callers that memoize toWeekdayData(data) to include the locale argument where needed to avoid stale labels.src/components/layout/Header.tsx (1)
184-197: Expose which language is currently selected.These buttons are visually stateful, but assistive tech does not get that state. Adding
aria-pressed={currentLanguage === language}would make the current selection clear without changing the UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/Header.tsx` around lines 184 - 197, The language buttons in Header.tsx are visually stateful but lack an accessibility state; update the button rendering inside the map (the element using currentLanguage and onLanguageChange) to include aria-pressed={currentLanguage === language} so assistive tech can detect which language is selected; ensure the attribute is added to the same <button> that currently uses key={language}, onClick={() => onLanguageChange(language)} and title={t(`app.languages.${language}`)}.src/components/Dashboard.tsx (1)
196-204:hasPreviousDrillDownandhasNextDrillDownin callback dependencies are redundant.Both
handleDrillDownPreviousandhandleDrillDownNextincludehasPreviousDrillDown/hasNextDrillDownin their dependency arrays, but these are derived fromdrillDownIndexanddrillDownSequence.lengthwhich are already dependencies. This won't cause bugs, but it's slightly redundant.♻️ Optional cleanup
const handleDrillDownPrevious = useCallback(() => { if (!hasPreviousDrillDown) return setDrillDownDate(drillDownSequence[drillDownIndex - 1]?.date ?? null) - }, [drillDownIndex, drillDownSequence, hasPreviousDrillDown, setDrillDownDate]) + }, [drillDownIndex, drillDownSequence, setDrillDownDate]) const handleDrillDownNext = useCallback(() => { if (!hasNextDrillDown) return setDrillDownDate(drillDownSequence[drillDownIndex + 1]?.date ?? null) - }, [drillDownIndex, drillDownSequence, hasNextDrillDown, setDrillDownDate]) + }, [drillDownIndex, drillDownSequence, setDrillDownDate])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Dashboard.tsx` around lines 196 - 204, The callbacks handleDrillDownPrevious and handleDrillDownNext include redundant dependencies hasPreviousDrillDown and hasNextDrillDown; remove those two from the useCallback dependency arrays and leave only drillDownIndex, drillDownSequence, and setDrillDownDate so the hooks depend on the actual sources of truth (drillDownIndex and drillDownSequence.length) used to compute those booleans.src/components/features/drill-down/DrillDownModal.tsx (1)
838-852: Consider addingaria-labelto token distribution segments.The token distribution bar segments use
titlefor tooltips, but screen readers may not announce them reliably. Addingaria-labelalongsidetitlewould improve accessibility.🔧 Suggested improvement
<div key={segment.id} className="h-full transition-all duration-500" style={{ width: `${(segment.value / tokensTotal) * 100}%`, backgroundColor: segment.color, }} title={`${segment.label}: ${formatTokens(segment.value)} (${((segment.value / tokensTotal) * 100).toFixed(1)}%)`} + aria-label={`${segment.label}: ${formatTokens(segment.value)} (${((segment.value / tokensTotal) * 100).toFixed(1)}%)`} + role="img" />🤖 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 838 - 852, Add an accessible label to each token segment div in DrillDownModal by adding an aria-label with the same descriptive text used for the title (e.g., `${segment.label}: ${formatTokens(segment.value)} (${((segment.value / tokensTotal) * 100).toFixed(1)}%)`) inside the tokenSegments.map callback where the div with className "h-full transition-all duration-500" is rendered (the block guarded by hasTokens). Keep the existing title for hover tooltips and ensure the aria-label is a simple string so screen readers announce the token distribution segments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 3: The changelog header currently reads "## [6.2.1] - 2026-04-15";
replace that line with an unreleased header (e.g., "## [Unreleased]" or "##
[6.2.1] - Unreleased") until the release is actually cut, and when publishing
change it back to "## [6.2.1] - 2026-04-15"; update the same header line in
CHANGELOG.md where "## [6.2.1] - 2026-04-15" appears.
In `@server.js`:
- Around line 1484-1509: recordDataLoad and clearDataLoadState only lock
SETTINGS_FILE via withFileMutationLock, which allows mutations to data.json
(e.g., delete/import) to interleave and leave lastLoadedAt/lastLoadSource
inconsistent; fix by introducing a higher-level helper that acquires both locks
in a fixed global order (e.g., lock SETTINGS_FILE then DATA_FILE via
withFileMutationLock or a new withTwoFileMutationLocks helper) and use that
helper for recordDataLoad, clearDataLoadState (and the other affected call sites
around lines noted) so the settings write (readSettingsForWrite, writeSettings,
toSettingsResponse) happens inside the same critical section that mutates or
validates data.json.
- Around line 319-329: The writeJsonAtomicAsync function can leave the temp file
(tempPath) behind if mkdir, writeFile, chmod, or rename throw; wrap the
create/write/chmod/rename sequence in a try/catch/finally (or try/finally) so
that on any error you attempt to remove tempPath via fsPromises.unlink
(guarding/ignoring ENOENT) before rethrowing the original error; update
references in writeJsonAtomicAsync to ensure tempPath is only unlinked when
created and that failures to unlink do not mask the primary error.
In `@src/components/cards/SecondaryMetrics.tsx`:
- Line 76: In the SecondaryMetrics component change the JSX string literal for
the container div's className from double quotes to single quotes: locate the
div in SecondaryMetrics.tsx with "grid grid-cols-2 gap-3 lg:grid-cols-4" and
replace the double-quoted className value with a single-quoted string so it
follows the repo's single-quote rule for TypeScript/React JSX.
In `@src/components/features/anomaly/AnomalyDetection.tsx`:
- Around line 74-85: The button rendered inside AnomalyDetection currently lacks
a visible keyboard focus state and remains interactive when onClickDay is
undefined; update the element so it adds accessible focus styles and a
non-interactive disabled state: augment cardClasses (and the button props where
the element is created) to include focus-ring classes (e.g., focus:outline-none,
focus:ring-<color>/<opacity>, focus:ring-offset) and disabled utility classes
(e.g., disabled:opacity-50, disabled:cursor-not-allowed) and set the button's
disabled attribute and aria-disabled when onClickDay is falsy (while preventing
onClick from being called), ensuring the same changes are applied to the other
button instance around line 114.
In `@src/components/features/request-quality/RequestQuality.tsx`:
- Around line 83-89: In the RequestQuality component, the progress bar width
currently uses Math.max(item.progress * 100, 6) which still shows a non-zero
fill when metrics.hasRequestData is false; change the width expression to check
metrics.hasRequestData as well (use inView || shouldReduceMotion &&
metrics.hasRequestData or equivalent) so that when metrics.hasRequestData is
false the width becomes '0%'; update the style calculation that references
item.progress and item.accent to only apply the minimum 6% clamp when
metrics.hasRequestData is true while preserving the existing inView and
shouldReduceMotion gating.
In `@src/components/tables/RecentDays.tsx`:
- Around line 36-44: getUniqueModelsForDay currently deduplicates only by
normalized model name, causing different providers with the same name to
collapse; update the filter in getUniqueModelsForDay to treat uniqueness as the
tuple (name, provider) by comparing both normalizeModelName(mb.modelName) and
getModelProvider(mb.modelName) in the findIndex predicate so entries are kept
distinct (this also stabilizes later render keys such as key={name} by allowing
you to use a compound key if needed).
- Around line 501-512: The table rows rendered in RecentDays.tsx are clickable
via the onClickDay handler but not keyboard-accessible; update the <tr> rendered
inside the rowData.map to be focusable and handle keyboard activation: add
tabIndex={0} and role="button" (or role="row" with an inner interactive element)
and implement an onKeyDown handler that calls onClickDay(day.date) when Enter or
Space is pressed (ensure Space calls preventDefault to avoid scrolling). Keep
existing styling and use the same onClickDay(day.date) invocation so
getDeferredRowStyle and the rest of the row logic remain unchanged.
In `@vitest.setup.ts`:
- Around line 43-49: The afterEach cleanup currently calls vi.useRealTimers(),
vi.restoreAllMocks(), and cleanup(); add a call to vi.unstubAllGlobals() in this
afterEach (alongside vi.restoreAllMocks()) to ensure globals stubbed via
vi.stubGlobal(...) (e.g., IntersectionObserver stubs used in tests like
tests/frontend/heatmap-calendar.test.tsx) are removed and do not leak between
tests; place the vi.unstubAllGlobals() call in the same afterEach block where
vi.restoreAllMocks() and cleanup() are invoked.
---
Outside diff comments:
In `@src/components/features/heatmap/HeatmapCalendar.tsx`:
- Around line 301-361: The ARIA tree is invalid because the SVG element and
week/day groups don't expose the required grid/row roles while each cell uses
role="gridcell"; fix by either (A) making the SVG a true grid: add role="grid"
and appropriate aria-rowcount/aria-colcount to the <svg> (the element rendering
months/days), wrap each week or day group (the <g> elements produced in the
cells map or around cells grouped by week) with role="row" and move
role="gridcell" to each <rect> so keyboard handling in handleCellKeyDown and
focus via dayButtonRefs still works, or (B) revert to a non-grid pattern: remove
role="gridcell" from the <rect> in the cells.map block and replace it with
role="button" (or no role) and ensure keyboard handlers (handleCellKeyDown) and
tabIndex logic remain intact; choose one approach and apply consistently to the
SVG, the <g> groups and the <rect> elements referenced in the cells.map code.
In `@src/components/layout/Header.tsx`:
- Around line 148-163: The icon-only Button components rendering CircleHelp and
Sun/Moon lack reliable accessible names; update the Button usages that call
onHelpOpenChange and onToggleTheme (the ones using t('header.help') and
t('commandPalette.commands.themeDark.description')) to include an explicit
aria-label prop matching the title text (e.g., aria-label={t('header.help')} and
aria-label={t('commandPalette.commands.themeDark.description')}) so screen
readers receive a proper name while keeping the existing title and behavior;
ensure both the help button and the theme toggle (which checks isDark) receive
the aria-label.
---
Duplicate comments:
In `@tests/frontend/sortable-tables.test.tsx`:
- Around line 158-194: The test never exercises the requestAnimationFrame
progressive-path in RecentDays; increase the generated days fixture to exceed
DEFAULT_VISIBLE_ROWS + SHOW_ALL_BATCH_SIZE (i.e., >150) so the component takes
the batching branch, then explicitly flush the rAF callbacks after clicking
"Show all" (e.g., mock window.requestAnimationFrame and invoke queued callbacks
or use act to await a promise resolved via requestAnimationFrame) so the
progressive reveal runs; update references to the RecentDays component,
DEFAULT_VISIBLE_ROWS and SHOW_ALL_BATCH_SIZE in the test to locate where to
change the array length and add the rAF flush.
---
Nitpick comments:
In `@src/components/Dashboard.tsx`:
- Around line 196-204: The callbacks handleDrillDownPrevious and
handleDrillDownNext include redundant dependencies hasPreviousDrillDown and
hasNextDrillDown; remove those two from the useCallback dependency arrays and
leave only drillDownIndex, drillDownSequence, and setDrillDownDate so the hooks
depend on the actual sources of truth (drillDownIndex and
drillDownSequence.length) used to compute those booleans.
In `@src/components/features/drill-down/DrillDownModal.tsx`:
- Around line 838-852: Add an accessible label to each token segment div in
DrillDownModal by adding an aria-label with the same descriptive text used for
the title (e.g., `${segment.label}: ${formatTokens(segment.value)}
(${((segment.value / tokensTotal) * 100).toFixed(1)}%)`) inside the
tokenSegments.map callback where the div with className "h-full transition-all
duration-500" is rendered (the block guarded by hasTokens). Keep the existing
title for hover tooltips and ensure the aria-label is a simple string so screen
readers announce the token distribution segments.
In `@src/components/layout/Header.tsx`:
- Around line 184-197: The language buttons in Header.tsx are visually stateful
but lack an accessibility state; update the button rendering inside the map (the
element using currentLanguage and onLanguageChange) to include
aria-pressed={currentLanguage === language} so assistive tech can detect which
language is selected; ensure the attribute is added to the same <button> that
currently uses key={language}, onClick={() => onLanguageChange(language)} and
title={t(`app.languages.${language}`)}.
In `@src/lib/data-transforms.ts`:
- Around line 294-296: The toWeekdayData function currently hides a global
locale dependency by calling buildDashboardChartTransforms(data) without
exposing locale; change toWeekdayData to accept an optional locale parameter
(e.g., locale?: string) and pass it through to buildDashboardChartTransforms
(call buildDashboardChartTransforms(data, locale)) so weekday labels are derived
from the provided locale; update any callers that memoize toWeekdayData(data) to
include the locale argument where needed to avoid stale labels.
🪄 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: d3d27426-6349-47c3-a25c-e275b28d7bd5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (122)
.github/workflows/ci.yml.github/workflows/release.yml.gitignore.prettierrc.jsonCHANGELOG.mdRELEASING.mdeslint.config.mjspackage.jsonserver.jsshared/dashboard-domain.d.tsshared/dashboard-domain.jsshared/dashboard-types.d.tsshared/model-colors.d.tsshared/model-colors.jssrc/App.tsxsrc/components/Dashboard.tsxsrc/components/EmptyState.tsxsrc/components/LoadErrorState.tsxsrc/components/cards/MetricCard.tsxsrc/components/cards/MonthMetrics.tsxsrc/components/cards/PrimaryMetrics.tsxsrc/components/cards/SecondaryMetrics.tsxsrc/components/cards/TodayMetrics.tsxsrc/components/charts/ChartCard.tsxsrc/components/charts/ChartLegend.tsxsrc/components/charts/CorrelationAnalysis.tsxsrc/components/charts/CostByModel.tsxsrc/components/charts/CostByModelOverTime.tsxsrc/components/charts/CostByWeekday.tsxsrc/components/charts/CostOverTime.tsxsrc/components/charts/CumulativeCost.tsxsrc/components/charts/CustomTooltip.tsxsrc/components/charts/DistributionAnalysis.tsxsrc/components/charts/ModelMix.tsxsrc/components/charts/RequestCacheHitRateByModel.tsxsrc/components/charts/RequestsOverTime.tsxsrc/components/charts/TokenEfficiency.tsxsrc/components/charts/TokenTypes.tsxsrc/components/charts/TokensOverTime.tsxsrc/components/charts/chart-theme.tssrc/components/dashboard/DashboardSections.tsxsrc/components/features/animations/CountUp.tsxsrc/components/features/animations/FadeIn.tsxsrc/components/features/anomaly/AnomalyDetection.tsxsrc/components/features/auto-import/AutoImportModal.tsxsrc/components/features/cache-roi/CacheROI.tsxsrc/components/features/command-palette/CommandPalette.tsxsrc/components/features/comparison/PeriodComparison.tsxsrc/components/features/drill-down/DrillDownModal.tsxsrc/components/features/forecast/CostForecast.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/features/help/HelpPanel.tsxsrc/components/features/help/InfoButton.tsxsrc/components/features/help/InfoHeading.tsxsrc/components/features/insights/UsageInsights.tsxsrc/components/features/limits/ProviderLimitsSection.tsxsrc/components/features/pdf-report/PDFReport.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/badge.tsxsrc/components/ui/button.tsxsrc/components/ui/card.tsxsrc/components/ui/dialog.tsxsrc/components/ui/expandable-card.tsxsrc/components/ui/formatted-value.tsxsrc/components/ui/section-header.tsxsrc/components/ui/select.tsxsrc/components/ui/skeleton.tsxsrc/components/ui/toast.tsxsrc/components/ui/tooltip.tsxsrc/hooks/use-app-settings.tssrc/hooks/use-computed-metrics.tssrc/hooks/use-dashboard-controller.tssrc/hooks/use-dashboard-filters.tssrc/hooks/use-provider-limits.tssrc/hooks/use-theme.tssrc/hooks/use-usage-data.tssrc/lib/api.tssrc/lib/app-settings.tssrc/lib/auto-import.tssrc/lib/calculations.tssrc/lib/cn.tssrc/lib/constants.tssrc/lib/csv-export.tssrc/lib/csv.tssrc/lib/dashboard-preferences.tssrc/lib/data-transforms.tssrc/lib/formatters.tssrc/lib/help-content.tssrc/lib/i18n.tssrc/lib/model-utils.tssrc/lib/motion.tssrc/lib/provider-limits.tssrc/locales/de/common.jsonsrc/locales/en/common.jsonsrc/main.tsxsrc/types/index.tstests/frontend/cache-roi.test.tsxtests/frontend/chart-card.test.tsxtests/frontend/dashboard-error-state.test.tsxtests/frontend/drill-down-modal.test.tsxtests/frontend/expandable-card.test.tsxtests/frontend/filter-bar.test.tsxtests/frontend/header-links.test.tsxtests/frontend/heatmap-calendar.test.tsxtests/frontend/info-heading.test.tsxtests/frontend/metric-ratio-locale.test.tsxtests/frontend/motion-accessibility.test.tsxtests/frontend/phase4-correctness.test.tsxtests/frontend/react-query-hooks.test.tsxtests/frontend/sortable-tables.test.tsxtests/frontend/toast.test.tsxtests/unit/api.test.tstests/unit/code-rabbit-phase4.test.tstests/unit/server-helpers.test.tsvitest.setup.ts
✅ Files skipped from review due to trivial changes (51)
- .prettierrc.json
- src/components/charts/CostByWeekday.tsx
- .gitignore
- shared/model-colors.d.ts
- src/components/features/animations/CountUp.tsx
- shared/dashboard-domain.d.ts
- src/lib/i18n.ts
- src/hooks/use-provider-limits.ts
- src/lib/csv.ts
- src/lib/csv-export.ts
- src/components/cards/TodayMetrics.tsx
- src/components/ui/button.tsx
- shared/dashboard-types.d.ts
- src/lib/cn.ts
- src/components/features/animations/FadeIn.tsx
- src/lib/provider-limits.ts
- src/lib/constants.ts
- shared/model-colors.js
- src/hooks/use-usage-data.ts
- tests/frontend/metric-ratio-locale.test.tsx
- src/lib/app-settings.ts
- src/components/features/pdf-report/PDFReport.tsx
- src/components/LoadErrorState.tsx
- src/components/cards/MonthMetrics.tsx
- src/components/features/help/InfoButton.tsx
- src/lib/model-utils.ts
- tests/unit/code-rabbit-phase4.test.ts
- src/lib/dashboard-preferences.ts
- src/components/ui/badge.tsx
- src/components/features/auto-import/AutoImportModal.tsx
- shared/dashboard-domain.js
- src/components/cards/PrimaryMetrics.tsx
- src/components/features/insights/UsageInsights.tsx
- src/lib/formatters.ts
- src/components/features/help/HelpPanel.tsx
- src/components/ui/formatted-value.tsx
- src/components/tables/ProviderEfficiency.tsx
- src/components/charts/CustomTooltip.tsx
- src/components/charts/chart-theme.ts
- src/components/features/comparison/PeriodComparison.tsx
- src/components/features/command-palette/CommandPalette.tsx
- src/components/ui/section-header.tsx
- src/lib/motion.ts
- src/lib/calculations.ts
- src/lib/auto-import.ts
- src/components/charts/CumulativeCost.tsx
- tests/frontend/drill-down-modal.test.tsx
- src/components/tables/ModelEfficiency.tsx
- src/components/features/help/InfoHeading.tsx
- src/types/index.ts
- src/hooks/use-theme.ts
🚧 Files skipped from review as they are similar to previous changes (32)
- tests/frontend/header-links.test.tsx
- tests/frontend/info-heading.test.tsx
- src/components/charts/DistributionAnalysis.tsx
- src/components/charts/CostByModelOverTime.tsx
- tests/frontend/expandable-card.test.tsx
- tests/frontend/chart-card.test.tsx
- tests/frontend/react-query-hooks.test.tsx
- src/components/cards/MetricCard.tsx
- src/components/features/limits/ProviderLimitsSection.tsx
- tests/unit/api.test.ts
- src/components/ui/tooltip.tsx
- src/components/features/forecast/CostForecast.tsx
- src/hooks/use-dashboard-filters.ts
- tests/frontend/toast.test.tsx
- src/components/dashboard/DashboardSections.tsx
- src/hooks/use-app-settings.ts
- src/components/ui/dialog.tsx
- src/components/ui/select.tsx
- src/components/ui/expandable-card.tsx
- tests/frontend/filter-bar.test.tsx
- src/components/charts/ChartLegend.tsx
- tests/frontend/motion-accessibility.test.tsx
- src/main.tsx
- src/App.tsx
- src/components/features/settings/SettingsModal.tsx
- src/components/ui/card.tsx
- src/components/features/cache-roi/CacheROI.tsx
- tests/frontend/phase4-correctness.test.tsx
- src/components/charts/CostByModel.tsx
- src/locales/en/common.json
- src/components/charts/RequestsOverTime.tsx
- src/components/ui/toast.tsx
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/features/drill-down/DrillDownModal.tsx (1)
255-262:⚠️ Potential issue | 🟠 MajorDon’t rank “top request” metrics when request counts are unavailable.
If imported data lacks request counts, these branches still compute a positive
requestRankingand pick atopRequestModelfrom all-zero values. The cards below then show a fake#1and an arbitrary “top request model” instead of an unavailable state.💡 Proposed fix
+ const hasRequestData = + day.requestCount > 0 || day.modelBreakdowns.some((mb) => mb.requestCount > 0) + const costRanking = [...contextData] .sort((a, b) => b.totalCost - a.totalCost) .findIndex((entry) => entry.date === day.date) + 1 - const requestRanking = - [...contextData] - .sort((a, b) => b.requestCount - a.requestCount) - .findIndex((entry) => entry.date === day.date) + 1 + const requestRanking = hasRequestData + ? [...contextData] + .sort((a, b) => b.requestCount - a.requestCount) + .findIndex((entry) => entry.date === day.date) + 1 + : 0 @@ - const topRequestModel = modelData.reduce( - (best, current) => (!best || current.requests > best.requests ? current : best), - null as (typeof modelData)[number] | null, - ) + const topRequestModel = hasRequestData + ? modelData.reduce( + (best, current) => (!best || current.requests > best.requests ? current : best), + null as (typeof modelData)[number] | null, + ) + : nullAlso applies to: 295-302
🤖 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 255 - 262, The requestRanking and related "top request" logic currently compute a rank and pick a model even when request counts are missing or all zero; modify the code around requestRanking (and the later topRequestModel logic) to first detect whether request counts exist (e.g., const hasRequestCounts = contextData.some(c => typeof c.requestCount === 'number' && c.requestCount > 0)), and only compute the sorted rank and select a topRequestModel when hasRequestCounts is true; otherwise set requestRanking/topRequestModel to null/undefined (or an explicit "unavailable" state) so the UI can render an unavailable state instead of showing `#1` or an arbitrary model. Ensure you update both the requestRanking calculation and the later selection code that uses request counts.tests/unit/code-rabbit-phase4.test.ts (1)
119-141:⚠️ Potential issue | 🟡 MinorThis test doesn’t actually cover the stale-label regression.
Because it calls
buildDashboardChartTransforms()twice with explicit locale strings, it will still pass even if the memoized caller stops depending onlocaleagain. The stale-label bug lived in the memoized path, so this should either exercisesrc/hooks/use-computed-metrics.ts:12directly or stay as a pure transform test and drop theinitI18n('de')side effect while asserting labels that actually differ between locales. As per coding guidelines, "Prefer focused*.test.tsor*.test.tsxcoverage for data transforms, hooks, or complex UI behavior".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/code-rabbit-phase4.test.ts` around lines 119 - 141, The test currently masks the stale-label regression by calling buildDashboardChartTransforms() with explicit locale strings and mutating i18n; change it to directly exercise the memoized code path in src/hooks/use-computed-metrics.ts (around the function referenced at line 12) or convert the test into a pure transform test that does not call initI18n; specifically either (A) import and call the memoized function from use-computed-metrics (or render the hook with a test renderer) to ensure the memoization depends on locale, or (B) remove initI18n('de') and assert weekday labels for two locales that actually differ when passed into buildDashboardChartTransforms('en-US') vs buildDashboardChartTransforms('de-CH'), and update assertions accordingly; make sure to reference and update tests to use the function name buildDashboardChartTransforms and the hook name/useComputedMetrics path so the memoized branch is covered.
♻️ Duplicate comments (1)
server.js (1)
319-346:⚠️ Potential issue | 🟠 MajorClean up temp files on failed writes too.
Because
tempPathCreatedis only flipped at Line 328, any exception from the write step skips cleanup entirely. If the temp file has already been created/truncated by then, this still leaves stale JSON on disk.In Node.js, can `fs.promises.writeFile()` create or truncate a file before the promise resolves, and can a rejected write leave a partially written file on disk?🧹 Proposed fix
async function writeJsonAtomicAsync(filePath, data) { const tempPath = `${filePath}.${process.pid}.${Date.now()}.tmp`; let tempPathCreated = false; try { await fsPromises.mkdir(path.dirname(filePath), { recursive: true, mode: SECURE_DIR_MODE }); + tempPathCreated = true; await fsPromises.writeFile(tempPath, JSON.stringify(data, null, 2), { mode: SECURE_FILE_MODE, }); - tempPathCreated = true; if (!IS_WINDOWS) { await fsPromises.chmod(tempPath, SECURE_FILE_MODE); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.js` around lines 319 - 346, In writeJsonAtomicAsync, tempPathCreated is set only after the await fsPromises.writeFile(...) so if writeFile creates/truncates the temp file then rejects, cleanup is skipped; set tempPathCreated = true before the write starts (i.e., immediately after computing tempPath and before the await fsPromises.writeFile call) so the catch block will always attempt to unlink the temp file if a partial/truncated file exists; keep the chmod step conditional after a successful write or leave as-is, but ensure tempPathCreated is set early (or alternatively create the temp file explicitly with fsPromises.open and set tempPathCreated immediately after open) so unlink runs on write failures.
🧹 Nitpick comments (3)
tests/unit/recent-days-reveal.test.ts (1)
14-29: Add explicit no-op branch coverage forscheduleProgressiveRowReveal.Consider adding one case where
totalRowsis already within the initial visible batch, assertingnullreturn and zero frame scheduling. It hardens the early-return contract.💡 Suggested test addition
describe('RecentDays progressive reveal helpers', () => { + it('returns null and does not schedule when all rows fit in the initial batch', () => { + const scheduleFrame = vi.fn<(callback: FrameRequestCallback) => number>() + const updates: number[] = [] + + const frameId = scheduleProgressiveRowReveal( + 150, + (value) => { + updates.push(typeof value === 'number' ? value : value(updates.at(-1) ?? 30)) + }, + scheduleFrame, + ) + + expect(frameId).toBeNull() + expect(updates).toEqual([150]) + expect(scheduleFrame).not.toHaveBeenCalled() + }) + it('schedules an animation frame only when rows remain after the initial batch', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/recent-days-reveal.test.ts` around lines 14 - 29, Add a unit test calling scheduleProgressiveRowReveal with totalRows that fits entirely within the initial visible batch (e.g., a small number) and assert it returns null, does not call the provided scheduleFrame mock, and does not schedule further updates (verify scheduleFrame called 0 times and updates array unchanged or contains only the initial synchronous update). Target the scheduleProgressiveRowReveal call and its callback/mock scheduleFrame to validate the early-return no-op branch.src/components/features/heatmap/HeatmapCalendar.tsx (1)
193-200: Minor: Redundant nullish coalescing.The ternary on line 195 already returns
nullwhen the condition is false, so the?? nullis unnecessary.🔧 Suggested simplification
const defaultFocusedDate = useMemo( () => - (availableDates.includes(todayStr) ? todayStr : null) ?? + (availableDates.includes(todayStr) ? todayStr : undefined) ?? cells.find((cell) => cell.value > 0)?.date ?? availableDates[0] ?? null, [availableDates, cells, todayStr], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/heatmap/HeatmapCalendar.tsx` around lines 193 - 200, The expression computing defaultFocusedDate contains a redundant "?? null" after the ternary that already yields null; remove the "?? null" so defaultFocusedDate is computed as (availableDates.includes(todayStr) ? todayStr : null) || cells.find((cell) => cell.value > 0)?.date || availableDates[0] || null, updating the useMemo return to eliminate the unnecessary nullish coalescing while keeping the same fallbacks referencing availableDates, todayStr, and cells.src/lib/data-transforms.ts (1)
97-104: Tighten themodelCostChartDatatype instead of casting around it.
Array<ChartDataPoint & Record<string, number>>does not match the object you build: the*_ma7properties are assigned via optional chaining and can beundefined, yet the type promises all Record values arenumber. The cast on line 232 suppresses this mismatch. Consider creating a dedicated point type with an honest index signature and constructingmodelCostPointdirectly against it. The downstream code already usescoerceNumber()defensively, but tightening the type will improve clarity and prevent misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/data-transforms.ts` around lines 97 - 104, The modelCostChartData element of DashboardChartTransforms is typed too loosely as Array<ChartDataPoint & Record<string, number>> while the built objects have optional numeric *_ma7 fields; create a dedicated interface (e.g., ModelCostChartPoint) that extends ChartDataPoint and declares an index signature like [key: string]: number | undefined (or explicit optional properties for *_ma7), then replace modelCostChartData's type with Array<ModelCostChartPoint> and construct modelCostPoint directly as that type (update any cast on modelCostPoint creation and ensure downstream uses still call coerceNumber() where needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 16: Update the CHANGELOG entry for "Ladeverhalten und Chunking des
Dashboards" to remove the contradictory claim about "ohne sichtbare Funktionen,
Inhalte oder Animationen zu verändern" and instead state that lazy loading was
introduced while preserving user-visible functionality but with some animation
behavior adjusted; specifically reconcile the first paragraph about lazy loading
with the later note that "animation behavior in dashboard sections was adjusted"
so both lines consistently mention preserved functionality but modified
animation behavior.
In `@server.js`:
- Around line 1813-1823: The reset code uses a bare catch around
fsPromises.unlink(DATA_FILE) which masks permission/IO errors; update the
try/catch in the withSettingsAndDataMutationLock blocks (where fsPromises.unlink
and updateDataLoadState are called) to only swallow ENOENT: catch the error,
check if err && err.code === 'ENOENT' and ignore only in that case, otherwise
rethrow the error so permission/IO failures stop the reset and prevent
updateDataLoadState from clearing lastLoadedAt/lastLoadSource; apply the same
change to both occurrences that call fsPromises.unlink(DATA_FILE) and then
updateDataLoadState.
In `@src/components/features/heatmap/HeatmapCalendar.tsx`:
- Around line 176-179: Replace the direct DOM read in HeatmapCalendar that sets
isDarkTheme with the useTheme hook so colors react to theme changes: import and
call useTheme(), assign isDarkTheme = isDark from the hook instead of using
typeof document !== 'undefined' and
document.documentElement.classList.contains('dark'), and keep axisColor and
todayOutlineColor defined as before so they update when the component
re-renders; ensure useTheme is imported where isDarkTheme is currently declared.
In `@src/components/features/request-quality/RequestQuality.tsx`:
- Around line 83-90: The width calculation currently forces a 6% minimum via
Math.max(item.progress * 100, 6) which turns real zero metrics into a visible
bar; update the style logic in RequestQuality.tsx (the element using
item.progress, metrics.hasRequestData, inView and shouldReduceMotion) so that if
item.progress === 0 you render '0%' explicitly, otherwise apply the minimum
(e.g. width = `${Math.max(item.progress * 100, 6)}%`) only for non-zero values
while preserving the existing gating with (inView || shouldReduceMotion) &&
metrics.hasRequestData.
In `@src/components/layout/Header.tsx`:
- Around line 160-163: The aria-label and title for the theme toggle in Header
incorrectly always use the "themeDark" description; update the JSX in the Header
component (where onClick={onToggleTheme} is used) to choose the label based on
the current isDark state (e.g., compute a themeToggleLabel variable using isDark
? t('commandPalette.commands.themeLight.description') :
t('commandPalette.commands.themeDark.description') and use that for both
aria-label and title) so screen readers announce the action that will occur
rather than the static "themeDark" string.
---
Outside diff comments:
In `@src/components/features/drill-down/DrillDownModal.tsx`:
- Around line 255-262: The requestRanking and related "top request" logic
currently compute a rank and pick a model even when request counts are missing
or all zero; modify the code around requestRanking (and the later
topRequestModel logic) to first detect whether request counts exist (e.g., const
hasRequestCounts = contextData.some(c => typeof c.requestCount === 'number' &&
c.requestCount > 0)), and only compute the sorted rank and select a
topRequestModel when hasRequestCounts is true; otherwise set
requestRanking/topRequestModel to null/undefined (or an explicit "unavailable"
state) so the UI can render an unavailable state instead of showing `#1` or an
arbitrary model. Ensure you update both the requestRanking calculation and the
later selection code that uses request counts.
In `@tests/unit/code-rabbit-phase4.test.ts`:
- Around line 119-141: The test currently masks the stale-label regression by
calling buildDashboardChartTransforms() with explicit locale strings and
mutating i18n; change it to directly exercise the memoized code path in
src/hooks/use-computed-metrics.ts (around the function referenced at line 12) or
convert the test into a pure transform test that does not call initI18n;
specifically either (A) import and call the memoized function from
use-computed-metrics (or render the hook with a test renderer) to ensure the
memoization depends on locale, or (B) remove initI18n('de') and assert weekday
labels for two locales that actually differ when passed into
buildDashboardChartTransforms('en-US') vs
buildDashboardChartTransforms('de-CH'), and update assertions accordingly; make
sure to reference and update tests to use the function name
buildDashboardChartTransforms and the hook name/useComputedMetrics path so the
memoized branch is covered.
---
Duplicate comments:
In `@server.js`:
- Around line 319-346: In writeJsonAtomicAsync, tempPathCreated is set only
after the await fsPromises.writeFile(...) so if writeFile creates/truncates the
temp file then rejects, cleanup is skipped; set tempPathCreated = true before
the write starts (i.e., immediately after computing tempPath and before the
await fsPromises.writeFile call) so the catch block will always attempt to
unlink the temp file if a partial/truncated file exists; keep the chmod step
conditional after a successful write or leave as-is, but ensure tempPathCreated
is set early (or alternatively create the temp file explicitly with
fsPromises.open and set tempPathCreated immediately after open) so unlink runs
on write failures.
---
Nitpick comments:
In `@src/components/features/heatmap/HeatmapCalendar.tsx`:
- Around line 193-200: The expression computing defaultFocusedDate contains a
redundant "?? null" after the ternary that already yields null; remove the "??
null" so defaultFocusedDate is computed as (availableDates.includes(todayStr) ?
todayStr : null) || cells.find((cell) => cell.value > 0)?.date ||
availableDates[0] || null, updating the useMemo return to eliminate the
unnecessary nullish coalescing while keeping the same fallbacks referencing
availableDates, todayStr, and cells.
In `@src/lib/data-transforms.ts`:
- Around line 97-104: The modelCostChartData element of DashboardChartTransforms
is typed too loosely as Array<ChartDataPoint & Record<string, number>> while the
built objects have optional numeric *_ma7 fields; create a dedicated interface
(e.g., ModelCostChartPoint) that extends ChartDataPoint and declares an index
signature like [key: string]: number | undefined (or explicit optional
properties for *_ma7), then replace modelCostChartData's type with
Array<ModelCostChartPoint> and construct modelCostPoint directly as that type
(update any cast on modelCostPoint creation and ensure downstream uses still
call coerceNumber() where needed).
In `@tests/unit/recent-days-reveal.test.ts`:
- Around line 14-29: Add a unit test calling scheduleProgressiveRowReveal with
totalRows that fits entirely within the initial visible batch (e.g., a small
number) and assert it returns null, does not call the provided scheduleFrame
mock, and does not schedule further updates (verify scheduleFrame called 0 times
and updates array unchanged or contains only the initial synchronous update).
Target the scheduleProgressiveRowReveal call and its callback/mock scheduleFrame
to validate the early-return no-op branch.
🪄 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: bd2ddebe-9003-469d-9300-50ca1ec5ee0b
📒 Files selected for processing (20)
CHANGELOG.mdserver.jssrc/components/features/drill-down/DrillDownModal.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/features/request-quality/RequestQuality.tsxsrc/components/layout/FilterBar.tsxsrc/components/layout/Header.tsxsrc/components/tables/RecentDays.tsxsrc/lib/data-transforms.tstests/frontend/drill-down-modal.test.tsxtests/frontend/filter-bar.test.tsxtests/frontend/header-links.test.tsxtests/frontend/heatmap-calendar.test.tsxtests/frontend/recent-days-model-dedupe.test.tsxtests/frontend/request-quality.test.tsxtests/frontend/sortable-tables.test.tsxtests/unit/code-rabbit-phase4.test.tstests/unit/recent-days-reveal.test.tstests/unit/server-helpers.test.tsvitest.setup.ts
✅ Files skipped from review due to trivial changes (2)
- tests/frontend/request-quality.test.tsx
- tests/frontend/drill-down-modal.test.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/frontend/header-links.test.tsx
- vitest.setup.ts
- tests/frontend/sortable-tables.test.tsx
- tests/frontend/heatmap-calendar.test.tsx
- tests/unit/server-helpers.test.ts
- src/components/layout/FilterBar.tsx
- src/components/tables/RecentDays.tsx
- tests/frontend/filter-bar.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.js (1)
275-397:⚠️ Potential issue | 🟠 MajorThese mutation locks stop at the process boundary.
fileMutationLocksonly coordinates requests inside one Node process, but this file already supports multiple running TTDash instances and they all resolve the sameDATA_FILE/SETTINGS_FILE. Two servers can still interleaveread -> modify -> writecycles and lose updates, so the new race-safety guarantee only holds in-process. If multi-instance usage is supported, this needs an OS-visible lock (lock file / lock dir) around the full logical mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.js` around lines 275 - 397, The in-memory fileMutationLocks Map only serializes mutations inside one process, so concurrent TTDash instances can still race; replace or augment with an OS-visible lock around the full mutation operations. Update withFileMutationLock (and transitively withOrderedFileMutationLocks and withSettingsAndDataMutationLock) to acquire a per-path OS lock (e.g., open the target file or a sidecar lock file and obtain an exclusive lock via flock or a cross-process lockfile library), run the operation while holding that lock, then release/close the lock in a finally block; retain the in-process Map to avoid redundant syscalls but ensure ordering by acquiring locks in sorted order (uniquePaths.sort()) before performing nested operations to prevent deadlocks, and handle lock acquisition failures/timeouts and cleanup (close/unlink) so the original error still propagates.
♻️ Duplicate comments (2)
src/components/features/heatmap/HeatmapCalendar.tsx (1)
176-179:⚠️ Potential issue | 🟠 MajorTheme-dependent colors still won't update when the theme toggles.
isDarkThemeis still derived fromdocument.documentElementduring render, so the heatmap stays on the old palette until some unrelated state change re-renders this component. Wire this to reactive theme state instead of reading the DOM here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/features/heatmap/HeatmapCalendar.tsx` around lines 176 - 179, The colors are computed from a one-time DOM read via isDarkTheme, so they don't update on theme toggle; replace the document.documentElement check with reactive theme state (e.g., consume the app's theme context or useTheme hook) inside the HeatmapCalendar component and compute axisColor and todayOutlineColor from that reactive value (or derive them from CSS variables each render), or alternatively subscribe to theme changes (via context or a MutationObserver that updates local state) so axisColor/todayOutlineColor update immediately when the theme toggles; update references to isDarkTheme, axisColor, and todayOutlineColor accordingly.server.js (1)
1867-1873:⚠️ Potential issue | 🟠 MajorOnly ignore
ENOENTwhen resetting settings.This bare
catchstill returns200on permission/I/O failures, so/api/settingscan claim success whilesettings.jsonwas never deleted. ReusingunlinkIfExists(SETTINGS_FILE)here would keep the reset semantics aligned with the data reset path.🛠️ Proposed fix
- await withFileMutationLock(SETTINGS_FILE, async () => { - try { - await fsPromises.unlink(SETTINGS_FILE); - } catch { - // Ignore missing settings files during reset. - } - }); + await withFileMutationLock(SETTINGS_FILE, async () => { + await unlinkIfExists(SETTINGS_FILE); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.js` around lines 1867 - 1873, The reset path currently swallows all errors when deleting SETTINGS_FILE inside withFileMutationLock, which can mask permission/I/O failures; replace the try/catch unlink block by calling the existing unlinkIfExists(SETTINGS_FILE) so only ENOENT is treated as benign and other errors propagate (or are handled consistently with the data reset path), keeping the reset semantics aligned; update the code inside the withFileMutationLock callback where SETTINGS_FILE is currently unlinked to use unlinkIfExists instead and remove the bare catch.
🧹 Nitpick comments (3)
tests/unit/code-rabbit-phase4.test.ts (1)
118-144: Optional: add an invariant assertion so rerender checks more than label text.You already verify label updates across locale changes. Consider also asserting a numeric bucket value remains unchanged to ensure only presentation changes, not aggregation.
♻️ Suggested tightening
expect(result.current.weekdayData[2]?.day).toBe('We') + expect(result.current.weekdayData[0]?.cost).toBe(9) rerender({ locale: 'de-CH' }) expect(result.current.weekdayData[2]?.day).toBe('Mi') + expect(result.current.weekdayData[0]?.cost).toBe(9)As per coding guidelines, "Prefer focused
*.test.tsor*.test.tsxcoverage for data transforms, hooks, or complex UI behavior".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/code-rabbit-phase4.test.ts` around lines 118 - 144, Add an invariant numeric assertion to the test so rerender verifies presentation only: after rendering useComputedMetrics(data, locale) and before rerender capture a numeric bucket value (e.g., result.current.weekdayData[2]?.totalTokens or requestCount) and assert it equals the expected number, then rerender({ locale: 'de-CH' }) and assert the same numeric value is unchanged while still checking the weekday label change; reference useComputedMetrics and weekdayData to locate where to read the numeric bucket.tests/frontend/anomaly-detection.test.tsx (2)
84-85: Deduplicate the repeated anomaly button selector to reduce test drift.The same role/name query appears twice; extracting it into a single constant/helper keeps future label changes localized.
♻️ Proposed refactor
describe('AnomalyDetection', () => { + const anomalyButtonName = /fri, 04\/03\/2026/i + beforeEach(async () => { @@ - const anomalyButton = screen.getByRole('button', { name: /fri, 04\/03\/2026/i }) + const anomalyButton = screen.getByRole('button', { name: anomalyButtonName }) @@ - const anomalyButton = screen.getByRole('button', { name: /fri, 04\/03\/2026/i }) + const anomalyButton = screen.getByRole('button', { name: anomalyButtonName })Also applies to: 101-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/anomaly-detection.test.tsx` around lines 84 - 85, The test repeats the same role/name query for the anomaly button; extract the getByRole(...) call into a single constant (e.g., const anomalyButton = screen.getByRole('button', { name: /fri, 04\/03\/2026/i })) placed once inside the test or a small helper function, then replace both occurrences on lines referencing the anomaly button with that constant/helper to keep the label centralized and reduce test drift; update any references to use anomalyButton so future label changes only require one edit.
64-75: Add local afterEach cleanup for consistency with other tests in the suite.While
vitest.setup.tsprovides global cleanup viaafterEach(() => vi.unstubAllGlobals()), several other frontend tests (e.g.,request-quality.test.tsx,motion-accessibility.test.tsx,cache-roi.test.tsx) include explicit localafterEachcleanup. AddingafterEach(() => vi.unstubAllGlobals())to this test aligns with the established pattern and makes per-test cleanup intentions explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/anomaly-detection.test.tsx` around lines 64 - 75, Add a local afterEach cleanup to match other tests: after the existing beforeEach that stubs global IntersectionObserver and calls initI18n('en'), add an afterEach that calls vi.unstubAllGlobals() so the test file explicitly restores globals per-test; reference the existing beforeEach, the IntersectionObserver stub class, and vi.unstubAllGlobals 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 `@src/components/dashboard/DashboardSections.tsx`:
- Around line 202-204: lazyCardFallback currently passes a height class to
ChartCardSkeleton but ChartCardSkeleton still hardcodes its body to h-[300px],
causing layout shifts; update ChartCardSkeleton (the component that renders the
skeleton body) to accept a prop (e.g., bodyClassName or bodyHeight) and use that
instead of the hardcoded "h-[300px]", then update lazyCardFallback to pass its
className (or extracted height token) through to ChartCardSkeleton so the outer
placeholder and inner skeleton body match sizes.
In `@src/components/features/heatmap/HeatmapCalendar.tsx`:
- Around line 188-190: Keyboard navigation is using chronological availableDates
while the grid is rendered by weekday rows (cellRows), causing
left/right/up/down/home/end to move incorrectly; fix this by computing a
navigation order based on the rendered grid (flatten cellRows row-wise) and use
that flattenedRowOrder wherever keyboard index lookups and moves are calculated
(replace usages of availableDates in your key handlers with the new flattened
row-major order so ArrowLeft/Right move within a row, ArrowUp/Down jump rows,
and Home/End go to row boundaries). Ensure functions that compute target indices
(the keydown handler(s) around where availableDates is used) reference the new
flattened order and update focus/selection using that index mapping.
---
Outside diff comments:
In `@server.js`:
- Around line 275-397: The in-memory fileMutationLocks Map only serializes
mutations inside one process, so concurrent TTDash instances can still race;
replace or augment with an OS-visible lock around the full mutation operations.
Update withFileMutationLock (and transitively withOrderedFileMutationLocks and
withSettingsAndDataMutationLock) to acquire a per-path OS lock (e.g., open the
target file or a sidecar lock file and obtain an exclusive lock via flock or a
cross-process lockfile library), run the operation while holding that lock, then
release/close the lock in a finally block; retain the in-process Map to avoid
redundant syscalls but ensure ordering by acquiring locks in sorted order
(uniquePaths.sort()) before performing nested operations to prevent deadlocks,
and handle lock acquisition failures/timeouts and cleanup (close/unlink) so the
original error still propagates.
---
Duplicate comments:
In `@server.js`:
- Around line 1867-1873: The reset path currently swallows all errors when
deleting SETTINGS_FILE inside withFileMutationLock, which can mask
permission/I/O failures; replace the try/catch unlink block by calling the
existing unlinkIfExists(SETTINGS_FILE) so only ENOENT is treated as benign and
other errors propagate (or are handled consistently with the data reset path),
keeping the reset semantics aligned; update the code inside the
withFileMutationLock callback where SETTINGS_FILE is currently unlinked to use
unlinkIfExists instead and remove the bare catch.
In `@src/components/features/heatmap/HeatmapCalendar.tsx`:
- Around line 176-179: The colors are computed from a one-time DOM read via
isDarkTheme, so they don't update on theme toggle; replace the
document.documentElement check with reactive theme state (e.g., consume the
app's theme context or useTheme hook) inside the HeatmapCalendar component and
compute axisColor and todayOutlineColor from that reactive value (or derive them
from CSS variables each render), or alternatively subscribe to theme changes
(via context or a MutationObserver that updates local state) so
axisColor/todayOutlineColor update immediately when the theme toggles; update
references to isDarkTheme, axisColor, and todayOutlineColor accordingly.
---
Nitpick comments:
In `@tests/frontend/anomaly-detection.test.tsx`:
- Around line 84-85: The test repeats the same role/name query for the anomaly
button; extract the getByRole(...) call into a single constant (e.g., const
anomalyButton = screen.getByRole('button', { name: /fri, 04\/03\/2026/i }))
placed once inside the test or a small helper function, then replace both
occurrences on lines referencing the anomaly button with that constant/helper to
keep the label centralized and reduce test drift; update any references to use
anomalyButton so future label changes only require one edit.
- Around line 64-75: Add a local afterEach cleanup to match other tests: after
the existing beforeEach that stubs global IntersectionObserver and calls
initI18n('en'), add an afterEach that calls vi.unstubAllGlobals() so the test
file explicitly restores globals per-test; reference the existing beforeEach,
the IntersectionObserver stub class, and vi.unstubAllGlobals when making the
change.
In `@tests/unit/code-rabbit-phase4.test.ts`:
- Around line 118-144: Add an invariant numeric assertion to the test so
rerender verifies presentation only: after rendering useComputedMetrics(data,
locale) and before rerender capture a numeric bucket value (e.g.,
result.current.weekdayData[2]?.totalTokens or requestCount) and assert it equals
the expected number, then rerender({ locale: 'de-CH' }) and assert the same
numeric value is unchanged while still checking the weekday label change;
reference useComputedMetrics and weekdayData to locate where to read the numeric
bucket.
🪄 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: 7249bab9-756f-4e03-99e7-7735a34ec3ac
📒 Files selected for processing (17)
CHANGELOG.mdserver.jssrc/components/charts/CostByModelOverTime.tsxsrc/components/dashboard/DashboardSections.tsxsrc/components/features/anomaly/AnomalyDetection.tsxsrc/components/features/drill-down/DrillDownModal.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/features/request-quality/RequestQuality.tsxsrc/components/layout/Header.tsxsrc/lib/data-transforms.tstests/frontend/anomaly-detection.test.tsxtests/frontend/drill-down-modal.test.tsxtests/frontend/header-links.test.tsxtests/frontend/request-quality.test.tsxtests/unit/code-rabbit-phase4.test.tstests/unit/recent-days-reveal.test.tstests/unit/server-helpers.test.ts
✅ Files skipped from review due to trivial changes (3)
- tests/frontend/request-quality.test.tsx
- tests/unit/recent-days-reveal.test.ts
- tests/frontend/drill-down-modal.test.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/frontend/header-links.test.tsx
- tests/unit/server-helpers.test.ts
- src/components/charts/CostByModelOverTime.tsx
- src/components/features/anomaly/AnomalyDetection.tsx
- src/lib/data-transforms.ts
- src/components/features/drill-down/DrillDownModal.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CHANGELOG.md (1)
3-3:⚠️ Potential issue | 🟡 MinorUse an unreleased header until 6.2.1 is actually published.
Since this PR is still open, consider keeping this as
Unreleasedand only stamping2026-04-15when the release is cut/tagged.Proposed edit
-## [6.2.1] - 2026-04-15 +## [6.2.1] - Unreleased🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 3, The changelog header currently hardcodes a release date: the line "## [6.2.1] - 2026-04-15" should be left as an unreleased entry until the tag is actually published; update that header to use "Unreleased" (e.g., "## [6.2.1] - Unreleased" or "## [Unreleased]") and only replace "Unreleased" with "2026-04-15" when the release is cut.
🧹 Nitpick comments (4)
CHANGELOG.md (1)
13-13: Consider consistent German terminology in this bullet.For consistency with the rest of the German changelog, replacing
AccessibilitywithBarrierefreiheitwould read cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 13, Update the German changelog bullet by replacing the English term "Accessibility" with the German equivalent "Barrierefreiheit" in the sentence starting with "Umfassende UI-Qualität im gesamten Dashboard" so the phrase reads "...mit besserer Barrierefreiheit, klarerer Zustandskommunikation, stärkerer Mobile-Discoverability..." and maintain the rest of the punctuation and wording unchanged.tests/frontend/skeleton.test.tsx (1)
10-15: Make this assertion unambiguous and less order-dependent.At Line 10 and Line 15, using the same class for both
classNameandbodyClassNameplus assertingskeletons[1]can mask propagation issues and couples the test to DOM order. Use distinct class markers per prop and assert by marker instead.♻️ Proposed test hardening
describe('ChartCardSkeleton', () => { it('passes custom body sizing through to the inner chart placeholder', () => { const { container } = render( - <ChartCardSkeleton className="h-[420px]" bodyClassName="h-[420px]" />, + <ChartCardSkeleton className="outer-shell-height" bodyClassName="inner-chart-height" />, ) const skeletons = container.querySelectorAll('.animate-pulse') expect(skeletons).toHaveLength(2) - expect(skeletons[1]).toHaveClass('h-[420px]') + expect(skeletons[0]).toHaveClass('outer-shell-height') + expect(skeletons[1]).toHaveClass('inner-chart-height') }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/skeleton.test.tsx` around lines 10 - 15, The test currently relies on DOM order by checking skeletons[1] and uses the same class on both props; update the ChartCardSkeleton render to pass distinct marker classes (e.g., className="card-marker" and bodyClassName="body-marker") and change assertions to query/select elements by those markers (e.g., container.querySelectorAll('.card-marker') / container.querySelector('.body-marker')) rather than by index; update the variable references (skeletons) and expect statements to assert presence and correct class per marker so the test is not order-dependent.tests/frontend/heatmap-calendar.test.tsx (1)
26-38: Consider a localDailyUsagefixture builder to reduce repetition.The repeated object setup makes future edits noisy; a tiny factory helper would improve readability and maintenance.
♻️ Optional refactor sketch
+const makeDay = (overrides: Partial<DailyUsage>): DailyUsage => ({ + date: '2026-04-07', + inputTokens: 10, + outputTokens: 5, + cacheCreationTokens: 0, + cacheReadTokens: 0, + thinkingTokens: 0, + totalTokens: 15, + totalCost: 5, + requestCount: 2, + modelsUsed: ['gpt-5.4'], + modelBreakdowns: [], + ...overrides, +}) - -const day: DailyUsage = { - date: '2026-04-07', - inputTokens: 10, - outputTokens: 5, - ... -} +const day = makeDay({})Also applies to: 61-73, 98-151, 194-206, 219-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/frontend/heatmap-calendar.test.tsx` around lines 26 - 38, Tests repeat verbose DailyUsage objects like the one assigned to const day; create a small fixture factory (e.g., a buildDailyUsage function) that returns a default DailyUsage and accepts an overrides Partial<DailyUsage> to merge test-specific fields, then replace repeated literal objects (the const day and the other occurrences) with calls to buildDailyUsage({ date: ..., inputTokens: ..., ... }) to reduce duplication and make future edits easier.src/components/dashboard/DashboardSections.tsx (1)
310-314: Consider adding an error boundary for lazy-loaded sections.While the Suspense fallbacks now properly show skeletons during loading (addressing the previous concern), there's no error boundary to catch chunk load failures. A network hiccup or deployment during a user's session could crash the entire dashboard if any lazy import fails.
A shared
ErrorBoundarywrapper around sections (or at the Suspense level) would allow graceful degradation—showing a "failed to load" card instead of a white screen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/dashboard/DashboardSections.tsx` around lines 310 - 314, Wrap this lazy-loaded section in an ErrorBoundary so chunk load failures render a graceful "failed to load" card instead of blowing up the dashboard: create or reuse an ErrorBoundary component and place it around the Suspense/ExpandableCard (the block containing Suspense, ExpandableCard, and CostForecast), have its fallback render a compact error state consistent with lazyCardFallback (e.g., a card-sized "failed to load" message or retry action), and ensure the ErrorBoundary catches and logs errors so CostForecast and its lazy import failures are isolated.
🤖 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 394-418: shouldReapFileMutationLock currently returns early when
owner.pid is an integer and isProcessRunning(pid) is true, which allows PID
reuse to block forever; update shouldReapFileMutationLock to also validate lock
age (compare Date.now() - stats.mtimeMs or parse owner.createdAt) and/or
validate owner.instanceId before returning false—i.e., if isProcessRunning(pid)
is true, still stat(lockDir) and reap if mtimeMs or owner.createdAt exceed
FILE_MUTATION_LOCK_STALE_MS, and if owner.instanceId is available compare it to
our current instanceId to decide whether to reap; ensure errors from reading
owner (JSON) still fall back to the age check and preserve the existing ENOENT
handling in fs.stat.
In `@tests/unit/server-helpers.test.ts`:
- Around line 179-266: Replace hardcoded '/tmp/...' paths in the three tests
that call withFileMutationLock and withOrderedFileMutationLocks by creating a
temporary directory (using tmpdir()/mkdtemp or fs.mkdtempSync + path.join) at
the start of each test, then build the file paths via path.join(tmpDir,
'settings.json') and path.join(tmpDir, 'data.json'); use those variables in
calls to withFileMutationLock and withOrderedFileMutationLocks and ensure the
temp dir is unique per test (and optionally cleaned up) so the tests run
cross-platform instead of writing to '/tmp'.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Line 3: The changelog header currently hardcodes a release date: the line "##
[6.2.1] - 2026-04-15" should be left as an unreleased entry until the tag is
actually published; update that header to use "Unreleased" (e.g., "## [6.2.1] -
Unreleased" or "## [Unreleased]") and only replace "Unreleased" with
"2026-04-15" when the release is cut.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 13: Update the German changelog bullet by replacing the English term
"Accessibility" with the German equivalent "Barrierefreiheit" in the sentence
starting with "Umfassende UI-Qualität im gesamten Dashboard" so the phrase reads
"...mit besserer Barrierefreiheit, klarerer Zustandskommunikation, stärkerer
Mobile-Discoverability..." and maintain the rest of the punctuation and wording
unchanged.
In `@src/components/dashboard/DashboardSections.tsx`:
- Around line 310-314: Wrap this lazy-loaded section in an ErrorBoundary so
chunk load failures render a graceful "failed to load" card instead of blowing
up the dashboard: create or reuse an ErrorBoundary component and place it around
the Suspense/ExpandableCard (the block containing Suspense, ExpandableCard, and
CostForecast), have its fallback render a compact error state consistent with
lazyCardFallback (e.g., a card-sized "failed to load" message or retry action),
and ensure the ErrorBoundary catches and logs errors so CostForecast and its
lazy import failures are isolated.
In `@tests/frontend/heatmap-calendar.test.tsx`:
- Around line 26-38: Tests repeat verbose DailyUsage objects like the one
assigned to const day; create a small fixture factory (e.g., a buildDailyUsage
function) that returns a default DailyUsage and accepts an overrides
Partial<DailyUsage> to merge test-specific fields, then replace repeated literal
objects (the const day and the other occurrences) with calls to
buildDailyUsage({ date: ..., inputTokens: ..., ... }) to reduce duplication and
make future edits easier.
In `@tests/frontend/skeleton.test.tsx`:
- Around line 10-15: The test currently relies on DOM order by checking
skeletons[1] and uses the same class on both props; update the ChartCardSkeleton
render to pass distinct marker classes (e.g., className="card-marker" and
bodyClassName="body-marker") and change assertions to query/select elements by
those markers (e.g., container.querySelectorAll('.card-marker') /
container.querySelector('.body-marker')) rather than by index; update the
variable references (skeletons) and expect statements to assert presence and
correct class per marker so the test is not order-dependent.
🪄 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: 8a35c40f-5523-465c-8e0a-31788f1f52bc
📒 Files selected for processing (11)
CHANGELOG.mdserver.jssrc/components/Dashboard.tsxsrc/components/dashboard/DashboardSections.tsxsrc/components/features/heatmap/HeatmapCalendar.tsxsrc/components/ui/skeleton.tsxtests/frontend/anomaly-detection.test.tsxtests/frontend/heatmap-calendar.test.tsxtests/frontend/skeleton.test.tsxtests/unit/code-rabbit-phase4.test.tstests/unit/server-helpers.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/frontend/anomaly-detection.test.tsx
- src/components/ui/skeleton.tsx
- tests/unit/code-rabbit-phase4.test.ts
- src/components/Dashboard.tsx
- src/components/features/heatmap/HeatmapCalendar.tsx
This was referenced Apr 15, 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
Improved
Fixed
Localization
Tests