From fdf0493c7f249a060467ff273813df854e1711c0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 16:46:30 +0200 Subject: [PATCH 1/3] fix(opportunities): cost-column sort toggle no-ops on all-upfront cells Monthly Cost sort direction changes had no effect after the first click because Math.min over all finite scaled costs scored every cell with at least one all-upfront variant at 0, making them all tie and leaving the direction multiplier nothing to act on. Fix: compute the minimum NON-ZERO recurring cost so mixed (all-upfront + no-upfront) cells get a meaningful score that responds to direction toggles. Fall back to 0 only when every finite value is 0 (pure all-upfront cell), and to POSITIVE_INFINITY when all values are null (preserving the existing null-sink behavior). Closes #1251 --- .../src/__tests__/recommendations.test.ts | 64 +++++++++++++++++++ frontend/src/recommendations.ts | 23 +++++-- 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index a0a1bc07c..62ad7c82c 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -6478,6 +6478,70 @@ describe('Issue #494: deterministic group sort on multi-variant cells', () => { expect(first.length).toBe(3); } }); + + // ------------------------------------------------------------------------- + // QA 4.13 - Monthly Cost: zero-cost (all-upfront) variants must not prevent + // sort-direction toggle from reordering rows. + // + // Before the fix, cells with at least one all-upfront variant (monthly_cost=0) + // all scored 0 via Math.min(...finite), tying every such cell so the direction + // multiplier had nothing to act on and subsequent sort clicks were no-ops. + // The fix uses the minimum NON-ZERO recurring cost, falling back to 0 only + // when all finite values are 0 (pure all-upfront cell). + // ------------------------------------------------------------------------- + test('Monthly Cost: mixed cell (all-upfront + no-upfront) sorts by non-zero recurring cost, not 0', async () => { + // cellLow: all-upfront (monthly=0) + no-upfront (monthly=20) -> score = 20 + const cellLow = multiVariantCell({ + resourceType: 'low-mixed', payment1y: 'all-upfront', payment3y: 'no-upfront', + upfront1y: 500, upfront3y: 0, monthly1y: 0, monthly3y: 20, + }); + // cellHigh: all-upfront (monthly=0) + no-upfront (monthly=80) -> score = 80 + const cellHigh = multiVariantCell({ + resourceType: 'high-mixed', payment1y: 'all-upfront', payment3y: 'no-upfront', + upfront1y: 500, upfront3y: 0, monthly1y: 0, monthly3y: 80, + }); + const recs = [...cellHigh, ...cellLow]; // wrong insertion order + + // Ascending: cellLow (score=20) must precede cellHigh (score=80). + setupTestFixture(recs, { column: 'monthly_cost', direction: 'asc' }); + await loadRecommendations(); + const ascOrder = renderedCellOrder(); + expect(indexOrFail(ascOrder, 'low-mixed')) + .toBeLessThan(indexOrFail(ascOrder, 'high-mixed')); + + // Descending: cellHigh (score=80) must now precede cellLow (score=20). + // This is the toggle that was broken before the fix -- both cells scored 0 + // so direction-flip was a no-op. + setupTestFixture(recs, { column: 'monthly_cost', direction: 'desc' }); + await loadRecommendations(); + const descOrder = renderedCellOrder(); + expect(indexOrFail(descOrder, 'high-mixed')) + .toBeLessThan(indexOrFail(descOrder, 'low-mixed')); + + // The two directions must produce OPPOSITE orderings (the toggle is live). + expect(ascOrder).not.toEqual(descOrder); + }); + + test('Monthly Cost: pure all-upfront cell scores 0 (not POSITIVE_INFINITY) and sorts before all-null cells', async () => { + // cellPureAllUpfront: both variants have monthly_cost=0 (all-upfront) -> score=0 + const cellPureAllUpfront = multiVariantCell({ + resourceType: 'pure-upfront', payment1y: 'all-upfront', payment3y: 'all-upfront', + upfront1y: 600, upfront3y: 1800, monthly1y: 0, monthly3y: 0, + }); + // cellAllNull: both variants have monthly_cost=null -> score=POSITIVE_INFINITY + const cellAllNull = multiVariantCell({ + resourceType: 'pure-null', payment1y: 'all-upfront', payment3y: 'all-upfront', + upfront1y: 1000, upfront3y: 3000, monthly1y: null, monthly3y: null, + }); + const recs = [...cellAllNull, ...cellPureAllUpfront]; + + // Ascending: pure-upfront (score=0) must come before all-null (score=+Inf). + setupTestFixture(recs, { column: 'monthly_cost', direction: 'asc' }); + await loadRecommendations(); + const order = renderedCellOrder(); + expect(indexOrFail(order, 'pure-upfront')) + .toBeLessThan(indexOrFail(order, 'pure-null')); + }); }); // Helper used by the issue-#479/#480/#481/#482/#483/#484 describes below to diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index e7c60611e..b54f32b31 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -1187,14 +1187,25 @@ function cellScoreFor( } if (column === 'monthly_cost') { const period = state.getCostPeriod(); - const finite: number[] = []; + // #494 best-case framing: prefer the smallest NON-ZERO recurring cost so + // that all-upfront variants (monthly_cost=0, which is a real value not a + // null sentinel) do not tie every cell at 0 and make direction-toggling a + // no-op. Tiers: + // 1. any non-zero finite value exists -> Math.min of those + // 2. all finite values are 0 (pure all-upfront cell) -> 0 + // 3. no finite value at all (all null) -> POSITIVE_INFINITY (sink to bottom) + const nonZero: number[] = []; + const anyFinite: number[] = []; for (const v of variants) { - const scaled = scaleCost(v.monthly_cost, period); - if (scaled != null) finite.push(scaled); + const s = scaleCost(v.monthly_cost, period); + if (s != null) { + anyFinite.push(s); + if (s > 0) nonZero.push(s); + } } - // Best-case framing: lowest recurring cost wins. All-null cells sink to - // the bottom via the existing POSITIVE_INFINITY sentinel logic. - return finite.length === 0 ? Number.POSITIVE_INFINITY : Math.min(...finite); + if (nonZero.length) return Math.min(...nonZero); + if (anyFinite.length) return 0; + return Number.POSITIVE_INFINITY; } if (column === 'effective_savings_pct') { const finite: number[] = []; From 8ced5a71a1ab45edfe8c631fdaf631af10fcb949 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 16:47:01 +0200 Subject: [PATCH 2/3] fix(opportunities): clear expand state on provider/account filter change Selecting one provider, clicking "Expand all", then switching the Global provider/account filter left groups for the newly shown provider collapsed, reset the "Collapse all" button back to "Expand all", and produced a mixed expanded/collapsed view under "All Providers". The module-level expandedCells and expandedSpGroups Sets retained stale keys from the prior provider because loadRecommendations re-fetched on filter change without clearing them. Fix: call resetExpandedCells() at the start of loadRecommendations so a provider/account filter change clears expand state before the new data renders. Column-filter and sort re-renders go through rerenderRecommendations (not loadRecommendations), so the intended "expand survives column-filter/sort" behavior is preserved. Closes #1251 --- .../src/__tests__/recommendations.test.ts | 32 +++++++++++++++++++ frontend/src/recommendations.ts | 7 ++++ 2 files changed, 39 insertions(+) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index 62ad7c82c..8a01d8879 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -5170,6 +5170,38 @@ describe('Issues #225 + #226: cell grouping with savings range and collapse/expa const variantRows = document.querySelectorAll('.rec-variant-row'); expect(variantRows.length).toBe(2); }); + + // QA 4.13: expand/collapse state must be reset when the data source + // reloads due to a provider/account Global filter change. Before the fix, + // stale expandedCells keys from the prior provider desynchronised the + // Expand-All button label and left newly-shown groups collapsed. + test('QA 4.13: expand state clears on loadRecommendations (provider filter change)', async () => { + const recs = multiVariantRecs(); + (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: recs }); + (state.getRecommendations as jest.Mock).mockReturnValue(recs); + + // 1. Load and expand all groups. + await loadRecommendations(); + const expandAllBtn = document.querySelector('.expand-all-toggle'); + expect(expandAllBtn).not.toBeNull(); + expandAllBtn!.click(); + + // Confirm we are now in "Collapse all" state. + expect(expandAllBtn!.textContent).toMatch(/Collapse all/); + + // 2. Simulate a provider filter change by calling loadRecommendations() + // again (this is exactly what the subscribeProvider callback invokes). + await loadRecommendations(); + + // 3. Expand state must have been cleared: button label reverts to + // "Expand all" and no variant rows are visible (groups are collapsed). + const expandAllBtn2 = document.querySelector('.expand-all-toggle'); + expect(expandAllBtn2).not.toBeNull(); + expect(expandAllBtn2!.textContent).toMatch(/Expand all/); + + const variantRows = document.querySelectorAll('.rec-variant-row'); + expect(variantRows.length).toBe(0); + }); }); }); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index b54f32b31..76383f73f 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -462,6 +462,13 @@ export async function loadRecommendations(): Promise { // the module default ("savings desc"). Idempotent on subsequent reloads. readSortFromUrl(); + // QA 4.13 (expand/collapse desync): clear expand state whenever the data + // source is reloaded due to a provider/account Global filter change. + // Column-filter and sort re-renders go through rerenderRecommendations() + // instead, so they intentionally do NOT reach this path -- preserving the + // "expand survives column-filter/sort" behavior documented near line 63. + resetExpandedCells(); + // Issue #344 T3: skeleton rows for the recommendations table so the // panel reads as "loading" instead of staying blank while the // (potentially multi-second) Promise.all resolves. 5 rows ≈ above- From 57252004f8b6f6009a632a7d638297e9f96b5588 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 26 Jun 2026 15:55:53 +0200 Subject: [PATCH 3/3] docs(opportunities): refresh expandedCells reset scope comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of #1259 noted the headers above `expandedCells` / `expandedSpGroups` still read "Cleared on page load / full refresh" after this PR moved the actual clear into `loadRecommendations()`. The behavior is now broader than that wording suggests: the reset fires on every loadRecommendations entry — page load, switchTab('opportunities'), provider/account Global filter change, manual refresh, lookback change, and stale auto-refresh. The "survives per-column filter/sort" guarantee still holds because column-filter and sort/period re-renders all go through rerenderRecommendations() instead. Update both headers to match the implementation so the next reader doesn't conclude that a tab switch or refresh preserves expand state (it doesn't). Comment-only change; no behavior delta. --- frontend/src/recommendations.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 76383f73f..10c4fd749 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -62,7 +62,11 @@ export function getAccountName(accountId: string): string { // issues #225 + #226: expand/collapse state for cell grouping. // Contains the cellKey strings of cells the user has explicitly expanded. -// Cleared on page load / full refresh; survives per-column filter/sort re-renders. +// Cleared on every loadRecommendations() entry (page load, tab switch back +// to Opportunities, provider/account Global filter change, manual refresh, +// lookback change, stale auto-refresh — see resetExpandedCells() call at +// the top of loadRecommendations). Survives per-column filter/sort/period +// re-renders, which go through rerenderRecommendations() instead. const expandedCells = new Set(); // Last computed group keys for the visible filtered set — used by the // Expand-All button handler to populate expandedCells without re-computing. @@ -70,7 +74,8 @@ let lastVisibleGroupKeys: string[] = []; // issue #135: expand/collapse state for SP plan-type group rows. // Contains the spGroupKey strings the user has explicitly expanded. -// Cleared together with expandedCells on page load / full refresh. +// Cleared together with expandedCells on every loadRecommendations() entry +// (see resetExpandedCells); survives column-filter/sort/period re-renders. const expandedSpGroups = new Set(); // #272 (CR follow-up): cache of the most-recent API-derived summary so