From 12c54d9ab35f77a5708a3921c272a1dc8755dfb0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 3 May 2026 13:23:21 +0200 Subject: [PATCH 1/3] fix(frontend/recs): enforce one-variant-per-cell radio selection (closes #224) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After PR #195 (issue #188) the recommendations refresh fans out across 2 terms × 3 payments per `(provider, account, service, region, resource_type, engine)` cell — up to 6 alternative rec rows per physical resource. They're alternatives, not additions, but the checkboxes had no mutual-exclusion logic, producing wrong purchase intent in two ways: 1. **Manual checking**: a user could check `1yr/all-upfront` AND `3yr/no-upfront` for the same EC2 m5.large in the same account. Both fed into the purchase plan → double commitment for one resource. 2. **`select-all` was the destructive form**: every visible row got added, so 3 cells × 6 variants = 18 commitments instead of the 3 the user expected when clicking "buy what's recommended". 6× the intended spend per cell. # What changes - New `cellKey(rec)` helper returns the `(provider, cloud_account_id, service, region, resource_type, engine)` prefix (same as the scheduler ID encoding from #189, minus the `(term, payment)` suffix). Recs sharing this key are alternatives for the same physical resource. - Per-row checkbox change handler now enforces radio behaviour: on check, scan the visible-recs list for any sibling in the same cell that's already selected, deselect it FIRST, then add the new rec. Cells are independent — selecting in cell X doesn't touch cell Y's selection. - `select-all` handler rewritten: clear current selection, then call new `pickBestVariantPerCell(recs)` which groups by cell and picks the variant with the highest **effective monthly savings**: `effective = savings - (upfront_cost / (term * 12))`. Amortizing the upfront over the commitment term means a 3yr/all-upfront with a huge lump-sum doesn't beat a 1yr/no-upfront just on raw `savings`. Sibling issue #223 will replace this tiebreaker with "matches resolved GlobalConfig.DefaultTerm + DefaultPayment" when it lands; until then, amortized savings is the right deterministic default. # Out of scope (deliberate) - **Native `` markup**. Per the issue: "Designer call — if cell-grouping (sibling issue #226) lands first, radios become visually correct." Stayed with checkboxes-with-radio-behaviour for this PR; markup switch waits for cell-grouping visual. - **Default-select per cell from settings** (sibling #223). # Tests 4 new tests in `frontend/src/__tests__/recommendations.test.ts` inside the `'Issue #224: one-variant-per-cell radio selection'` describe block: (a) Manual toggle within a cell — checking variant B with variant A already selected: A is removed, B is added. Sibling A was not also added; B was not also removed. (b) Cross-cell independence — selecting in cell X must NOT remove cell Y's existing selection. (c) `select-all` collapses 18 → 3: 3 cells × 6 variants. After click, exactly 3 add calls; clearSelectedRecommendations was called first to drop stale state. (d) Tiebreaker pin — single cell with 3 variants whose `(savings, upfront_cost, term)` produce known amortized values ($200, $300, $400). The middle variant wins ($400 effective) despite the high-upfront variant having $1200 raw savings, proving the amortization is actually computed. `tsc --noEmit` clean. `npx jest --testPathPatterns="recommendations"` exit 0. `npm run build` (webpack production) exit 0. --- .../src/__tests__/recommendations.test.ts | 162 ++++++++++++++++++ frontend/src/recommendations.ts | 75 +++++++- 2 files changed, 236 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 23e1b179..b30aebca 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -2023,3 +2023,165 @@ describe('Issue #132: bulk-buy collapses SP plan types into one bucket', () => { expect(sectionTitles.some((t) => t.includes('AWS / ec2'))).toBe(true); }); }); + +// Issue #224: at most one (term, payment) variant per physical-resource +// cell can be selected at any time. After PR #195's per-cell fan-out (2 +// terms × 3 payments per cell), naive selection produces wrong purchase +// intent — manual checking lets the user accumulate sibling commitments, +// and `select-all` over-commits 6×. Cell = `(provider, account, service, +// region, resource_type, engine)`. The fix lives in two places: the +// per-row checkbox change handler (deselect any in-cell sibling on check) +// and the select-all handler (group by cell, pick highest-effective per). +describe('Issue #224: one-variant-per-cell radio selection', () => { + beforeEach(() => { + document.body.replaceChildren(); + const recsTab = document.createElement('div'); + recsTab.id = 'recommendations-tab'; + recsTab.className = 'tab-content active'; + const summary = document.createElement('div'); + summary.id = 'recommendations-summary'; + const list = document.createElement('div'); + list.id = 'recommendations-list'; + recsTab.appendChild(summary); + recsTab.appendChild(list); + document.body.appendChild(recsTab); + const purchaseModal = document.createElement('div'); + purchaseModal.id = 'purchase-modal'; + purchaseModal.className = 'hidden'; + document.body.appendChild(purchaseModal); + jest.clearAllMocks(); + (state.getRecommendationsColumnFilters as jest.Mock).mockReturnValue({}); + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set()); + }); + + // (a) Manual toggle: two variants of the same cell. Checking variant B + // when variant A is already selected must remove A first, leaving only B. + test('(a) checking variant B in same cell deselects sibling variant A', async () => { + const recs = [ + { id: 'cellA-1y-allup', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 1, savings: 100, upfront_cost: 500 }, + { id: 'cellA-3y-noup', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 3, savings: 200, upfront_cost: 0 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + // Pretend variant A is already selected (the "user previously checked it" state). + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['cellA-1y-allup'])); + await loadRecommendations(); + + // Tick variant B in the same cell. + const cbs = Array.from(document.querySelectorAll('tbody input[data-rec-id]')); + const variantB = cbs.find((cb) => cb.dataset['recId'] === 'cellA-3y-noup'); + expect(variantB).toBeDefined(); + variantB!.checked = true; + variantB!.dispatchEvent(new Event('change')); + + // The handler must have removed sibling A AND added B. + const removed = (state.removeSelectedRecommendation as jest.Mock).mock.calls.map((c) => c[0]); + const added = (state.addSelectedRecommendation as jest.Mock).mock.calls.map((c) => c[0]); + expect(removed).toContain('cellA-1y-allup'); + expect(added).toContain('cellA-3y-noup'); + // Sanity: B was not also removed, A was not also added. + expect(removed).not.toContain('cellA-3y-noup'); + expect(added).not.toContain('cellA-1y-allup'); + }); + + // (b) Cross-cell independence: selecting a rec in cell X must not + // affect cell Y's selection state. The radio enforcement is per-cell, + // not global. + test('(b) selecting in cell X does not affect cell Y selections', async () => { + const recs = [ + { id: 'cellX-1y', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 1, savings: 100, upfront_cost: 500 }, + { id: 'cellY-1y', provider: 'aws', cloud_account_id: 'acct-1', service: 'rds', resource_type: 'db.r5.large', region: 'us-east-1', engine: 'mysql', count: 1, term: 1, savings: 200, upfront_cost: 1000 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + // Pretend cellY is already selected. + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['cellY-1y'])); + await loadRecommendations(); + + // Tick cellX. + const cbs = Array.from(document.querySelectorAll('tbody input[data-rec-id]')); + const cellX = cbs.find((cb) => cb.dataset['recId'] === 'cellX-1y'); + expect(cellX).toBeDefined(); + cellX!.checked = true; + cellX!.dispatchEvent(new Event('change')); + + // cellY must NOT have been removed — cells are independent. + const removed = (state.removeSelectedRecommendation as jest.Mock).mock.calls.map((c) => c[0]); + expect(removed).not.toContain('cellY-1y'); + const added = (state.addSelectedRecommendation as jest.Mock).mock.calls.map((c) => c[0]); + expect(added).toContain('cellX-1y'); + }); + + // (c) Select-all collapses to one-per-cell. Three distinct cells × six + // variants each = 18 recs. After select-all, exactly 3 should be added, + // not 18 (one per cell). This is the headline money-impact fix. + test('(c) select-all picks exactly one variant per cell (3 cells × 6 variants → 3 selected)', async () => { + const cells = [ + { account: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '' }, + { account: 'acct-1', service: 'ec2', resource_type: 'm5.large', region: 'us-east-1', engine: '' }, + { account: 'acct-1', service: 'rds', resource_type: 'db.r5.large', region: 'eu-west-1', engine: 'mysql' }, + ]; + const variants = [ + { term: 1, payment: 'all-upfront' }, + { term: 1, payment: 'partial-upfront' }, + { term: 1, payment: 'no-upfront' }, + { term: 3, payment: 'all-upfront' }, + { term: 3, payment: 'partial-upfront' }, + { term: 3, payment: 'no-upfront' }, + ]; + const recs: Array> = []; + let i = 0; + for (const c of cells) { + for (const v of variants) { + recs.push({ + id: `c${i++}`, + provider: 'aws', cloud_account_id: c.account, service: c.service, + resource_type: c.resource_type, region: c.region, engine: c.engine, + count: 1, term: v.term, savings: 100 + i, upfront_cost: 500, + }); + } + } + expect(recs).toHaveLength(18); + + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + await loadRecommendations(); + + const selectAll = document.getElementById('select-all-recs') as HTMLInputElement; + expect(selectAll).not.toBeNull(); + selectAll.checked = true; + selectAll.dispatchEvent(new Event('change')); + + // Exactly 3 add calls — one per cell. + expect((state.addSelectedRecommendation as jest.Mock).mock.calls).toHaveLength(3); + // And clearSelectedRecommendations was called first to drop any stale state. + expect(state.clearSelectedRecommendations).toHaveBeenCalled(); + }); + + // (d) Tiebreaker: when multiple variants share a cell, select-all picks + // the variant with the highest EFFECTIVE monthly savings (amortizing + // upfront over term * 12 months) — NOT the highest raw `savings`. + // Concrete example: a 3yr/all-upfront with $36000 upfront + $1200/mo + // headline savings has effective = 1200 - 36000/36 = $200. A 1yr/no-upfront + // with $0 upfront + $300/mo headline savings has effective = $300. The + // 1yr/no-upfront wins despite the 3yr's higher raw `savings`. + test('(d) select-all picks highest-effective-savings (amortized) per cell', async () => { + const recs = [ + // 3yr/all-upfront — high raw savings ($1200/mo) but huge upfront drags effective to $200/mo. + { id: 'big-upfront', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 3, savings: 1200, upfront_cost: 36000 }, + // 1yr/no-upfront — lower raw ($300/mo) but $0 upfront → effective stays at $300/mo. + { id: 'no-upfront', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 1, savings: 300, upfront_cost: 0 }, + // 3yr/partial-upfront — middle of the road ($600/mo savings, $7200 upfront → effective = 600 - 7200/36 = $400). + { id: 'middle', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 3, savings: 600, upfront_cost: 7200 }, + ]; + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + await loadRecommendations(); + + const selectAll = document.getElementById('select-all-recs') as HTMLInputElement; + selectAll.checked = true; + selectAll.dispatchEvent(new Event('change')); + + // Exactly one add call (single cell, one variant picked). + const addCalls = (state.addSelectedRecommendation as jest.Mock).mock.calls; + expect(addCalls).toHaveLength(1); + // The "middle" variant has the highest effective ($400/mo > $300 > $200) — picked. + expect(addCalls[0]![0]).toBe('middle'); + }); +}); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index abf17259..8c4cee6e 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -142,6 +142,55 @@ const SORTABLE_STRING_COLUMNS: Record string region: (r) => r.region ?? '', }; +// cellKey identifies the physical-resource cell a rec belongs to. +// After PR #195's per-(term, payment) fan-out, a single physical resource +// produces up to 6 alternative rec rows (2 terms × 3 payments). The +// `(provider, cloud_account_id, service, region, resource_type, engine)` +// prefix is the cell — recs sharing this prefix are alternatives, not +// additions. Same prefix the scheduler ID encoding uses +// (scheduler.go:856-858, PR #189) but without the (term, payment) suffix. +// +// Used by issue #224's radio enforcement: at most one variant per cell +// can be selected at any time. +function cellKey(rec: LocalRecommendation): string { + return `${rec.provider}|${rec.cloud_account_id ?? ''}|${rec.service}|${rec.region}|${rec.resource_type}|${rec.engine ?? ''}`; +} + +// pickBestVariantPerCell collapses a list of recs to one rec per cell, +// choosing the variant with the highest effective monthly savings. +// +// Effective savings amortizes the upfront cost across the term: +// effective = savings - (upfront_cost / (term * 12)) +// +// Two `(savings, upfront_cost, term)` tuples that look identical on raw +// `savings` can score very differently on the amortized number — e.g. a +// 3yr all-upfront commitment with a large lump-sum upfront has a much +// lower effective monthly savings than a no-upfront variant with the +// same headline savings. Picking by amortization picks the variant +// that's actually best for the operator's wallet over the term. +// +// Used by issue #224's `select-all` handler. Sibling issue #223 will +// replace this tiebreaker with "matches resolved GlobalConfig.DefaultTerm +// + DefaultPayment" once that lands; until then, amortized savings is +// the right deterministic default. +function pickBestVariantPerCell(recs: readonly LocalRecommendation[]): LocalRecommendation[] { + const byCell = new Map(); + const effective = (r: LocalRecommendation): number => { + // Defensive clamp: if a rec arrives with term=0 from a malformed + // fixture, treat it as 1yr so the division doesn't blow up. + const monthsInTerm = Math.max(1, r.term * 12); + return r.savings - (r.upfront_cost / monthsInTerm); + }; + for (const r of recs) { + const k = cellKey(r); + const existing = byCell.get(k); + if (!existing || effective(r) > effective(existing)) { + byCell.set(k, r); + } + } + return Array.from(byCell.values()); +} + const SORT_HEADER_LABELS: Record = { provider: 'Provider', account: 'Account', @@ -1930,7 +1979,16 @@ function renderRecommendationsList(loadedRecs: LocalRecommendation[]): void { if (selectAllCheckbox) { selectAllCheckbox.addEventListener('change', () => { if (selectAllCheckbox.checked) { - recommendations.forEach((r) => state.addSelectedRecommendation(r.id)); + // Issue #224: select-all picks ONE variant per cell (highest-effective- + // savings) rather than every visible row. After PR #195's per-(term, + // payment) fan-out, naive "select every row" produces 6× the intended + // commitments per resource — wrong purchase intent. Clear current + // selection first so a stale choice from a different filter context + // doesn't bleed through. + state.clearSelectedRecommendations(); + for (const r of pickBestVariantPerCell(recommendations)) { + state.addSelectedRecommendation(r.id); + } } else { state.clearSelectedRecommendations(); } @@ -1942,11 +2000,26 @@ function renderRecommendationsList(loadedRecs: LocalRecommendation[]): void { // changes so a stale selection from a previous filter is a no-op // once the user narrows, rather than pointing at whichever rec // happens to occupy the old index position. + // + // Issue #224: enforce one-variant-per-cell radio behaviour on check. + // When the user checks a variant, deselect any other variant of the + // same cell that's already selected — a single physical resource + // can only carry one (term, payment) commitment at a time. container.querySelectorAll('input[data-rec-id]').forEach(cb => { cb.addEventListener('change', () => { const id = cb.dataset['recId'] || ''; if (!id) return; if (cb.checked) { + const newRec = recommendations.find((r) => r.id === id); + if (newRec) { + const newCell = cellKey(newRec); + const selected = state.getSelectedRecommendationIDs(); + for (const r of recommendations) { + if (r.id !== id && selected.has(r.id) && cellKey(r) === newCell) { + state.removeSelectedRecommendation(r.id); + } + } + } state.addSelectedRecommendation(id); } else { state.removeSelectedRecommendation(id); From a89f8c9af1d8473297971f57038f611e64994f2d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 3 May 2026 13:54:32 +0200 Subject: [PATCH 2/3] =?UTF-8?q?fix(frontend/recs):=20address=20CR=20feedba?= =?UTF-8?q?ck=20on=20PR#231=20=E2=80=94=20term=20clamp=20+=20full-set=20si?= =?UTF-8?q?bling=20eviction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pickBestVariantPerCell: fix term=0 clamp from Math.max(1, r.term*12) to Math.max(1, r.term||1)*12 so a malformed zero-term rec is treated as 1yr (12 months) not 1 month; keeps the effective-savings winner correct (CR actionable: 1-month fallback could skew per-cell winner selection) - checkbox change handler: iterate state.getRecommendations() (full loaded set) instead of filtered `recommendations` when evicting same-cell siblings, so variants hidden by a column filter are also deselected, preserving the one-variant-per-cell contract across filter changes (CR actionable: hidden siblings could remain selected, breaking invariant) - test: update issue-#224 test (a) and (b) to mock state.getRecommendations(); add new regression test for the hidden-by-filter scenario --- .../src/__tests__/recommendations.test.ts | 40 +++++++++++++++++++ frontend/src/recommendations.ts | 8 +++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index b30aebca..a5e282cc 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -371,6 +371,43 @@ describe('Recommendations Module', () => { expect(calledIds).not.toContain('rec-azure-sub2'); }); + // Issue #224: when a sibling variant is selected but currently hidden by a + // column filter (e.g. user filtered to 3yr, but 1yr sibling is still + // selected in state), checking a 3yr sibling must evict the hidden 1yr + // selection. The fix iterates state.getRecommendations() (full loaded set) + // rather than the filtered `recommendations` array rendered in the DOM. + test('checking a visible variant evicts a same-cell sibling hidden by filter', async () => { + const allRecs = [ + { id: 'rec-1yr', provider: 'aws', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, savings: 100, upfront_cost: 0 }, + { id: 'rec-3yr', provider: 'aws', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 3, savings: 300, upfront_cost: 0 }, + ]; + // Simulate: API returns all recs, but only the 3yr one is visible (1yr is + // hidden by filter). state.getRecommendations() returns the full set. + (api.getRecommendations as jest.Mock).mockResolvedValue({ + summary: {}, + recommendations: [allRecs[1]], // only 3yr visible in rendered list + regions: [] + }); + (state.getRecommendations as jest.Mock).mockReturnValue(allRecs); // full set in state + // Simulate: 1yr is already selected in state. + (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['rec-1yr'])); + + await loadRecommendations(); + + // Tick the 3yr checkbox. + const cb = document.querySelector('input[data-rec-id="rec-3yr"]'); + expect(cb).not.toBeNull(); + cb!.checked = true; + cb!.dispatchEvent(new Event('change')); + + // The hidden 1yr sibling must be evicted. + const removeCalls = (state.removeSelectedRecommendation as jest.Mock).mock.calls.map(c => c[0]); + expect(removeCalls).toContain('rec-1yr'); + // And the 3yr rec is added. + const addCalls = (state.addSelectedRecommendation as jest.Mock).mock.calls.map(c => c[0]); + expect(addCalls).toContain('rec-3yr'); + }); + test('purchase button opens modal for that recommendation', async () => { const mockRecs = [ { id: 'rec-11', provider: 'aws', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', count: 1, term: 1, savings: 100, upfront_cost: 500 } @@ -2062,6 +2099,8 @@ describe('Issue #224: one-variant-per-cell radio selection', () => { { id: 'cellA-3y-noup', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 3, savings: 200, upfront_cost: 0 }, ]; (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + // state.getRecommendations() is the full loaded set (used by the sibling-eviction loop). + (state.getRecommendations as jest.Mock).mockReturnValue(recs); // Pretend variant A is already selected (the "user previously checked it" state). (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['cellA-1y-allup'])); await loadRecommendations(); @@ -2092,6 +2131,7 @@ describe('Issue #224: one-variant-per-cell radio selection', () => { { id: 'cellY-1y', provider: 'aws', cloud_account_id: 'acct-1', service: 'rds', resource_type: 'db.r5.large', region: 'us-east-1', engine: 'mysql', count: 1, term: 1, savings: 200, upfront_cost: 1000 }, ]; (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); // Pretend cellY is already selected. (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['cellY-1y'])); await loadRecommendations(); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 8c4cee6e..ec2d8f5f 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -178,7 +178,7 @@ function pickBestVariantPerCell(recs: readonly LocalRecommendation[]): LocalReco const effective = (r: LocalRecommendation): number => { // Defensive clamp: if a rec arrives with term=0 from a malformed // fixture, treat it as 1yr so the division doesn't blow up. - const monthsInTerm = Math.max(1, r.term * 12); + const monthsInTerm = Math.max(1, r.term || 1) * 12; return r.savings - (r.upfront_cost / monthsInTerm); }; for (const r of recs) { @@ -2014,7 +2014,11 @@ function renderRecommendationsList(loadedRecs: LocalRecommendation[]): void { if (newRec) { const newCell = cellKey(newRec); const selected = state.getSelectedRecommendationIDs(); - for (const r of recommendations) { + // Scan the full loaded set (not just the filtered view) so that + // hidden siblings (e.g. filtered-out term variants) are also + // deselected, preserving the one-variant-per-cell contract. + const allLoaded = state.getRecommendations() as unknown as LocalRecommendation[]; + for (const r of allLoaded) { if (r.id !== id && selected.has(r.id) && cellKey(r) === newCell) { state.removeSelectedRecommendation(r.id); } From 41de9a5581c556903a1bf3ba7174e3e9e0382b72 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 3 May 2026 16:46:58 +0200 Subject: [PATCH 3/3] fix(frontend): address issue 224 review comments --- frontend/src/__tests__/recommendations.test.ts | 6 ++++++ frontend/src/recommendations.ts | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index a5e282cc..8d145729 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -38,6 +38,7 @@ jest.mock('../state', () => ({ getCurrentAccountIDs: jest.fn().mockReturnValue([]), setCurrentAccountIDs: jest.fn(), getRecommendations: jest.fn().mockReturnValue([]), + getRecommendationByID: jest.fn().mockReturnValue(undefined), setRecommendations: jest.fn(), getSelectedRecommendationIDs: jest.fn().mockReturnValue(new Set()), clearSelectedRecommendations: jest.fn(), @@ -389,6 +390,7 @@ describe('Recommendations Module', () => { regions: [] }); (state.getRecommendations as jest.Mock).mockReturnValue(allRecs); // full set in state + (state as unknown as { getRecommendationByID: jest.Mock }).getRecommendationByID.mockImplementation((id: string) => allRecs.find((r) => r.id === id)); // Simulate: 1yr is already selected in state. (state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set(['rec-1yr'])); @@ -417,6 +419,7 @@ describe('Recommendations Module', () => { recommendations: mockRecs, regions: [] }); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(mockRecs); await loadRecommendations(); @@ -445,6 +448,7 @@ describe('Recommendations Module', () => { recommendations: mockRecs, regions: [] }); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(mockRecs); await loadRecommendations(); @@ -2182,6 +2186,7 @@ describe('Issue #224: one-variant-per-cell radio selection', () => { expect(recs).toHaveLength(18); (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); await loadRecommendations(); const selectAll = document.getElementById('select-all-recs') as HTMLInputElement; @@ -2212,6 +2217,7 @@ describe('Issue #224: one-variant-per-cell radio selection', () => { { id: 'middle', provider: 'aws', cloud_account_id: 'acct-1', service: 'ec2', resource_type: 't3.medium', region: 'us-east-1', engine: '', count: 1, term: 3, savings: 600, upfront_cost: 7200 }, ]; (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs, regions: [] }); + (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs); await loadRecommendations(); const selectAll = document.getElementById('select-all-recs') as HTMLInputElement; diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index ec2d8f5f..80f90da6 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -178,7 +178,7 @@ function pickBestVariantPerCell(recs: readonly LocalRecommendation[]): LocalReco const effective = (r: LocalRecommendation): number => { // Defensive clamp: if a rec arrives with term=0 from a malformed // fixture, treat it as 1yr so the division doesn't blow up. - const monthsInTerm = Math.max(1, r.term || 1) * 12; + const monthsInTerm = Math.max(12, (r.term || 1) * 12); return r.savings - (r.upfront_cost / monthsInTerm); }; for (const r of recs) {