feat(ui): statistics view with recharts dashboard#97
Merged
Conversation
- Replace `StatisticsView` placeholder with a four-chart dashboard. - Period selector (7d / 30d / all-time) drives `stats_get(period)` and filters history-derived charts client-side. - Seven KPI cards: total volume, total files, avg/peak speed, success rate, cumulative download time and a CAPTCHA placeholder. - Charts (Recharts): daily volume bar (BarChart) and top hosts donut (PieChart) come straight from `stats_get`; type breakdown horizontal bar and average speed line are derived from `history_list` (extension parsing + UTC-day grouping). - Top-5 modules card uses `stats_top_modules` (limit 5). - Charts pull their primary color from `var(--color-accent)` so the user's accent setting is respected; a fixed palette covers multi-series. Each chart sets `role="img"` + axis `label` for a11y and falls back to an empty hint when there is no data. - New hook `useStatsQuery(period)` aggregates the three IPC queries via `useQueries`; loading/error states bubble up consistently. - New i18n namespace `statistics.*` (en + fr). - Add `recharts` dependency. Tests: 41 (derive, format, KpiCard, StatisticsView, useStatsQuery). Coverage: StatisticsView dir 93.96 % lines, useStatsQuery 90 %.
github-actions
Bot
added
documentation
Greptile SummaryThis PR replaces the Confidence Score: 5/5Safe to merge — all remaining findings are P2 style and test-coverage suggestions with no correctness impact. Previously flagged P1 concerns (history 500-row cap, keyboard navigation) have been addressed. No new P0/P1 issues found. Remaining comments are about a missing sub-minute test case, fragile testid generation, and a duplicated helper — none affect runtime behavior. No files require special attention.
|
| Filename | Overview |
|---|---|
| src/hooks/useStatsQuery.ts | New hook merging three parallel Tauri queries with per-query status flags; passes dateFrom + limit:500 to history_list; exposes a refetch helper that rethrows the first error. |
| src/views/StatisticsView/StatisticsView.tsx | Main dashboard component wiring period state, KPI cards, four chart sections, and TopModulesCard; handles full-page vs. inline error states; contains a duplicate nowSeconds() helper. |
| src/views/StatisticsView/ChartCard.tsx | Chart wrapper with loading/empty states; data-testid derived from translated title prop makes test IDs fragile to i18n string changes. |
| src/views/StatisticsView/derive.ts | Pure derivation utilities (extractExtension, deriveTypeBreakdown, deriveSpeedSeries, periodToCutoffSeconds, filterEntriesByPeriod); well-tested and correct. |
| src/views/StatisticsView/format.ts | Formatting helpers for bytes, speed, percent, count, and duration; formatDurationFromSeconds correctly handles < 1min edge case but that branch is not covered by tests. |
| src/views/StatisticsView/PeriodSelector.tsx | Tablist with roving tabIndex; keyboard navigation (ArrowLeft/Right/Up/Down, Home, End) and focus-follow-selection via useEffect are fully implemented and tested. |
| src/views/StatisticsView/tests/format.test.ts | Covers all format helpers but missing a test case for the 1–59 second (< 1min) branch highlighted in the PR description as a bug fix. |
| src/views/StatisticsView/tests/StatisticsView.test.tsx | Integration tests cover loading, empty, error, period switching, and history-loading states; chart state test IDs are coupled to English translation strings. |
| src/hooks/tests/useStatsQuery.test.tsx | Hook unit tests validate period passing, dateFrom cutoff, all-time omission, TOP_MODULES_LIMIT, and error surfacing; comprehensive coverage. |
Sequence Diagram
sequenceDiagram
participant U as User
participant SV as StatisticsView
participant USQ as useStatsQuery
participant BE as Tauri Backend
U->>SV: Select period (7d/30d/all)
SV->>USQ: useStatsQuery(period)
par Three parallel queries
USQ->>BE: stats_get({ period })
USQ->>BE: stats_top_modules({ limit: 5 })
USQ->>BE: history_list({ limit: 500, dateFrom? })
end
BE-->>USQ: StatsView (KPIs + dailyVolumes + topHosts)
BE-->>USQ: ModuleStats[]
BE-->>USQ: HistoryView[]
USQ-->>SV: { stats, topModules, history, statsStatus, historyStatus, topModulesStatus }
SV->>SV: filterEntriesByPeriod(history)
SV->>SV: deriveTypeBreakdown(filteredHistory)
SV->>SV: deriveSpeedSeries(filteredHistory)
SV-->>U: Render KPI cards + 4 charts + TopModulesCard
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/views/StatisticsView/__tests__/format.test.ts
Line: 39-50
Comment:
**`< 1min` branch is not tested**
The PR description specifically calls out `formatDurationFromSeconds` returning `'< 1min'` for sub-minute durations as a bug fix, but the `it.each` table never exercises the 1–59 second range. Every input is either `<= 0` (early return `'0h'`) or `>= 30 minutes`. A regression to that branch would go unnoticed.
```suggestion
it.each([
[0, '0h'],
[-100, '0h'],
[45, '< 1min'],
[60 * 30, '30min'],
[3_600, '1h'],
[3_600 * 2 + 60 * 5, '2h05'],
])('formats %i seconds as %s', (input, expected) => {
expect(formatDurationFromSeconds(input)).toBe(expected);
});
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/views/StatisticsView/ChartCard.tsx
Line: 26-35
Comment:
**`data-testid` derived from translated title is fragile**
The testid is computed by lower-casing and hyphenating the `title` prop, which is a translated string (e.g. `t('statistics.charts.typeBreakdown.title')`). Tests in `StatisticsView.test.tsx` then assert exact IDs like `chart-loading-type-breakdown` and `chart-loading-average-speed`. Any rename of the English translation key values will silently break those assertions without any type error. Consider passing an explicit `testId` prop instead.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/views/StatisticsView/StatisticsView.tsx
Line: 37-39
Comment:
**`nowSeconds()` duplicated across two files**
`function nowSeconds()` is defined identically here and in `src/hooks/useStatsQuery.ts`. Consider extracting it to a shared utility (e.g. `src/lib/time.ts`) so there is a single source of truth for the epoch-seconds helper.
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "fix(ui): drive widget loading from query..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a full Statistics feature: a new Changes
Sequence DiagramsequenceDiagram
participant User
participant React as StatisticsView (React)
participant Query as TanStack Query
participant Tauri as Tauri IPC
participant Backend as Backend
User->>React: open view / select period
activate React
React->>Query: useStatsQuery(period)
activate Query
Query->>Tauri: invoke('stats_get', { period })
Query->>Tauri: invoke('stats_top_modules', { limit: 5 })
Query->>Tauri: invoke('history_list', { limit: 500, dateFrom? })
par Backend handlers
Tauri->>Backend: stats_get(period)
Tauri->>Backend: stats_top_modules(limit: 5)
Tauri->>Backend: history_list(...)
end
Backend-->>Tauri: responses (stats, topModules, history)
Tauri-->>Query: return responses
deactivate Query
React->>React: filterEntriesByPeriod, deriveTypeBreakdown, deriveSpeedSeries
React->>React: formatBytes/formatSpeed/formatDuration
React->>React: render KPI cards, Chart components, TopModules
deactivate React
React-->>User: display dashboard / loading / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (9)
src/views/StatisticsView/__tests__/KpiCard.test.tsx (1)
6-17: Consider adding icon rendering verification.The tests cover label, value, and hint rendering well. Consider adding a test to verify the icon is rendered (e.g., checking for an SVG element or the icon's accessible name).
💡 Optional: Add icon rendering test
it('omits hint when absent', () => { render(<KpiCard label="Files" value="0" />); expect(screen.queryByText('last 7 days')).not.toBeInTheDocument(); }); + + it('renders icon when provided', () => { + render(<KpiCard label="Speed" value="10 MB/s" icon={Activity} />); + expect(document.querySelector('svg')).toBeInTheDocument(); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/__tests__/KpiCard.test.tsx` around lines 6 - 17, Add a test to verify the icon is rendered when KpiCard receives an icon prop: render KpiCard with icon={Activity} (same as existing tests) and assert the icon is present by checking for an SVG element (e.g., container.querySelector('svg') exists) or by using an accessible query like getByRole('img' or 'presentation') / getByLabelText if the icon exposes an aria-label; update the test file to include a new it('renders icon') case referencing KpiCard and Activity.src/hooks/__tests__/useStatsQuery.test.tsx (1)
29-44: Consider extracting shared mock setup to reduce duplication.These blocks repeat the same command branching and default payload shape. A small helper (e.g.,
setupStatsMocks(overrides)) would make future schema updates less error-prone.Also applies to: 54-69, 85-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/__tests__/useStatsQuery.test.tsx` around lines 29 - 44, The repeated mockInvoke.mockImplementation branching for 'stats_get', 'stats_top_modules', and 'history_list' should be extracted into a single helper (e.g., setupStatsMocks or createStatsInvokeMock) that installs mockInvoke.mockImplementation with a canonical default payload for 'stats_get' and default returns for the other commands, and accepts an overrides argument to merge test-specific values; update the tests that currently call mockInvoke.mockImplementation (the blocks around useStatsQuery tests at the shown locations) to call this helper instead so future schema changes only need to be updated in one place.src/views/StatisticsView/__tests__/StatisticsView.test.tsx (1)
130-138: Avoid locale-coupled assertions in UI tests.These assertions rely on specific English copy, which can cause false failures when translation content/default locale changes. Prefer stable selectors (
data-testid, semantic roles with stable labels, or centralized test i18n fixtures).Also applies to: 148-148, 164-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/__tests__/StatisticsView.test.tsx` around lines 130 - 138, The test uses locale-dependent text/aria strings with getByText/getByLabelText which will break when translations change; update the test in StatisticsView.test.tsx to assert using stable selectors instead (e.g., switch from screen.getByText('Total volume'), 'Total files', 'Success rate' and screen.getByLabelText('Daily download volume bar chart'), 'Top hosts donut chart', 'Type breakdown horizontal bar chart', 'Average speed line chart' to screen.getByRole or screen.getByTestId calls that target the chart components or summary widgets), or use centralized i18n test fixtures to supply expected labels; modify the render/test helpers so the StatisticsView renders test ids or stable accessible names and update the three mentioned assertions (and the similar cases at the other noted lines) to use those stable selectors.src/views/StatisticsView/StatisticsView.tsx (1)
69-75: Prefer partial rendering over full-page failure for secondary query errors.Line 69 currently returns an error view for any query error, which can suppress usable stats when only a secondary source fails. Consider showing the full-page error only when core
statsis unavailable, and surface partial errors inline for the affected sections.Suggested direction
- if (error) { + if (error && !stats) { return ( <div className="flex h-full items-center justify-center p-6 text-sm text-destructive"> {error.message} </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/StatisticsView.tsx` around lines 69 - 75, The current unconditional full-page return when `error` exists causes secondary query failures to hide usable `stats`; update the `StatisticsView` logic to only render the full-page error when `stats` is falsy (no core data), and otherwise allow the component to render normal UI while surfacing the secondary error inline or in the affected sections; locate the early-return that checks `if (error) { ... }` and change it to check `if (!stats) { /* full-page error using error.message */ }` and pass the `error` (or its message) down to the specific child section components or set a local per-section error state so those sections can render their own inline error UI.src/views/StatisticsView/DailyVolumeChart.tsx (1)
41-44: Consider removing redundant memoization.The
useMemohere mapsDailyVolumeto an object with identical fields (date,bytes,count). If theDailyVolumetype already has this shape, Recharts can consumedatadirectly without the intermediate mapping.♻️ Suggested simplification
- const formatted = useMemo( - () => data.map((d) => ({ date: d.date, bytes: d.bytes, count: d.count })), - [data], - ); - return ( <div role="img" aria-label={ariaLabel} className="h-64 w-full"> <ResponsiveContainer width="100%" height="100%"> - <BarChart data={formatted} margin={{ top: 8, right: 12, bottom: 8, left: 4 }}> + <BarChart data={data} margin={{ top: 8, right: 12, bottom: 8, left: 4 }}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/DailyVolumeChart.tsx` around lines 41 - 44, The useMemo-based mapping that creates formatted (const formatted = useMemo(...)) is redundant because it returns objects with the same shape as the DailyVolume items; remove the useMemo and the formatted variable and pass the original data prop directly to the chart (replace uses of formatted with data in the DailyVolumeChart component) so Recharts consumes the DailyVolume[] without an unnecessary allocation.src/views/StatisticsView/format.ts (1)
31-37: Consider sub-minute duration display.When
secondsis between 0 and 60 (exclusive),hoursandminutesare both 0, resulting in"0min". For a "time saved" KPI, showing"< 1min"might be more informative than"0min".♻️ Optional improvement
export function formatDurationFromSeconds(seconds: number): string { if (!Number.isFinite(seconds) || seconds <= 0) return '0h'; const hours = Math.floor(seconds / SECONDS_PER_HOUR); const minutes = Math.floor((seconds % SECONDS_PER_HOUR) / 60); + if (hours === 0 && minutes === 0) return '< 1min'; if (hours === 0) return `${minutes}min`; return minutes === 0 ? `${hours}h` : `${hours}h${String(minutes).padStart(2, '0')}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/format.ts` around lines 31 - 37, formatDurationFromSeconds currently returns "0min" when seconds > 0 but < 60 because hours and minutes compute to 0; change the logic in formatDurationFromSeconds to detect sub-minute positive durations (seconds > 0 && seconds < 60) and return "< 1min" before computing hours/minutes, keeping the existing behavior for zero/negative values and the existing hours/minutes formatting (use SECONDS_PER_HOUR, Math.floor, and padStart as before).src/hooks/useStatsQuery.ts (2)
54-56: Refetch errors are silently swallowed.
Promise.allwill reject if any refetch fails, but the returnedPromise<void>doesn't surface this. Callers cannot distinguish success from failure.♻️ Suggested fix to propagate errors
refetch: async () => { - await Promise.all(results.map((r) => r.refetch())); + const outcomes = await Promise.all(results.map((r) => r.refetch())); + const firstError = outcomes.find((o) => o.error)?.error; + if (firstError) throw firstError; },Alternatively, if you want to surface all errors or handle partial failures, consider
Promise.allSettled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useStatsQuery.ts` around lines 54 - 56, The current refetch implementation in useStatsQuery.ts (the refetch function that does await Promise.all(results.map((r) => r.refetch()))) swallows failures because callers only get a Promise<void> with no error semantics; change the implementation to propagate errors by returning the Promise returned by Promise.all (i.e., remove swallowing await/handle/wrap so rejections bubble to callers) or explicitly switch to Promise.allSettled and surface partial failures (e.g., throw aggregated error when any result.status === "rejected"); update the refetch function around results.map((r) => r.refetch()) to either rethrow Promise.all rejections or transform allSettled results into a clear error/response so callers of refetch can detect failures.
27-30: Query key structure could be more specific.The
topModulesquery key usesstatsQueries.all()as a prefix, which may cause unintended cache invalidations if other stats queries share this prefix. Consider using a more specific key structure.♻️ Suggested key structure
{ - queryKey: [...statsQueries.all(), 'topModules', TOP_MODULES_LIMIT] as const, + queryKey: [...statsQueries.overview(), 'topModules', TOP_MODULES_LIMIT] as const, queryFn: () => tauriInvoke<ModuleStats[]>('stats_top_modules', { limit: TOP_MODULES_LIMIT }), staleTime: 60_000, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useStatsQuery.ts` around lines 27 - 30, The query key for the topModules query is too generic because it uses statsQueries.all() as a prefix; replace that with a specific key shape that uniquely identifies the top-modules query and its parameter (e.g., a dedicated factory like statsQueries.topModules(TOP_MODULES_LIMIT) or an explicit tuple ['stats','topModules', TOP_MODULES_LIMIT]) so cache entries and invalidations won’t collide with other stats queries; update the queryKey in the useStatsQuery hook (the location using queryKey: [...statsQueries.all(), 'topModules', TOP_MODULES_LIMIT]) and any invalidation or refetch calls that rely on the old key.src/views/StatisticsView/PeriodSelector.tsx (1)
17-26: Consider addingtabIndexfor proper tab keyboard navigation.For a complete tablist implementation, only the selected tab should be in the tab sequence (
tabIndex={0}), with unselected tabs havingtabIndex={-1}. Arrow keys typically navigate between tabs.This is a minor accessibility enhancement; the current implementation is functional with click/focus.
♻️ Optional keyboard navigation improvement
<Button key={period} role="tab" aria-selected={value === period} + tabIndex={value === period ? 0 : -1} variant={value === period ? 'default' : 'ghost'} size="sm" onClick={() => onChange(period)} >Note: Full keyboard navigation would also require handling arrow key events to move focus between tabs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/PeriodSelector.tsx` around lines 17 - 26, Add explicit tabIndex to the tab Buttons so only the selected tab is in the tab sequence: for each Button rendered in the PeriodSelector loop (the element using key={period}, aria-selected and onClick={() => onChange(period)}), set tabIndex to 0 when value === period and -1 otherwise; this ensures proper keyboard tab navigation (optional: implement arrow key handlers on the tab container to move focus between Button instances for full tablist behavior).
🤖 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/hooks/__tests__/useStatsQuery.test.tsx`:
- Around line 29-44: The repeated mockInvoke.mockImplementation branching for
'stats_get', 'stats_top_modules', and 'history_list' should be extracted into a
single helper (e.g., setupStatsMocks or createStatsInvokeMock) that installs
mockInvoke.mockImplementation with a canonical default payload for 'stats_get'
and default returns for the other commands, and accepts an overrides argument to
merge test-specific values; update the tests that currently call
mockInvoke.mockImplementation (the blocks around useStatsQuery tests at the
shown locations) to call this helper instead so future schema changes only need
to be updated in one place.
In `@src/hooks/useStatsQuery.ts`:
- Around line 54-56: The current refetch implementation in useStatsQuery.ts (the
refetch function that does await Promise.all(results.map((r) => r.refetch())))
swallows failures because callers only get a Promise<void> with no error
semantics; change the implementation to propagate errors by returning the
Promise returned by Promise.all (i.e., remove swallowing await/handle/wrap so
rejections bubble to callers) or explicitly switch to Promise.allSettled and
surface partial failures (e.g., throw aggregated error when any result.status
=== "rejected"); update the refetch function around results.map((r) =>
r.refetch()) to either rethrow Promise.all rejections or transform allSettled
results into a clear error/response so callers of refetch can detect failures.
- Around line 27-30: The query key for the topModules query is too generic
because it uses statsQueries.all() as a prefix; replace that with a specific key
shape that uniquely identifies the top-modules query and its parameter (e.g., a
dedicated factory like statsQueries.topModules(TOP_MODULES_LIMIT) or an explicit
tuple ['stats','topModules', TOP_MODULES_LIMIT]) so cache entries and
invalidations won’t collide with other stats queries; update the queryKey in the
useStatsQuery hook (the location using queryKey: [...statsQueries.all(),
'topModules', TOP_MODULES_LIMIT]) and any invalidation or refetch calls that
rely on the old key.
In `@src/views/StatisticsView/__tests__/KpiCard.test.tsx`:
- Around line 6-17: Add a test to verify the icon is rendered when KpiCard
receives an icon prop: render KpiCard with icon={Activity} (same as existing
tests) and assert the icon is present by checking for an SVG element (e.g.,
container.querySelector('svg') exists) or by using an accessible query like
getByRole('img' or 'presentation') / getByLabelText if the icon exposes an
aria-label; update the test file to include a new it('renders icon') case
referencing KpiCard and Activity.
In `@src/views/StatisticsView/__tests__/StatisticsView.test.tsx`:
- Around line 130-138: The test uses locale-dependent text/aria strings with
getByText/getByLabelText which will break when translations change; update the
test in StatisticsView.test.tsx to assert using stable selectors instead (e.g.,
switch from screen.getByText('Total volume'), 'Total files', 'Success rate' and
screen.getByLabelText('Daily download volume bar chart'), 'Top hosts donut
chart', 'Type breakdown horizontal bar chart', 'Average speed line chart' to
screen.getByRole or screen.getByTestId calls that target the chart components or
summary widgets), or use centralized i18n test fixtures to supply expected
labels; modify the render/test helpers so the StatisticsView renders test ids or
stable accessible names and update the three mentioned assertions (and the
similar cases at the other noted lines) to use those stable selectors.
In `@src/views/StatisticsView/DailyVolumeChart.tsx`:
- Around line 41-44: The useMemo-based mapping that creates formatted (const
formatted = useMemo(...)) is redundant because it returns objects with the same
shape as the DailyVolume items; remove the useMemo and the formatted variable
and pass the original data prop directly to the chart (replace uses of formatted
with data in the DailyVolumeChart component) so Recharts consumes the
DailyVolume[] without an unnecessary allocation.
In `@src/views/StatisticsView/format.ts`:
- Around line 31-37: formatDurationFromSeconds currently returns "0min" when
seconds > 0 but < 60 because hours and minutes compute to 0; change the logic in
formatDurationFromSeconds to detect sub-minute positive durations (seconds > 0
&& seconds < 60) and return "< 1min" before computing hours/minutes, keeping the
existing behavior for zero/negative values and the existing hours/minutes
formatting (use SECONDS_PER_HOUR, Math.floor, and padStart as before).
In `@src/views/StatisticsView/PeriodSelector.tsx`:
- Around line 17-26: Add explicit tabIndex to the tab Buttons so only the
selected tab is in the tab sequence: for each Button rendered in the
PeriodSelector loop (the element using key={period}, aria-selected and
onClick={() => onChange(period)}), set tabIndex to 0 when value === period and
-1 otherwise; this ensures proper keyboard tab navigation (optional: implement
arrow key handlers on the tab container to move focus between Button instances
for full tablist behavior).
In `@src/views/StatisticsView/StatisticsView.tsx`:
- Around line 69-75: The current unconditional full-page return when `error`
exists causes secondary query failures to hide usable `stats`; update the
`StatisticsView` logic to only render the full-page error when `stats` is falsy
(no core data), and otherwise allow the component to render normal UI while
surfacing the secondary error inline or in the affected sections; locate the
early-return that checks `if (error) { ... }` and change it to check `if
(!stats) { /* full-page error using error.message */ }` and pass the `error` (or
its message) down to the specific child section components or set a local
per-section error state so those sections can render their own inline error UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae7bdae6-1df7-437c-8864-262d8cdf26a3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
CHANGELOG.mdpackage.jsonsrc/hooks/__tests__/useStatsQuery.test.tsxsrc/hooks/useStatsQuery.tssrc/i18n/__tests__/issue30-ui-fr.test.tsxsrc/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/views/StatisticsView.tsxsrc/views/StatisticsView/ChartCard.tsxsrc/views/StatisticsView/DailyVolumeChart.tsxsrc/views/StatisticsView/KpiCard.tsxsrc/views/StatisticsView/PeriodSelector.tsxsrc/views/StatisticsView/SpeedCurveChart.tsxsrc/views/StatisticsView/StatisticsView.tsxsrc/views/StatisticsView/TopHostsChart.tsxsrc/views/StatisticsView/TopModulesCard.tsxsrc/views/StatisticsView/TypeBreakdownChart.tsxsrc/views/StatisticsView/__tests__/KpiCard.test.tsxsrc/views/StatisticsView/__tests__/StatisticsView.test.tsxsrc/views/StatisticsView/__tests__/derive.test.tssrc/views/StatisticsView/__tests__/format.test.tssrc/views/StatisticsView/chartColors.tssrc/views/StatisticsView/derive.tssrc/views/StatisticsView/format.tssrc/views/StatisticsView/index.ts
💤 Files with no reviewable changes (2)
- src/views/StatisticsView.tsx
- src/i18n/tests/issue30-ui-fr.test.tsx
- Pass dateFrom cutoff to history_list so type breakdown and speed curve stay consistent with stats_get KPIs within 7d/30d windows - Rename top modules card "Top modules (all time)" to document the backend stats_top_modules ranking is period-agnostic - Surface partial errors inline instead of replacing the dashboard when stats_get succeeded but secondary queries failed - PeriodSelector tabs set tabIndex=0 on selected, -1 otherwise - formatDurationFromSeconds returns "< 1min" for sub-minute durations - useStatsQuery.refetch propagates Promise.all failures to callers - Top-modules query key uses statsQueries.overview() prefix - Drop redundant useMemo in DailyVolumeChart; pass data directly - KpiCard tests cover icon render/absence
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/views/StatisticsView/StatisticsView.tsx (1)
60-63: Rolling period cutoff can become stale in long-lived sessions.
nowSeconds()is captured only whenhistoryorperiodchanges, so time-window filtering won’t naturally roll forward while the view stays open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/StatisticsView.tsx` around lines 60 - 63, The memoized filteredHistory captures nowSeconds() only once and becomes stale; make the time cutoff advance by adding a changing "now" dependency: create a nowSeconds value that updates on an interval (useEffect + setInterval -> state) and include that state in the useMemo dependency list, or directly call nowSeconds() inside the dependency (e.g., now) so filterEntriesByPeriod(history ?? [], period, now) re-runs; update references to filteredHistory/useMemo and ensure the timer is cleared on unmount.src/views/StatisticsView/__tests__/StatisticsView.test.tsx (1)
140-158: Add keyboard navigation coverage for period switching.This test only validates mouse click. Please add a keyboard-path assertion (arrow keys if keeping tab semantics) to prevent a11y regressions in
PeriodSelector.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/__tests__/StatisticsView.test.tsx` around lines 140 - 158, Add a keyboard-navigation assertion to the existing test so it also verifies keyboard-based period switching for the PeriodSelector: after setupMocks() and renderView(), focus the tablist/tab (use screen.getByRole('tab', { name: 'Last 30 days' }) or the tablist) and simulate keyboard navigation with userEvent.keyboard (e.g., send ArrowRight/ArrowLeft as appropriate) to move focus to the target period, then waitFor the same mockInvoke assertion that checks mockInvoke was called with 'stats_get' and period '30d'; keep using the same helpers (setupMocks, renderView, mockInvoke, userEvent) and add the keyboard path in addition to the existing mouse click check.
🤖 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/views/StatisticsView/PeriodSelector.tsx`:
- Around line 15-24: The PeriodSelector tab list uses role="tab" with
tabIndex={-1} on inactive items but lacks roving keyboard navigation, so
implement arrow-key focus movement and Home/End handling: in the PeriodSelector
component keep a ref array for the mapped Button tabs (tied to PERIODS), add an
onKeyDown handler on each tab that listens for ArrowLeft/ArrowRight (or
ArrowUp/ArrowDown if vertical) and Home/End to compute the next index, call
focus() on the appropriate ref, update aria-selected and tabIndex so only the
focused/selected tab is tabbable, and still call onChange(period) on Enter/Space
or click; ensure handlers reference the Button elements rendered in the PERIODS
map and preserve existing props (role, aria-selected, onClick).
In `@src/views/StatisticsView/StatisticsView.tsx`:
- Around line 69-75: The current fatal-error rendering uses "if (error &&
!stats)" too early and can show a full-screen error while data is still loading;
update the conditions to prefer loading first by checking the loading flag from
the data hook (e.g., use isLoading or isFetching alongside stats and error) and
only render the fatal error when error exists AND loading is false (for example:
if (error && !isLoading && !stats)); apply the same change to the other error
block referenced (the second error render around the later return) so both error
paths only show after loading has settled.
---
Nitpick comments:
In `@src/views/StatisticsView/__tests__/StatisticsView.test.tsx`:
- Around line 140-158: Add a keyboard-navigation assertion to the existing test
so it also verifies keyboard-based period switching for the PeriodSelector:
after setupMocks() and renderView(), focus the tablist/tab (use
screen.getByRole('tab', { name: 'Last 30 days' }) or the tablist) and simulate
keyboard navigation with userEvent.keyboard (e.g., send ArrowRight/ArrowLeft as
appropriate) to move focus to the target period, then waitFor the same
mockInvoke assertion that checks mockInvoke was called with 'stats_get' and
period '30d'; keep using the same helpers (setupMocks, renderView, mockInvoke,
userEvent) and add the keyboard path in addition to the existing mouse click
check.
In `@src/views/StatisticsView/StatisticsView.tsx`:
- Around line 60-63: The memoized filteredHistory captures nowSeconds() only
once and becomes stale; make the time cutoff advance by adding a changing "now"
dependency: create a nowSeconds value that updates on an interval (useEffect +
setInterval -> state) and include that state in the useMemo dependency list, or
directly call nowSeconds() inside the dependency (e.g., now) so
filterEntriesByPeriod(history ?? [], period, now) re-runs; update references to
filteredHistory/useMemo and ensure the timer is cleared on unmount.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6a46da9-e358-452e-ad07-181da490b602
📒 Files selected for processing (11)
CHANGELOG.mdsrc/hooks/__tests__/useStatsQuery.test.tsxsrc/hooks/useStatsQuery.tssrc/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/views/StatisticsView/DailyVolumeChart.tsxsrc/views/StatisticsView/PeriodSelector.tsxsrc/views/StatisticsView/StatisticsView.tsxsrc/views/StatisticsView/__tests__/KpiCard.test.tsxsrc/views/StatisticsView/__tests__/StatisticsView.test.tsxsrc/views/StatisticsView/format.ts
✅ Files skipped from review due to trivial changes (2)
- src/i18n/locales/en.json
- src/views/StatisticsView/format.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/views/StatisticsView/tests/KpiCard.test.tsx
- src/hooks/tests/useStatsQuery.test.tsx
- src/hooks/useStatsQuery.ts
- src/views/StatisticsView/DailyVolumeChart.tsx
- src/i18n/locales/fr.json
- CHANGELOG.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/views/StatisticsView/PeriodSelector.tsx">
<violation number="1" location="src/views/StatisticsView/PeriodSelector.tsx:21">
P2: This `tabIndex` change makes unselected period tabs unreachable by keyboard because roving-tabindex behavior is added without arrow-key navigation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- PeriodSelector: roving tabindex now paired with ArrowLeft/Right,
ArrowUp/Down, Home and End handlers + focus follows the new
selection so keyboard users can switch periods (was unreachable
with tabIndex={-1} on inactive tabs)
- StatisticsView: loading takes precedence over error when stats is
missing, so a fast secondary-query failure during initial load no
longer flashes the fatal screen before stats arrives
- Add French smoke test for StatisticsView covering title,
description, period tabs, KPI labels and top-modules card
- Add PeriodSelector unit tests for arrow/Home/End navigation and
tabIndex semantics
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/views/StatisticsView/__tests__/PeriodSelector.test.tsx (1)
14-63: Make the keyboard tests stateful so they cover focus movement.These cases only assert
onChangeon a controlled component with a fixedvalue, so they will not catch regressions in the rerender path that moves focus to the newly selected tab. A tiny stateful harness here would let you assert the actual focused tab afterArrow*,Home, andEndinstead of only the callback payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/__tests__/PeriodSelector.test.tsx` around lines 14 - 63, The tests currently render PeriodSelector as a controlled component with a fixed value and only assert the onChange callback; replace each render with a small stateful test harness component that uses React.useState to hold the selected period, passes that state as value to PeriodSelector and updates it in the onChange handler (or provide an inline setState callback instead of vi.fn()), then drive keyboard events with userEvent.setup() and assert the actual focused tab (screen.getByRole('tab', { name: '…' })) after ArrowRight/ArrowLeft/Home/End rather than only checking the onChange payload so focus movement and rerender behavior are covered.src/views/StatisticsView/__tests__/StatisticsView.test.tsx (1)
72-200: Add one staggered-response test for the secondary queries.All of these mocks resolve synchronously, so the suite never covers the intermediate render where
stats_gethas completed buthistory_list/stats_top_modulesare still pending. A deferred-promise case here would lock down the “loading vs empty” behavior for the history-derived charts, duration KPI, and top-modules card.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/StatisticsView/__tests__/StatisticsView.test.tsx` around lines 72 - 200, Add a new test that simulates staggered responses: use the existing mockInvoke (or setupMocks) to return an immediate resolved value for 'stats_get' and return controllable deferred promises for 'stats_top_modules' and 'history_list' (so you can resolve them later). Render the view (renderView) and assert the intermediate state after 'stats_get' completes but before resolving the deferreds (e.g., KPI duration/derived charts show loading/empty and top-modules shows the empty placeholder/testid 'top-modules-empty'), then resolve the deferred promises and await the final UI updates to assert the history cards and top-modules appear as expected; reference mockInvoke, 'stats_get', 'stats_top_modules', and 'history_list' when setting up the deferred behavior.
🤖 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/views/StatisticsView/StatisticsView.tsx`:
- Around line 60-67: The code collapses in-flight or failed secondary queries
into empty metrics by coercing history/topModules to []/0; instead, stop
defaulting history to [] in the useMemo calls and propagate an "unresolved"
value so the UI can show loading/unavailable. Concretely, change filteredHistory
to only compute when history !== undefined (e.g. filteredHistory = useMemo(() =>
history ? filterEntriesByPeriod(history, period, nowSeconds()) : undefined,
[history, period]) ), and similarly make typeBreakdown, speedSeries and
timeSavedSeconds compute only when filteredHistory is defined (calling
deriveTypeBreakdown, deriveSpeedSeries, sumDurations only if filteredHistory !==
undefined). Ensure the rendering code treats these derived values as possibly
undefined and keeps charts/Top5/time-saved in loading/unavailable state until
the backing query settles.
---
Nitpick comments:
In `@src/views/StatisticsView/__tests__/PeriodSelector.test.tsx`:
- Around line 14-63: The tests currently render PeriodSelector as a controlled
component with a fixed value and only assert the onChange callback; replace each
render with a small stateful test harness component that uses React.useState to
hold the selected period, passes that state as value to PeriodSelector and
updates it in the onChange handler (or provide an inline setState callback
instead of vi.fn()), then drive keyboard events with userEvent.setup() and
assert the actual focused tab (screen.getByRole('tab', { name: '…' })) after
ArrowRight/ArrowLeft/Home/End rather than only checking the onChange payload so
focus movement and rerender behavior are covered.
In `@src/views/StatisticsView/__tests__/StatisticsView.test.tsx`:
- Around line 72-200: Add a new test that simulates staggered responses: use the
existing mockInvoke (or setupMocks) to return an immediate resolved value for
'stats_get' and return controllable deferred promises for 'stats_top_modules'
and 'history_list' (so you can resolve them later). Render the view (renderView)
and assert the intermediate state after 'stats_get' completes but before
resolving the deferreds (e.g., KPI duration/derived charts show loading/empty
and top-modules shows the empty placeholder/testid 'top-modules-empty'), then
resolve the deferred promises and await the final UI updates to assert the
history cards and top-modules appear as expected; reference mockInvoke,
'stats_get', 'stats_top_modules', and 'history_list' when setting up the
deferred behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d734eb9-1ad7-4110-9ac3-db7ba191e1c6
📒 Files selected for processing (5)
src/i18n/__tests__/issue30-ui-fr.test.tsxsrc/views/StatisticsView/PeriodSelector.tsxsrc/views/StatisticsView/StatisticsView.tsxsrc/views/StatisticsView/__tests__/PeriodSelector.test.tsxsrc/views/StatisticsView/__tests__/StatisticsView.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/i18n/tests/issue30-ui-fr.test.tsx
- src/views/StatisticsView/PeriodSelector.tsx
Before: history and topModules were coerced to [] / 0 while still loading, so a fast stats_get response painted empty charts and a zero "time saved" KPI as if those were real zeros. Now: - filteredHistory, typeBreakdown, speedSeries, timeSavedSeconds stay undefined until history actually resolves - ChartCard accepts isLoading + loadingHint, renders a distinct placeholder separate from the empty state - TopModulesCard treats data=undefined as loading with its own test id - timeSaved KPI shows "—" until history lands instead of "0min"
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/views/StatisticsView/StatisticsView.tsx`:
- Around line 198-201: The chart cards use `isLoading={typeBreakdown ===
undefined}` and `isLoading={speedSeries === undefined}`, which leaves them stuck
if the history_list secondary query fails and those variables remain undefined;
update the logic so a settled failure renders the fallback instead of loading —
either initialize `typeBreakdown` and `speedSeries` to an empty array on error
or add explicit loading flags (e.g.,
`isTypeBreakdownLoading`/`isSpeedSeriesLoading`) and set them false on query
completion/error, then change the card props to use those flags (or check for
null vs undefined) so cards transition out of loading when the queries finish
(reference `typeBreakdown`, `speedSeries`, and the `isLoading` prop usage).
In `@src/views/StatisticsView/TopModulesCard.tsx`:
- Around line 20-35: The component currently treats `data === undefined` as
loading (the `isLoading` const in TopModulesCard.tsx), which conflates
failed/pending states; change TopModulesCard to accept an explicit boolean prop
`isLoading` and use that prop instead of computing `isLoading = data ===
undefined`, then render the loading placeholder only when the passed `isLoading`
is true; update the caller in StatisticsView to pass `isLoading={topModules ===
undefined && isLoading}` (or equivalent composed flag) so failures that leave
`data` undefined but overall settled will show the empty/fallback state rather
than a perpetual loading UI.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f05db87e-8697-4197-98f7-c712e8e0b3cd
📒 Files selected for processing (4)
src/views/StatisticsView/ChartCard.tsxsrc/views/StatisticsView/StatisticsView.tsxsrc/views/StatisticsView/TopModulesCard.tsxsrc/views/StatisticsView/__tests__/StatisticsView.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/views/StatisticsView/ChartCard.tsx
- src/views/StatisticsView/tests/StatisticsView.test.tsx
There was a problem hiding this comment.
4 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/views/StatisticsView/StatisticsView.tsx">
<violation number="1" location="src/views/StatisticsView/StatisticsView.tsx:198">
P2: The loading condition uses `undefined` as a loading sentinel, so history query failures can leave this chart stuck in a loading state.</violation>
<violation number="2" location="src/views/StatisticsView/StatisticsView.tsx:215">
P2: This loading check also conflates failed history data with in-flight loading, which can keep the speed chart in an incorrect loading state.</violation>
<violation number="3" location="src/views/StatisticsView/StatisticsView.tsx:233">
P2: Passing `topModules` directly can leave the Top Modules card stuck in loading after a modules query error.</violation>
</file>
<file name="src/views/StatisticsView/TopModulesCard.tsx">
<violation number="1" location="src/views/StatisticsView/TopModulesCard.tsx:20">
P2: Derive loading from the query state, not just `data === undefined`. As written, a failed `stats_top_modules` request leaves `data` undefined and this card stuck in a perpetual loading placeholder.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Round 3 used data === undefined as a loading sentinel, so a failed secondary query left the affected widget stuck in a loading placeholder forever (data stays undefined after the query settles with an error). useStatsQuery now exposes per-query isLoading/isError flags. The StatisticsView passes the matching flag to ChartCard / TopModulesCard / the timeSaved KPI so: - in-flight query → loading placeholder - settled with error → empty/fallback (alongside the inline error banner already rendered above) - settled with data → real values
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
• Replace StatisticsView placeholder with fully functional dashboard using Recharts
• Add 4 interactive charts: daily volume (bar), top hosts (donut), type breakdown (horizontal bar), speed curve (line)
• Implement 7 KPI metric cards and top 5 modules ranking
• Period selector for 7d/30d/all time filtering
Type
feat
Summary by cubic
Replaces the Statistics view placeholder with a real dashboard powered by
recharts, adding period filters, KPI cards, four charts, and a Top Modules ranking. Completes task 04 with accessible, theme‑aware visuals and robust loading/error handling.New Features
stats_get(period); history‑derived charts filter client‑side.stats_get), type breakdown horizontal bar and average speed line (fromhistory_list).stats_top_modules(limit: 5).useStatsQuery(period)mergesstats_get,stats_top_modules,history_listwith unified loading/error; a11y (role="img", axis labels), empty states, accent‑color theming, and i18nstatistics.*(en/fr).Bug Fixes
dateFromandlimit: 500tohistory_list; top‑modules card labeled “Top modules (all time)”.useStatsQuery.refetchrethrows the first error;formatDurationFromSecondsreturns "< 1min" for sub‑minute durations.Written for commit 8373635. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores