Skip to content

fix(opportunities): selection toggle no longer reorders table rows #775

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
May 28, 2026
Merged

fix(opportunities): selection toggle no longer reorders table rows #775

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 88 additions & 30 deletions frontend/src/__tests__/recommendations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5713,47 +5713,105 @@ describe('Issue #494: deterministic group sort on multi-variant cells', () => {
// distinct score (or, if genuinely tied, a stable cellKey tiebreaker), so
// repeated invocations MUST produce the same order.
// -------------------------------------------------------------------------
// Selection-independent Term sort: cells with different term distributions
// must sort correctly by cellSummary score (termMin*100+termMax) regardless
// of which variants the user has selected. Fix for Issue #768: the previous
// "selected-variant short-circuit" in cellScoreFor() switched the score to
// the selected variant's individual term value, which caused rows to reorder
// on every checkbox toggle.
// -------------------------------------------------------------------------
// Selected-variant interaction: when a cell has a selected variant, the
// cell's sort score must use that variant's value but must remain on the
// same scale as non-selected cells (otherwise a selected term=1 ranks
// above a non-selected mixed cell when it should rank below the
// 1yr-only non-selected cell).
// -------------------------------------------------------------------------
test('Selected-variant cell sorts on the same scale as non-selected cells (Term)', async () => {
// Non-selected mixed (1, 3) cell.
test('Term sort is selection-independent: cells rank by term distribution not by selected variant', async () => {
// cell-1y-only: both variants are term=1 (termMin=1, termMax=1, score=101)
const cell1yOnly: LocalRecommendation[] = [
{ id: '1yonly-nu', provider: 'aws', cloud_account_id: 'a1', service: 'ec2',
resource_type: '1y-only', region: 'us-east-1', count: 1, term: 1,
payment: 'no-upfront', savings: 100, upfront_cost: 0, monthly_cost: 50 } as unknown as LocalRecommendation,
{ id: '1yonly-au', provider: 'aws', cloud_account_id: 'a1', service: 'ec2',
resource_type: '1y-only', region: 'us-east-1', count: 1, term: 1,
payment: 'all-upfront', savings: 110, upfront_cost: 800, monthly_cost: 0 } as unknown as LocalRecommendation,
];
// cell-mixed: term=1 and term=3 variants (termMin=1, termMax=3, score=103)
const cellMixed = multiVariantCell({
resourceType: 'mixed-cell', payment1y: 'no-upfront', payment3y: 'no-upfront',
upfront1y: 0, upfront3y: 0,
});
// Selected cell: user picked the 3yr variant inside a (1, 3) cell.
const cellSel3y = multiVariantCell({
resourceType: 'sel3y-cell', payment1y: 'no-upfront', payment3y: 'no-upfront',
upfront1y: 0, upfront3y: 0,
});
// Selected cell: user picked the 1yr variant inside a (1, 3) cell.
const cellSel1y = multiVariantCell({
resourceType: 'sel1y-cell', payment1y: 'no-upfront', payment3y: 'no-upfront',
resourceType: 'mixed', payment1y: 'no-upfront', payment3y: 'no-upfront',
upfront1y: 0, upfront3y: 0,
});
const recs = [...cellMixed, ...cellSel3y, ...cellSel1y];
// cell-3y-only: both variants are term=3 (termMin=3, termMax=3, score=303)
const cell3yOnly: LocalRecommendation[] = [
{ id: '3yonly-nu', provider: 'aws', cloud_account_id: 'a1', service: 'ec2',
resource_type: '3y-only', region: 'us-east-1', count: 1, term: 3,
payment: 'no-upfront', savings: 300, upfront_cost: 0, monthly_cost: 40 } as unknown as LocalRecommendation,
{ id: '3yonly-au', provider: 'aws', cloud_account_id: 'a1', service: 'ec2',
resource_type: '3y-only', region: 'us-east-1', count: 1, term: 3,
payment: 'all-upfront', savings: 320, upfront_cost: 2400, monthly_cost: 0 } as unknown as LocalRecommendation,
];
const recs = [...cell3yOnly, ...cellMixed, ...cell1yOnly];

// Select the 3yr variant from the mixed cell. Under the old (buggy) code
// this caused mixed-cell's score to become 3*100+3=303, tying it with
// 3y-only and pushing it behind 1y-only in asc order.
setupTestFixture(recs, { column: 'term', direction: 'asc' });
// Override selection mock AFTER setupTestFixture so it sticks.
(state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(
new Set(['sel3ycell-3y', 'sel1ycell-1y']),
new Set(['mixed-3y']),
);
await loadRecommendations();

const order = renderedCellOrder();
// Expected under term asc:
// selected-1y cell (score = 1*100+1 = 101)
// mixed cell (score = 1*100+3 = 103)
// selected-3y cell (score = 3*100+3 = 303)
expect(indexOrFail(order, 'sel1y-cell'))
.toBeLessThan(indexOrFail(order, 'mixed-cell'));
expect(indexOrFail(order, 'mixed-cell'))
.toBeLessThan(indexOrFail(order, 'sel3y-cell'));
// Expected under term asc (selection must NOT affect order):
// 1y-only (score=101) < mixed (score=103) < 3y-only (score=303)
expect(indexOrFail(order, '1y-only'))
.toBeLessThan(indexOrFail(order, 'mixed'));
expect(indexOrFail(order, 'mixed'))
.toBeLessThan(indexOrFail(order, '3y-only'));
});

// -------------------------------------------------------------------------
// Issue #768: toggling checkboxes must not change row sort order.
//
// Before the fix, cellScoreFor() switched to the selected variant's
// individual value when selectedRecs contained a variant id, causing
// groupsInSortOrder() to produce a different order after a checkbox toggle.
// -------------------------------------------------------------------------
test('Issue #768: row order is identical before and after toggling checkboxes', async () => {
// Three cells with distinct savings so they sort in a predictable order.
// Cell A: savings = 10 (lowest) → should be last under desc
// Cell B: savings = 50 (middle)
// Cell C: savings = 90 (highest) → should be first under desc
const cellA = multiVariantCell({
resourceType: 'cell-A', payment1y: 'no-upfront', payment3y: 'no-upfront',
upfront1y: 0, upfront3y: 0, savings1y: 10, savings3y: 15,
});
const cellB = multiVariantCell({
resourceType: 'cell-B', payment1y: 'no-upfront', payment3y: 'no-upfront',
upfront1y: 0, upfront3y: 0, savings1y: 50, savings3y: 55,
});
const cellC = multiVariantCell({
resourceType: 'cell-C', payment1y: 'no-upfront', payment3y: 'no-upfront',
upfront1y: 0, upfront3y: 0, savings1y: 90, savings3y: 95,
});
const recs = [...cellA, ...cellB, ...cellC];

// --- render with no selection ---
setupTestFixture(recs, { column: 'savings', direction: 'desc' });
(state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(new Set<string>());
await loadRecommendations();
const orderBefore = renderedCellOrder();

// Verify base order (desc: C > B > A).
expect(indexOrFail(orderBefore, 'cell-C'))
.toBeLessThan(indexOrFail(orderBefore, 'cell-B'));
expect(indexOrFail(orderBefore, 'cell-B'))
.toBeLessThan(indexOrFail(orderBefore, 'cell-A'));

// --- simulate toggling two checkboxes (select 1yr variant of A and 3yr of C) ---
setupTestFixture(recs, { column: 'savings', direction: 'desc' });
(state.getSelectedRecommendationIDs as jest.Mock).mockReturnValue(
new Set(['cellA-1y', 'cellC-3y']),
);
await loadRecommendations();
const orderAfter = renderedCellOrder();

// Row order must be identical after selection change.
expect(orderAfter).toEqual(orderBefore);
});

test('Sort is deterministic across repeated renders for each affected column', async () => {
Expand Down
37 changes: 10 additions & 27 deletions frontend/src/recommendations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,34 +825,16 @@ function sortVariantsInCell(variants: LocalRecommendation[]): LocalRecommendatio
function cellScoreFor(
column: string,
variants: LocalRecommendation[],
selectedRecs: ReadonlySet<string>,
): number | string {
// Selected-variant short-circuit: when one variant is selected in the cell,
// sort the cell by THAT variant's value. Matches the rendered summary row,
// which also surfaces the selected variant's individual values.
const selected = variants.find((r) => selectedRecs.has(r.id));
// Issue #768: do NOT vary the score by selection state. Passing selectedRecs
// into the comparator caused rows to reorder on every checkbox toggle because
// a selected variant's individual value differs from the cell-summary value
// used for unselected cells. Sort order is always derived from the
// deterministic cell-summary score below, independent of selection.

const numericKey = SORTABLE_NUMERIC_COLUMNS[column];
const stringKey = SORTABLE_STRING_COLUMNS[column];

if (selected) {
// Term uses the same termMin*100+termMax encoding for selected and
// non-selected cells so both paths produce comparable scores. (For a
// selected variant, min == max == selected.term, so the encoding
// simplifies to term*101, still ordered correctly against multi-variant
// cells encoded the same way.)
if (column === 'term') return selected.term * 100 + selected.term;
// Upfront uses raw selected.upfront_cost which matches the non-selected
// path's primary key (upfrontMin); the tiny tiebreaker term in the
// non-selected path is too small to flip a real comparison.
if (numericKey) return numericKey(selected);
if (stringKey && column === 'payment') {
// Selected-variant payment: use PAYMENT_ORDER index for semantic sort.
return String(PAYMENT_ORDER[selected.payment ?? ''] ?? 99);
}
if (stringKey) return stringKey(selected);
}

if (variants.length === 0) {
// Defensive: empty cells should not occur post-groupRecsByCell, but if one
// slips through return a sentinel that sorts last.
Expand Down Expand Up @@ -929,7 +911,6 @@ function cellScoreFor(
function groupsInSortOrder(
groups: Map<string, LocalRecommendation[]>,
sort: { column: string; direction: 'asc' | 'desc' },
selectedRecs: ReadonlySet<string>,
): string[] {
const direction = sort.direction === 'asc' ? 1 : -1;

Expand All @@ -938,8 +919,8 @@ function groupsInSortOrder(
const va = groups.get(ka)!;
const vb = groups.get(kb)!;

const scoreA = cellScoreFor(sort.column, va, selectedRecs);
const scoreB = cellScoreFor(sort.column, vb, selectedRecs);
const scoreA = cellScoreFor(sort.column, va);
const scoreB = cellScoreFor(sort.column, vb);

if (typeof scoreA === 'number' && typeof scoreB === 'number') {
// Nulls are encoded as POSITIVE_INFINITY. Always sort them last
Expand Down Expand Up @@ -2561,7 +2542,9 @@ function buildListMarkup(

// issues #225 + #226: group by cell, sort groups, then render.
const groups = groupRecsByCell(recommendations);
const sortedKeys = groupsInSortOrder(groups, sort, selectedRecs);
// Issue #768: sort order is selection-independent; selectedRecs is used only
// for per-row rendering (checked state, CSS class) and the summary cards.
const sortedKeys = groupsInSortOrder(groups, sort);

// Update module-level group key cache for the Expand-All button.
// Only multi-variant cells count — single-variant cells render flat with no chevron.
Expand Down
Loading