Repository navigation
ux(frontend): group SP plan types in bulk-buy fan-out modal (closes #249) - #826
Conversation
) When a mixed-SP bucket (2+ plan types) lands in the fan-out modal, append a collapsible <details> section listing per-plan-type subtotals. Collapsed by default to keep the modal compact; the native chevron lets operators inspect the Compute/SageMaker/EC2 Instance/Database split before submitting. Non-SP and single-plan-type SP buckets are unaffected. Adds one DOM integration test asserting the "+2 plan types" summary and per-plan-type rows render for a mixed-SP + EC2 fan-out scenario.
cristim
added
triaged
|
@coderabbitai review |
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR renders a collapsed per-plan-type ChangesSavings Plans Per-Plan-Type Breakdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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)
Comment |
✅ 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 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`:
- Around line 3559-3574: The code currently builds byPlanType from b.recs using
r.service and counts all keys, which lets umbrella SP slugs inflate the "+N plan
types" and row rendering; change the logic that populates and later reads
byPlanType (used in the size check, summaryEl.textContent and subsequent row
rendering) to first filter out umbrella/umbrella-like slugs (e.g., detect via
the project's umbrella slug pattern or a helper like isUmbrellaSlug) so only
concrete plan-type slugs from b.recs (r.service) are added to byPlanType and
used for the size >= 2 check, the summary count, and the rows rendered. Ensure
the same filtered map is used for the later rendering block noted at 3576-3589.
🪄 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: 3713d5ec-e330-43c0-b4fe-09d2c02ef064
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
…count
UMBRELLA_SLUGS ("savings-plans", "savingsplans") represent the SP family
as a whole, not a concrete plan type. Including them in byPlanType inflated
the "+N plan types" count and caused a spurious non-concrete row to render
in the collapsible breakdown. Filter them out before populating the map so
the size check and rendered rows only reflect real plan types.
Exports UMBRELLA_SLUGS from purchase-compatibility for reuse, adds a
regression test covering the mixed umbrella + concrete slug case.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/lib/purchase-compatibility.ts (1)
112-112: ⚡ Quick winConsider making the exported Set immutable.
Since
UMBRELLA_SLUGSis exported as a constant collection, consumers could accidentally mutate it via.add()or.delete(). UseReadonlySet<string>to prevent unintended modifications.🔒 Proposed fix to make the Set read-only
-export const UMBRELLA_SLUGS = new Set<string>([SAVINGS_PLANS_BUCKET_KEY, 'savingsplans']); +export const UMBRELLA_SLUGS: ReadonlySet<string> = new Set<string>([SAVINGS_PLANS_BUCKET_KEY, 'savingsplans']);🤖 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/lib/purchase-compatibility.ts` at line 112, UMBRELLA_SLUGS is currently a mutable Set and consumers can call .add()/.delete(); change its declaration to a ReadonlySet by typing it as ReadonlySet<string> (e.g., UMBRELLA_SLUGS: ReadonlySet<string> = new Set([...])) so TypeScript prevents mutation, and ensure no code calls mutating methods on UMBRELLA_SLUGS (update any such usages to create a new Set before mutating).
🤖 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 3517-3525: The test selects the first ".fanout-bucket" whose h4
includes "Savings Plans", which is ambiguous; update the lookup so it
deterministically targets the AWS Savings Plans bucket (e.g., find the bucket
whose header contains both "Savings Plans" and "AWS" or whose bucket element has
an AWS-specific data attribute), or alternatively iterate and assert the
behavior for every SP bucket found; update the variable spSection selection
logic (the Array.from(...).find(...) call) and keep the subsequent assertion on
details.fanout-sp-plan-types the same.
---
Nitpick comments:
In `@frontend/src/lib/purchase-compatibility.ts`:
- Line 112: UMBRELLA_SLUGS is currently a mutable Set and consumers can call
.add()/.delete(); change its declaration to a ReadonlySet by typing it as
ReadonlySet<string> (e.g., UMBRELLA_SLUGS: ReadonlySet<string> = new Set([...]))
so TypeScript prevents mutation, and ensure no code calls mutating methods on
UMBRELLA_SLUGS (update any such usages to create a new Set before mutating).
🪄 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: 45f128ed-a825-4d43-922e-dbe32e411b50
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations.test.tsfrontend/src/lib/purchase-compatibility.tsfrontend/src/recommendations.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/recommendations.ts
…826) Switch find+find from .find() to .filter()+loop so the umbrella-slug exclusion regression is asserted against every SP fanout-bucket, not just the first match, preventing false-positive passes with multi-provider SP fixtures.
|
Addressed CR finding in commit 1bd1c5c (test hardening: filter+loop over all SP buckets instead of find+first-match). Finding 1 (recommendations.ts umbrella slug filter) was already addressed in 024272b. @coderabbitai review |
|
Triggering a review of the latest commits now. ✅ Actions performedReview triggered.
|
|
All CR findings addressed: umbrella slug filter in 024272b, SP bucket test hardening in 1bd1c5c. @coderabbitai review |
|
Triggering a review of the latest commits now. ✅ Actions performedReview triggered.
|
|
Fixed the remaining nitpick from CR #826 (second review pass, 2026-05-30):
All 2144 frontend tests pass; build clean. @coderabbitai review |
|
Triggering a review of the latest commits now. ✅ Actions performedReview triggered.
|
1 participant
Summary
<details>breakdown inside each mixed-SP fan-out bucket section, listing per-plan-type commitment/upfront/savings subtotalsTest plan
cd frontend && npx tsc --noEmitclean (0 errors)npx jest2143 pass, 0 fail across 65 suites (2144 total, 1 pre-existing skip)<details class="fanout-sp-plan-types">present with "+2 plan types" summary, per-plan-type rows labelled "Savings Plans (Compute)" / "Savings Plans (SageMaker)", and EC2 bucket has no breakdownfeedback_innerhtml_xss.mdapplied: all new DOM writes usetextContentfeedback_event_listener_dedup.md: no newaddEventListenercalls in this changeSummary by CodeRabbit
New Features
Tests