From 0b6f3b53cbda6f9e2ce975b0a2a8878874b9d702 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 5 May 2026 11:17:45 +0200 Subject: [PATCH 1/6] feat(frontend/recs): summary cards narrow on selection in real time (closes #278 #279) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (`
`) 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. --- .../src/__tests__/recommendations.test.ts | 106 ++++++++++++- frontend/src/recommendations.ts | 141 ++++++++++++++---- 2 files changed, 213 insertions(+), 34 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 1abe0edf..792205c8 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -238,6 +238,95 @@ describe('Recommendations Module', () => { (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); }); + test('summary cards narrow to selection in real time (closes #279)', async () => { + // 4 recs / 2 cells. With no selection: cards reflect the full + // visible set (savings $300–$400, upfront $0–$3000). After ticking + // cell1's pricey variant, cards narrow to that single rec + // ($150 savings / $1000 upfront). Tick the second cell's cheap + // variant too: cards sum the two selected variants ($350 savings / + // $1000 upfront). Cards must update on every selection toggle — + // the selection-toggle handler triggers a list rerender, which + // calls renderRecommendationsSummary with the new selection. + const recs = [ + { id: 'cell1-cheap', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment_option: 'no-upfront', savings: 100, upfront_cost: 0 }, + { id: 'cell1-pricey', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment_option: 'all-upfront', savings: 150, upfront_cost: 1000 }, + { id: 'cell2-cheap', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment_option: 'no-upfront', savings: 200, upfront_cost: 0 }, + { id: 'cell2-pricey', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment_option: 'all-upfront', savings: 250, upfront_cost: 2000 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: { total_count: 4, total_monthly_savings: 700, total_upfront_cost: 3000, avg_payback_months: 2 }, + recommendations: recs, + regions: [], + }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + + const cardValue = (titlePattern: RegExp): string => { + const summary = document.getElementById('recommendations-summary'); + const card = Array.from(summary?.querySelectorAll('.card') ?? []) + .find((c) => titlePattern.test(c.querySelector('h3')?.textContent ?? '')); + return card?.querySelector('.value')?.textContent ?? ''; + }; + const cardTitle = (valuePattern: RegExp): string => { + const summary = document.getElementById('recommendations-summary'); + const card = Array.from(summary?.querySelectorAll('.card') ?? []) + .find((c) => valuePattern.test(c.querySelector('.value')?.textContent ?? '')); + return card?.querySelector('h3')?.textContent ?? ''; + }; + + // No selection: cards reflect the full visible set. + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set()); + await loadRecommendations(); + expect(cardValue(/Recommendations/)).toBe('2'); // 2 cells, not 4 variants + expect(cardValue(/Monthly Savings/)).toMatch(/\$300\b/); + expect(cardValue(/Monthly Savings/)).toMatch(/\$400\b/); + expect(cardValue(/Upfront/)).toMatch(/\$0\b/); + expect(cardValue(/Upfront/)).toMatch(/\$3,?000\b/); + // Card titles reflect the "all visible" mode. + expect(cardTitle(/^2$/)).toBe('Total Recommendations'); + + // Tick one cell's pricey variant: cards narrow to that one rec. + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['cell1-pricey'])); + await loadRecommendations(); + expect(cardValue(/Recommendations/)).toBe('1'); + // Single rec → range collapses to a single value. + expect(cardValue(/Monthly Savings/)).toMatch(/^\$150$/); + expect(cardValue(/Upfront/)).toMatch(/^\$1,?000$/); + // Title switches to "Selected ..." form. + expect(cardTitle(/^1$/)).toBe('Selected Recommendations'); + + // Tick a second cell: cards sum the two selected variants. + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['cell1-pricey', 'cell2-cheap'])); + await loadRecommendations(); + expect(cardValue(/Recommendations/)).toBe('2'); + expect(cardValue(/Monthly Savings/)).toMatch(/^\$350$/); // 150 + 200 + expect(cardValue(/Upfront/)).toMatch(/^\$1,?000$/); // 1000 + 0 + }); + + test('"Showing X of Y" surfaces selection count (closes #279)', async () => { + const recs = [ + { id: 'r1', provider: 'aws', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, savings: 100, upfront_cost: 0 }, + { id: 'r2', provider: 'aws', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, savings: 200, upfront_cost: 0 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + + // No selection: line just shows "Showing X of Y". + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set()); + await loadRecommendations(); + const liveText = (): string => + document.querySelector('.recommendations-filter-live')?.textContent ?? ''; + expect(liveText()).toMatch(/^Showing 2 of 2 recommendations$/); + + // ≥1 selected: line prepends the selection count so the user has + // visible feedback that their selection is influencing the cards. + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['r1'])); + await loadRecommendations(); + expect(liveText()).toMatch(/^1 selected · Showing 2 of 2 recommendations$/); + }); + test('renders recommendations list', async () => { (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, @@ -2967,15 +3056,24 @@ describe('Issues #225 + #226: cell grouping with savings range and collapse/expa expect(variantRows.length).toBe(2); }); - test('page-level range banner appears when multi-variant cells exist', async () => { + test('page-level range banner is removed (closes #278) — savings card is the canonical surface', async () => { + // The "Recommended range" banner under the table was redundant with + // the Potential Monthly Savings card after #272 / #279 brought the + // same range to the summary header. Closing #278 removed the + // banner; the card is the single source of truth. const recs = multiVariantRecs(); (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs }); (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); await loadRecommendations(); - const banner = document.querySelector('.rec-range-banner'); - expect(banner).not.toBeNull(); - expect(banner!.textContent).toMatch(/Recommended range/); + // Banner gone. + expect(document.querySelector('.rec-range-banner')).toBeNull(); + // Card carries the range. + const summary = document.getElementById('recommendations-summary'); + const savingsCard = Array.from(summary?.querySelectorAll('.card') ?? []) + .find((c) => /Monthly Savings/.test(c.querySelector('h3')?.textContent ?? '')); + expect(savingsCard?.querySelector('.value.savings')?.textContent).toMatch(/\$\d/); }); test('single-variant cell renders a flat row, no summary row', async () => { diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 46669137..ab94d4f4 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -170,42 +170,69 @@ export async function loadRecommendations(): Promise { } function renderRecommendationsSummary( - summary: RecommendationsSummary, + _summary: RecommendationsSummary, recommendations: readonly LocalRecommendation[], ): void { const container = document.getElementById('recommendations-summary'); if (!container) return; - // Potential Monthly Savings is computed from the same source as the - // "Recommended range" banner under the table (closes #272). The previous - // implementation rendered `summary.total_monthly_savings` from the API — - // a flat sum across every (term, payment) variant of every cell — so a - // typical 12-cell page with 2 terms × 3 payments per cell showed up to - // ~6× the achievable savings (the user can only buy one variant per - // cell). pageLevelRange sums per-cell min/max and stays consistent with - // the banner. - const groups = groupRecsByCell(recommendations); + // All four summary cards (Total Recommendations, Potential Monthly + // Savings, Total Upfront Cost, Payback Period) recompute client-side + // from the same source on every list rerender (closes #279). The API- + // derived `summary` is no longer read — it summed every (term, payment) + // variant of every cell and overstated achievable totals by ~6× on a + // typical fan-out, the same bug class #272 closed for the savings card + // alone. Now extended to the full header. + // + // Selection narrowing: when the user has ticked ≥1 checkbox visible in + // the table, the cards narrow to `selected ∩ visible`. Cleared selection + // → cards snap back to the full visible set. The cards therefore reflect + // exactly what a Purchase / Plan click would commit to. + // + // The unused `_summary` arg is retained so the call sites in + // loadRecommendations / renderRecommendationsList don't have to change + // shape; it can be removed when the field is also dropped from + // RecommendationsSummary. + const selected = state.getSelectedRecommendationIDs(); + const target: readonly LocalRecommendation[] = selected.size > 0 + ? recommendations.filter((r) => selected.has(r.id)) + : recommendations; + const groups = groupRecsByCell(target); const plr = pageLevelRange(groups); + const isSelectionView = selected.size > 0 && plr.cellCount > 0; + const savingsText = plr.cellCount > 0 && plr.savingsMax > 0 ? formatSavingsRange(plr.savingsMin, plr.savingsMax) : formatCurrency(0); + const upfrontText = plr.cellCount > 0 && plr.upfrontMax > 0 + ? formatSavingsRange(plr.upfrontMin, plr.upfrontMax) + : formatCurrency(0); + const paybackText = plr.cellCount > 0 && plr.paybackMonthsMax > 0 + ? formatPaybackRange(plr.paybackMonthsMin, plr.paybackMonthsMax) + : '0 months'; + // Total Recommendations counts CELLS not variants — each cell is the + // user's actual decision unit. + const countLabel = isSelectionView ? 'Selected Recommendations' : 'Total Recommendations'; + const savingsLabel = isSelectionView ? 'Selected Monthly Savings' : 'Potential Monthly Savings'; + const upfrontLabel = isSelectionView ? 'Selected Upfront Cost' : 'Total Upfront Cost'; + const paybackLabel = 'Payback Period'; container.innerHTML = `
-

Total Recommendations

-

${summary.total_count || 0}

+

${countLabel}

+

${plr.cellCount}

-

Potential Monthly Savings

+

${savingsLabel}

${savingsText}

-

Total Upfront Cost

-

${formatCurrency(summary.total_upfront_cost)}

+

${upfrontLabel}

+

${upfrontText}

-

Payback Period

-

${summary.avg_payback_months ? summary.avg_payback_months.toFixed(1) : 0} months

+

${paybackLabel}

+

${paybackText}

`; } @@ -311,21 +338,64 @@ export function cellSummary(variants: readonly LocalRecommendation[]): CellRange return { savingsMin, savingsMax, upfrontMin, upfrontMax, termMin, termMax }; } +/** Page-level totals for the summary header (closes #279). All ranges are + * the cell-by-cell min/max sums — the user buys at most one variant per + * cell, so the achievable totals are bounded by `sum(cell.{min,max})` not + * by `sum(every variant)`. */ +export interface PageLevelRange { + savingsMin: number; + savingsMax: number; + upfrontMin: number; + upfrontMax: number; + /** payback months at the min-savings end of the range; clamped to 0 + * when both savings and upfront are 0 (nothing to pay back). */ + paybackMonthsMin: number; + /** payback months at the max-savings end. */ + paybackMonthsMax: number; + cellCount: number; +} + /** - * Compute the page-level savings range by summing per-cell min/max savings. - * overallMin = sum of savingsMin per cell - * overallMax = sum of savingsMax per cell + * Compute the page-level summary by summing per-cell min/max envelopes. + * Used by both the summary cards and the (now-removed) banner; lives at + * module scope so dashboard.ts can reuse it for the cross-page parity in + * #279. + * + * savingsMin = sum of cellSummary.savingsMin + * savingsMax = sum of cellSummary.savingsMax + * upfrontMin = sum of cellSummary.upfrontMin (matches the variant + * whose savingsMin was contributed) + * upfrontMax = sum of cellSummary.upfrontMax + * paybackMonthsMin = page-aggregate payback at the savingsMax end + * (paying off faster) — i.e. upfrontMin / savingsMax. + * paybackMonthsMax = page-aggregate payback at the savingsMin end. + * * Exported for tests. */ -export function pageLevelRange(groups: Map): { savingsMin: number; savingsMax: number; cellCount: number } { +export function pageLevelRange(groups: Map): PageLevelRange { let savingsMin = 0; let savingsMax = 0; + let upfrontMin = 0; + let upfrontMax = 0; for (const variants of groups.values()) { const s = cellSummary(variants); savingsMin += s.savingsMin; savingsMax += s.savingsMax; + upfrontMin += s.upfrontMin; + upfrontMax += s.upfrontMax; } - return { savingsMin, savingsMax, cellCount: groups.size }; + // Payback months: ratio is signed because the "fastest payback" comes + // from the max-savings / min-upfront end. Clamp to 0 when there's no + // savings at all (avoid Infinity / NaN). Negative-payback rows (where + // upfront amortization outweighs monthly savings) flow through naturally. + const paybackMonthsMin = savingsMax > 0 ? upfrontMin / savingsMax : 0; + const paybackMonthsMax = savingsMin > 0 ? upfrontMax / savingsMin : 0; + return { + savingsMin, savingsMax, + upfrontMin, upfrontMax, + paybackMonthsMin, paybackMonthsMax, + cellCount: groups.size, + }; } // Fixed payment ordering for within-cell variant sort. @@ -393,6 +463,15 @@ function formatSavingsRange(min: number, max: number): string { return lo === hi ? lo : `${lo} – ${hi}`; } +/** Format a payback-months range, collapsing identical endpoints to a + * single value and rounding to 1 decimal. Used by the Payback Period + * summary card after #279 broadened it to a range. */ +function formatPaybackRange(min: number, max: number): string { + const lo = min.toFixed(1); + const hi = max.toFixed(1); + return lo === hi ? `${lo} months` : `${lo} – ${hi} months`; +} + /** Format a term range, collapsing "1yr – 1yr" to "1yr". */ function formatTermRange(min: number, max: number): string { const lo = formatTerm(min); @@ -1215,8 +1294,13 @@ function renderFilterStatusBar(loadedCount: number, visibleCount: number): void bar.appendChild(live); } // Always update the live region (even when no filters active) so the - // spoken count reflects state after every render. - live.textContent = `Showing ${visibleCount} of ${loadedCount} recommendations`; + // spoken count reflects state after every render. #279: when the user + // has ticked ≥1 row, the line surfaces the selection count too — same + // narrowing source-of-truth as the summary cards above. + const selectedCount = state.getSelectedRecommendationIDs().size; + live.textContent = selectedCount > 0 + ? `${selectedCount} selected · Showing ${visibleCount} of ${loadedCount} recommendations` + : `Showing ${visibleCount} of ${loadedCount} recommendations`; const filters = state.getRecommendationsColumnFilters(); const activeCount = Object.keys(filters).length; @@ -1586,11 +1670,9 @@ function buildListMarkup(recommendations: LocalRecommendation[], selectedRecs: R // Only multi-variant cells count — single-variant cells render flat with no chevron. lastVisibleGroupKeys = sortedKeys.filter((k) => (groups.get(k)?.length ?? 0) > 1); - // Page-level range banner (summed across all visible cells). - const plr = pageLevelRange(groups); - const bannerHtml = plr.cellCount > 0 && plr.savingsMax > 0 - ? `
Recommended range: ${formatSavingsRange(plr.savingsMin, plr.savingsMax)}/mo across ${plr.cellCount} cell${plr.cellCount === 1 ? '' : 's'}
` - : ''; + // The "Recommended range" banner that used to live here was redundant + // with the Potential Monthly Savings card after #272 / #279 brought + // the same range to the summary header. Removed (closes #278). // Build tbody rows: grouped for multi-variant cells, flat for single-variant. const rows: string[] = []; @@ -1651,7 +1733,6 @@ function buildListMarkup(recommendations: LocalRecommendation[], selectedRecs: R } return ` - ${bannerHtml} From 6f1399b83fd8ed17e485c075779828a1a4331ba2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 5 May 2026 11:31:38 +0200 Subject: [PATCH 2/6] ux(frontend/recs): action-box summary shows financial range, not selection count (closes #281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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}). --- .../src/__tests__/recommendations.test.ts | 11 ++++-- frontend/src/recommendations.ts | 34 ++++++++++++++----- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 792205c8..df834f3c 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -729,9 +729,14 @@ describe('Recommendations Module', () => { await loadRecommendations(); const summary = document.getElementById('recommendations-action-summary'); - // #273: summary text shows just the selection count (drops the - // "of N cells visible" suffix — the visible fallback is gone). - expect(summary?.textContent).toContain('1 selected'); + // #281: summary text now surfaces the financial impact of the + // current action target (savings/mo + upfront across N cells) + // instead of just selection counts. The user can already see + // selection state from row checkboxes; the action box is prime + // real estate for the dollar figures they're authorising. + expect(summary?.textContent).toMatch(/\$100\/mo/); + expect(summary?.textContent).toMatch(/\$500 upfront/); + expect(summary?.textContent).toMatch(/1 cell\b/); // Old bulk-toolbar surface is gone. expect(document.querySelector('.recommendations-bulk-toolbar')).toBeNull(); }); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index ab94d4f4..fddad07d 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -1998,21 +1998,39 @@ function updateBottomActionBox(visibleCount: number, loadedCount: number): void 0, ); - // Action buttons require an explicit selection (closes #273): a misclick - // on "Purchase visible" / "Plan from visible" with no checkboxes ticked - // could trigger an irreversible bulk purchase across every visible row, - // and the visible set silently changes when the user clicks Refresh or - // adjusts filters. Only the "selected" form of the action buttons remains. + // Action-box summary line surfaces the *financial* impact of the + // current action target, not just selection counts (closes #281). The + // selected-vs-visible count is the least useful info at this point — + // the user can already see selection state from row checkboxes — and + // the action box is prime real estate for the dollar figures the user + // is about to authorise. Source-of-truth matches the summary cards + // above: selection ∩ visible if ≥1 selected, else the visible set. + const target: readonly LocalRecommendation[] = selectedVisibleCount > 0 + ? visible.filter((r) => selected.has(r.id)) + : visible; + const targetGroups = groupRecsByCell(target); + const targetRange = pageLevelRange(targetGroups); + const summary = document.getElementById('recommendations-action-summary'); if (summary) { if (loadedCount === 0) { summary.textContent = '(No recommendations loaded)'; } else if (visibleCount === 0) { summary.textContent = '(0 visible — adjust filters)'; - } else if (selectedVisibleCount > 0) { - summary.textContent = `(${selectedVisibleCount} selected)`; - } else { + } else if (selectedVisibleCount === 0) { summary.textContent = '(Select cells to act on)'; + } else if (targetRange.cellCount > 0) { + const savingsText = targetRange.savingsMax > 0 + ? formatSavingsRange(targetRange.savingsMin, targetRange.savingsMax) + : formatCurrency(0); + const upfrontText = targetRange.upfrontMax > 0 + ? formatSavingsRange(targetRange.upfrontMin, targetRange.upfrontMax) + : formatCurrency(0); + const cellWord = targetRange.cellCount === 1 ? 'cell' : 'cells'; + summary.textContent = `(${savingsText}/mo · ${upfrontText} upfront across ${targetRange.cellCount} ${cellWord})`; + } else { + // Shouldn't happen given the gating above, but defensive. + summary.textContent = `(${selectedVisibleCount} selected)`; } } From 048a592f934565dad697269301481bbd831aba97 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 5 May 2026 11:35:40 +0200 Subject: [PATCH 3/6] ux(frontend/recs): enlarge cell-expand chevron + move to checkbox column (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. --- .../src/__tests__/recommendations.test.ts | 20 ++++++++++++++++++- frontend/src/recommendations.ts | 9 +++++---- frontend/src/styles/components.css | 9 +++++++-- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index df834f3c..9fe4882c 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -3046,6 +3046,20 @@ describe('Issues #225 + #226: cell grouping with savings range and collapse/expa expect(variantRows.length).toBe(0); }); + test('chevron button lives inside td.checkbox-col of the summary row (closes #280)', async () => { + const recs = multiVariantRecs(); + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + await loadRecommendations(); + + const summaryRow = document.querySelector('.rec-cell-summary-row'); + expect(summaryRow).not.toBeNull(); + const checkboxCell = summaryRow!.querySelector('td.checkbox-col'); + expect(checkboxCell).not.toBeNull(); + const chevron = checkboxCell!.querySelector('.rec-cell-chevron'); + expect(chevron).not.toBeNull(); + }); + test('clicking chevron expands the cell and shows variant rows', async () => { const recs = multiVariantRecs(); (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs }); @@ -3054,11 +3068,15 @@ describe('Issues #225 + #226: cell grouping with savings range and collapse/expa const chevron = document.querySelector('.rec-cell-chevron'); expect(chevron).not.toBeNull(); + // aria-expanded starts false (collapsed) + expect(chevron!.getAttribute('aria-expanded')).toBe('false'); chevron!.click(); - // After expand: variant rows should appear + // After expand: variant rows should appear and aria-expanded flips const variantRows = document.querySelectorAll('.rec-variant-row'); expect(variantRows.length).toBe(2); + const updatedChevron = document.querySelector('.rec-cell-chevron'); + expect(updatedChevron!.getAttribute('aria-expanded')).toBe('true'); }); test('page-level range banner is removed (closes #278) — savings card is the canonical surface', async () => { diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index fddad07d..9dc3b8fc 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -1711,14 +1711,15 @@ function buildListMarkup(recommendations: LocalRecommendation[], selectedRecs: R rows.push(` - + diff --git a/frontend/src/styles/components.css b/frontend/src/styles/components.css index 5fff9fc4..b4775761 100644 --- a/frontend/src/styles/components.css +++ b/frontend/src/styles/components.css @@ -913,10 +913,15 @@ tr.rec-cell-summary-row:hover { background: transparent; border: none; cursor: pointer; - font-size: 0.8em; + font-size: 1.2em; color: #555; - padding: 0 0.25rem; + padding: 4px; + min-width: 24px; + min-height: 24px; line-height: 1; + display: inline-flex; + align-items: center; + justify-content: center; } .rec-cell-chevron:hover { color: #1a73e8; From 6369745f6008db97aec17e4a2ef0bc4763614494 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 5 May 2026 11:41:05 +0200 Subject: [PATCH 4/6] =?UTF-8?q?fix(frontend/recs):=20address=20PR=20#283?= =?UTF-8?q?=20CodeRabbit=20review=20=E2=80=94=20visible-selection=20narrow?= =?UTF-8?q?ing=20+=20attainable=20payback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/__tests__/recommendations.test.ts | 101 ++++++++++++++++++ frontend/src/recommendations.ts | 84 ++++++++++++--- 2 files changed, 168 insertions(+), 17 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 9fe4882c..ee1895ac 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -327,6 +327,62 @@ describe('Recommendations Module', () => { expect(liveText()).toMatch(/^1 selected · Showing 2 of 2 recommendations$/); }); + test('selection filtered out of view: cards and live line treat it as no selection (PR #283 CR)', async () => { + // 4 recs / 2 cells. The user has selected cell2's variant ('r3'), but + // then applies a column filter that hides cell2. The visible set is + // cell1 only (r1, r2). The selected row (r3) is NOT in the visible set. + // + // Expected behaviour: + // - Summary cards reflect the visible (unselected-from-visible) set: + // cell1 range $100–$150, title "Total Recommendations" not "Selected". + // - Live status line reads "Showing 2 of 4 recommendations" — no + // "1 selected" prefix, because r3 is hidden. + const allRecs = [ + { id: 'r1', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment_option: 'no-upfront', savings: 100, upfront_cost: 0 }, + { id: 'r2', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment_option: 'all-upfront', savings: 150, upfront_cost: 1000 }, + { id: 'r3', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment_option: 'no-upfront', savings: 200, upfront_cost: 0 }, + { id: 'r4', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment_option: 'all-upfront', savings: 250, upfront_cost: 2000 }, + ]; + const visibleRecs = allRecs.filter((r) => r.service === 'ec2'); // cell1 only + + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: {}, recommendations: allRecs, regions: [], + }); + (state.getRecommendations as jest.Mock).mockReturnValue(allRecs); + // Visible set is cell1 only — column filter hides rds rows. + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(visibleRecs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({ + service: { kind: 'set', values: ['ec2'] }, + }); + // r3 is selected, but it is NOT in the visible set. + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['r3'])); + + await loadRecommendations(); + + const summary = document.getElementById('recommendations-summary'); + const savingsCard = Array.from(summary?.querySelectorAll('.card') ?? []) + .find((c) => /Monthly Savings/.test(c.querySelector('h3')?.textContent ?? '')); + const savingsValue = savingsCard?.querySelector('.value')?.textContent ?? ''; + const savingsTitle = savingsCard?.querySelector('h3')?.textContent ?? ''; + + // Cards reflect the visible set (cell1: $100–$150), not the hidden selection. + expect(savingsValue).toContain('$100'); + expect(savingsValue).toContain('$150'); + expect(savingsValue).not.toContain('$200'); + // Title must not flip to "Selected …" since no visible row is selected. + expect(savingsTitle).toBe('Potential Monthly Savings'); + + // Live line must not prefix "1 selected" because r3 is hidden. + const liveText = document.querySelector('.recommendations-filter-live')?.textContent ?? ''; + expect(liveText).not.toMatch(/selected/); + expect(liveText).toMatch(/Showing/); + + // Reset mocks so this filter doesn't leak into subsequent tests. + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue([]); + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set()); + }); + test('renders recommendations list', async () => { (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, @@ -3002,6 +3058,51 @@ describe('Issues #225 + #226: cell grouping with savings range and collapse/expa expect(plr.savingsMin).toBe(0); expect(plr.savingsMax).toBe(0); }); + + test('paybackMonthsMin uses per-cell paired variants, not cross-extrema (PR #283 CR)', () => { + // Cell A has two variants: + // v1: savings=50, upfront=0 → ratio = 0 (best payback: 0 months) + // v2: savings=200, upfront=300 → ratio = 1.5 (1.5 months) + // + // Cell B has two variants: + // v3: savings=100, upfront=200 → ratio = 2 (2 months) + // v4: savings=150, upfront=600 → ratio = 4 (4 months) + // + // Attainable min-payback selection: cell A picks v1 (ratio 0), cell B + // picks v3 (ratio 2). Total upfront=0+200=200, total savings=50+100=150. + // paybackMonthsMin = 200/150 ≈ 1.333... + // + // Cross-extrema (old code) would compute: + // upfrontMin = 0 (from v1) + 200 (from v3) = 200 + // savingsMax = 200 (from v2) + 150 (from v4) = 350 + // paybackMonthsMin = 200/350 ≈ 0.571 + // That's not attainable because it mixes upfront from v1 with savings + // from v2 (different variants of cell A). + // + // The new per-cell paired logic must produce 200/150 ≈ 1.333, not 0.571. + const cellA = [ + mkRec({ id: 'a-v1', resource_type: 'm5.large', savings: 50, upfront_cost: 0 }), + mkRec({ id: 'a-v2', resource_type: 'm5.large', savings: 200, upfront_cost: 300 }), + ]; + const cellB = [ + mkRec({ id: 'b-v3', resource_type: 't3.medium', savings: 100, upfront_cost: 200 }), + mkRec({ id: 'b-v4', resource_type: 't3.medium', savings: 150, upfront_cost: 600 }), + ]; + const groups = groupRecsByCell([...cellA, ...cellB]); + const plr = pageLevelRange(groups); + + // Per-cell paired: best ratio for A is v1 (0/50=0), best for B is v3 (200/100=2). + // Combined: upfront=0+200=200, savings=50+100=150 → paybackMin=200/150≈1.333 + expect(plr.paybackMonthsMin).toBeCloseTo(200 / 150, 5); + + // Cross-extrema would give 200/350≈0.571 — must NOT be the result. + expect(plr.paybackMonthsMin).not.toBeCloseTo(200 / 350, 5); + + // paybackMonthsMax: worst payback per cell. A picks v2 (300/200=1.5), + // B picks v4 (600/150=4). Combined: upfront=300+600=900, savings=200+150=350. + // paybackMax = 900/350 ≈ 2.571 + expect(plr.paybackMonthsMax).toBeCloseTo(900 / 350, 5); + }); }); describe('DOM rendering (cell grouping integrated)', () => { diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 9dc3b8fc..b7538e7e 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -194,12 +194,17 @@ function renderRecommendationsSummary( // shape; it can be removed when the field is also dropped from // RecommendationsSummary. const selected = state.getSelectedRecommendationIDs(); - const target: readonly LocalRecommendation[] = selected.size > 0 - ? recommendations.filter((r) => selected.has(r.id)) + // Narrow selection to visible rows only — if the user has ticked rows that + // are currently hidden by a column filter, those rows must not drive the + // summary cards (the cards should reflect what the user can actually see + // and act on). An empty visible-intersection means no effective selection. + const selectedVisible = recommendations.filter((r) => selected.has(r.id)); + const target: readonly LocalRecommendation[] = selectedVisible.length > 0 + ? selectedVisible : recommendations; const groups = groupRecsByCell(target); const plr = pageLevelRange(groups); - const isSelectionView = selected.size > 0 && plr.cellCount > 0; + const isSelectionView = selectedVisible.length > 0 && plr.cellCount > 0; const savingsText = plr.cellCount > 0 && plr.savingsMax > 0 ? formatSavingsRange(plr.savingsMin, plr.savingsMax) @@ -366,9 +371,21 @@ export interface PageLevelRange { * upfrontMin = sum of cellSummary.upfrontMin (matches the variant * whose savingsMin was contributed) * upfrontMax = sum of cellSummary.upfrontMax - * paybackMonthsMin = page-aggregate payback at the savingsMax end - * (paying off faster) — i.e. upfrontMin / savingsMax. - * paybackMonthsMax = page-aggregate payback at the savingsMin end. + * paybackMonthsMin = sum of per-cell best-payback variant upfronts / + * sum of their savings. Each cell independently + * picks the variant with the lowest upfront/savings + * ratio. Using independent cross-extrema + * (upfrontMin / savingsMax) is NOT attainable when a + * cell's lowest-upfront variant ≠ its highest-savings + * variant — it would imply buying one impossible + * combination. Full convolution (trying every + * cross-cell combination) is O(variants^cells) and + * impractical for pages with 30+ cells. Per-cell + * paired choice is O(cells × variants) and produces + * bounds that match what a min-payback / max-payback + * per-cell selection would actually commit to. + * paybackMonthsMax = same logic, each cell picks the worst-payback + * variant. * * Exported for tests. */ @@ -377,19 +394,48 @@ export function pageLevelRange(groups: Map): Page let savingsMax = 0; let upfrontMin = 0; let upfrontMax = 0; + // Payback accumulators: per-cell paired variant sums. + let paybackBestUpfront = 0; + let paybackBestSavings = 0; + let paybackWorstUpfront = 0; + let paybackWorstSavings = 0; for (const variants of groups.values()) { const s = cellSummary(variants); savingsMin += s.savingsMin; savingsMax += s.savingsMax; upfrontMin += s.upfrontMin; upfrontMax += s.upfrontMax; + + // For payback: pick the variant with the best (lowest) payback ratio for + // the min-end, and the worst (highest) for the max-end. Treat savings ≤ 0 + // as Infinity payback so zero-savings variants sort to the worst end. + let bestRatio = Infinity; + let bestUpfront = 0; + let bestSavings = 0; + let worstRatio = -Infinity; + let worstUpfront = 0; + let worstSavings = 0; + for (const v of variants) { + const ratio = v.savings > 0 ? v.upfront_cost / v.savings : Infinity; + if (ratio < bestRatio) { + bestRatio = ratio; + bestUpfront = v.upfront_cost; + bestSavings = v.savings; + } + if (ratio > worstRatio) { + worstRatio = ratio; + worstUpfront = v.upfront_cost; + worstSavings = v.savings; + } + } + paybackBestUpfront += bestUpfront; + paybackBestSavings += bestSavings; + paybackWorstUpfront += worstUpfront; + paybackWorstSavings += worstSavings; } - // Payback months: ratio is signed because the "fastest payback" comes - // from the max-savings / min-upfront end. Clamp to 0 when there's no - // savings at all (avoid Infinity / NaN). Negative-payback rows (where - // upfront amortization outweighs monthly savings) flow through naturally. - const paybackMonthsMin = savingsMax > 0 ? upfrontMin / savingsMax : 0; - const paybackMonthsMax = savingsMin > 0 ? upfrontMax / savingsMin : 0; + // Clamp to 0 when total savings is non-positive to avoid Infinity / NaN. + const paybackMonthsMin = paybackBestSavings > 0 ? paybackBestUpfront / paybackBestSavings : 0; + const paybackMonthsMax = paybackWorstSavings > 0 ? paybackWorstUpfront / paybackWorstSavings : 0; return { savingsMin, savingsMax, upfrontMin, upfrontMax, @@ -1295,11 +1341,15 @@ function renderFilterStatusBar(loadedCount: number, visibleCount: number): void } // Always update the live region (even when no filters active) so the // spoken count reflects state after every render. #279: when the user - // has ticked ≥1 row, the line surfaces the selection count too — same - // narrowing source-of-truth as the summary cards above. - const selectedCount = state.getSelectedRecommendationIDs().size; - live.textContent = selectedCount > 0 - ? `${selectedCount} selected · Showing ${visibleCount} of ${loadedCount} recommendations` + // has ticked ≥1 visible row, the line surfaces the selection count too — + // same narrowing source-of-truth as the summary cards. Selections that + // are filtered out of view are NOT counted here, matching the card + // narrowing behaviour (only selected ∩ visible drives the UI). + const selectedIDs = state.getSelectedRecommendationIDs(); + const visibleRecs = state.getVisibleRecommendations(); + const selectedVisibleCount = visibleRecs.filter((r) => selectedIDs.has(r.id)).length; + live.textContent = selectedVisibleCount > 0 + ? `${selectedVisibleCount} selected · Showing ${visibleCount} of ${loadedCount} recommendations` : `Showing ${visibleCount} of ${loadedCount} recommendations`; const filters = state.getRecommendationsColumnFilters(); From ba222f605dc532b2d62b95e0b5c37b33cc461efe Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 5 May 2026 11:52:43 +0200 Subject: [PATCH 5/6] feat(frontend/recs): add Payment column + drop redundant bulk Payment dropdown (closes #282) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .../src/__tests__/recommendations.test.ts | 84 +++++++++++++++++-- frontend/src/recommendations.ts | 84 ++++++++++--------- frontend/src/state.ts | 2 +- 3 files changed, 123 insertions(+), 47 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index ee1895ac..5cc7edf5 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -746,15 +746,15 @@ describe('Recommendations Module', () => { }); }); - test('renders sortable column headers with indicators (Bundle B: 11 columns)', async () => { + test('renders sortable column headers with indicators (Bundle B + #282: 12 columns)', async () => { await loadRecommendations(); const list = document.getElementById('recommendations-list'); - // Bundle B: every data column is sortable. 11 sortable data columns: - // provider, account, service, resource_type, region, count, term, + // Bundle B + issue #282: every data column is sortable. 12 sortable data columns: + // provider, account, service, resource_type, region, count, term, payment, // savings, upfront_cost, monthly_cost, effective_savings_pct. // The leading checkbox column is not sortable. const sortables = list?.querySelectorAll('th.sortable'); - expect(sortables?.length).toBe(11); + expect(sortables?.length).toBe(12); // The default sort is savings desc → that header shows an active ▼. const savingsHeader = list?.querySelector('th[data-sort="savings"]'); expect(savingsHeader?.innerHTML).toContain('active'); @@ -767,6 +767,73 @@ describe('Recommendations Module', () => { expect(state.setRecommendationsSort).toHaveBeenCalledWith({ column: 'upfront_cost', direction: 'desc' }); }); + test('Payment column header renders and is sortable (#282)', async () => { + await loadRecommendations(); + const list = document.getElementById('recommendations-list'); + const paymentTh = list?.querySelector('th[data-sort="payment"]'); + expect(paymentTh).not.toBeNull(); + expect(paymentTh?.textContent).toContain('Payment'); + // Clicking the Payment header should call setRecommendationsSort with 'payment'. + (paymentTh as HTMLElement)?.click(); + expect(state.setRecommendationsSort).toHaveBeenCalledWith({ column: 'payment', direction: 'desc' }); + }); + + test('Payment column cell renders human-readable labels (#282)', async () => { + const recs = [ + { id: 'p1', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.small', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', savings: 100, upfront_cost: 1000 }, + { id: 'p2', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment: 'partial-upfront', savings: 200, upfront_cost: 500 }, + { id: 'p3', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-west-2', count: 1, term: 1, payment: 'no-upfront', savings: 300, upfront_cost: 0 }, + { id: 'p4', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.m5', region: 'us-west-2', count: 1, term: 3, payment: 'monthly', savings: 400, upfront_cost: 0 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set()); + await loadRecommendations(); + + const rows = document.querySelectorAll('tr.recommendation-row'); + const paymentCells = Array.from(rows).map((row) => { + // Payment column is the 9th + @@ -1698,8 +1710,8 @@ function buildVariantRowMarkup(rec: LocalRecommendation, selectedRecs: ReadonlyS `; } -// The total column count for colspan on summary rows: checkbox col + 11 data cols = 12. -const TABLE_COL_COUNT = 12; +// The total column count for colspan on summary rows: checkbox col + 12 data cols = 13. +const TABLE_COL_COUNT = 13; function buildListMarkup(recommendations: LocalRecommendation[], selectedRecs: ReadonlySet): string { const sort = state.getRecommendationsSort(); @@ -1835,9 +1847,13 @@ const BULK_PURCHASE_LS_KEY = 'cudly.recommendations.bulkPurchase.v1'; // BulkPurchaseToolbarState used to carry a `term` field that overrode each // row's recommended term at API-call time. Bundle B drops it: each rec is // purchased with its own per-row term (see term-aware bucketing in -// handleBulkPurchaseClick). loadBulkPurchaseState explicitly picks known -// fields so any legacy `term` from older localStorage values is silently -// ignored on read — no migration shim needed. +// handleBulkPurchaseClick). Issue #282 drops the global Payment dropdown +// from the toolbar: the `payment` field is kept internally (seeded from +// GlobalConfig or 'all-upfront') so the fan-out modal's override/fallback +// logic continues to work, but it is no longer exposed in the UI or +// persisted to localStorage. loadBulkPurchaseState explicitly picks known +// fields so any legacy `term` or `payment` from older localStorage values +// is silently ignored on read — no migration shim needed. type BulkPurchasePayment = 'all-upfront' | 'partial-upfront' | 'no-upfront' | 'monthly'; // Centralized bucket-level payment compatibility check. A bucket is @@ -1869,11 +1885,11 @@ function loadBulkPurchaseState(): BulkPurchaseToolbarState { if (!raw) return { ...defaultBulkPurchaseState }; const parsed = JSON.parse(raw) as Partial & { term?: unknown }; // Explicit field-pick rather than spread-and-omit — avoids leaking a - // legacy `term` value into the returned object even at runtime. + // legacy `term` or `payment` from older localStorage values at runtime. + // Payment is seeded from GlobalConfig only (issue #282 drops the toolbar + // dropdown; the field is internal-only, not persisted). return { - // issue #223: fall back to resolved GlobalConfig.DefaultPayment rather - // than the literal 'all-upfront' when localStorage has no stored value. - payment: (parsed.payment || cachedGlobalDefaultPayment) as BulkPurchasePayment, + payment: cachedGlobalDefaultPayment as BulkPurchasePayment, capacity: Math.max(1, Math.min(100, Number(parsed.capacity) || 100)), }; } catch { @@ -1883,7 +1899,9 @@ function loadBulkPurchaseState(): BulkPurchaseToolbarState { function saveBulkPurchaseState(s: BulkPurchaseToolbarState): void { try { - localStorage.setItem(BULK_PURCHASE_LS_KEY, JSON.stringify(s)); + // Only persist capacity — payment is dropped from the toolbar (issue #282) + // and is now session-only, seeded from GlobalConfig. + localStorage.setItem(BULK_PURCHASE_LS_KEY, JSON.stringify({ capacity: s.capacity })); } catch { // Private-browsing / quota-exceeded — non-fatal, just lose the // sticky choice. The bottom box still works in-session. @@ -1897,11 +1915,15 @@ function saveBulkPurchaseState(s: BulkPurchaseToolbarState): void { // leaving the input/select elements (and their in-progress values) alone. // // IDs preserved for backward compatibility: -// #bulk-purchase-payment (Payment dropdown) // #bulk-purchase-capacity (Capacity % input — read by app.ts:307) // #bulk-purchase-btn (Purchase one-off button) // #create-plan-btn (Create Purchase Plan button — relocated from // the old top filter bar) +// +// Issue #282: the bulk Payment dropdown (#bulk-purchase-payment) is removed. +// Each rec carries its own payment_option from the API fan-out; the per-cell +// radio enforcement caps purchase to one variant per cell. A global override +// was misleading and is redundant. function mountBottomActionBox(): HTMLElement | null { const recsTab = document.getElementById('recommendations-tab'); if (!recsTab) return null; @@ -1923,21 +1945,6 @@ function mountBottomActionBox(): HTMLElement | null { summary.className = 'recommendations-action-summary'; box.appendChild(summary); - // Payment dropdown — preserved ID - const paymentLabel = document.createElement('label'); - paymentLabel.textContent = 'Payment '; - const paymentSelect = document.createElement('select'); - paymentSelect.id = 'bulk-purchase-payment'; - [['all-upfront', 'All Upfront'], ['partial-upfront', 'Partial Upfront'], ['no-upfront', 'No Upfront'], ['monthly', 'Monthly']].forEach(([v, t]) => { - const opt = document.createElement('option'); - opt.value = v as string; - opt.textContent = t as string; - if (v === tbState.payment) opt.selected = true; - paymentSelect.appendChild(opt); - }); - paymentLabel.appendChild(paymentSelect); - box.appendChild(paymentLabel); - // Capacity % input — preserved ID (app.ts:307 reads this) const capacityLabel = document.createElement('label'); capacityLabel.textContent = 'Capacity % '; @@ -1986,11 +1993,10 @@ function mountBottomActionBox(): HTMLElement | null { const persist = (): void => { saveBulkPurchaseState({ - payment: paymentSelect.value as BulkPurchaseToolbarState['payment'], + payment: tbState.payment, capacity: Math.max(1, Math.min(100, parseInt(capacityInput.value, 10) || 100)), }); }; - paymentSelect.addEventListener('change', persist); capacityInput.addEventListener('change', persist); purchaseBtn.addEventListener('click', () => { diff --git a/frontend/src/state.ts b/frontend/src/state.ts index af8f3166..647efeeb 100644 --- a/frontend/src/state.ts +++ b/frontend/src/state.ts @@ -15,7 +15,7 @@ export interface RecommendationsSort { // Typo-safety: misspellings at call sites become compile errors. 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'; export type RecommendationsColumnFilter = From 31a55a523b573ca1bb59f244cc465c12629c2d28 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 5 May 2026 12:49:23 +0200 Subject: [PATCH 6/6] =?UTF-8?q?fix(frontend/recs):=20address=20PR=20#283?= =?UTF-8?q?=20CodeRabbit=20pass-2=20review=20=E2=80=94=20sort-union=20+=20?= =?UTF-8?q?selection-event=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **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. --- .../src/__tests__/recommendations.test.ts | 119 ++++++++++++++++++ frontend/src/state.ts | 16 ++- 2 files changed, 129 insertions(+), 6 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 5cc7edf5..135f458b 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -304,6 +304,125 @@ describe('Recommendations Module', () => { expect(cardValue(/Upfront/)).toMatch(/^\$1,?000$/); // 1000 + 0 }); + test('row checkbox change event updates summary cards in place (PR #283 CR pass-2)', async () => { + // Regression guard for the real DOM event path: verifies that dispatching + // a `change` event on a row checkbox updates summary cards WITHOUT a + // second loadRecommendations() call. The earlier mock-and-reload test + // above confirms the rendering math; this test confirms the event + // handler itself triggers renderRecommendationsList. + // + // Each rec is a DISTINCT cell (different resource_type) so they render as + // flat rows — not grouped summary rows — making their checkboxes directly + // queryable from the DOM. Multi-variant cells collapse into a summary row + // with a chevron button; variant-level checkboxes are only in the DOM when + // the cell is expanded. + // + // Mock coordination: addSelectedRecommendation / removeSelectedRecommendation + // mutate a shared Set so getSelectedRecommendationIDs reflects the toggle + // immediately — the same pattern used by checkboxes-evict-siblings tests. + const recs = [ + { id: 'rec-a', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, savings: 150, upfront_cost: 1000 }, + { id: 'rec-b', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, savings: 200, upfront_cost: 0 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: { total_count: 2, total_monthly_savings: 350, total_upfront_cost: 1000, avg_payback_months: 1 }, + recommendations: recs, + regions: [], + }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + + // Wire add/remove to a shared Set so getSelectedRecommendationIDs + // reflects in-flight selection state after every checkbox event. + const selectedIds = new Set(); + (state.getSelectedRecommendationIDs as jest.Mock).mockImplementation(() => new Set(selectedIds)); + (state.addSelectedRecommendation as jest.Mock).mockImplementation((id: string) => { selectedIds.add(id); }); + (state.removeSelectedRecommendation as jest.Mock).mockImplementation((id: string) => { selectedIds.delete(id); }); + (state.clearSelectedRecommendations as jest.Mock).mockImplementation(() => { selectedIds.clear(); }); + + // Initial render: no selection, cards show full-set values. + await loadRecommendations(); + + const cardValue = (titlePattern: RegExp): string => { + const summary = document.getElementById('recommendations-summary'); + const card = Array.from(summary?.querySelectorAll('.card') ?? []) + .find((c) => titlePattern.test(c.querySelector('h3')?.textContent ?? '')); + return card?.querySelector('.value')?.textContent ?? ''; + }; + + // Each rec is its own cell → 2 cells, each with 1 variant → range = single value. + expect(cardValue(/Recommendations/)).toBe('2'); + + // Fire the real DOM change event on rec-a's checkbox — do NOT + // call loadRecommendations() again. The handler inside recommendations.ts + // calls renderRecommendationsList() which updates the cards in place. + const checkbox = document.querySelector('input[data-rec-id="rec-a"]'); + expect(checkbox).not.toBeNull(); + checkbox!.checked = true; + checkbox!.dispatchEvent(new Event('change')); + + // Cards must now reflect only rec-a ($150 savings / $1000 upfront). + expect(cardValue(/Monthly Savings/)).toMatch(/^\$150$/); + expect(cardValue(/Upfront/)).toMatch(/^\$1,?000$/); + // Recommendations card must show "1 selected". + expect(cardValue(/Recommendations/)).toBe('1'); + }); + + test('select-all change event updates summary cards in place (PR #283 CR pass-2)', async () => { + // Verifies the select-all checkbox event path: after checking select-all, + // summary cards reflect the picked-best-per-cell selection WITHOUT a + // second loadRecommendations() call. + // + // pickBestVariantPerCell picks one variant per (provider, account, service, + // region, resource_type, engine) group — for these 2 cells it picks one + // from cell1 and one from cell2, so both cards must show 2 rows selected. + const recs = [ + { id: 'cell1-cheap', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment_option: 'no-upfront', savings: 100, upfront_cost: 0 }, + { id: 'cell1-pricey', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, payment_option: 'all-upfront', savings: 150, upfront_cost: 1000 }, + { id: 'cell2-cheap', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment_option: 'no-upfront', savings: 200, upfront_cost: 0 }, + { id: 'cell2-pricey', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment_option: 'all-upfront', savings: 250, upfront_cost: 2000 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: { total_count: 4, total_monthly_savings: 700, total_upfront_cost: 3000, avg_payback_months: 2 }, + recommendations: recs, + regions: [], + }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + + // Wire add/remove/clear to a shared Set (same coordination pattern as the + // row-checkbox test above and the evict-siblings test). + const selectedIds = new Set(); + (state.getSelectedRecommendationIDs as jest.Mock).mockImplementation(() => new Set(selectedIds)); + (state.addSelectedRecommendation as jest.Mock).mockImplementation((id: string) => { selectedIds.add(id); }); + (state.removeSelectedRecommendation as jest.Mock).mockImplementation((id: string) => { selectedIds.delete(id); }); + (state.clearSelectedRecommendations as jest.Mock).mockImplementation(() => { selectedIds.clear(); }); + + await loadRecommendations(); + + const cardValue = (titlePattern: RegExp): string => { + const summary = document.getElementById('recommendations-summary'); + const card = Array.from(summary?.querySelectorAll('.card') ?? []) + .find((c) => titlePattern.test(c.querySelector('h3')?.textContent ?? '')); + return card?.querySelector('.value')?.textContent ?? ''; + }; + + // Fire the real select-all change event — do NOT call loadRecommendations(). + const selectAll = document.getElementById('select-all-recs') as HTMLInputElement; + expect(selectAll).not.toBeNull(); + selectAll.checked = true; + selectAll.dispatchEvent(new Event('change')); + + // addSelectedRecommendation was called once per cell (pickBestVariantPerCell + // picks one per cell → 2 cells → 2 adds). + expect((state.addSelectedRecommendation as jest.Mock).mock.calls.length).toBe(2); + + // Recommendations card shows "2" (2 selected cells, not 4 raw variants). + expect(cardValue(/Recommendations/)).toBe('2'); + }); + test('"Showing X of Y" surfaces selection count (closes #279)', async () => { const recs = [ { id: 'r1', provider: 'aws', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, savings: 100, upfront_cost: 0 }, diff --git a/frontend/src/state.ts b/frontend/src/state.ts index 647efeeb..b79415e4 100644 --- a/frontend/src/state.ts +++ b/frontend/src/state.ts @@ -5,12 +5,6 @@ import type { AppState } from './types'; import type { Recommendation } from './api/types'; -export type RecommendationsSortColumn = 'savings' | 'upfront_cost' | 'count' | 'term' | 'payback' | 'monthly_cost' | 'effective_savings_pct'; -export interface RecommendationsSort { - column: RecommendationsSortColumn; - direction: 'asc' | 'desc'; -} - // Closed enumeration of column ids the per-column filters target. // Typo-safety: misspellings at call sites become compile errors. export type RecommendationsColumnId = @@ -18,6 +12,16 @@ export type RecommendationsColumnId = | 'count' | 'term' | 'payment' | 'savings' | 'upfront_cost' | 'monthly_cost' | 'effective_savings_pct'; +// Every visible column is sortable, so the sort column type is exactly the +// column id set. Aliasing here keeps the two in sync automatically — adding +// a future column to RecommendationsColumnId automatically makes it sortable +// without a separate edit to this type. +export type RecommendationsSortColumn = RecommendationsColumnId; +export interface RecommendationsSort { + column: RecommendationsSortColumn; + direction: 'asc' | 'desc'; +} + export type RecommendationsColumnFilter = | { kind: 'set'; values: string[] } // categorical — values always string-form | { kind: 'expr'; expr: string }; // numeric — parsed on apply
+ + ${escapeHtml(providerDisplayName(rep.provider))} ${escapeHtml(accountName)} ${escapeHtml(rep.service)} - ${escapeHtml(rep.resource_type)}${rep.engine ? ` (${escapeHtml(rep.engine)})` : ''} — ${escapeHtml(rep.region)} — ${variants.length} variants ${savingsDisplay} · upfront: ${upfrontDisplay} · term: ${termDisplay} (0-indexed: 0=checkbox, 1=provider, + // 2=account, 3=service, 4=resource_type, 5=region, 6=count, 7=term, 8=payment) + return row.querySelectorAll('td')[8]?.textContent?.trim() ?? ''; + }); + expect(paymentCells).toContain('All Upfront'); + expect(paymentCells).toContain('Partial Upfront'); + expect(paymentCells).toContain('No Upfront'); + expect(paymentCells).toContain('Monthly'); + // Em-dash rendered for missing payment (no dedicated test rec here, + // but the helper returns '—' for undefined — covered by formatPayment unit test). + }); + + test('Payment column filter narrows visible rows (#282)', async () => { + const recs = [ + { id: 'f1', provider: 'aws', cloud_account_id: 'a1', service: 'ec2', resource_type: 't3.small', region: 'us-east-1', count: 1, term: 1, payment: 'all-upfront', savings: 100, upfront_cost: 1000 }, + { id: 'f2', provider: 'aws', cloud_account_id: 'a1', service: 'rds', resource_type: 'db.t3', region: 'us-east-1', count: 1, term: 1, payment: 'no-upfront', savings: 200, upfront_cost: 0 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set()); + + // Apply a payment filter so only the all-upfront rec is visible. + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({ + payment: { kind: 'set', values: ['all-upfront'] }, + }); + + await loadRecommendations(); + + const rows = document.querySelectorAll('tr.recommendation-row'); + // Only the all-upfront rec should be rendered. + expect(rows.length).toBe(1); + // Payment cell text should be "All Upfront". + const paymentCell = rows[0]?.querySelectorAll('td')[8]; + expect(paymentCell?.textContent?.trim()).toBe('All Upfront'); + + // Restore filter mock so it doesn't leak into subsequent tests. + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + }); + test('bottom action box selection summary reflects current selection (Bundle B)', async () => { // Bundle B replaced the floating .recommendations-bulk-toolbar with // selection-summary text inside the sticky bottom action box. @@ -1398,7 +1465,7 @@ describe('Bundle B: column header filter triggers', () => { const buttons = document.querySelectorAll('th .column-filter-btn[data-column]'); const cols = Array.from(buttons).map((b) => b.dataset['column']); expect(cols.sort()).toEqual( - ['account', 'count', 'effective_savings_pct', 'monthly_cost', 'provider', 'region', 'resource_type', 'savings', 'service', 'term', 'upfront_cost'].sort(), + ['account', 'count', 'effective_savings_pct', 'monthly_cost', 'payment', 'provider', 'region', 'resource_type', 'savings', 'service', 'term', 'upfront_cost'].sort(), ); }); @@ -1723,9 +1790,12 @@ describe('Bundle B: sticky bottom action box', () => { (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set()); }); - test('bottom box exposes Payment / Capacity % / Purchase / Create Plan; no Term selector', async () => { + test('bottom box no longer exposes the bulk Payment dropdown (#282)', async () => { await loadRecommendations(); - expect(document.getElementById('bulk-purchase-payment')).not.toBeNull(); + // Issue #282: global Payment dropdown removed — each rec carries its own + // payment_option from the API fan-out; a global override was misleading. + expect(document.getElementById('bulk-purchase-payment')).toBeNull(); + // Other controls remain. expect(document.getElementById('bulk-purchase-capacity')).not.toBeNull(); expect(document.getElementById('bulk-purchase-btn')).not.toBeNull(); expect(document.getElementById('create-plan-btn')).not.toBeNull(); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index b7538e7e..60eaa774 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -131,15 +131,9 @@ export async function loadRecommendations(): Promise { if (g.default_payment && (validPayments as string[]).includes(g.default_payment)) { cachedGlobalDefaultPayment = g.default_payment as CompatPayment; } - // Seed the bulk-toolbar default with the resolved value so that - // loadBulkPurchaseState()'s first-visit fallback uses it. - // BulkPurchasePayment is a subset of CompatPayment ('upfront' is - // Azure-only and not a valid bulk-toolbar value); skip the seed - // if the global default is outside that subset. - const validBulkPayments: BulkPurchasePayment[] = ['all-upfront', 'partial-upfront', 'no-upfront', 'monthly']; - if ((validBulkPayments as string[]).includes(cachedGlobalDefaultPayment)) { - defaultBulkPurchaseState.payment = cachedGlobalDefaultPayment as BulkPurchasePayment; - } + // cachedGlobalDefaultPayment is now read directly by loadBulkPurchaseState() + // (issue #282 dropped the toolbar dropdown; no longer need to seed + // defaultBulkPurchaseState — the module-level cache is the source of truth). } accountNamesCache = new Map(accounts.map(a => [a.id, a.name])); state.setRecommendations((data.recommendations || []) as unknown as api.Recommendation[]); @@ -269,6 +263,7 @@ const SORTABLE_STRING_COLUMNS: Record string service: (r) => r.service ?? '', resource_type: (r) => r.resource_type ?? '', region: (r) => r.region ?? '', + payment: (r) => r.payment ?? '', }; // cellKey identifies the physical-resource cell a rec belongs to. @@ -652,6 +647,7 @@ const SORT_HEADER_LABELS: Record = { region: 'Region', count: 'Count', term: 'Term', + payment: 'Payment', savings: 'Monthly Savings', upfront_cost: 'Upfront Cost', monthly_cost: 'Monthly Cost', @@ -769,6 +765,7 @@ function categoricalCellValue(r: LocalRecommendation, col: state.Recommendations case 'resource_type': return r.resource_type ?? ''; case 'region': return r.region ?? ''; case 'term': return r.term == null ? '' : String(r.term); + case 'payment': return r.payment ?? ''; // Numeric columns shouldn't reach this branch; return empty for type-safety. case 'count': case 'savings': @@ -796,7 +793,8 @@ function numericCellValue(r: LocalRecommendation, col: state.RecommendationsColu case 'service': case 'resource_type': case 'region': - case 'term': return Number.NaN; + case 'term': + case 'payment': return Number.NaN; } } @@ -1664,9 +1662,22 @@ function providerBadgeClass(provider: string): string { // neither sortable nor filterable). const FILTERABLE_COLUMNS: readonly state.RecommendationsColumnId[] = [ 'provider', 'account', 'service', 'resource_type', 'region', - 'count', 'term', 'savings', 'upfront_cost', 'monthly_cost', 'effective_savings_pct', + 'count', 'term', 'payment', 'savings', 'upfront_cost', 'monthly_cost', 'effective_savings_pct', ]; +// Map raw payment_option values to display labels for the Payment column. +const PAYMENT_DISPLAY_LABELS: Record = { + 'all-upfront': 'All Upfront', + 'partial-upfront': 'Partial Upfront', + 'no-upfront': 'No Upfront', + 'monthly': 'Monthly', +}; + +function formatPayment(payment: string | undefined): string { + if (!payment) return '\u2014'; + return PAYMENT_DISPLAY_LABELS[payment] ?? payment; +} + // Helper: render one variant row (a single LocalRecommendation) with optional // indentation styling for multi-variant cells. function buildVariantRowMarkup(rec: LocalRecommendation, selectedRecs: ReadonlySet, isNested: boolean): string { @@ -1691,6 +1702,7 @@ function buildVariantRowMarkup(rec: LocalRecommendation, selectedRecs: ReadonlyS ${escapeHtml(rec.region)} ${rec.count} ${formatTerm(rec.term)}${formatPayment(rec.payment)} ${formatCurrency(rec.savings)} ${formatCurrency(rec.upfront_cost)} ${rec.monthly_cost != null ? formatCurrency(rec.monthly_cost) : '—'}