feat(recommendations): re-enable bulk-buy across SP plan types (#132)#180
Open
cristim wants to merge 2 commits intofeat/multicloud-web-frontendfrom
Open
feat(recommendations): re-enable bulk-buy across SP plan types (#132)#180cristim wants to merge 2 commits intofeat/multicloud-web-frontendfrom
cristim wants to merge 2 commits intofeat/multicloud-web-frontendfrom
Conversation
📝 WalkthroughWalkthroughAdds utilities to canonicalize and label Savings Plans, collapses Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (Bulk-buy Modal)
participant Rec as Recommendations Module
participant Utils as purchase-compatibility.ts
participant Backend as Execution Payload
UI->>Rec: Request bulk-buy buckets for selected recs
Rec->>Utils: Identify savings-plan recs & compute bucket keys (collapse savings-plans-*)
Utils-->>Rec: Return buckets with `SAVINGS_PLANS_BUCKET_KEY`
Rec->>Utils: Request bucket label & per-rec compatibility
Utils-->>Rec: Return `savingsPlansBucketLabel(...)` and compatibility results
Rec->>UI: Render modal with bucket headers and compatibility states
UI->>Backend: Submit bulk-buy payload (per-rec original `service` preserved)
Backend-->>UI: Confirmation / errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 0/5 reviews remaining, refill in 56 minutes and 29 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.
🧹 Nitpick comments (2)
frontend/src/recommendations.ts (1)
1415-1419: Extract a shared bucket-compatibility helper to avoid logic drift.Compatibility is computed twice (Line 1415 and Line 1571) with slightly different inputs. Centralizing this into one helper keeps behavior consistent as rules evolve.
♻️ Proposed refactor
+function isBucketPaymentCompatible( + recs: readonly LocalRecommendation[], + payment: BulkPurchaseToolbarState['payment'], +): boolean { + return recs.every((r) => + isPaymentSupported(r.provider as CompatProvider, r.service, r.term as 1 | 3, payment), + ); +} + const incompatible = bucketEntries.filter(([_key, recs]) => { - return recs.some( - (r) => !isPaymentSupported(r.provider as CompatProvider, r.service, r.term as 1 | 3, tb.payment), - ); + return !isBucketPaymentCompatible(recs, tb.payment); }); ... - const compat = b.recs.every((r) => - isPaymentSupported(b.provider, r.service, b.term, b.payment), - ); + const compat = isBucketPaymentCompatible(b.recs, b.payment);Also applies to: 1571-1573
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/recommendations.ts` around lines 1415 - 1419, Extract the repeated compatibility check into a single helper (e.g., isBucketIncompatible or isBucketCompatible) that encapsulates the logic currently used in the inline filter: iterate a bucket's recs and call isPaymentSupported(r.provider as CompatProvider, r.service, r.term as 1 | 3, tb.payment); then replace both inline usages (the bucketEntries.filter(...) that creates incompatible and the similar check around the other occurrence) to call this helper, keeping the same parameter types (pass the bucket entry's recs and tb.payment) so behavior and typing remain identical and avoid logic drift.frontend/src/lib/purchase-compatibility.ts (1)
103-113: Normalize canonicalsavings-plansin label composition.If
serviceSlugsincludessavings-plans, Line 109 currently rendersSavings Plans (savings-plans). Treat the canonical key as non-plan-type input so UI labels stay clean.♻️ Proposed tweak
export function savingsPlansBucketLabel(serviceSlugs: readonly string[]): string { const seen = new Set<string>(); const parts: string[] = []; for (const slug of serviceSlugs) { - if (!isSavingsPlanService(slug) || seen.has(slug)) continue; + if (!isSavingsPlanService(slug) || seen.has(slug)) continue; + if (slug === SAVINGS_PLANS_BUCKET_KEY) continue; seen.add(slug); parts.push(SP_SHORT_LABEL[slug] ?? slug); } if (parts.length === 0) return 'Savings Plans'; return `Savings Plans (${parts.join(' + ')})`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/purchase-compatibility.ts` around lines 103 - 113, The label builder savingsPlansBucketLabel currently includes the canonical key "savings-plans" in the composed parenthetical; update its loop to treat the canonical "savings-plans" as non-plan-type input by skipping that slug (in addition to the existing isSavingsPlanService and seen checks) so it does not appear as "savings-plans" in the UI; adjust the for-loop conditional (referencing savingsPlansBucketLabel, serviceSlugs, isSavingsPlanService, and SP_SHORT_LABEL) to ignore slug === 'savings-plans' before adding to parts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/lib/purchase-compatibility.ts`:
- Around line 103-113: The label builder savingsPlansBucketLabel currently
includes the canonical key "savings-plans" in the composed parenthetical; update
its loop to treat the canonical "savings-plans" as non-plan-type input by
skipping that slug (in addition to the existing isSavingsPlanService and seen
checks) so it does not appear as "savings-plans" in the UI; adjust the for-loop
conditional (referencing savingsPlansBucketLabel, serviceSlugs,
isSavingsPlanService, and SP_SHORT_LABEL) to ignore slug === 'savings-plans'
before adding to parts.
In `@frontend/src/recommendations.ts`:
- Around line 1415-1419: Extract the repeated compatibility check into a single
helper (e.g., isBucketIncompatible or isBucketCompatible) that encapsulates the
logic currently used in the inline filter: iterate a bucket's recs and call
isPaymentSupported(r.provider as CompatProvider, r.service, r.term as 1 | 3,
tb.payment); then replace both inline usages (the bucketEntries.filter(...) that
creates incompatible and the similar check around the other occurrence) to call
this helper, keeping the same parameter types (pass the bucket entry's recs and
tb.payment) so behavior and typing remain identical and avoid logic drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14a36b1f-282f-4db6-bca7-d73a1593b9a7
📒 Files selected for processing (4)
frontend/src/__tests__/purchase-compatibility.test.tsfrontend/src/__tests__/recommendations.test.tsfrontend/src/lib/purchase-compatibility.tsfrontend/src/recommendations.ts
3 tasks
cristim
added a commit
that referenced
this pull request
Apr 28, 2026
…nonical SP key in label Addresses CodeRabbit nitpicks on PR #180: 1. Extract `isBucketPaymentCompatible(recs, payment)` helper from the two existing inline `recs.every(...)` / `recs.some(...)` checks at `handleBulkPurchaseClick` and `renderFanOutBucketSection`. One source of truth for "all recs in this bucket support the chosen payment" so future rule changes can't cause drift between the two sites. Lifts the inline `BulkPurchaseToolbarState['payment']` union type into a top-level `BulkPurchasePayment` alias the helper signature can name. 2. `savingsPlansBucketLabel` now also skips `SAVINGS_PLANS_BUCKET_KEY` itself if it appears in the input — defensively, in case a caller passes the canonical bucket key as one of the slugs, so the rendered label doesn't end up with "(savings-plans)" inside the parens. Test added pinning the canonical-key skip behavior. All 1370 frontend tests pass. No behavior change for non-pathological inputs.
Member
Author
|
@coderabbitai review Addressed both nitpicks:
|
|
Triggering a fresh review now. ✅ Actions performedReview triggered.
|
cristim
added
triaged
Member
Author
|
CR loop closed: original review's 2 nitpicks were addressed in commit Note: PR #181 (issue #137) introduces the same |
7 tasks
cristim
added a commit
that referenced
this pull request
Apr 29, 2026
…ce override (closes #111) Implements sub-option (ii) of issue #111 — the user-approved "default override pre-populated at purchase time, allowing user to select other options if desired" design. # What changes Each section of the bulk-purchase fan-out modal now renders its own Payment <select> dropdown. The default-selected option per bucket is seeded by `resolveBucketPaymentSeed`, which walks the precedence: 1. **Override**: every rec in the bucket carries the same non-empty `cloud_account_id` AND that account has a saved AccountServiceOverride matching `(provider, recs[0].service)` AND the override's `payment` is supported by `(provider, service, term)` → seed from `override.payment`. The bucket section renders a small "(from account override)" note next to the dropdown. 2. **Toolbar**: any other case (multi-account bucket, no override, override has no payment, override payment unsupported) → seed from `toolbar.payment` (the bulk-toolbar value). This is the pre-#111 behaviour; existing UI flows are preserved. The dropdown's options come from `paymentOptionsFor(provider, service, term)`, which already filters to the supported set. On `change`, the handler updates `currentFanOutBuckets[idx].payment` and rewrites the bucket's compatibility status line. `getFanOutBuckets()` returns the per-bucket-edited values; `app.ts::handleFanOutExecute` already POSTs each bucket with its own `b.payment`, so the user's edits round-trip without an API contract change. # Implementation notes - `openFanOutModal` is now `async`. It pre-fetches `listAccountServiceOverrides(accountID)` once per distinct single-account-bucket account ID, in parallel via `Promise.all`, and caches the responses in a per-call `Map`. Errors are swallowed — toolbar fallback always works, so a transient API blip shouldn't block a purchase. - The override lookup matches on `recs[0].service` (the per-rec service slug), NOT `bucket.service`. This future-proofs against the post-#132 SP-canonical-bucket-key landing in PR #180 — when `bucket.service` becomes the canonical `'savings-plans'` for a mixed-plan-type SP bucket, the saved override is still keyed on the per-plan-type slug (`savings-plans-compute`, etc.), so the lookup stays correct under either bucket-key encoding. - `FanOutBucket` gains `paymentSource: 'override' | 'toolbar'` so the modal can render the source note honestly without re-deriving it. - The call site in `handleBulkPurchaseClick` becomes `void openFanOutModal(...)` since the function is async — the returned promise is fire-and-forget, the modal is the user's surface. # Out of scope (deliberately) - **Recommendations stay unfiltered.** Overrides are advisory at this one surface only; the rec list is unchanged. - **No save-side enforcement.** The executePurchase handler still accepts whatever payment the user picks. Overrides are a UI affordance, not a constraint. - **Multi-account buckets fall back to toolbar.** When recs in one bucket span 2+ accounts, no single account's override applies cleanly. Documented as a TODO(#111-followup) in `resolveBucketPaymentSeed` — would need either per-rec dropdowns inside the bucket or a "split this bucket by account" UX, both bigger than this PR's scope. # Tests 4 new tests in `frontend/src/__tests__/recommendations.test.ts` inside the `'Issue #111: per-bucket Payment seed …'` describe block: (a) Single-account bucket with matching override → bucket payment seeded from override; dropdown selected; source note rendered. (b) Single-account bucket with NO matching override (override exists but for a different service) → bucket payment from toolbar; no source note. (c) Multi-account bucket → toolbar regardless of overrides; the sibling single-account bucket in the same fan-out still honours its override. (d) User-edited dropdown change → `getFanOutBuckets()` reflects the new value (module state updated). All 1365 frontend tests pass (1361 baseline + 4 new). Three clean verification passes (jest + tsc + webpack build) before committing. Go smoke test on `internal/api/...` clean.
cristim
added a commit
that referenced
this pull request
Apr 29, 2026
…purchase time (#111, ii+iii) (#193) * feat(recommendations): per-bucket payment seed from per-account service override (closes #111) Implements sub-option (ii) of issue #111 — the user-approved "default override pre-populated at purchase time, allowing user to select other options if desired" design. # What changes Each section of the bulk-purchase fan-out modal now renders its own Payment <select> dropdown. The default-selected option per bucket is seeded by `resolveBucketPaymentSeed`, which walks the precedence: 1. **Override**: every rec in the bucket carries the same non-empty `cloud_account_id` AND that account has a saved AccountServiceOverride matching `(provider, recs[0].service)` AND the override's `payment` is supported by `(provider, service, term)` → seed from `override.payment`. The bucket section renders a small "(from account override)" note next to the dropdown. 2. **Toolbar**: any other case (multi-account bucket, no override, override has no payment, override payment unsupported) → seed from `toolbar.payment` (the bulk-toolbar value). This is the pre-#111 behaviour; existing UI flows are preserved. The dropdown's options come from `paymentOptionsFor(provider, service, term)`, which already filters to the supported set. On `change`, the handler updates `currentFanOutBuckets[idx].payment` and rewrites the bucket's compatibility status line. `getFanOutBuckets()` returns the per-bucket-edited values; `app.ts::handleFanOutExecute` already POSTs each bucket with its own `b.payment`, so the user's edits round-trip without an API contract change. # Implementation notes - `openFanOutModal` is now `async`. It pre-fetches `listAccountServiceOverrides(accountID)` once per distinct single-account-bucket account ID, in parallel via `Promise.all`, and caches the responses in a per-call `Map`. Errors are swallowed — toolbar fallback always works, so a transient API blip shouldn't block a purchase. - The override lookup matches on `recs[0].service` (the per-rec service slug), NOT `bucket.service`. This future-proofs against the post-#132 SP-canonical-bucket-key landing in PR #180 — when `bucket.service` becomes the canonical `'savings-plans'` for a mixed-plan-type SP bucket, the saved override is still keyed on the per-plan-type slug (`savings-plans-compute`, etc.), so the lookup stays correct under either bucket-key encoding. - `FanOutBucket` gains `paymentSource: 'override' | 'toolbar'` so the modal can render the source note honestly without re-deriving it. - The call site in `handleBulkPurchaseClick` becomes `void openFanOutModal(...)` since the function is async — the returned promise is fire-and-forget, the modal is the user's surface. # Out of scope (deliberately) - **Recommendations stay unfiltered.** Overrides are advisory at this one surface only; the rec list is unchanged. - **No save-side enforcement.** The executePurchase handler still accepts whatever payment the user picks. Overrides are a UI affordance, not a constraint. - **Multi-account buckets fall back to toolbar.** When recs in one bucket span 2+ accounts, no single account's override applies cleanly. Documented as a TODO(#111-followup) in `resolveBucketPaymentSeed` — would need either per-rec dropdowns inside the bucket or a "split this bucket by account" UX, both bigger than this PR's scope. # Tests 4 new tests in `frontend/src/__tests__/recommendations.test.ts` inside the `'Issue #111: per-bucket Payment seed …'` describe block: (a) Single-account bucket with matching override → bucket payment seeded from override; dropdown selected; source note rendered. (b) Single-account bucket with NO matching override (override exists but for a different service) → bucket payment from toolbar; no source note. (c) Multi-account bucket → toolbar regardless of overrides; the sibling single-account bucket in the same fan-out still honours its override. (d) User-edited dropdown change → `getFanOutBuckets()` reflects the new value (module state updated). All 1365 frontend tests pass (1361 baseline + 4 new). Three clean verification passes (jest + tsc + webpack build) before committing. Go smoke test on `internal/api/...` clean. * feat(recommendations): editable per-row Term/Payment in single-bucket purchase modal (issue #111 sub-option iii) Implements the per-row counterpart of issue #111's user-approved design ("default override pre-populated at purchase time, allowing user to select other options if desired"). Sub-option (ii) — per-bucket Payment in the fan-out modal — landed in the prior commit. This commit handles the OTHER purchase entry point: the single-bucket `openPurchaseModal`, which opens when the bulk-purchase selection collapses to one (provider, service, term) bucket. # What changes - `openPurchaseModal` now renders editable Term and Payment dropdowns per row. Defaults walk the precedence: 1. Override: `rec.cloud_account_id` has a saved `AccountServiceOverride` matching `(provider, service)` whose `payment` is supported by `(provider, service, term)` → seed from override; row's source-note span renders "(from account override)". 2. Rec's own payment (the API stamps it at collection time): seed from `rec.payment` if non-empty AND supported. 3. Defensive fallback: `paymentOptionsFor(provider, service, term)[0]`. Reachable only from malformed test fixtures or pre-#111 cached responses where the rec lacks a payment. - Edits mutate `currentPurchaseRecommendations[idx]` in place; `getPurchaseModalRecommendations()` returns the user's choices. `app.ts::handleExecutePurchase` now reads `r.payment` per rec (with a defensive `?? 'all-upfront'` for direct test-harness callers that bypass the modal). This replaces the historical hardcoded `'all-upfront'` on the single-bucket path that silently dropped the toolbar's Payment for every single-bucket purchase since the bulk-purchase toolbar shipped — a pre-existing bug surfaced and fixed by this commit. - Term changes (1yr ↔ 3yr) rebuild only that row's Payment `<select>` options; if the prior Payment is no longer supported for the new term, the first valid option wins and live state is mirrored. The modal does NOT re-render mid-edit so other rows' in-progress edits are preserved. # Implementation notes - `openPurchaseModal` is now `async`. It pre-fetches `listAccountServiceOverrides(id)` once per distinct non-empty `cloud_account_id` in the input set, in parallel via `Promise.all`, and caches the responses in a per-call `Map`. Errors are swallowed — the rec-payment / paymentOptionsFor[0] fallback always works, so a transient API blip shouldn't block a purchase. Same pattern as `openFanOutModal` (ii). - The DOM build switches from a template-literal `innerHTML` rewrite to `createElement` construction so the per-row controls can carry live event listeners. All cell text is `textContent` assignment — no HTML interpolation, no XSS surface. - `LocalRecommendation` gains an optional `payment?: string` field in `frontend/src/types.ts`. The runtime data already carries it (the API `Recommendation` defines `payment: string`), so this is a type-additive change. No mapping changes required. - The override-fetch+cache pattern is duplicated across `openFanOutModal` (ii) and `openPurchaseModal` (iii). Documented in code; follow-up issue will consolidate them into a shared `frontend/src/lib/overrides.ts` helper once both surfaces have shipped (avoiding scope creep on this PR). # Out of scope (deliberately, with follow-up issues) - **Recommendations stay unfiltered.** The recommendations page itself is unchanged — overrides are applied at purchase time, not at listing time. - **`enabled=false`, `coverage`, and include/exclude lists are still decorative.** Only `payment` is consumed (at purchase time, by this PR + ii). For those other override fields to do anything, `ListStoredRecommendations` / `RecommendationFilter` need to become account-aware (option B). Filed as a follow-up. - **Multi-account fan-out buckets still fall back to toolbar.** Same scope discipline as (ii). Filed as a follow-up. # Tests 5 new tests in `frontend/src/__tests__/recommendations.test.ts` inside the `'Issue #111 (iii): per-row Payment seed in openPurchaseModal'` describe block: (a) Single rec, override matches and has supported payment → live state + select value + source-note all reflect override. (b) Single rec, no matching override → seed from rec.payment; no source-note. (c) Single rec, override has unsupported payment for the `(provider, service, term)` cell (AWS RDS 3yr no-upfront, blocked by `cmd/validators.go:warnRDS3YearNoUpfront`) → override ignored; rec.payment wins. (d) User changes Term 1→3 → rec.term updates; row's Payment options rebuilt to the 3yr-supported set; live state consistent with the dropdown. (e) User changes Payment dropdown → live state reflects the new value (which `app.ts::handleExecutePurchase` reads verbatim). Existing `openPurchaseModal` tests (4) updated to `await` the now- async function and read `.textContent` instead of `.innerHTML` (content unchanged, just the rendering mode). All 1371 frontend tests pass (1366 baseline including (ii)'s 4 tests + 5 new). Three clean verification passes (jest + tsc + webpack build) before commit. Go smoke test on `internal/api/...` clean. Closes #111 in combination with the prior (ii) commit.
Closes #132. PR #123 split the single 'savings-plans' service into four per-plan- type slugs (savings-plans-{compute,ec2instance,sagemaker,database}). The bulk-buy modal in `frontend/src/recommendations.ts` keys buckets by `(provider, service, term)`, so an operator with recs across all four plan types now had to bulk-buy four separate times — four buckets, four approval emails, four executePurchase POSTs, four clicks in the inbox. This restores the pre-split one-click behaviour while preserving the per-plan-type fidelity the rest of the stack expects: 1. **Bucket key collapses SP slugs** in the bucket Map (lines ~1383): if `isSavingsPlanService(rec.service)` is true, the bucket key uses the canonical `'savings-plans'` slug. Compute and SageMaker SPs at term=1 now share one bucket. Different terms still split (the bucket key still includes term). 2. **Per-rec service is preserved** on `recs[].service`. The FanOutBucket carries the canonical bucket key as `service` (so the modal can render "Savings Plans (Compute + SageMaker)"), but each rec round-trips its real per-plan-type slug into the executePurchase POST body. The backend handler loops per rec and uses `rec.service` for suppressions, audit records, and the email summary, so a mixed-SP POST is functionally identical to four separate POSTs except that there's only one approval token / email. 3. **Bucket section title shows mixed plan types**. The modal renders "AWS / Savings Plans (Compute + SageMaker) — N commitments" rather than "AWS / savings-plans-compute — N". `savingsPlansBucketLabel` in `frontend/src/lib/purchase-compatibility.ts` deduplicates and formats the per-plan-type short labels in input order. Single-plan buckets render as "Savings Plans (Compute)", non-SP buckets keep their raw service slug. 4. **Per-rec compatibility check** in mixed-SP buckets: bucket-level compatibility now `every`-checks every rec's service rather than only `recs[0].service`. SP plan types share AWS's payment rules today (no SP variant rejects no-upfront the way RDS 3yr does), so this is a defensive belt-and-suspenders against a future provider- side asymmetry. New shared utilities live in `frontend/src/lib/purchase-compatibility.ts`: - `isSavingsPlanService(service)` — `startsWith('savings-plans')`, mirror of Go's `common.IsSavingsPlan` (pkg/common/types.go) so a future plan type added on the backend is picked up automatically. - `SAVINGS_PLANS_BUCKET_KEY = 'savings-plans'` — canonical bucket slug. - `savingsPlansBucketLabel(slugs)` — formats the mixed-plan-type label. Tests: - 9 new tests in `purchase-compatibility.test.ts` pinning the predicate and label utility (single, mixed, dedup, all-four, fallback, SP+non-SP filtering). - 4 new tests in `recommendations.test.ts` driving the actual bulk-buy flow: (a) two SPs at term=1 collapse into one bucket and hit the single-bucket happy path with both recs preserved, (b) three SPs + one EC2 produce 1 SP bucket + 1 EC2 bucket, (c) different SP terms still split by term, (d) the mixed-SP fan-out modal renders the combined plan-type label. - All 1369 existing frontend tests still pass. Backend untouched: per-card save path, executePurchase handler, suppression machinery and email summary all consume `rec.service` unchanged. The feat/multicloud-web-frontend deployment will see one approval email per bucket instead of one per plan type — the auditor-visible details (per-rec service, per-rec count, per-rec savings) round-trip identically.
…nonical SP key in label Addresses CodeRabbit nitpicks on PR #180: 1. Extract `isBucketPaymentCompatible(recs, payment)` helper from the two existing inline `recs.every(...)` / `recs.some(...)` checks at `handleBulkPurchaseClick` and `renderFanOutBucketSection`. One source of truth for "all recs in this bucket support the chosen payment" so future rule changes can't cause drift between the two sites. Lifts the inline `BulkPurchaseToolbarState['payment']` union type into a top-level `BulkPurchasePayment` alias the helper signature can name. 2. `savingsPlansBucketLabel` now also skips `SAVINGS_PLANS_BUCKET_KEY` itself if it appears in the input — defensively, in case a caller passes the canonical bucket key as one of the slugs, so the rendered label doesn't end up with "(savings-plans)" inside the parens. Test added pinning the canonical-key skip behavior. All 1370 frontend tests pass. No behavior change for non-pathological inputs.
cristim
force-pushed
the
fix/issue-132-bulk-buy-sp
branch
from
cbcfba6 to
09463ef
Compare
Member
Author
|
Rebased onto current Conflicts resolved (
Tests: @coderabbitai review |
|
Triggering a fresh review on the rebased branch now. ✅ Actions performedReview triggered.
|
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/recommendations.ts`:
- Around line 1134-1147: BulkPurchasePayment can end up with the legacy alias
"upfront" via casts which causes UI/desync; add a normalizeBulkPayment(value:
string): BulkPurchasePayment helper that maps "upfront" → "all-upfront" and
validates/returns the other allowed values
('all-upfront','partial-upfront','no-upfront','monthly'), and call
normalizeBulkPayment wherever override/seed values are assigned into
BulkPurchasePayment-typed fields (e.g., in the code paths that seed bucket state
and before calling isBucketPaymentCompatible or storing payment state) so all
bucket payment state and checks (BulkPurchasePayment, isBucketPaymentCompatible)
always use normalized values.
🪄 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: 28099688-107e-45d6-9005-a302b9eda447
📒 Files selected for processing (4)
frontend/src/__tests__/purchase-compatibility.test.tsfrontend/src/__tests__/recommendations.test.tsfrontend/src/lib/purchase-compatibility.tsfrontend/src/recommendations.ts
Comment on lines
+1134
to
+1147
| type BulkPurchasePayment = 'all-upfront' | 'partial-upfront' | 'no-upfront' | 'monthly'; | ||
|
|
||
| // Centralized bucket-level payment compatibility check. A bucket is | ||
| // compatible iff EVERY rec in it has a supported (provider, service, | ||
| // term, payment) combination. Used by the bulk-buy fan-out path to | ||
| // flag buckets the user has built but won't be allowed to submit. | ||
| function isBucketPaymentCompatible( | ||
| recs: readonly LocalRecommendation[], | ||
| payment: BulkPurchasePayment, | ||
| ): boolean { | ||
| return recs.every((r) => | ||
| isPaymentSupported(r.provider as CompatProvider, r.service, r.term as 1 | 3, payment), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Normalize 'upfront' before storing bulk bucket payment state.
BulkPurchasePayment excludes 'upfront', but override-seeded values can still enter this state via casts, which can desync displayed dropdown options vs bucket/payment state. Normalize alias values (upfront → all-upfront) at seed boundaries.
Suggested direction
type BulkPurchasePayment = 'all-upfront' | 'partial-upfront' | 'no-upfront' | 'monthly';
+
+function normalizeBulkPayment(payment: CompatPayment | undefined): BulkPurchasePayment {
+ if (payment === 'upfront') return 'all-upfront';
+ if (payment === 'partial-upfront' || payment === 'no-upfront' || payment === 'monthly') return payment;
+ return 'all-upfront';
+}Then use normalizeBulkPayment(...) where override/seed values are assigned into BulkPurchasePayment fields.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type BulkPurchasePayment = 'all-upfront' | 'partial-upfront' | 'no-upfront' | 'monthly'; | |
| // Centralized bucket-level payment compatibility check. A bucket is | |
| // compatible iff EVERY rec in it has a supported (provider, service, | |
| // term, payment) combination. Used by the bulk-buy fan-out path to | |
| // flag buckets the user has built but won't be allowed to submit. | |
| function isBucketPaymentCompatible( | |
| recs: readonly LocalRecommendation[], | |
| payment: BulkPurchasePayment, | |
| ): boolean { | |
| return recs.every((r) => | |
| isPaymentSupported(r.provider as CompatProvider, r.service, r.term as 1 | 3, payment), | |
| ); | |
| } | |
| type BulkPurchasePayment = 'all-upfront' | 'partial-upfront' | 'no-upfront' | 'monthly'; | |
| function normalizeBulkPayment(payment: CompatPayment | undefined): BulkPurchasePayment { | |
| if (payment === 'upfront') return 'all-upfront'; | |
| if (payment === 'partial-upfront' || payment === 'no-upfront' || payment === 'monthly') return payment; | |
| return 'all-upfront'; | |
| } | |
| // Centralized bucket-level payment compatibility check. A bucket is | |
| // compatible iff EVERY rec in it has a supported (provider, service, | |
| // term, payment) combination. Used by the bulk-buy fan-out path to | |
| // flag buckets the user has built but won't be allowed to submit. | |
| function isBucketPaymentCompatible( | |
| recs: readonly LocalRecommendation[], | |
| payment: BulkPurchasePayment, | |
| ): boolean { | |
| return recs.every((r) => | |
| isPaymentSupported(r.provider as CompatProvider, r.service, r.term as 1 | 3, payment), | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/recommendations.ts` around lines 1134 - 1147,
BulkPurchasePayment can end up with the legacy alias "upfront" via casts which
causes UI/desync; add a normalizeBulkPayment(value: string): BulkPurchasePayment
helper that maps "upfront" → "all-upfront" and validates/returns the other
allowed values ('all-upfront','partial-upfront','no-upfront','monthly'), and
call normalizeBulkPayment wherever override/seed values are assigned into
BulkPurchasePayment-typed fields (e.g., in the code paths that seed bucket state
and before calling isBucketPaymentCompatible or storing payment state) so all
bucket payment state and checks (BulkPurchasePayment, isBucketPaymentCompatible)
always use normalized values.
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.
Summary
Closes #132. Restores the pre-PR-#123 one-click bulk-buy experience for AWS Savings Plans by collapsing all four per-plan-type slugs (
savings-plans-{compute,ec2instance,sagemaker,database}) into a single bulk-buy bucket per(provider, term)pair.What changed
frontend/src/recommendations.ts: any rec withisSavingsPlanService(rec.service) === trueuses the canonical'savings-plans'slug in the bucket key instead of its per-plan-type slug. Compute + SageMaker SPs at term=1 now share one bucket. Different terms still split.recs[].servicekeeps the real per-plan-type slug. The backendexecutePurchasehandler iterates per rec and usesrec.servicefor suppressions, audit records and the email summary, so a mixed-SP POST is functionally identical to four separate POSTs except that there's only one approval token / one approval email / one click in the inbox.renderFanOutBucketSectionshows"AWS / Savings Plans (Compute + SageMaker) — N commitments"instead of"AWS / savings-plans-compute — N". Single-plan buckets render as"Savings Plans (Compute)". Non-SP buckets keep their raw service slug.every-checks every rec's service, defending against a future provider-side asymmetry between SP plan types (today they share AWS's payment rules — only RDS 3yr rejects no-upfront).New utilities (
frontend/src/lib/purchase-compatibility.ts)isSavingsPlanService(service)—startsWith('savings-plans'), mirror of Go'scommon.IsSavingsPlan(pkg/common/types.go).startsWithrather than a hardcoded set so a future plan type added on the backend is picked up automatically.SAVINGS_PLANS_BUCKET_KEY = 'savings-plans'— canonical bucket slug.savingsPlansBucketLabel(slugs)— formats the mixed-plan-type label, dedupes and preserves input order.Tests
purchase-compatibility.test.tspinning the predicate and label utility.recommendations.test.tsdriving the actual bulk-buy flow: (a) two SPs at term=1 collapse into one bucket and hit the single-bucket happy path with both recs preserved, (b) three SPs + one EC2 produce 1 SP bucket + 1 EC2 bucket, (c) different SP terms still split by term, (d) the mixed-SP fan-out modal renders the combined plan-type label.Backend untouched
Per-card save path,
executePurchasehandler, suppression machinery and email summary all consumerec.serviceunchanged.Test plan
🤖 Generated with claude-flow
Summary by CodeRabbit
Tests
Improvements