feat(home/chart): stack Current/Committed band on per-service potential-range bars - #968
Conversation
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe ChangesPer-Service Savings Chart Redesign
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/__tests__/dashboard.test.ts (1)
1394-1418: ⚡ Quick winAdd one tooltip-total assertion in this stacked-layer test block.
You validate layer values, but not that tooltip
Totalmatches the displayed stack breakdown. A callback assertion here would guard the exact regression in total arithmetic.🤖 Prompt for 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. In `@frontend/src/__tests__/dashboard.test.ts` around lines 1394 - 1418, In the "three stacked datasets" test that calls renderSavingsByService and inspects the Chart mock (Chart as jest.Mock, lastCall = chartCtor.mock.calls[...], datasets, currentDs/lowestDs/upsideDs), add one assertion that invokes the chart tooltip callbacks to verify the displayed "Total" equals the sum of the stack layers (0 + 100 + 300) — e.g., grab the tooltip callbacks from lastCall[1].options.plugins.tooltip.callbacks and call the appropriate callback (label/footer) with a synthetic tooltip context for the first datapoint, then expect the returned "Total" string/value to match the computed total; this ensures tooltip arithmetic matches the dataset values.
🤖 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 774-777: The total/sort logic currently uses (e.current +
e.maxRec) which diverges from the bar rendering (which sums current + lowest
option + upside); change entries.filter and positive.sort to compute totals
using the exact same expression or helper used by the bar rendering (e.g.,
extract or call the renderTotal helper that sums the same fields the renderer
uses — current + lowestOption + upside) so filtering, tooltips, and sorting all
use that single canonical total; also update the other occurrences in the
818-883 range to use the same helper/expression.
---
Nitpick comments:
In `@frontend/src/__tests__/dashboard.test.ts`:
- Around line 1394-1418: In the "three stacked datasets" test that calls
renderSavingsByService and inspects the Chart mock (Chart as jest.Mock, lastCall
= chartCtor.mock.calls[...], datasets, currentDs/lowestDs/upsideDs), add one
assertion that invokes the chart tooltip callbacks to verify the displayed
"Total" equals the sum of the stack layers (0 + 100 + 300) — e.g., grab the
tooltip callbacks from lastCall[1].options.plugins.tooltip.callbacks and call
the appropriate callback (label/footer) with a synthetic tooltip context for the
first datapoint, then expect the returned "Total" string/value to match the
computed total; this ensures tooltip arithmetic matches the dataset values.
🪄 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: 8c01cddb-b467-46e3-b273-a9e62ca9b024
📒 Files selected for processing (2)
frontend/src/__tests__/dashboard.test.tsfrontend/src/dashboard.ts
…+ rename layers Merge the two-grouped-bar layout (potential stack + separate current stack) into a single stacked bar per service with three layers, bottom to top: 1. Current / Committed (darkest shade): realized savings from active commitments 2. Lowest option (solid base hue): max(0, min(rec) - current), the gap between the cheapest available option and what is already committed 3. Upside (35% opacity): max - min spread across recommendation options Renames: "Min potential" -> "Lowest option"; "Range" -> "Upside" (consistent across legend, tooltip, aria/aria-label, and JSDoc). Sort order changes from max-potential-desc to (current + max-potential) desc so the bar with the most total visible value is leftmost. Services present in byService but absent from recs now render a Current-only bar (commitment exists, no further upside surfaced). Tooltip shows all three layers with a total line and % of all services. Updates renderSavingsByService tests; adds two new focused tests: - service with commitments only (byService-only) renders Current band - Lowest option clamps to 0 when committed savings exceed min rec
f86ff5b to
9b05a29
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ar layers Total/sort/filter used `current + maxRec` while the bar renders `current + lowestOption + upside`. For a service with current=250, min=100, max=400 the old code reported Total=650 in the tooltip but the bar only showed 550. Fix: compute lowestOption and upside once on SvcEntry, derive visibleTotal from those, and use visibleTotal in the filter, sort key, totalSavings accumulator, and tooltip total. Update the sort-order test comment to reflect the corrected totals.
Add one assertion in the "three stacked datasets" test that invokes the tooltip label callback and verifies the returned Total line matches the sum of all three visible bar layers (current + lowestOption + upside). Guards the regression that Total/sort math now uses the same components as the rendered bar (current + lowestOption + upside) instead of the prior current + maxRec divergence.
|
Addressed CR pass-1 findings:
@coderabbitai review |
|
CR pass-1 findings disposition:
Both findings already resolved. PR is merge-ready (mergeStateStatus=CLEAN, CI green). CR re-ping deferred: org-level usage credits blocked. Re-invoke To clear after billing is fixed: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
(current + max_potential) DESCso the bar with the most total visible value is leftmost.byServicebut absent from recs now render a Current-only bar (commitment exists, no new recommendation surfaced).Design
Layer order (bottom to top, all in
stack: 'savings'):byService[svc].current_savingsdarkenHexColor(base)max(0, min(rec) - current)base(solid)max(rec) - min(rec)rgba(base, 0.35)Data sources:
recs[](fromgetRecommendations) for the rec-derived layers;byServicemap (fromloadDashboard'sgetDashboardSummary().by_service) for the Current layer. Union of both sources drives the service set.Tooltip: shows Service, Total, Current / Committed, Lowest option, Upside, Min/Max option labels.
Test plan
npx tsc --noEmitclean (zero errors)renderSavingsByServiceDOM tests: renamed dataset labels, adjusted stack assertions, updated sort-order test to include current in sort keybyServiceonly (no recs) renders Current band at the correct value with Lowest option = 0 and Upside = 0computeServiceStats/computeServiceStatsFromRecsunit tests unchanged (no logic changes to those helpers)Summary by CodeRabbit