fix(ux/opportunities): deterministic sort on Term/Payment/Cost/Effective % cells by cristim · Pull Request #496 · LeanerCloud/CUDly

cristim · 2026-05-19T21:02:26Z

Summary

Fixes #494 — on the Opportunities table, clicking the sort headers for Term / Payment / Upfront Cost / Monthly Cost / Effective % produced apparently-random row order because Math.max(...va.map(numericKey)) collapsed every multi-variant cell to the same score after PR #195's per-(term, payment) fan-out (every cell has both 1yr and 3yr variants → always max=3 for term; any null monthly_cost / pct → POSITIVE_INFINITY; cells with all-no-upfront variants → tie at 0).

PR #491 (closes #480) flipped the default direction for these columns from desc to asc; this PR fixes the upstream "every cell ties" symptom. The two PRs touch independent code paths (#491's diff against the same base doesn't modify groupsInSortOrder).

What changed

New cellScoreFor(column, variants, selectedRecs) helper in frontend/src/recommendations.ts that returns ONE deterministic per-cell score per column:
- term → summary.termMin * 100 + summary.termMax (encodes both ends; 1yr-only < mixed < 3yr-only)
- upfront_cost → summary.upfrontMin (matches rendered range)
- monthly_cost → Math.min over non-null variants; POSITIVE_INFINITY only when ALL variants are null
- effective_savings_pct → Math.max over non-null variants; same null rule
- payment (categorical) → PAYMENT_ORDER index of the first variant in sortVariantsInCell order (replaces alphabetic comparator that placed all-upfront before no-upfront)
Selected-variant cells use the selected variant's value on the SAME numeric scale as non-selected cells (specifically for term, otherwise a selected term=1 cell would sort below a non-selected mixed (1, 3) cell when ascending).
Stable cellKey.localeCompare tiebreaker (direction-invariant) for genuinely-tied cells so renders are deterministic across reloads and browsers.
Other numeric columns (savings, count, on_demand_monthly) keep the established Math.max semantics — #494 does not flag them.
Other string columns (provider, account, service, region, resource_type) keep va[0] — the value is invariant across a cell's variants by definition of cellKey.

Regression coverage

10 new tests in frontend/src/__tests__/recommendations.test.ts exercise multi-variant fixtures (the bug only manifests post per-(term, payment) fan-out):

4.9 Term asc and 4.9 Term desc — 1yr-only / mixed / 3yr-only cells ordered correctly
4.10 Payment asc — no-upfront < partial-upfront < all-upfront
4.12 Upfront Cost asc — sorted by upfrontMin
4.13 Monthly Cost asc — sorted by Math.min over non-null variants
4.13 Monthly Cost null-handling — all-null cells stay last in both asc and desc
4.15 Effective % asc — sorted by Math.max over non-null variants
4.15 Effective % null-handling — all-null cells stay last in both directions
Selected-variant scale parity — selected term=1 ranks above non-selected mixed (1, 3)
Determinism — repeating the same sort twice produces identical row order for all 5 columns (regression for "two clicks of the same header produce different broken orders" per QA)

Test plan

npm test — 1752 pass / 1 skip / 0 fail (54 suites; was 1742 pre-PR + 10 new regression tests)
npx tsc --noEmit — clean
npm run build — production bundle compiles cleanly (no new warnings)
Em-dash gate: zero net delta in recommendations.ts and recommendations.test.ts (per project convention)
Post-merge: verify on a deployed preview that clicking each of the five sort headers on a multi-variant page reorders rows deterministically and in the expected semantic order

Out of scope

PR fix(ux/opportunities): table interaction bugs (#479-#484) #491's domain (default direction, URL sort sync, tri-state checkboxes, popover scroll, numeric filter rounding) — independent branch, independent merge.
on_demand_monthly column — same root cause technically applies but is not flagged in the issue body's QA rows (4.9-4.15); leaving unchanged here to keep blast radius tight. If the same pattern matters there too, filing a follow-up after merge per CLAUDE.md §1.

Closes #494

Summary by CodeRabbit

Bug Fixes
- Recommendation cell sorting made deterministic and stable: consistent ordering across rerenders, ties resolved predictably, null-like values sort last, and selected variants participate without changing the scoring scale.
Tests
- Added comprehensive tests covering multi-variant cells, column-specific ordering (term, payment, upfront, monthly, effective savings), null handling, and repeatability.

…ive % cells The previous `groupsInSortOrder` collapsed every multi-variant cell to the same score via `Math.max(...va.map(numericKey))`: after PR #195's per-(term, payment) fan-out every cell has BOTH 1yr and 3yr variants, so `Math.max` always = 3 for the `term` column; cells with any null monthly_cost or null pct collapse to `POSITIVE_INFINITY`; cells with all-no-upfront variants tie at 0 for `upfront_cost`. Every cell tying produces apparently-random row order (Array.prototype.sort stability is implementation-defined). Extracts `cellScoreFor(column, variants, selectedRecs)` and replaces the collapse with per-column scores that match what the user sees in the rendered summary row: * term: `summary.termMin * 100 + summary.termMax` (encodes both ends so 1yr-only sorts before mixed sorts before 3yr-only) * upfront_cost: `summary.upfrontMin` (matches the rendered `formatSavingsRange(upfrontMin, upfrontMax)`) * monthly_cost: `min` over non-null variants; POSITIVE_INFINITY only when ALL variants are null * effective_savings_pct: `max` over non-null variants; same null rule * payment (categorical): `PAYMENT_ORDER` index of the first variant in `sortVariantsInCell` order; replaces the alphabetic comparator (which placed `all-upfront` before `no-upfront`, semantically wrong) Selected-variant cells (user clicked a radio inside the cell) keep using the selected variant's value, but on the SAME numeric scale as the non-selected path for `term` so the two are directly comparable (otherwise a selected `term=1` cell would rank below a non-selected mixed (1, 3) cell when it should rank above). For other numeric columns (savings, count, on_demand_monthly) the established `Math.max` semantics are preserved. For the string columns that aren't payment (provider, account, service, region, resource_type) the value is invariant across a cell's variants by definition of cellKey, so the first variant suffices. Adds a stable `cellKey.localeCompare` tiebreaker (direction-invariant) so genuinely-tied cells render in a deterministic order across renders and across browsers. 10 new regression tests in `recommendations.test.ts` cover: * Term ascending: 1yr-only cell < mixed cell < 3yr-only cell * Term descending: exact reverse * Payment ascending: no-upfront < partial-upfront < all-upfront * Upfront Cost ascending: sorted by upfrontMin * Monthly Cost ascending: sorted by `Math.min` over non-null variants * Monthly Cost: all-null cells sort last in BOTH asc and desc * Effective % ascending: sorted by `Math.max` over non-null variants * Effective %: all-null cells sort last in both directions * Selected-variant interaction: selected term=1 cell sorts above non-selected mixed (1, 3) cell when ascending * Determinism: repeating the same sort twice produces identical row order for all 5 affected columns Closes #494

coderabbitai · 2026-05-19T21:02:33Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: da5d0543-0756-4d70-9341-e319c963ad2c

📥 Commits

Reviewing files that changed from the base of the PR and between 31c64c2 and c0f2440.

📒 Files selected for processing (2)

frontend/src/__tests__/recommendations.test.ts
frontend/src/recommendations.ts

🚧 Files skipped from review as they are similar to previous changes (1)

frontend/src/recommendations.ts

📝 Walkthrough

Walkthrough

This PR implements deterministic per-cell per-column scoring (cellScoreFor()) and integrates it into groupsInSortOrder() with POSITIVE_INFINITY null sentinels and a stable cellKey tie-breaker; it adds comprehensive tests validating term/payment/upfront/monthly/effective sorting, null handling, selected-variant parity, and repeatability across renders.

Changes

Deterministic Cell Sorting

Layer / File(s)	Summary
Per-cell deterministic score computation `frontend/src/recommendations.ts`	New `cellScoreFor()` computes per-cell scores per column: term tuple, upfront min+micro-tiebreak, monthly_cost min across non-null with POSITIVE_INFINITY sentinel, effective_savings_pct max non-null with sentinel, PAYMENT_ORDER index for payment, Math.max for other numerics, first-variant strings; selected-variant short-circuits.
Grouped cell sort with score-based comparison and tie-breaking `frontend/src/recommendations.ts`	`groupsInSortOrder()` now compares cells using `cellScoreFor()` results, handles numeric null-last semantics in both directions, compares string scores via `localeCompare`, and tie-breaks equal scores by `cellKey` localeCompare for stable ordering.
Test infrastructure and fixture generation `frontend/src/__tests__/recommendations.test.ts`	Test utilities: DOM builder for loadRecommendations, multi-variant fixture generator, rendered summary-row order extractor, and standard API/state/sort mocks.
Term and payment column sorting tests `frontend/src/__tests__/recommendations.test.ts`	Term sorting validated for asc/desc using per-cell term bounds; payment sorting validated against canonical PAYMENT_ORDER.
Upfront and monthly cost column sorting tests `frontend/src/__tests__/recommendations.test.ts`	Upfront cost sorts by per-cell minimum; monthly cost sorts by min of non-null variants, with all-null cells sorting last and determinism regression for all-null tie cases.
Effective savings percentage and null-handling edge cases `frontend/src/__tests__/recommendations.test.ts`	effective_savings_pct sorting uses max non-null per cell with POSITIVE_INFINITY sentinel; all-variant-null cells sort last regardless of direction.
Selected-variant parity and determinism regression tests `frontend/src/__tests__/recommendations.test.ts`	Selected-variant cells use selected metrics while preserving the same scoring scale as non-selected cells; repeatability tests confirm identical ordering across repeated renders for multiple columns.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

ux(opportunities): Term/Payment/Cost/Effective columns sort non-deterministically (cell-score collapse, not direction) #494: This PR implements the deterministic per-cell per-column scoring and stable tie-breaking behavior described in the issue.

Possibly related PRs

LeanerCloud/CUDly#253: Related prior work on grouped/collapsed multi-variant recommendation cells that this deterministic sorting changes build upon.
LeanerCloud/CUDly#231: Related changes around selected-variant handling and UI selection that interact with per-cell selected-variant scoring.
LeanerCloud/CUDly#242: Related modifications to monthly_cost / effective_savings_pct handling that overlap with the new deterministic scoring logic.

Poem

🐰
Variants nested, cells aligned,
Scores computed, no ties unkind—
Nulls rest last, the order stays,
Selected scales keep even ways,
Repeatable rows make testers grin.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main fix: deterministic sorting for multi-variant cells across key columns (Term, Payment, Cost, Effective %), which is the core change in both test and production files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/issue-494-sort-cell-score-collapse

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

cristim · 2026-05-19T21:02:36Z

@coderabbitai review

coderabbitai · 2026-05-19T21:02:41Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 4910-4918: The sort assertions use
renderedCellOrder().findIndex(...) directly which can return -1 and produce
false-positive comparisons; update each test (e.g., the block using
renderedCellOrder and keys like 'bbb-1y-only', 'aaa-mixed', 'ccc-3y-only') to
first assert each expected key exists (e.g., expect(order.some(k =>
k.includes('...'))).toBeTrue()/toBeTruthy() or use a shared helper like
assertIndexExists(order, key) that throws a clear error if missing), then
perform the index comparisons using order.findIndex(...) so the tests fail
deterministically when a key is absent; apply the same pattern to the other
occurrences listed in the comment.

In `@frontend/src/recommendations.ts`:
- Around line 804-815: In the numeric-comparison branch (where scoreA/scoreB are
numbers) handle the case where both scores are Number.POSITIVE_INFINITY before
computing and returning the numeric diff: add an explicit check (e.g. if (scoreA
=== Number.POSITIVE_INFINITY && scoreB === Number.POSITIVE_INFINITY) { /* treat
as equal so fallback tie-breaker runs */ }) so you don't compute (Infinity -
Infinity) which yields NaN and causes an early return; leave the logic to fall
through to the cellKey fallback tie-breaker used elsewhere in this comparator.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8c749c7c-6135-4f7c-8ce3-1a3772a24a22

📥 Commits

Reviewing files that changed from the base of the PR and between 90f37e4 and 31c64c2.

📒 Files selected for processing (2)

frontend/src/__tests__/recommendations.test.ts
frontend/src/recommendations.ts

CodeRabbit flagged two issues on the initial commit: 1. **Major / production**: `(Infinity - Infinity) === NaN` in the numeric-diff branch of `groupsInSortOrder`. The `if (diff !== 0) return diff` short-circuit returns NaN (because `NaN !== 0`), so when both cells score POSITIVE_INFINITY (all-null monthly_cost or all-null effective_savings_pct on both sides) the cellKey tiebreaker is bypassed and the rendered order becomes implementation-defined. Guard the numeric diff with `if (!aNullish)` so both-nullish pairs fall through to the cellKey localeCompare tiebreaker. 2. **Minor / tests**: `order.findIndex(...)` returning `-1` would silently satisfy `<` comparisons and pass tests even when a key is missing. Extracted an `indexOrFail(order, fragment)` helper that asserts presence before returning the index; converted all 36 ordering assertions in the new #494 describe block to use it. Also adds one regression test (`two all-null cells sort deterministically via cellKey tiebreaker`) that exercises the both-POSITIVE_INFINITY path and asserts the cellKey-based fallback order is stable across repeated invocations. Per-file scope: * `frontend/src/recommendations.ts`: 5 lines added (guard + comment), 2 lines removed. * `frontend/src/__tests__/recommendations.test.ts`: indexOrFail helper + 36 call-site rewrites + 1 new regression test. Refs #494, #496 CR review.

cristim · 2026-05-19T21:12:38Z

Addressed both CR findings in c0f2440:

Major (Infinity - Infinity -> NaN in groupsInSortOrder): guarded the numeric diff with if (!aNullish) so both-nullish pairs fall through to the cellKey.localeCompare tiebreaker. Added a regression test (Monthly Cost: two all-null cells sort deterministically via cellKey tiebreaker) that pre-commit would have caught the bug, and asserts the cellKey-based fallback order is stable across repeated invocations.
Minor (findIndex returning -1 silently passing < assertions): extracted an indexOrFail(order, fragment) helper that asserts presence before returning the index, and converted all 36 ordering assertions in the new #494 describe block to use it. The helper lives next to renderedCellOrder for readability.

Frontend Jest suite: 1753 pass / 1 skip / 0 fail across 54 suites (was 1752 pre-CR-fix). npx tsc --noEmit clean. Production build clean.

@coderabbitai review

coderabbitai · 2026-05-19T21:12:58Z

@cristim Thanks for the detailed follow-up! Both fixes look well-reasoned:

The if (!aNullish) guard cleanly sidesteps the Infinity - Infinity → NaN pitfall, and the regression test for the two all-null cells tiebreaker is a great addition.
The indexOrFail helper is a much safer pattern than relying on silent -1 from findIndex — converting all 36 assertions to use it gives real confidence in the test results.

Triggering a review of the new commit now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim added priority/p1 Next up; this sprint severity/high Significant harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/m Days type/bug Defect triaged Item has been triaged bug Something isn't working labels May 19, 2026

coderabbitai Bot reviewed May 19, 2026

View reviewed changes

Comment thread frontend/src/__tests__/recommendations.test.ts

Comment thread frontend/src/recommendations.ts

cristim merged commit 887f51e into feat/multicloud-web-frontend May 19, 2026
5 checks passed

cristim deleted the fix/issue-494-sort-cell-score-collapse branch May 19, 2026 21:36

cristim mentioned this pull request May 19, 2026

ux(opportunities): Term/Payment/Cost/Effective columns sort non-deterministically (cell-score collapse, not direction) #494

Closed

coderabbitai Bot mentioned this pull request Jun 20, 2026

fix(opportunities): cost-column sort toggle + expand/collapse persistence on filter change #1259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ux/opportunities): deterministic sort on Term/Payment/Cost/Effective % cells - #496

fix(ux/opportunities): deterministic sort on Term/Payment/Cost/Effective % cells#496
cristim merged 2 commits into
feat/multicloud-web-frontendfrom
fix/issue-494-sort-cell-score-collapse

cristim commented May 19, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 19, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

Uh oh!

cristim commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

cristim commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

cristim commented May 19, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What changed

Regression coverage

Test plan

Out of scope

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

Uh oh!

cristim commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cristim commented May 19, 2026

Uh oh!

coderabbitai Bot commented May 19, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristim commented May 19, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 19, 2026 •

edited

Loading