fix(frontend/recs): enforce one-variant-per-cell radio selection (closes #224)

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(),
@@ -371,6 +372,44 @@ 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
+      (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']));
+
+      await loadRecommendations();
+
+      // Tick the 3yr checkbox.
+      const cb = document.querySelector<HTMLInputElement>('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 }
@@ -380,6 +419,7 @@ describe('Recommendations Module', () => {
         recommendations: mockRecs,
         regions: []
       });
+      (state.getVisibleRecommendations as jest.Mock).mockReturnValue(mockRecs);
 
       await loadRecommendations();
 
@@ -408,6 +448,7 @@ describe('Recommendations Module', () => {
         recommendations: mockRecs,
         regions: []
       });
+      (state.getVisibleRecommendations as jest.Mock).mockReturnValue(mockRecs);
 
       await loadRecommendations();
 
@@ -2023,3 +2064,170 @@ 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: [] });
+    // 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();
+
+    // Tick variant B in the same cell.
+    const cbs = Array.from(document.querySelectorAll<HTMLInputElement>('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: [] });
+    (state.getRecommendations as jest.Mock).mockReturnValue(recs);
+    // 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<HTMLInputElement>('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<Record<string, unknown>> = [];
+    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: [] });
+    (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs);
+    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: [] });
+    (state.getVisibleRecommendations as jest.Mock).mockReturnValue(recs);
+    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');
+  });
+});

frontend/src/recommendations.ts

@@ -142,6 +142,55 @@ const SORTABLE_STRING_COLUMNS: Record<string, (r: LocalRecommendation) => 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<string, LocalRecommendation>();
+  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(12, (r.term || 1) * 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<string, string> = {
   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,30 @@ 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<HTMLInputElement>('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();
+          // 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);
+            }
+          }
+        }
         state.addSelectedRecommendation(id);
       } else {
         state.removeSelectedRecommendation(id);