[cristim]

Summary

Fixes findings 1-4 of #1686 on the Home page. Finding 5 (toasts covering chart content) is global toast placement and is tracked in #728.

1. Trend empty state names the range. "No purchase history yet." was shown whenever the selected range had no points, even with older history. It now reads "No savings recorded in the last 90 days." (or "the last year"), names the active provider/account filter if any, and on 7d/30d/90d adds "Older savings may show in the 1y range." A failed analytics fetch now says "Savings history could not be loaded. Reload the page to try again." instead of "Savings history is not available yet."
2. YTD Savings is labelled as an estimate. The backend computes it as monthly estimated savings times whole elapsed months (internal/api/handler_dashboard.go), so the caption changes from "From commitment purchases" to "Estimated from active commitments".
3. Per-service chart total explained. The text alternative said "$X in total", which read as a third potential-savings figure. It now says the bars add up to $X, made of current committed savings plus each service's highest-savings option, so it is not the Potential Monthly Savings range.
4. Loading state on both chart cards. Savings over time and Potential savings range per service show the shared skeleton in place of their canvas until their first chart is drawn, like the KPI cards. Later reloads (range clicks, filter changes) keep the current chart on screen instead of flashing a skeleton. The per-service skeleton is cleared in a finally, and the trend skeleton is also cleared when the dashboard load fails before the trend loader runs.

Frontend only; no API change.

Tests

• New and updated jest tests in frontend/src/__tests__/dashboard.test.ts: range-named empty text for 90d, 1y and a filtered 30d; fetch-error text; YTD caption; per-service text alternative; trend skeleton shown (canvas hidden) until the fetch settles; per-service skeleton shown until render and cleared on load failure.
• 10 of them fail against main. The "cleared on load failure" test passes on main trivially because main never shows that skeleton; it guards the failure path.
• Locally: npm test (4722 passed), npm run lint (0 errors), npm run build pass.
• Not verified in a browser locally; Frontend E2E runs in CI.

Closes #1686

🤖 Generated with Claude Code

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 47 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/issue-1686-home-savings
• File: .coderabbit.yaml
• Commit: eb79acb
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3e91649e-f380-414e-9fb6-848bb625950d

Commits

Reviewing files that changed from the base of the PR and between 18b2a9e and eb79acb.

Files selected for processing: 3

• frontend/src/__tests__/dashboard.test.ts
• frontend/src/dashboard.ts
• frontend/src/index.html

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[coderabbitai]

Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Cold review: claude-qa-ui-3, verdict APPROVE at a0ef461

I didn't author this PR. I checked it against #1686's expected behaviour and the backend source.

Checked

• YTD vs history. These come from different sources, by design. YTD is each active commitment's monthly EstimatedSavings × whole months since max(year start, purchase date) (internal/api/handler_dashboard.go:718-758). The trend uses recorded savings snapshots. The PR doesn't make them equal. It labels YTD "Estimated from active commitments" (dashboard.ts, renderDashboardSummary), which matches ux(frontend): Home savings figures disagree and the trend empty state hides existing history #1686's expected "estimated figures are labelled as estimates". So the YTD-next-to-"no history" case now reads as an estimate next to a range-scoped "No savings recorded in the last N days", not a contradiction.
• Per-service total. The text alternative now explains that the bar total is committed savings plus each service's highest option, not the Potential Monthly Savings range.
• Empty and error states.
  • Empty states name the range (the last ${days} days or the last year) and the active filter, and point to 1y for short ranges.
  • A fetch error now reads "could not be loaded", no longer "not available yet" or "no history".
  • The static "No purchase history yet" copy is gone from index.html.
• Loading state.
  • showChartSkeleton runs only before a card's first chart.
  • Both renderers un-hide their canvas: per-service at dashboard.ts ~:1051, trend at ~:1329.
  • The trend skeleton is torn down only by the latest generation.
  • The dashboard failure path tears down both skeletons, and the per-service skeleton is also cleared in a finally.
• Tests fail on main. I ran dashboard.test.ts with main's dashboard.ts and index.html. 11 tests fail: the 7 new ones, plus 4 existing tests updated for the new wording. The body says 10, which is a minor miscount. "Clears the per-service skeleton on load failure" passes on main, as the body says.
• Local gate at the head (macOS, fresh npm ci):
  • jest: 182 suites, 4722 passed, 1 skipped;
  • lint: 0 errors (existing warnings only);
  • build: compiled.

Non-blocking notes

1. Reloading after an empty or error result shows the skeleton while the previous savings-trend-empty / savings-by-service-empty text is still visible, because showChartSkeleton doesn't hide the empty element. Minor visual overlap during a reload.
2. When the whole dashboard load fails, the trend card ends with its canvas hidden and no message. The summary shows the error callout, and main left that card as a blank canvas too.
3. "Older savings may show in the 1y range." is shown even when nothing older exists. The "may" keeps it accurate.

No blocking findings. I didn't merge.

[cristim]

Independent review (claude-qa-ui-4, cold): CHANGES REQUESTED a0ef461

The copy changes are right and match #1686's expected list. YTD Savings is now captioned "Estimated from active commitments". That matches calculateCommitmentMetrics (internal/api/handler_dashboard.go:718-758: EstimatedSavings × whole elapsed months, active commitments only). The trend reads recorded savings, so the two figures still differ by design, and the caption now says why. The empty and error texts name the range, the filter and a failed load. The per-service text alternative explains its total. What's missing is test coverage of three loading behaviours the PR body claims, plus one small display bug.

1. Claimed loading behaviours with no test (mutation probes survive at this head)

• "Later reloads keep the current chart on screen instead of flashing a skeleton." Changing showChartSkeleton's guard to (chart && false) ||, which shows the skeleton even when a chart exists, passes all 143 dashboard tests. (Deleting chart || outright reads as killed only because noUnusedParameters fails the compile.)
• The trend finally only tears down for the current generation. Changing if (loadingEl && generation === savingsTrendGeneration) to if (loadingEl) passes.
• "The trend skeleton is also cleared when the dashboard load fails before the trend loader runs." Replacing if (trendLoadingEl) teardownSkeleton(trendLoadingEl); in the loadDashboard catch with void trendLoadingEl; passes. The new failure test covers only the per-service skeleton.

Please add one test per behaviour: a second loadSavingsTrendChart() with a chart drawn shows no skeleton and keeps the canvas; an older load settling after a newer one leaves the newer load's skeleton in place; a failed loadDashboard leaves no skeleton in #savings-trend-loading.

2. Bug: stale empty text next to the loading skeleton
When the previous range was empty, savingsTrendChart is null (it is destroyed on the empty branch). A range click then shows the skeleton, but #savings-trend-empty keeps the previous range's text ("No savings recorded in the last 90 days. ...") visible until the response lands. The card briefly shows one range's empty state next to another range's loading state. Hiding empty when showChartSkeleton actually shows the skeleton fixes it.

3. Non-blocking

• After a failed first loadDashboard, both chart cards end as a heading over a blank body: the canvas is hidden, the skeleton is torn down, and there is no text. The summary shows the error callout, so this is minor. A one-line "Could not load" in each card would match the "error state, not blank" intent of ux(frontend): Home savings figures disagree and the trend empty state hides existing history #1686.
• "Older savings may show in the 1y range." is appended whenever a short range is empty, even when no older data exists. The issue asked to point to a longer range "when older data exists". The hedged "may" keeps it truthful, so I'm fine with leaving it.
• Pre-existing, not from this PR, and not tracked: the Active Commitments card's "$X/mo committed" (committed_monthly) is the sum of active commitments' EstimatedSavings (aggregateActiveCommitmentsPerService, handler_dashboard.go:640-650), so it is monthly savings, not committed spend. I'll raise it with the QA lead for a separate issue.

Evidence

• Fail-before: the PR's dashboard.test.ts against main's dashboard.ts + index.html gives 11 failures / 143. That's 7 new tests, plus the updated text-alternative test and three updated empty-state tests. The body says 10; the gap is updated tests, so it doesn't matter. "Clears the per-service skeleton on failure" passes on main, as the body says.
• At this head, in a scratch worktree: npm ci, full jest 182 suites / 4722 passed (1 skipped), lint 0 errors (141 existing warnings), tsc --noEmit clean, and the build compiles.
• Probes were run one at a time on a scratch copy and reverted from git after each; the tree is clean.

Not merging.

[cristim]

Author reply to the claude-qa-ui-4 review (CHANGES REQUESTED at a0ef4616). New head: 346aa4733b1e05631948b06c44e89ec7e7dcf814, rebased onto main at af5d0b54 (#1716 landed on the adjacent KPI tile lines; the rebase applied cleanly and both captions are present).

1. Untested loading behaviours: tests added, and each of your probes now fails a test

• "A reload with a chart already drawn keeps the chart and shows no skeleton."
• "An older load settling after a newer one leaves the newer load's skeleton."
• "A failed dashboard load clears the trend skeleton and says the load failed."
• Your three probes, applied one at a time to a scratch copy and then restored: (chart && false) ||, if (loadingEl) without the generation check, and void trendLoadingEl; in the catch. Each fails exactly one test (147 total).

2. Bug: stale empty text next to the skeleton. Fixed.

showChartSkeleton now also hides the card's empty-state element when it shows the skeleton. The test "the previous range's empty text is hidden while the next range loads" fails on a0ef4616 and passes now.

3. Non-blocking: blank cards after a failed load. Done.

When loadDashboard fails and a card has no chart drawn, the trend card says "Savings history could not be loaded. Reload the page to try again." and the per-service card says "Potential savings could not be loaded. Reload the page to try again." Both are covered by tests, and both fail on a0ef4616. The hedged "Older savings may show in the 1y range." is unchanged, as you suggested. The committed_monthly label became #1709, merged as #1716.

Local, on the rebased head: npm test (182 suites, 4733 passed, 1 skipped), lint 0 errors, build OK.

[cristim]

Cold re-review: claude-qa-ui-3, verdict APPROVE at 346aa47

This reviews the fix round for claude-qa-ui-4's CHANGES REQUESTED at a0ef461. The head is 81871a0 (rebased) plus 346aa47, on main af5d0b5. The range-diff of the rebased commit against a0ef461 shows only #1716's "estimated savings" caption picked up from main.

Verified

• Item 2, stale empty text. showChartSkeleton now also hides the card's empty element when it actually shows the skeleton, on both the trend and per-service cards. Both renderers rewrite their empty text on each empty result (dashboard.ts per-service empty branch, trend empty and error branches), so a previous failure message can't reappear on a later empty result.

• Item 3, blank cards after a failed load. On a failed loadDashboard, each card with no chart drawn says it could not be loaded. A concurrent successful trend load still hides it, because the success path hides the empty element.

• Item 1, untested behaviours, re-probed on a scratch copy, one at a time, reverted after each:

  • (chart && false) || in showChartSkeleton: 1 failed / 148.
  • The trend finally without the generation check: 1 failed / 148.
  • void trendLoadingEl; in the loadDashboard catch: 1 failed / 148.
  • Deleting the new empty-hide line: the suite fails to compile (emptyId unused under noUnusedParameters), so a test doesn't catch it. But "the previous range's empty text is hidden while the next range loads" fails against a0ef461's dashboard.ts, so the behaviour is pinned.

• Fail-before against the previous head. With a0ef461's dashboard.ts, 4 of 148 fail:

  • empty text hidden during the next load;
  • trend skeleton cleared plus "could not be loaded" on a failed dashboard load;
  • per-service skeleton cleared on failure;
  • the fix(dashboard): call the Active Commitments figure estimated savings #1716 caption, which comes from main.

  "A reload keeps the chart" and "an older load leaves the newer skeleton" pass on a0ef461, as expected: they pin existing behaviour, and the probes above show they catch a regression.

• Local gate at the head (macOS, fresh npm ci):

  • jest: 182 suites, 4733 passed, 1 skipped;
  • lint: 0 errors;
  • build: compiled.

No new findings. My earlier non-blocking notes (stale empty text, blank card on failure) are both addressed. Not merged.

[cristim]

Independent re-review (claude-qa-ui-4, cold, delta to 346aa47): APPROVED 346aa47

git range-diff shows 81871a0 is a0ef461 rebased onto main after #1716. The only change is the context line #1716 relabelled ("/mo estimated savings"), and the YTD caption change is intact. The fix commit 346aa47 resolves both points of my CHANGES REQUESTED:

1. Stale empty text next to the skeleton. showChartSkeleton now takes the card's empty-state id and hides it whenever it actually shows a skeleton. A range click after an empty range no longer pairs the old range's message with the new load.
2. Untested loading behaviours. New tests pin a reload with a chart drawn (no skeleton, canvas kept), an older trend load settling after a newer one (the newer skeleton stays), and a failed loadDashboard (no trend skeleton left).

The non-blocking blank-card point is fixed too: a failed first dashboard load now shows "… could not be loaded. Reload the page to try again." in each chart card that has no chart yet.

Mutation probes (each run against dashboard.test.ts on a scratch edit, reverted from git; written so TypeScript still compiles):

• M1, skeleton shown even when a chart exists ((chart && false) ||): killed.
• M2, trend finally tears down regardless of generation: killed.
• M3, the loadDashboard catch keeps the trend skeleton (void trendLoadingEl;): killed.
• M4, the skeleton no longer hides the stale empty text (void emptyId;): killed.
• M5, a failed load leaves the trend card blank (if (false) showChartLoadFailed(): killed.

Gate at this head (scratch worktree, heavy-review lock): npm ci; full jest 182 suites / 4733 passed (1 skipped); lint 0 errors (141 existing warnings); tsc --noEmit clean; build compiles.

Not merging.

[cristim]

Rebased onto main at 18b2a9e6 after #1710 merged. New head: eb79acb04a9fcc965bc5a96d1852e18aa87bb19c; the previously approved head was 346aa473. #1710 also edited the per-service text alternative and imports, so the rebase had conflicts in dashboard.ts and dashboard.test.ts. git range-diff: commit 2 is identical. In commit 1 the only content change is that the summary keeps this PR's wording and uses #1710's serviceLabel(top.svc) ("Largest: EC2 at $300"), and the test asserts both. Both import lines are kept. Local on the new head: npm test 183 suites, 4749 passed, 1 skipped; lint 0 errors; build OK. This needs delta approvals at eb79acb0.

[cristim]

Cold re-review: claude-qa-ui-3, verdict APPROVE at eb79acb

This reviews the rebase of my approved 346aa47 onto main 18b2a9e (after #1710).

Verified

• Range-diff af5d0b54..346aa473 vs 18b2a9e6..eb79acb0:
  • Commit 2 (eb79acb) is identical (=).
  • Commit 1 (fb4ae01) differs only in the per-service text alternative. The merged line keeps this PR's wording ("The bars add up to $X: current committed savings plus each service's highest-savings option, so this is not the Potential Monthly Savings range.") and fix(frontend): show service display names instead of raw slugs #1710's serviceLabel(top.svc).
  • Its jest expectation is updated to Largest: EC2 at $300.
  • The "View as table" rows still use serviceLabel(e.svc) (dashboard.ts:1090), so neither side was lost.
• Local gate at the head (macOS, fresh npm ci, under heavy-review-2.lock):
  • jest: 183 suites, 4749 passed, 1 skipped;
  • lint: 0 errors;
  • build: compiled.
  • Playwright chart-text-alternative.spec.ts: 2 passed (CI=1, under the port-4173 lock). That's fix(frontend): show service display names instead of raw slugs #1710's literal-name rows spec.

No findings. Not merged.

[cristim]

Second review: APPROVE at eb79acb (rebased onto main 18b2a9e)

What I checked:

• Range-diff against the previously reviewed head: commit 2 (eb79acb, stale empty text and skeleton paths) is identical. Commit 1 (fb4ae01) changes only the conflict line where fix(frontend): show service display names instead of raw slugs #1710's serviceLabel display names meet this PR's Home copy. The resolution keeps both: display names from fix(frontend): show service display names instead of raw slugs #1710 and the new savings and empty-state wording from this PR.
• Exact-head local run, frontend:
  • dashboard jest: 148 passed, 1 skipped;
  • full jest: 183 suites, 4749 passed, 1 skipped;
  • lint: 0 errors (warnings unchanged);
  • tsc --noEmit: clean;
  • build: clean.
• Chart and Home E2E (chart-text-alternative, savings-history-average, savings-kpi-parity, savings-trend-sparse, first-run-empty-state): 21 passed.
• Full Playwright suite: 315 passed, 0 failed.

No findings. At posting time, 2 CI checks were still in progress; the rest are green.