fix(frontend/recs): Potential Monthly Savings card mirrors page-level range (closes #272)#276
Merged
Conversation
… range (closes #272) The stats card and the "Recommended range" banner under the table were showing different numbers for the same data, in a way that overstated achievable savings: - Card: `summary.total_monthly_savings` from the API — a flat sum across **every variant of every cell**. - Banner: `pageLevelRange(groups).savingsMin/Max` — sums per-cell min/max savings (one variant per cell). A typical 12-cell page with 2 terms × 3 payment options per cell returns up to 72 variant rows. The user can only buy ONE variant per cell, so summing all variants overstates achievable monthly savings by roughly 6× on a fully-fanned-out page (and ~75% even on smaller 2-cell × 2-variant cases). On the 33-rec dev cache the card showed $576 while the banner correctly showed $235 – $344/mo across 12 cells. Fix in `renderRecommendationsSummary` (recommendations.ts:158): compute the savings card from `pageLevelRange(groupRecsByCell(recommendations))` using the same source the banner already uses. Render via the existing `formatSavingsRange` helper, which collapses "$X – $X" to "$X" when min and max coincide (single-variant cells) — visually consistent with the banner. Empty-data path: when `cellCount === 0` or `savingsMax === 0`, the card renders `$0` (matches the previous empty behaviour). `summary.total_monthly_savings` is no longer read by the card; the backend field is left in place for now (consumers may exist outside this view), but the new comment in the function flags the next reader to recompute client-side from the same source rather than reuse the flat sum. New test `Potential Monthly Savings card mirrors page-level range, not the API sum (#272)` builds 2 cells × 2 variants where the API sum is $700 but achievable is $300 – $400, and asserts the card shows the range and never the inflated total.
cristim
added
priority/p2
Member
Author
|
@coderabbitai review |
|
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:
📝 WalkthroughWalkthroughThe PR fixes issue ChangesSavings Range Calculation Fix
Suggested labels
Possibly related PRs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 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 |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/recommendations.ts`:
- Around line 141-142: The summary card is built from the original loaded set
but the banner uses the filtered visible set, so keep a cached initial summary
(e.g., store data.summary into a module-level variable like initialSummary on
load) and then, inside the function that rerenders the list
(renderRecommendationsList or its caller), recompute or derive the card values
by combining that cached initialSummary with the current visible recommendations
(the filtered array passed into renderRecommendationsList) and call
renderRecommendationsSummary with that recomputed summary; update both the
initial load path (where you set initialSummary from data.summary) and the
list-rerender path so the card is recalculated every time filters change (also
apply same change for the other occurrence around lines 173-177).
🪄 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: 56c81ca0-9382-4166-9fe8-746305e25336
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
Comment on lines
141
to
142
| renderRecommendationsSummary(data.summary || {}, data.recommendations || []); | ||
| renderRecommendationsList(data.recommendations || []); |
There was a problem hiding this comment.
Summary card can still diverge from the banner after filters are applied
At Line 141, renderRecommendationsSummary(...) is fed the full loaded set (data.recommendations), but the banner range is computed from the filtered visible set in renderRecommendationsList (Line 1555 path). With active column filters, the two values diverge again.
A practical fix is to cache the latest summary once on load and recompute the card using the current visible recommendations on every list rerender.
Suggested direction
+let lastRecommendationsSummary: RecommendationsSummary = {};
export async function loadRecommendations(): Promise<void> {
...
- renderRecommendationsSummary(data.summary || {}, data.recommendations || []);
- renderRecommendationsList(data.recommendations || []);
+ lastRecommendationsSummary = data.summary || {};
+ renderRecommendationsList(data.recommendations || []);
+ const visible = state.getVisibleRecommendations() as unknown as LocalRecommendation[];
+ renderRecommendationsSummary(lastRecommendationsSummary, visible);
...
}
function rerenderRecommendations(): void {
const loaded = state.getRecommendations() as unknown as LocalRecommendation[];
renderRecommendationsList(loaded);
+ const visible = state.getVisibleRecommendations() as unknown as LocalRecommendation[];
+ renderRecommendationsSummary(lastRecommendationsSummary, visible);
}Also applies to: 173-177
🤖 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/recommendations.ts` around lines 141 - 142, The summary card is
built from the original loaded set but the banner uses the filtered visible set,
so keep a cached initial summary (e.g., store data.summary into a module-level
variable like initialSummary on load) and then, inside the function that
rerenders the list (renderRecommendationsList or its caller), recompute or
derive the card values by combining that cached initialSummary with the current
visible recommendations (the filtered array passed into
renderRecommendationsList) and call renderRecommendationsSummary with that
recomputed summary; update both the initial load path (where you set
initialSummary from data.summary) and the list-rerender path so the card is
recalculated every time filters change (also apply same change for the other
occurrence around lines 173-177).
…set (#272 CR) The first pass of #272 fed renderRecommendationsSummary the full loaded set on initial render, but the per-cell range banner under the table uses the post-filter visible set computed inside renderRecommendationsList. With an active column filter, the card and the banner diverged again — the same bug class #272 was supposed to close. CR caught this on the original PR; the fix moves the savings-card recompute into renderRecommendationsList so every entry to the list render (initial load, sort-header click, column-filter commit, Clear-filters badge, popover commit) recomputes the card from the *post-filter* set. Mechanics: - New module-level `lastRecommendationsSummary` caches the API-derived summary. loadRecommendations writes to it once on each successful load; renderRecommendationsList reads it on every entry. - renderRecommendationsList now calls renderRecommendationsSummary with the post-applyColumnFilters array (the same one used for the banner range and the table body). - rerenderRecommendations no longer needs to call the summary separately — every path it uses goes through renderRecommendationsList, which handles the recompute. Tests: - Existing `Potential Monthly Savings card mirrors page-level range (#272)` updated to mock state.getVisibleRecommendations so the initial-render path's new visible-set source is observable. - New `savings card recomputes from visible set on filter change (#272 CR follow-up)` builds 4 recs / 2 cells, applies a real service=ec2 column-filter mock, triggers a sort-header rerender, and asserts the card narrows from "$300 – $400" (loaded set) to "$100 – $150" (cell1 only). Test isolation: the new test explicitly resets state.getRecommendationsColumnFilters at the end. jest.clearAllMocks resets `.mock.calls` but NOT `.mockReturnValue`, so a leaked filter value would otherwise narrow the rec set in subsequent tests (observed: select-all-checkbox tests dropped from 2 → 1 selected rec because the leaked ec2 filter excluded the rds rec).
Member
Author
|
Addressed CR feedback. See follow-up commit. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
This was referenced May 5, 2026
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
Closes #272. Aligns the Potential Monthly Savings stats card with
the Recommended range banner under the table.
Before: card showed
summary.total_monthly_savingsfrom the API— a flat sum across every (term, payment) variant of every cell. On
the 33-rec dev cache the card read $576 while the banner correctly
read $235 – $344/mo across 12 cells. The user can only buy one
variant per cell, so the card overstated achievable savings by ~75-500%
depending on the variant fan-out per cell.
After: the card is computed client-side from
pageLevelRange(groupRecsByCell(recommendations))— exactly the samesource the banner uses. Renders as a range via the existing
formatSavingsRangehelper, which collapses "$X – $X" to "$X" forsingle-variant cells (so single-cell pages still show one value).
Changes
frontend/src/recommendations.ts:158::renderRecommendationsSummarynow takes
recommendationsand recomputes the savings figure fromthe same per-cell min/max sum the banner uses.
summary.total_monthly_savingsis no longer read by this view; thebackend field is left in place for callers outside this view.
Test plan
Potential Monthly Savings card mirrors page-level range, not the API sum (#272)— builds 2 cells × 2 variants wherethe API sum is $700 but achievable is $300 – $400, asserts the card
shows the range and never the inflated total.
npm test -- --testPathPattern=recommendations— 141 / 141pass.
npm run typecheck— clean.🤖 Generated with claude-flow
Summary by CodeRabbit
Bug Fixes
Tests