Repository navigation
feat(frontend): show a sparse-data state on the savings trend chart - #1334
Conversation
…e chart With a single data point (for example one ~$1 purchase) the 90-day chart painted one dot in the corner of an otherwise blank 300px canvas, which reads as broken rather than "just started". When the analytics response has fewer than two points, hide the canvas and reuse the existing #savings-trend-empty slot for "Not enough history yet to show a trend. So far: $X in cumulative savings as of <date>." so the one value we have is still visible. Two or more points draw the chart exactly as before, keeping the full-window x-axis that QA 3.1 asked for, so the range is not auto-fitted. The threshold and message live in modules/chart-utils.ts to keep dashboard.ts from growing. Adds jest coverage for the message and the state transitions, and a Playwright spec for the rendered state. Refs LeanerCloud#188 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 5 billable files and costs up to $1.25. Or wait 59 minutes for your next included review. View limit details
Comment |
Conflicts: - frontend/src/modules/chart-utils.ts: kept both additions, this branch's MIN_TREND_POINTS + formatSparseTrendMessage (LeanerCloud#188) and main's formatTrendTooltipTitle (LeanerCloud#1323), as separate exports. - frontend/src/dashboard.ts: import line now pulls formatTrendAxisTick, formatTrendTooltipTitle, formatSparseTrendMessage and MIN_TREND_POINTS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No conflicts. Checked loadSavingsTrendChart in dashboard.ts: the sparse (<2 points) path from LeanerCloud#188 sits after main's empty-state path and is independent of main's new loadYtdSparkline (LeanerCloud#1344), so both behaviours are kept as merged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE Blocking:
|
|
Local verification at
|
|
Marked ready and merged by IAmJSD's agent, at IAmJSD's request (IAmJSD reviewed the before/after screenshots; cristim is traveling), after the independent review and local verification above, all at |
Conflicts: - frontend/src/modules/chart-utils.ts: import block only; kept this branch's chart.js import (for upsertChart) and main's formatCurrency import (LeanerCloud#1376). - frontend/src/dashboard.ts: chart-utils import line only; import both main's formatSparseTrendMessage/MIN_TREND_POINTS (LeanerCloud#1334) and this branch's upsertChart. The sparse-data early return from main sits before the upsertChart call and destroys the chart like the empty state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main's LeanerCloud#1334 (issue LeanerCloud#188) shows a sparse-data message instead of a chart when the trend has fewer than MIN_TREND_POINTS (2) points. The stale-response and update-in-place tests used one point, so no chart was drawn. Use two points so they still exercise the chart path. Refs LeanerCloud#747 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts: - frontend/src/dashboard.ts: import block only. Kept this branch's lib/upcoming-scope import and took main's widened chart-utils import (formatSparseTrendMessage, MIN_TREND_POINTS from LeanerCloud#1334). renderUpcomingPurchases merged cleanly with main's LeanerCloud#1376 formatter changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts: - frontend/src/dashboard.ts (imports): kept this branch's lib/kpi-tile import and took main's widened chart-utils import (formatSparseTrendMessage, MIN_TREND_POINTS from LeanerCloud#1334). - frontend/src/dashboard.ts (renderDashboardSummary): this branch replaced the hand-built tile loop with buildKpiTile; main (LeanerCloud#1376, LeanerCloud#739) added a guard to that loop so an absent NO_VALUE figure is not styled as a saving. Kept buildKpiTile and moved main's guard into it (lib/kpi-tile.ts: valueSavings && value !== NO_VALUE), so Home keeps main's behaviour. Because Opportunities uses the same builder, its savings tile showing NO_VALUE is now neutral too, matching LeanerCloud#739's intent. Added a kpi-tile.test.ts case for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts: - frontend/src/dashboard.ts (imports): took main's widened chart-utils import (formatSparseTrendMessage, MIN_TREND_POINTS from LeanerCloud#1334) and kept this branch's lib/chart-a11y import. - frontend/src/dashboard.ts (loadSavingsTrendChart): kept this branch's removeChartTable in the no-data branch and main's new sparse-data branch (LeanerCloud#1334). The sparse branch also hides the canvas, so it now calls removeChartTable too, so a table from an earlier render does not outlive its chart (the empty and error states already do this). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e state Main's LeanerCloud#1334 shows a "not enough history yet" message instead of a chart when the savings trend has fewer than MIN_TREND_POINTS (2) points. The LeanerCloud#744 test that checks the trend table is removed on empty data started from a single point, which is now the sparse state with no chart and no table. It now starts from two points, and a new case checks the table is also removed when the data drops to the sparse state. Refs LeanerCloud#744 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#1381) * feat(frontend): add a chart text-alternative helper (summary label + table) Chart canvases are role="img", so their aria-label is all a screen reader can announce, and every Home/Purchases chart uses a fixed label with no values. lib/chart-a11y.ts adds setChartSummary (a data-driven aria-label) and renderChartTable / removeChartTable, a collapsed "View as table" <details> listing every plotted value with column and row headers. It is inserted after an anchor the caller picks so it never sits inside a box Chart.js measures, keeps its open state across data refreshes, and builds cells with textContent only. Refs #744 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(frontend): give the savings charts a value summary and a data table The Home "Savings over time" and "Potential savings range per service" charts and the Purchases savings-history chart now set an aria-label that states what is plotted (count, total, largest service or latest cumulative value) and render a "View as table" disclosure with every value; the table is removed when the chart switches to its empty or error state. The per-service table lists every service, including any beyond the 10 bars drawn. The per-service chart's stacked layers also get a 2px card-colored gap on top of each segment, so adjacent layers are separated by more than their shade difference. Refs #744 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(frontend): use two points in the trend-table test after the sparse state Main's #1334 shows a "not enough history yet" message instead of a chart when the savings trend has fewer than MIN_TREND_POINTS (2) points. The #744 test that checks the trend table is removed on empty data started from a single point, which is now the sparse state with no chart and no table. It now starts from two points, and a new case checks the table is also removed when the data drops to the sparse state. Refs #744 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts:
- frontend/src/dashboard.ts:
- imports: kept this branch's filterByEnabledProviders import and all of
main's new imports (upcoming-scope LeanerCloud#1365, sparse-trend helpers LeanerCloud#1334,
chart-a11y LeanerCloud#1381, first-run empty state LeanerCloud#1389).
- loadDashboard Promise.allSettled: combined both sides into five
fetches; this branch's loadEnabledProviders() (LeanerCloud#180) plus main's
listAccountsMinimal() for the first-run onboarding card (LeanerCloud#1389).
- call site: kept renderDashboardSummary(summaryData, recs, recsCount)
and main's renderHomeOnboarding(...). The onboarding check uses the
Enabled-Providers-filtered recs, matching the Opportunities first-run
check, which also runs on the preference-filtered list.
- helpers: kept both loadEnabledProviders() and main's
renderHomeOnboarding(); renderDashboardSummary keeps this branch's
recsCount parameter.
- savings tile detail: this branch's filtered count with main's
NO_VALUE placeholder (LeanerCloud#1376): recsCount ?? total_recommendations ??
NO_VALUE; Active Commitments takes main's NO_VALUE.
- frontend/src/recommendations.ts: import block; main's NO_VALUE added to
the utils import, kept this branch's enabled-providers import.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…1383) * fix(frontend): update savings charts in place and drop stale trend responses The Home per-service and "Savings over time" charts and the Purchases savings-history chart were destroyed and recreated on every filter, range, period or unit change, so Chart.js replayed its grow-from-zero entry animation each time. A new upsertChart helper (modules/chart-utils.ts) swaps the data and options into the existing instance and redraws with update('none'); it only creates a chart on first render or when the canvas was replaced, and the empty states still destroy the chart. loadSavingsTrendChart had no request-generation guard, so clicking one range and then another could let the older, slower response paint last and show its data under the newer range button. It now takes a generation number per call and ignores responses (and errors) from superseded calls, the same pattern riexchange.ts uses. The ladder timeline item from the issue was already fixed in #823, which updates that chart in place. Refs #747 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(frontend): type the Trends chart handle as a bar chart After merging main, webpack's ts-loader rejected trends.ts (new on main) with TS2322 assigning Chart<'bar', (number | null)[], string> to the default Chart type. tsc --noEmit still passed: Chart.js's generic variance check depends on the order files are checked in, and adding upsertChart (which takes the default ChartConfiguration) to chart-utils changed that order for ts-loader. Declaring trendsChart with the exact type it holds removes the cross-type assignment. Refs #747 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(frontend): give the #747 trend tests two data points Main's #1334 (issue #188) shows a sparse-data message instead of a chart when the trend has fewer than MIN_TREND_POINTS (2) points. The stale-response and update-in-place tests used one point, so no chart was drawn. Use two points so they still exercise the chart path. Refs #747 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
With a single data point (for example one ~$1.10 purchase), the Home "Savings over time" chart drew one tiny dot in the corner of an otherwise blank 90-day canvas, which looked broken rather than "just started". This PR replaces the canvas with a short message that still shows the one value and its date when there are fewer than two points.
Closes #188
What changed
frontend/src/modules/chart-utils.ts: newMIN_TREND_POINTS = 2andformatSparseTrendMessage(point).frontend/src/dashboard.ts(loadSavingsTrendChart): with fewer thanMIN_TREND_POINTSpoints, the canvas is hidden, any old chart is destroyed, and the existing#savings-trend-emptyslot (same muted card style and height as the no-data state) shows "Not enough history yet to show a trend. So far: $1.10 in cumulative savings as of Oct 20, 2026." Two or more points render exactly as before. The function's doc comment is updated.__tests__/chart-utils.test.ts, new cases in__tests__/dashboard.test.ts, andtests-e2e/savings-trend-sparse.spec.ts. Four existing single-point chart tests now use two points.Why this approach
The issue suggests a sparse message and/or auto-fitting the range. This uses the message: auto-fitting would undo the QA 3.1 decision that the x-axis spans the full selected window, and with one point it would collapse to a zero-width range anyway. Reusing the empty-state element avoids new markup and CSS and keeps the layout from jumping, and the message keeps the one value visible. The helpers live in
chart-utils.tsbecausedashboard.tsis already well over the 500-line guideline.Screenshots
Before is main (
a14dc8f2), after is this branch. Both are headless Chromium at 1440x900 against the repo's mocked e2e API. The scenario is the Home Savings over time card with one $1.10 point on the 90d range.Verification
Run on this branch's head
dcb4c6a8on top of maina14dc8f2:chart-utils.test.ts(exact copy, thousands separators, missing value, threshold);dashboard.test.ts(one point shows the message and creates no Chart; going from one point to two redraws the chart). New tests re-run 3x with--randomize: 14/14. With the wholedashboard.test.tsrandomized, 2loadDashboardtests are order-dependent and fail the same way on unmodified base.e2e-local.sh: 159/159. Newsavings-trend-sparse.spec.ts: one point shows the message and hides the canvas with the card height staying above 200px; two points show the chart.test-git-secrets-allowlist.sh: 137 pass.Notes for review
/purchases/history) is a different widget and is not touched.Overlaps
loadSavingsTrendChartfunction (chart summary and data table, plusremoveChartTableon the empty/error paths) and the import line, so expect a conflict. When combining, the sparse path should also callremoveChartTable.loadSavingsTrendChartandchart-utils.ts.dashboard.tsanddashboard.test.ts.Those PRs will be opened alongside this one and merged in sequence, with conflicts resolved on the later branch.
Note for the UI owner
This changes UI code that @cristim owns. IAmJSD reviewed the before/after screenshots and asked for it to be merged; any design point you disagree with can be a follow-up.
🤖 Generated with Claude Code