feat(recommendations): group per-plan-type SP rows in table (closes #135)#807
Conversation
cristim
added
triaged
|
Warning Review limit reached
More reviews will be available in 5 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR groups Savings Plans rows by provider/account/region, tracking group expansion with ChangesSavings Plans Plan-Type Grouping
Sequence DiagramsequenceDiagram
participant User
participant Chevron as .rec-sp-group-chevron Handler
participant SpGroups as expandedSpGroups State
participant Renderer as buildListMarkup()
participant DOM
User->>Chevron: click chevron
Chevron->>SpGroups: toggle spGroupKey in Set
Chevron->>Renderer: trigger rerender(loadedRecs)
Renderer->>Renderer: compute spGroupKey / groupSpCellKeys
alt group expanded
Renderer->>DOM: render parent + child rows (aggregated savings)
else group collapsed
Renderer->>DOM: render parent row only (aggregated savings)
end
DOM-->>User: update table display
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labelspriority/p2, severity/medium, urgency/this-sprint, effort/m 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 unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai review Re-pinging - the previous |
|
(ɔ◔‿◔)ɔ ♥ ✅ Actions performedFull review 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
🧹 Nitpick comments (2)
frontend/src/__tests__/recommendations.test.ts (2)
6623-6638: ⚡ Quick winAssert full in-group order, not just the first element.
Line 6637 only validates index 0, so a regression that reorders the remaining keys would still pass. Assert the whole ordered sequence.
Diff suggestion
- // order must match sortedKeys order (insertion order here) - expect(cellKeys).toHaveLength(3); - expect(cellKeys[0]).toBe(sortedKeys[0]); + // order must match sortedKeys order (insertion order here) + expect(cellKeys).toHaveLength(3); + expect(cellKeys).toEqual(sortedKeys);🤖 Prompt for 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. In `@frontend/src/__tests__/recommendations.test.ts` around lines 6623 - 6638, The test currently only asserts the first element of the in-group ordering (expect(cellKeys[0]).toBe(sortedKeys[0])) which misses regressions that reorder later elements; update the assertion to verify the entire sequence order by comparing cellKeys to sortedKeys (or asserting each index equals the corresponding sortedKeys entry) after calling groupRecsByCell and groupSpCellKeys so the test enforces full in-group order for the variables sortedKeys and cellKeys produced by groupRecsByCell and groupSpCellKeys.
6778-6799: ⚡ Quick winAdd collapse-after-expand assertion to lock toggle symmetry.
This test validates expand only. Add a second click assertion so collapse regressions are caught too.
Diff suggestion
// aria-expanded should flip const updatedChevron = document.querySelector<HTMLButtonElement>('.rec-sp-group-chevron'); expect(updatedChevron!.getAttribute('aria-expanded')).toBe('true'); + + // Collapse back: child rows should hide again and aria-expanded should reset. + updatedChevron!.click(); + expect(document.querySelectorAll('.recommendation-row').length).toBe(0); + const collapsedChevron = document.querySelector<HTMLButtonElement>('.rec-sp-group-chevron'); + expect(collapsedChevron!.getAttribute('aria-expanded')).toBe('false'); });🤖 Prompt for 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. In `@frontend/src/__tests__/recommendations.test.ts` around lines 6778 - 6799, Add a second click and assertions to verify collapse symmetry: in the test "clicking SP group chevron expands and shows child rows" (after calling loadRecommendations and the first click/asserts), call chevron.click() again on the same '.rec-sp-group-chevron' element and assert that document.querySelectorAll('.recommendation-row') now has length 0 (no child rows) and that the chevron's aria-expanded attribute is 'false'; this ensures loadRecommendations test and toggle behavior are validated both for expand and collapse.
🤖 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/recommendations.ts`:
- Line 2647: The SP child-row calls to buildVariantRowMarkup are missing the
showCheckboxes argument, causing readonly/viewer sessions to still render
checkboxes; update the two calls that pass (childVariants[0]!, selectedRecs,
true, visibleCols) (and the similar one at the other occurrence) to include the
showCheckboxes variable instead of the hardcoded true so the signature
buildVariantRowMarkup(..., showCheckboxes, visibleCols) receives the correct
boolean from the surrounding scope.
- Around line 4022-4042: Remove the duplicate unconditional row-click listener:
the block that calls
container.querySelectorAll('tr.recommendation-row').forEach(...) and adds a
'click' handler which finds cb via
tr.querySelector('input[type="checkbox"][data-rec-id]') is redundant with the
handler already added inside the if (showCheckboxes) branch; delete this
unconditional block so row clicks are only wired when showCheckboxes is true
(or, alternatively, gate this listener with the existing showCheckboxes
condition and ensure you bail if cb is null before dispatching).
---
Nitpick comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 6623-6638: The test currently only asserts the first element of
the in-group ordering (expect(cellKeys[0]).toBe(sortedKeys[0])) which misses
regressions that reorder later elements; update the assertion to verify the
entire sequence order by comparing cellKeys to sortedKeys (or asserting each
index equals the corresponding sortedKeys entry) after calling groupRecsByCell
and groupSpCellKeys so the test enforces full in-group order for the variables
sortedKeys and cellKeys produced by groupRecsByCell and groupSpCellKeys.
- Around line 6778-6799: Add a second click and assertions to verify collapse
symmetry: in the test "clicking SP group chevron expands and shows child rows"
(after calling loadRecommendations and the first click/asserts), call
chevron.click() again on the same '.rec-sp-group-chevron' element and assert
that document.querySelectorAll('.recommendation-row') now has length 0 (no child
rows) and that the chevron's aria-expanded attribute is 'false'; this ensures
loadRecommendations test and toggle behavior are validated both for expand and
collapse.
🪄 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: 4b5404f3-8f2c-4909-bc84-7e295f4ff363
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
|
Rebased on Addressed CR pass-1 findings:
Verified locally: @coderabbitai review |
|
Kicking off a fresh review of the rebased branch now. 🧠 Learnings used✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Addressed the one open CR finding:
Added a regression test ( Verification: @coderabbitai review |
|
Kicking off a fresh review now. 🧠 Learnings used✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/__tests__/recommendations.test.ts (2)
6834-6852: ⚡ Quick winPin the dedicated SP child-row contract, not just generic row counts.
These assertions only count
.recommendation-row, so a regression that stops rendering.rec-sp-group-childrows correctly—or breaks the nested child-chevron path for multi-variant plan types—could still pass. Please add one case that asserts the child-row class/structure after expanding the parent, and one grouped child that is itself multi-variant.Also applies to: 6905-6932
🤖 Prompt for 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. In `@frontend/src/__tests__/recommendations.test.ts` around lines 6834 - 6852, The test "two SP plan types render one parent group row and no flat child rows (collapsed)" only asserts generic .recommendation-row counts; update it to pin the dedicated SP child-row contract by (1) simulating expansion of the parent group (e.g. trigger the same click/expand action used elsewhere after loadRecommendations()) and then asserting that .rec-sp-group-child elements are rendered (use document.querySelectorAll('.rec-sp-group-child') and expect a count of 2), and (2) include a grouped child that is multi-variant (create one recommendation via mkSpRec that represents a multi-variant plan) and assert that its nested multi-variant structure or chevron toggles render the expected child elements (check for the specific class used for multi-variant children). Keep references to mkSpRec, loadRecommendations, .rec-sp-group-row, .rec-sp-group-child and the parent expand action so the test fails if SP child-row rendering or multi-variant nesting breaks.
6750-6765: ⚡ Quick winMake the order test independent of
groups.keys()insertion order.This currently derives
sortedKeysfromArray.from(groups.keys()), so it would still pass ifgroupSpCellKeys()ignored thesortedKeysargument and iterated the map directly. Build a deliberately permutedsortedKeysinput here and assert the output follows that order.🤖 Prompt for 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. In `@frontend/src/__tests__/recommendations.test.ts` around lines 6750 - 6765, The test currently derives sortedKeys from Array.from(groups.keys()) which ties it to the map insertion order; instead construct a deliberately permuted sortedKeys array (e.g., reorder the known cell key strings created via mkSpRec) rather than using groups.keys(), call groupSpCellKeys(sortedKeys, groups), and assert the returned cellKeys array matches that permuted sortedKeys order; update the test 'preserves sort order within each group' so sortedKeys is an explicit hard-coded permutation of the three keys and the expectations check equality to that permutation.frontend/src/__tests__/recommendations-permissions.test.ts (1)
358-359: 💤 Low valueOptional: assert the exact child-row count to tighten the guard.
This scope has two plan types, so the expanded group should produce a deterministic number of child rows.
toBeGreaterThan(0)would still pass if grouping silently collapsed or duplicated rows. Asserting the exact count (e.g.,toBe(2)) makes the test fail loudly on regressions in the grouping logic.Also applies to: 374-375
🤖 Prompt for 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. In `@frontend/src/__tests__/recommendations-permissions.test.ts` around lines 358 - 359, The assertion using childRows.length > 0 is too loose; update the test in recommendations-permissions.test.ts to assert the exact expected number of expanded child rows (use childRows.length === expectedCount or Jest's toBe/toHaveLength) for the selector 'tr.rec-variant-row' (variable childRows from table.querySelectorAll) — replace expect(childRows.length).toBeGreaterThan(0) with an exact count (e.g., toBe(2)) and make the same replacement for the similar assertion around lines 374-375.
🤖 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-permissions.test.ts`:
- Around line 352-380: The test should target the per-variant child rows (not
the SP-group summary row) so update the test selectors to assert against the
nested variant markup produced by buildVariantRowMarkup (i.e. use
"tr.rec-variant-row") and validate checkbox presence via "td.checkbox-col" and
"input[data-rec-id]"; ensure both readonly and admin tests call expandSpGroup()
then query for "tr.rec-variant-row" and assert checkbox absence when
showCheckboxes is false and presence when true (refer to buildVariantRowMarkup,
showCheckboxes, and expandSpGroup to locate the rendering path).
---
Nitpick comments:
In `@frontend/src/__tests__/recommendations-permissions.test.ts`:
- Around line 358-359: The assertion using childRows.length > 0 is too loose;
update the test in recommendations-permissions.test.ts to assert the exact
expected number of expanded child rows (use childRows.length === expectedCount
or Jest's toBe/toHaveLength) for the selector 'tr.rec-variant-row' (variable
childRows from table.querySelectorAll) — replace
expect(childRows.length).toBeGreaterThan(0) with an exact count (e.g., toBe(2))
and make the same replacement for the similar assertion around lines 374-375.
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 6834-6852: The test "two SP plan types render one parent group row
and no flat child rows (collapsed)" only asserts generic .recommendation-row
counts; update it to pin the dedicated SP child-row contract by (1) simulating
expansion of the parent group (e.g. trigger the same click/expand action used
elsewhere after loadRecommendations()) and then asserting that
.rec-sp-group-child elements are rendered (use
document.querySelectorAll('.rec-sp-group-child') and expect a count of 2), and
(2) include a grouped child that is multi-variant (create one recommendation via
mkSpRec that represents a multi-variant plan) and assert that its nested
multi-variant structure or chevron toggles render the expected child elements
(check for the specific class used for multi-variant children). Keep references
to mkSpRec, loadRecommendations, .rec-sp-group-row, .rec-sp-group-child and the
parent expand action so the test fails if SP child-row rendering or
multi-variant nesting breaks.
- Around line 6750-6765: The test currently derives sortedKeys from
Array.from(groups.keys()) which ties it to the map insertion order; instead
construct a deliberately permuted sortedKeys array (e.g., reorder the known cell
key strings created via mkSpRec) rather than using groups.keys(), call
groupSpCellKeys(sortedKeys, groups), and assert the returned cellKeys array
matches that permuted sortedKeys order; update the test 'preserves sort order
within each group' so sortedKeys is an explicit hard-coded permutation of the
three keys and the expectations check equality to that permutation.
🪄 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: 5f29dfc1-0202-42a3-917f-28021fc09301
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations-permissions.test.tsfrontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
|
Rebased on Resolved additive conflicts in:
Verification post-rebase:
Note on CI: No CR findings outstanding from prior passes. Requesting an incremental review pass on the rebased diff. @coderabbitai review |
|
Kicking off an incremental review of the rebased diff now. 🧠 Learnings used✅ Action performedReview finished.
|
) Add an SP plan-type group row in the Recommendations table whenever two or more savings-plans-* cell rows share the same provider / account / region scope. - New module-level expandedSpGroups Set; cleared via resetExpandedCells - spGroupKey(rec) canonical scope key (provider|account|region) - groupSpCellKeys(sortedKeys, groups) returns Map<sgk, cellKey[]> for scopes with 2+ plan types; singletons fall through to flat rendering - buildListMarkup renders a rec-sp-group-row parent with a rec-sp-group-chevron, combined savingsPlansBucketLabel badge, +N plan types count badge, and aggregated savings text - Clicking the chevron toggles expandedSpGroups and re-renders - 18 new tests (10 pure-helper, 8 DOM integration); 337 pass total
Address CodeRabbit nitpicks on the issue #135 SP-grouping tests: - groupSpCellKeys in-group order: assert the entire ordered sequence (expect(cellKeys).toEqual(sortedKeys)) instead of only index 0, so a regression that reorders later keys is caught. - SP group chevron toggle: add a second click + assertions so the test exercises collapse as well as expand (child rows hide, aria-expanded resets to false), locking toggle symmetry.
The two buildVariantRowMarkup calls that render expanded Savings Plans plan-type group child rows omitted the showCheckboxes argument, so it defaulted to true. In readonly/viewer sessions (showCheckboxes === false) the SP-group child rows rendered a checkbox cell while the column header and non-SP rows did not, leaving the table misaligned and exposing an inert checkbox to a role with no purchase/plan actions. Forward showCheckboxes from the enclosing buildListMarkup scope into both nested calls so SP-group child rows match the non-SP path and the header. Add a regression test (readonly + admin) that expands an SP plan-type group and asserts the child variant rows honor the role's checkbox visibility. The readonly assertion fails before the fix and passes after.
|
Re-rebased on Resolved additive conflicts in:
Verification post-rebase:
CI note: any @coderabbitai review |
|
Kicking off an incremental review of the latest rebased diff now. 🧠 Learnings used✅ Action performedReview finished.
|
1 participant
Summary
savings-plans-*cell rows share the same AWS account and regionSavings Plans (Compute + SageMaker + ...)label via the existingsavingsPlansBucketLabelhelper, a+N plan typescount badge, an aggregated savings figure, and an expand/collapse chevronImplementation shape
Parent + children: one
rec-sp-group-rowparent row per multi-plan-type scope; children rendered asrec-sp-group-childrows inside the expanded panel. Pure helpersspGroupKeyandgroupSpCellKeysexported for unit testing.Test plan
npx tsc --noEmitclean (0 errors)npx jest --testPathPattern=recommendations.test337 pass, 0 fail (319 pre-existing + 18 new)spGroupKey,groupSpCellKeys(scoping, sort order, non-SP exclusion, singleton passthrough)Summary by CodeRabbit
New Features
Tests