feat(frontend/recs): summary cards narrow on selection + drop redundant banner (closes #278 #279)#283
Conversation
…loses #278 #279) Two related fixes that together turn the Recommendations summary header into a real-time projection of the user's current intent: **#279 — all four cards + filter-status line narrow on selection** Previously only the Potential Monthly Savings card recomputed client-side from the visible set (#272). The other three (Total Recommendations, Total Upfront Cost, Payback Period) still rendered the API's flat `summary.*` fields, which sum every (term, payment) variant of every cell — overstating achievable totals by ~6× on a typical fan-out, the same bug class #272 closed for savings alone. Now every card on the header recomputes from the same source on every list rerender: - Source = `selected ∩ visible` if ≥1 row is ticked, else `visible`. - All four values are cell-by-cell range sums (savings, upfront, payback) — the user buys at most one variant per cell, so the achievable total is bounded by `sum(cell.{min,max})` not `sum(every variant)`. - When narrowing by selection, card titles flip from "Total/Potential" to "Selected" so the user knows why the number moved. Card title switches automatically on the toggle. Real-time updates: the existing checkbox change handler at `recommendations.ts:2643` already calls `renderRecommendationsList(...)` on every selection toggle, and `renderRecommendationsList` calls `renderRecommendationsSummary` (from #272 follow-up). So selection changes refresh the cards without any new wiring. The "Showing X of Y" filter-status line now also surfaces the selection count: "1 selected · Showing 2 of 2 recommendations". `pageLevelRange()` extended with `upfrontMin/Max`, `paybackMonthsMin/ Max` (cell-by-cell range sums; payback months = upfront / savings, clamped to 0 on zero-savings to avoid Infinity). `formatPaybackRange` helper added for the new range-shaped Payback Period card. The unused `summary` arg on `renderRecommendationsSummary` is renamed `_summary` for now — it can be dropped from the function signature (and the `RecommendationsSummary` field references) in a follow-up, but keeping the call-site shape stable for this PR. **#278 — drop redundant "Recommended range" banner** The banner under the table (`<div class="rec-range-banner">`) showed "Recommended range: $X – $Y/mo across N cells" — same data the Potential Monthly Savings card now renders prominently in the header. Removed. **Tests added**: - `summary cards narrow to selection in real time (closes #279)` — builds 4 recs / 2 cells, asserts cards reflect the full visible set with no selection (savings $300–$400, upfront $0–$3000, count 2), narrow to a single rec on tick (savings $150, upfront $1000, count 1), and sum two selected variants on second tick. Card titles flip from "Total/Potential ..." to "Selected ..." on the narrow. - `"Showing X of Y" surfaces selection count (closes #279)` — pins the filter-status line's selection-aware copy. - `page-level range banner is removed (closes #278)` — replaces the earlier "banner appears" assertion; verifies the banner DOM node is gone and the savings card carries the range.
|
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 moves header summary computation client-side: header cards (count, savings, upfront, payback) are computed from visible recommendations or the intersection of selected∩visible, the under-table “Recommended range” banner is removed, a Payment column/filter is added, the global bulk Payment dropdown is removed, and tests updated to cover these behaviors. ChangesSelection-Driven Summary, Table, and Actions
Sequence Diagram(s)(omitted — conditions for sequence diagrams are not met) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 |
cristim
added
triaged
|
@coderabbitai review |
✅ 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: 2
🧹 Nitpick comments (1)
frontend/src/__tests__/recommendations.test.ts (1)
241-328: ⚡ Quick winExercise the checkbox-driven rerender path in the
#279tests.These assertions swap the selection mock and call
loadRecommendations()again, so they only re-test initial load with different state snapshots. They would still pass if the checkboxchangehandler stopped rerendering the cards/live text in real time. Prefer toggling the rendered checkbox and asserting the DOM updates from that event path.🤖 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__/recommendations.test.ts` around lines 241 - 328, The tests currently flip selection by changing the state.getSelectedRecommendationIDs mock and re-calling loadRecommendations(), which skips the real checkbox change handler path; instead, after the initial loadRecommendations() call, find the rendered checkbox element corresponding to a recommendation (use the DOM input[type="checkbox"] whose value or data attribute equals the rec id, e.g. for 'cell1-pricey'), simulate a user toggle/click on that checkbox to trigger the actual change handler, then assert the summary cards/live text update; repeat for the second checkbox to test cumulative selection. Keep loadRecommendations() only for the initial render and do not replace the state.getSelectedRecommendationIDs mock between assertions so the DOM event path is exercised.
🤖 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 196-202: Replace the current global-selection logic with a
visible-selection intersection: compute selectedVisible by filtering
recommendations with state.getSelectedRecommendationIDs() (e.g., const selected
= state.getSelectedRecommendationIDs(); const selectedVisible =
recommendations.filter(r => selected.has(r.id));), then set target =
selectedVisible.length > 0 ? selectedVisible : recommendations, compute groups
via groupRecsByCell(target) and plr = pageLevelRange(groups), and set
isSelectionView = selectedVisible.length > 0 && plr.cellCount > 0; apply the
same change to the other occurrence referenced (lines 1296-1303) so both
surfaces derive from the selectedVisible slice rather than the global selected
set.
- Around line 359-392: The current pageLevelRange() computes payback by summing
savingsMin/savingsMax and upfrontMin/upfrontMax independently, which can produce
unattainable cross-combinations; change it to compute attainable total
(upfront,savings) pairs across cells and derive payback from those. Concretely:
in pageLevelRange(), for each group (variants: LocalRecommendation[]), build the
per-variant (upfront, savings) pairs (you can use the same metrics used in
cellSummary()), then perform a fold/convolution across groups starting from a
single total (0,0) and for each cell add every variant to every existing total
to produce the new set of achievable totals; after processing all groups compute
paybackMonthsMin/paybackMonthsMax from the achievable totals (upfront_total /
savings_total) for totals with savings_total>0, clamping to 0 when no positive
savings exist. Keep the other aggregates (savingsMin/Max, upfrontMin/Max) if
needed for summary cards but compute payback only from the attainable totals
computed above; update pageLevelRange to return those payback values.
---
Nitpick comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 241-328: The tests currently flip selection by changing the
state.getSelectedRecommendationIDs mock and re-calling loadRecommendations(),
which skips the real checkbox change handler path; instead, after the initial
loadRecommendations() call, find the rendered checkbox element corresponding to
a recommendation (use the DOM input[type="checkbox"] whose value or data
attribute equals the rec id, e.g. for 'cell1-pricey'), simulate a user
toggle/click on that checkbox to trigger the actual change handler, then assert
the summary cards/live text update; repeat for the second checkbox to test
cumulative selection. Keep loadRecommendations() only for the initial render and
do not replace the state.getSelectedRecommendationIDs mock between assertions so
the DOM event path is exercised.
🪄 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: 7ad49a4f-50b2-44a6-9960-611f12ffef06
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
…ction count (closes #281) The bottom-action-box summary line used to read "(1 selected)" — the selection count is the least useful info at this point, since the user can already see selection state from the row checkboxes and the visible count is in the filter status bar. Surface the *financial* impact of the current action target instead: "($100/mo · $500 upfront across 1 cell)". Same source-of-truth as the summary cards above (selected ∩ visible if ≥1 selected, else the visible set), via `pageLevelRange` on the target's cell groups. The action box is prime real estate for the dollar figures the user is about to authorise — purchase / plan flows commit to these numbers. Empty / no-selection states keep their previous copy: - 0 loaded → "(No recommendations loaded)" - 0 visible → "(0 visible — adjust filters)" - 0 selected → "(Select cells to act on)" (#273 prompt) Updated the existing test that pinned the old "1 selected" copy to assert the new format ($/mo + upfront + N cell{s}).
…umn (closes #280) Move the rec-cell-chevron button from inside the summary-content td into the checkbox-col td so it occupies the same column as variant row checkboxes. Increase hit target to 24x24 px minimum (font-size 1.2em, padding 4px, min-width/min-height 24px, display inline-flex). colspan on the summary-content td is unchanged (TABLE_COL_COUNT - 4 = 8) because the chevron was an inline child of that cell, not a separate td. Add a test asserting the button is a descendant of td.checkbox-col and update the expand test to also verify aria-expanded toggles on click.
…tion narrowing + attainable payback Finding 1 — visible-selection narrowing (renderRecommendationsSummary, renderFilterStatusBar): - Summary cards now derive `selectedVisible = recommendations ∩ selected` (where `recommendations` is the already-filtered visible set passed in from `renderRecommendationsList`). Only when `selectedVisible.length > 0` do the cards switch to selection view; selections hidden by a column filter no longer drive card values or the "Selected …" title. - `renderFilterStatusBar` intersects `state.getVisibleRecommendations()` with `state.getSelectedRecommendationIDs()` for the live-region "N selected" prefix. A selection that is entirely filtered out of view produces no prefix, matching the card behaviour. Finding 2 — attainable payback range (pageLevelRange): - Replaced the independent cross-extrema formula (upfrontMin / savingsMax) with per-cell paired variant selection. For each cell, payback-best picks the variant with the lowest upfront/savings ratio; payback-worst picks the highest. Sums of the paired (upfront, savings) pairs across cells produce bounds that correspond to purchase combinations the user can actually make. Full convolution is O(variants^cells) and impractical for pages with 30+ cells; per-cell paired choice is O(cells × variants). The docstring explains the tradeoff for future readers. - savingsMin/Max and upfrontMin/Max keep their independent cross-extrema (each is rendered on its own card with no cross-combinatorial concern). Tests (+2, 150 → 152): - "selection filtered out of view: cards and live line treat it as no selection" — column filter hides the selected row; asserts cards show the visible-set range and live line omits "N selected". - "paybackMonthsMin uses per-cell paired variants, not cross-extrema" — two cells where the lowest-upfront variant ≠ the highest-savings variant; asserts the new formula produces the correct attainable ratio and not the old misleading one.
|
Two CR findings addressed:
Tests: 150 → 152 (focused regressions for both findings; both fail on the old code). @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
The two new regression tests that fail on the old code are a good safety net. Triggering a fresh review now. [review] |
… dropdown (closes #282) Part A — Drop bulk Payment dropdown from bottom action box. Each rec carries its own payment_option from the API fan-out, and the per-cell radio enforcement caps purchase to one variant per cell, so a global toolbar override was misleading and redundant. The `payment` field is kept on BulkPurchaseToolbarState for the fan-out modal's override/fallback logic (seeded from GlobalConfig; no longer persisted to localStorage). Part B — Add sortable + filterable Payment column to the Recommendations table. New column appears between Term and Monthly Savings. Raw values are capitalised for display (all-upfront → All Upfront, etc.); missing values render as em-dash. Wired into FILTERABLE_COLUMNS, SORTABLE_STRING_COLUMNS, categoricalCellValue, numericCellValue, and SORT_HEADER_LABELS. TABLE_COL_COUNT updated 12 → 13. Tests: +4 new (Payment column header/sort, cell label rendering, categorical filter); updated 3 existing (column count 11 → 12, bottom-box dropdown assertion inverted, filter-button column list updated).
Commit ba222f6 — closes #282Two coordinated changes landed in a single commit: Part A — Drop bulk Payment dropdown Part B — Add Payment column Tests: 155 passing (net +3 new: Payment column header/sort, cell label rendering, categorical filter; 3 existing updated: column count 11 → 12, bottom-box dropdown assertion inverted, filter-button column list). @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a full review now. [review] |
|
@coderabbitai review Re-ping for the full review on commits
|
|
Tip For best results, initiate chat on the files or code changes.
Triggering the full review now across both commits. [review] |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/__tests__/recommendations.test.ts (1)
241-328: ⚡ Quick winExercise the real selection event path here, not just a fresh load.
These cases swap
getSelectedRecommendationIDs()and callloadRecommendations()again, so they only verify the narrowed cards/live text on a full rerender. They won’t catch the regression this PR is really guarding against: row-checkbox or select-all changes failing to update the summary/filter-status in place. I’d add at least one assertion that dispatches a checkboxchangeafter the initial render and verifies the cards + live line update without reloading.🤖 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__/recommendations.test.ts` around lines 241 - 328, The test currently toggles selection by mocking state.getSelectedRecommendationIDs and re-calling loadRecommendations(), but the reviewer asks to exercise the actual DOM event path so checkbox changes update the summary in-place; update the test (functions: loadRecommendations, document.querySelector / .recommendations-filter-live, and the cardValue/cardTitle helpers) to after the initial render find a row checkbox (or the select-all checkbox), change its checked state and dispatch a 'change' event on that input, then assert the cards (cardValue(/Recommendations/), cardValue(/Monthly Savings/), cardValue(/Upfront/)) and the live text update accordingly without calling loadRecommendations() again; ensure you exercise both selecting a single row checkbox and toggling the select-all to cover incremental and bulk selection paths.
🤖 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/state.ts`:
- Around line 16-19: The sort-state type is missing the new 'payment' column and
must be aligned with the column union: update RecommendationsSortColumn (or
wherever the sort union is declared) to include 'payment' (or replace the
explicit union with RecommendationsColumnId) so setRecommendationsSort({ column:
'payment', ... }) is type-safe; locate the declaration referenced by
RecommendationsSortColumn and add 'payment' to its union or refactor it to reuse
RecommendationsColumnId.
---
Nitpick comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 241-328: The test currently toggles selection by mocking
state.getSelectedRecommendationIDs and re-calling loadRecommendations(), but the
reviewer asks to exercise the actual DOM event path so checkbox changes update
the summary in-place; update the test (functions: loadRecommendations,
document.querySelector / .recommendations-filter-live, and the
cardValue/cardTitle helpers) to after the initial render find a row checkbox (or
the select-all checkbox), change its checked state and dispatch a 'change' event
on that input, then assert the cards (cardValue(/Recommendations/),
cardValue(/Monthly Savings/), cardValue(/Upfront/)) and the live text update
accordingly without calling loadRecommendations() again; ensure you exercise
both selecting a single row checkbox and toggling the select-all to cover
incremental and bulk selection paths.
🪄 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: 781e5e0e-c021-410b-ad03-853579b75d2a
📒 Files selected for processing (4)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsfrontend/src/state.tsfrontend/src/styles/components.css
✅ Files skipped from review due to trivial changes (1)
- frontend/src/recommendations.ts
| export type RecommendationsColumnId = | ||
| | 'provider' | 'account' | 'service' | 'resource_type' | 'region' | ||
| | 'count' | 'term' | 'savings' | 'upfront_cost' | ||
| | 'count' | 'term' | 'payment' | 'savings' | 'upfront_cost' | ||
| | 'monthly_cost' | 'effective_savings_pct'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Align the sort-state type with the new Payment column.
This PR adds payment as a first-class recommendations column here, and the updated tests now drive setRecommendationsSort({ column: 'payment', ... }), but RecommendationsSortColumn above still excludes it. That leaves the sort API out of sync with the UI and forces unsafe casts around the new header path. At minimum, payment should be added to the sort union in the same pass; ideally the union should match the full set of sortable headers.
🤖 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/state.ts` around lines 16 - 19, The sort-state type is missing
the new 'payment' column and must be aligned with the column union: update
RecommendationsSortColumn (or wherever the sort union is declared) to include
'payment' (or replace the explicit union with RecommendationsColumnId) so
setRecommendationsSort({ column: 'payment', ... }) is type-safe; locate the
declaration referenced by RecommendationsSortColumn and add 'payment' to its
union or refactor it to reuse RecommendationsColumnId.
…nion + selection-event tests **Sort union (state.ts)** Refactored `RecommendationsSortColumn` to alias `RecommendationsColumnId` rather than maintaining a separate union. The old union was missing `'payment'` (and the string columns `provider`, `account`, `service`, `resource_type`, `region`) added in PR #282/#283. The alias approach keeps the two in sync automatically so any future column addition never requires a second edit. The stale `'payback'` member (never a real table column) is removed; it was unused at all call sites. **Selection DOM event tests (recommendations.test.ts)** Added two new tests that exercise the real checkbox `change` event path without calling `loadRecommendations()` a second time: - `row checkbox change event updates summary cards in place` — fires a `change` on a single variant's checkbox, asserts that `renderRecommendationsList` updates cards in-place. Uses single-variant cells (distinct `resource_type`) so checkboxes are directly in the DOM. - `select-all change event updates summary cards in place` — fires `change` on `#select-all-recs`, asserts `pickBestVariantPerCell` is called once per cell and the Recommendations card shows 2 selected. Both tests wire `addSelectedRecommendation`/`removeSelectedRecommendation` mocks to mutate a shared `Set` that `getSelectedRecommendationIDs` reads back, mirroring the pattern already in use for the evict-siblings tests.
CR pass-2 fixes (commit 31a55a5)Finding 1 — sort union missing
|
|
Tip For best results, initiate chat on the files or code changes.
Finding 1 — Finding 2 — Real DOM event path tests: Triggering the full review now. [review] |
1 participant
Summary
Closes #278, closes #279. First slice of the post-#275 UX polish — rest
of the issues (#280 chevron, #281 action-summary, #282 Payment column +
drop bulk dropdown) coming as additional commits on this branch as I
work through them.
#279 — summary cards narrow on selection in real time
The four cards on the Recommendations header now all recompute client-
side from the same source on every list rerender:
selected ∩ visiblewhen ≥1 row is ticked, elsevisible.payback) — cell-by-cell min/max sums, not flat variant sums.
narrow so the user knows why the number moved.
1 selected · Showing 2 of 2 recommendations.Real-time updates land for free: the existing checkbox change handler
already calls
renderRecommendationsList(...)on every toggle, whichcalls
renderRecommendationsSummary(from #272 follow-up).#278 — drop redundant "Recommended range" banner
Same data is now front-and-centre on the savings card; the banner
under the table is just visual noise. Removed.
Test plan
summary cards narrow to selection in real time (closes #279)—4 recs / 2 cells; asserts no-selection (range), single-tick (collapse
to single value, title flips to "Selected …"), double-tick (sum of
two selected variants).
"Showing X of Y" surfaces selection count (closes #279).page-level range banner is removed (closes #278)— replacesthe earlier "banner appears" assertion.
npm test -- --testPathPattern=recommendations— 149 / 149pass.
npm run typecheck— clean.Followups in scope on this PR (incoming commits)
visible counts.
🤖 Generated with claude-flow
Summary by CodeRabbit
New Features
Improvements