feat(home/chart): add per-service savings-range bar chart (closes #765) - #766
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR implements the "Savings range by service" feature (Issue ChangesPer-service savings range chart
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/dashboard.ts`:
- Around line 861-863: The per-service chart is missing the provider filter
because loadSavingsTrendChart() calls only forward account_ids; update the call
sites that now reuse loadSavingsTrendChart() (the places that call
renderSavingsByService([]) then return and the other similar call) to pass the
selected provider filter through (e.g., provider_ids or providerIds) into
loadSavingsTrendChart(), and update loadSavingsTrendChart()'s request-building
logic to include provider_ids in the analytics fetch payload so the backend
query is filtered by provider when a provider chip is selected.
- Around line 774-787: The tooltip currently labels `Median` but calculates
`s.sum / s.count` (the mean) in the `label` function (see `label: (ctx) => { ...
}`, `stats`, `totalSavings`, `s.sum`, `s.count`). Fix by either (A) renaming the
line to "Mean" (replace `Median: $${Number(median)...}` with `Mean: ...`) to
reflect the existing calculation, or (B) compute the true median by ensuring
`stats` stores the raw sample values (e.g., `s.samples`), sorting that array and
taking the middle value (or average of two middle values for even counts) and
using that value for the displayed `Median`; then remove the `s.sum / s.count`
usage for the median display. Ensure the formatted label string and variable
names match the chosen approach.
- Around line 707-724: When positive.length === 0 the function returns early but
doesn't reset the section heading, leaving a previous "(+N more)" suffix
visible; update the early-return branch to locate the heading (use
section?.querySelector('h3') or the same selector used later) and set
heading.textContent = 'Savings range by service' (the default) before returning
so the title is cleared when the chart is empty; keep the existing
canvas.classList/emptyEl toggles.
🪄 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: e8325905-ed8f-45b4-ad9f-a2ba9d2c80a2
📒 Files selected for processing (3)
frontend/src/__tests__/dashboard.test.tsfrontend/src/dashboard.tsfrontend/src/index.html
|
@coderabbitai review |
- Finding 1: move heading query above empty-state guard and reset heading.textContent to default on early return, preventing stale "+N more" suffix after dataset empties - Finding 2: replace mean (sum/count) mislabeled as Median with true median via medianOf(samples); add samples[] field to ServiceSavingsStats and populate it in computeServiceStats - Finding 3: forward currentProvider to getSavingsAnalytics so the per-service bar chart honours the provider chip filter end-to-end - Tests: extend dashboard.test.ts with three targeted assertions for each finding (heading reset, samples accumulation, provider forwarding)
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…uffix PR #766 inadvertently appended the unit suffix to Period Savings: periodSavingsEl.textContent = formatCurrency(displayTotal) + ' ' + suffix; This contradicted the inline comment one line above ("Period Savings is the cumulative total ... no per-unit rate suffix -- it is already a dollar total") and broke 4 tests in savings-history.test.ts that pin the expected output as `$5.00K`, `$500.00`, etc. (no suffix). Period Savings is a cumulative dollar total over the selected date range, not a rate -- adding "/mo" / "/hr" / "/yr" implies it is a per-unit value, which it is not. The avg / peak rows correctly retain the suffix because those ARE rates. Drop the suffix concat. The 4 failing tests now pass without modification, restoring the contract the comment documented.
Add a stacked vertical bar chart below the cumulative savings line chart on the Home page. One bar per service, split into two stacked datasets: - Floor (solid): height = min savings observed across the selected window. - Range (35% opacity): height = max - min (upside above the floor). Services are sorted by max savings descending; at most 10 are shown with a "+N more" note in the heading when truncated. Hover tooltip shows: service / min / median / max / sample count / % of total. The chart shares the existing getSavingsAnalytics() response consumed by the trend line chart, so no extra fetch is needed. It honours the same account/provider/timeframe filters because it is rendered from within loadSavingsTrendChart() on every re-fetch. Empty state: when no service has positive savings (no data points, all zeros, or on API error), the canvas is hidden and a descriptive paragraph is shown, matching the existing line-chart empty-state pattern. Changes: - frontend/src/index.html: new <section id="savings-by-service-section"> with canvas + hidden empty-state paragraph, inserted above the existing "Potential Savings by Service" section. - frontend/src/dashboard.ts: new SERVICE_BAR_COLORS palette constant, savingsByServiceChart instance variable, exported computeServiceStats() helper, exported renderSavingsByService() function, wired into the data / empty / error paths of loadSavingsTrendChart(). - frontend/src/__tests__/dashboard.test.ts: 15 new tests covering computeServiceStats accumulation, empty/zero state, two-service geometry, floor/range dataset values, sort order, chart destroy-before-recreate, missing-canvas no-op, and filter-driven re-render.
- Finding 1: move heading query above empty-state guard and reset heading.textContent to default on early return, preventing stale "+N more" suffix after dataset empties - Finding 2: replace mean (sum/count) mislabeled as Median with true median via medianOf(samples); add samples[] field to ServiceSavingsStats and populate it in computeServiceStats - Finding 3: forward currentProvider to getSavingsAnalytics so the per-service bar chart honours the provider chip filter end-to-end - Tests: extend dashboard.test.ts with three targeted assertions for each finding (heading reset, samples accumulation, provider forwarding)
…uffix PR #766 inadvertently appended the unit suffix to Period Savings: periodSavingsEl.textContent = formatCurrency(displayTotal) + ' ' + suffix; This contradicted the inline comment one line above ("Period Savings is the cumulative total ... no per-unit rate suffix -- it is already a dollar total") and broke 4 tests in savings-history.test.ts that pin the expected output as `$5.00K`, `$500.00`, etc. (no suffix). Period Savings is a cumulative dollar total over the selected date range, not a rate -- adding "/mo" / "/hr" / "/yr" implies it is a per-unit value, which it is not. The avg / peak rows correctly retain the suffix because those ARE rates. Drop the suffix concat. The 4 failing tests now pass without modification, restoring the contract the comment documented.
a741b11 to
8ed66a1
Compare
Replace mis-citations of #769 (button-disable bug) in the renderSavingsByService code path with the correct issues #908/#766 (savings-by-service chart feature). Four sites updated: - dashboard.ts: two JSDoc comments - styles/charts.css: one CSS comment - dashboard.test.ts: two comments (describe label + inline note) Genuine #769 references in recommendations.ts and recommendations.test.ts are untouched.
Summary
getSavingsAnalytics()response so no extra fetch is needed.Design
The chart is wired into the existing
loadSavingsTrendChart()call, so it automatically honours the same account/provider/timeframe filters and re-renders on every range-button click or filter-chip change. ThecomputeServiceStats()helper is a pure function (exported for testing) that iteratesdata_points[].by_serviceto produce per-service min/max/sum/count stats.Test plan
computeServiceStatsaccumulates min/max/sum/count correctly for single and multiple services, skips data points with noby_service.#savings-by-service-emptyvisible).loadSavingsTrendChart()re-renders bars with fresh data.data_pointsrestores empty state on bar chart.npx tsc --noEmitpasses with no errors.Summary by CodeRabbit