feat(recommendations): All Savings Plans tri-state in service column-filter (#137)#181
feat(recommendations): All Savings Plans tri-state in service column-filter (#137)#181cristim wants to merge 2 commits intofeat/multicloud-web-frontendfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an "All Savings Plans" tri-state group toggle to the Recommendations service-column filter popover when 2+ savings-plan service slugs exist, plus an exported Changes
Sequence Diagram(s)sequenceDiagram
participant U as User (UI)
participant P as Popover (service filter)
participant S as State (setRecommendationsColumnFilter)
participant R as RecommendationsLoader
participant B as Backend
U->>P: open/popover renders checkboxes + SP-group toggle
P->>S: toggle ON -> setRecommendationsColumnFilter(all SP slugs)
S->>R: reload recommendations with SP filter
R->>B: request filtered recommendations
B-->>R: returns filtered results
R-->>U: render updated list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 6 minutes and 9 seconds. Comment |
|
@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: 1
🤖 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 1173-1181: The tests rely on mocked column-filter state but only
clear call history; add an explicit reset or set a known return for the
service-filter mock so this suite doesn't inherit prior state. In the beforeEach
alongside (api.getRecommendations, state.getRecommendations,
state.getVisibleRecommendations) either call
(state.getRecommendationsColumnFilters as jest.Mock).mockReset() or set a
deterministic return value with (state.getRecommendationsColumnFilters as
jest.Mock).mockReturnValue([]) to ensure consistent SP-popover ordering.
🪄 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: 4f9c2dfa-cc6d-41bb-80be-42b3ff082d9c
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations.test.tsfrontend/src/lib/purchase-compatibility.tsfrontend/src/recommendations.ts
Addresses CodeRabbit actionable on PR #181. The Issue #137 SP tri-state describe block's `beforeEach` only set the api / state.getRecommendations / state.getVisibleRecommendations mocks; it relied on the parent beforeEach's `jest.clearAllMocks()` + the global `state.getRecommendationsColumnFilters` mock default (empty filters) to keep tests order-independent. But specific tests in the suite override that mock with `{ service: { values: [...] } }` to drive the resync path — `clearAllMocks()` clears call history but leaves `mockReturnValue` set. A test that runs after one of those override-tests would then see the prior test's filter state during its popover build / resync, producing flaky orderings of the SP checkbox tri-state. Fix: explicitly reset the `state.getRecommendationsColumnFilters` mock to `{}` in the SP-suite beforeEach so every test starts from a known-empty filter state regardless of run order. Mirror of how the parent beforeEach pins the other state accessors. All 1363 frontend tests pass; this is a test-isolation hardening, no production-code change.
|
@coderabbitai review Addressed the actionable: SP-suite |
|
Triggering an incremental review now to pick up the latest commits. ✅ Actions performedReview triggered.
|
cristim
added
triaged
|
CR loop closed: original review's actionable was addressed in commit Note: PR #180 (issue #132) introduces the same |
…filter Closes #137. PR #123 split the single 'savings-plans' service slug into four per- plan-type slugs (savings-plans-{compute,ec2instance,sagemaker,database}), removing the operator's ability to one-click "show me all SP recs" from the Recommendations service-filter. Bundle B replaced the old service-filter dropdown with the per-column popover (checkbox list of distinct service values), so the previous fix proposed in the issue — adding an `<option value="all-sp">` to the optgroup — doesn't apply to the current UI. The equivalent affordance for a checkbox popover is a tri-state group toggle. This adds an "All Savings Plans" tri-state checkbox between the existing `(All)` row and the per-value list in the service column popover, conditional on 2+ SP slugs being present in the distinct values. Behaviour: - **checked**: every SP slug is in the active filter - **indeterminate**: a strict subset of SP slugs is in the filter - **unchecked**: no SP slug is in the filter - **click while checked → unchecked**: unticks every SP box, commits the resulting filter (clears it if no other slugs were ticked) - **click while unchecked/indeterminate → checked**: ticks every SP box, commits the filter with the SP slug values appended to any already-checked non-SP boxes - The four per-plan-type checkboxes remain individually selectable for narrowing — the group toggle is purely a bulk-select shortcut The per-plan-type filtering downstream is unchanged: applyColumnFilters still receives the array of slug values from the existing `{ kind: 'set', values: [...] }` filter shape, so the rec-load path sees per-plan-type values exactly as it did before. The group toggle does NOT render in: - Non-service columns (provider, account, region, term, etc.) - The service column when fewer than 2 SP slugs are present (no benefit to a "group toggle" with one box, and zero with none) Implementation: - New shared predicate `isSavingsPlanService` in `frontend/src/lib/purchase-compatibility.ts` mirroring `common.IsSavingsPlan` (`pkg/common/types.go`). `startsWith` form so a future plan type added on the backend is picked up automatically without a frontend edit. - New `updateSPTriState()` helper in `buildPopoverContent`, symmetric to the existing `updateAllTriState()`, scoped to SP slugs. - `commit()` now updates both tri-states; click handler on spBox ticks/unticks the SP boxes and re-uses the same `commit()` flow. - `resyncOpenPopover` recomputes the SP tri-state from current filter state on every anchor re-bind. Tests (7 new in `recommendations.test.ts`): - group toggle renders when 2+ SP slugs present - group toggle does NOT render for non-service columns - group toggle does NOT render when 0 or 1 SP slugs present - clicking ON commits filter with all 3 SP slug values - clicking OFF (when all SPs were already filtered) clears the filter - group toggle resyncs to indeterminate when only some SPs are active - individual SP checkbox change commits the partial-SP filter All 1363 frontend tests still pass (added 7 over the prior 1356). Note: the small `isSavingsPlanService` predicate is also added by PR first, the second will hit a trivial 3-line conflict in `purchase-compatibility.ts`; either version of the export is correct to keep.
Addresses CodeRabbit actionable on PR #181. The Issue #137 SP tri-state describe block's `beforeEach` only set the api / state.getRecommendations / state.getVisibleRecommendations mocks; it relied on the parent beforeEach's `jest.clearAllMocks()` + the global `state.getRecommendationsColumnFilters` mock default (empty filters) to keep tests order-independent. But specific tests in the suite override that mock with `{ service: { values: [...] } }` to drive the resync path — `clearAllMocks()` clears call history but leaves `mockReturnValue` set. A test that runs after one of those override-tests would then see the prior test's filter state during its popover build / resync, producing flaky orderings of the SP checkbox tri-state. Fix: explicitly reset the `state.getRecommendationsColumnFilters` mock to `{}` in the SP-suite beforeEach so every test starts from a known-empty filter state regardless of run order. Mirror of how the parent beforeEach pins the other state accessors. All 1363 frontend tests pass; this is a test-isolation hardening, no production-code change.
cristim
force-pushed
the
fix/issue-137-all-sp-filter
branch
from
8af52c4 to
f690c70
Compare
|
Rebased onto current Conflicts resolved:
The popover-rendering changes in Tests: @coderabbitai review |
|
Triggering an incremental review to pick up the rebased commits now. ✅ Actions performedReview triggered.
|
1 participant
Summary
Closes #137. Restores the one-click "filter to all Savings Plans" affordance that PR #123 removed when it split the single
savings-plansservice slug into four per-plan-type slugs (savings-plans-{compute,ec2instance,sagemaker,database}). The original issue proposed adding<option value="all-sp">to the optgroup — but Bundle B replaced the legacy service-filter dropdown with the per-column popover (checkbox list of distinct service values), so the equivalent affordance for a checkbox UI is a tri-state group toggle.What changed
In
frontend/src/recommendations.ts:buildPopoverContent, when the column isserviceAND 2+ SP slugs are in the distinct values, render a new "All Savings Plans" tri-state checkbox between the existing(All)row and the per-value list:Per-plan-type checkboxes remain individually selectable for narrowing — the group toggle is purely a bulk-select shortcut.
The toggle does NOT render in non-service columns or when fewer than 2 SP slugs are present.
New utility
isSavingsPlanService(service)infrontend/src/lib/purchase-compatibility.ts—startsWith('savings-plans'), mirror of Go'scommon.IsSavingsPlan(pkg/common/types.go).startsWithform so a future plan type added on the backend is picked up automatically.Tests
7 new tests in
recommendations.test.tspinning:All 1363 frontend tests pass. TypeScript typecheck + webpack build clean.
Conflict notice
The small
isSavingsPlanServicepredicate is also added by PR #180 (issue #132 — bulk-buy across SP plan types). Whichever lands first, the second hits a trivial 3-line conflict inlib/purchase-compatibility.ts; either version of the export is correct to keep.Test plan
🤖 Generated with claude-flow
Summary by CodeRabbit
New Features
Tests