fix(frontend/recs): enforce one-variant-per-cell radio selection (closes #224)#231
Merged
Merged
Conversation
#224) 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 `<input type="radio" name="cell-X">` 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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEnforces one-variant-per-physical-resource selection: adds ChangesRecommendation Cell Selection Enforcement
Sequence Diagram(s)sequenceDiagram
participant UI as Recommendations UI
participant State as Selection State Store
participant Data as Loaded / Visible Recommendations
UI->>State: user clicks "select all"
State->>Data: getVisibleRecommendations()
Data-->>State: visible recs
State->>State: compute pickBestVariantPerCell(visible recs)
State->>State: clear existing selections
State->>State: addSelectedRecommendation(best-per-cell ids)
State-->>UI: update checkboxes (one-per-cell)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 3/5 reviews remaining, refill in 21 minutes and 51 seconds. Comment |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/recommendations.ts`:
- Around line 179-182: The current defensive clamp uses monthsInTerm =
Math.max(1, r.term * 12) which treats a malformed term === 0 as 1 month; change
the clamp to ensure a minimum of 12 months (1 year) before dividing: compute
monthsInTerm as Math.max(12, r.term * 12) (or Math.max(12, (r.term || 1) * 12)
to handle missing/falsy term values) and keep the rest of the calculation that
returns r.savings - (r.upfront_cost / monthsInTerm) unchanged.
- Around line 2013-2021: When evicting sibling variants, don't iterate the
filtered recommendations list; use the full loaded recommendation set so hidden
siblings get deselected too. Replace the loop "for (const r of recommendations)"
with iteration over the full collection (e.g., state.getAllRecommendations() or
the loadedRecommendations array) and keep the existing logic: compute newCell
via cellKey(newRec), check "r.id !== id && selected.has(r.id) && cellKey(r) ===
newCell", and call state.removeSelectedRecommendation(r.id) for matches.
🪄 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: 53724e9a-f05c-45ef-bb97-cee3f072ae39
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
cristim
added
priority/p1
…-set sibling eviction - 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
Member
Author
CR feedback addressedActionable fix 1 (minor) — term clamp
Actionable fix 2 (major) — hidden sibling eviction
Tests updated
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
The new regression test for the hidden-sibling scenario is a good addition to lock in this behaviour. Triggering a fresh review now. [review] |
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 379-409: The test fails because the change handler now calls
state.getRecommendationByID(id) but this mock isn't provided; update the test to
mock state.getRecommendationByID to return the corresponding recommendation
object from the full set before dispatching the checkbox change. Specifically,
in this test (and the similar block around lines 2096-2151) add a jest mock on
state.getRecommendationByID that maps the id argument to the correct entry from
the allRecs array (e.g. return allRecs.find(r => r.id === id)) so that
loadRecommendations → checkbox change → state.getRecommendationByID(id) resolves
and the sibling-eviction assertions (state.removeSelectedRecommendation /
state.addSelectedRecommendation) run reliably.
- Around line 2156-2226: The tests for select-all are not seeding
state.getVisibleRecommendations(), so the select-all handler reads an empty list
instead of the mocked recs; update the two tests that call loadRecommendations()
(the tests around the select-all behavior) to mock
state.getVisibleRecommendations to return the same recommendations array you
pass to api.getRecommendations (e.g., set (state.getVisibleRecommendations as
jest.Mock).mockReturnValue(recs) before triggering select-all), ensuring the
select-all handler sees the same candidates as loadRecommendations populated.
🪄 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: 23ac19a0-78f6-4fc0-9486-a431e532bf7c
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/recommendations.ts
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced May 3, 2026
This was referenced May 4, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #224. After PR #195's per-(term, payment) fan-out, the rec-table checkboxes had no mutual-exclusion logic — manual checking accumulated sibling commitments AND
select-allover-committed 6× per cell (3 cells × 6 variants = 18 selected instead of the intended 3). This PR enforces "at most one variant per cell selected at any time", where cell =(provider, account, service, region, resource_type, engine).Summary
cellKey(rec)helper atfrontend/src/recommendations.ts(just below the SORTABLE_*_COLUMNS block) — same prefix the scheduler ID encoding uses (fix(scheduler): include term + account + engine in rec ID hash (#187, #188) #189), minus the(term, payment)suffix.select-allrewritten: clear current selection, thenpickBestVariantPerCell(recommendations)returns one rec per cell (highest effective monthly savings:savings - upfront_cost / (term * 12)). Sibling feat(frontend/recs): default-select per-cell variant matching configured term + payment #223 will replace the tiebreaker with "matches resolved settings"; until then, amortized-savings is the right deterministic default.Out of scope (deliberate)
<input type="radio">markup — Designer call per the issue; cell-grouping (feat(frontend/recs): group rec rows by physical-resource cell so variants are visually adjacent #226) lands first then radios become visually correct.Test plan
frontend/src/__tests__/recommendations.test.ts'Issue #224: one-variant-per-cell radio selection'describe:select-allpicks exactly one variant per cell (3 cells × 6 variants → 3 selected)select-allpicks highest-effective-savings (amortized) per cell — concrete fixture where the highest-RAW-savings variant has the LOWEST effective and is NOT pickedtsc --noEmitclean. Pre-commit hooks (frontend tests + build + scanners) all pass on the local commit.Summary by CodeRabbit
Bug Fixes
Tests