fix(ui/purchases): preserve per-rec payment in Configure Purchase fan-out#710
Conversation
…igure Purchase preserves per-rec payment
Before this fix, recs with identical (provider, service, term) but different
per-rec payment values were bucketed together under the same key and seeded
from toolbar.payment ('all-upfront' by default), silently overriding each
rec's actual payment option. The Configure Purchase modal then submitted all
commitments as 'all-upfront' regardless of what the recommendations specified.
Two-part fix:
1. Add `payment` to the bucket key at handleBulkPurchaseClick so recs with
different payments fan into separate, payment-uniform buckets. Mirrors
the existing 'bundle B' term-key approach.
2. Update resolveBucketPaymentSeed to seed from recs[0].payment (uniform
within the bucket after fix 1) before falling back to toolbar.payment,
matching the resolvePerRecPaymentSeed precedence used by the single-bucket
path.
Closes #699
cristim
added
triaged
|
Warning Review limit reached
More reviews will be available in 15 minutes and 42 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 (2)
📝 WalkthroughWalkthroughIncludes normalized per-rec payment in bulk-purchase bucket keys and rewrites resolveBucketPaymentSeed so each bucket is seeded from the bucket's rec payment (or a supported account override); tests clear fan-out state and add a regression validating multi-payment bucketing. ChangesFix multi-bucket payment seeding for Issue
Sequence DiagramsequenceDiagram
participant User
participant handleBulkPurchase as handleBulkPurchaseClick
participant Buckets as Fan-out Buckets
participant resolveSeed as resolveBucketPaymentSeed
participant Modal as Configure Purchase Modal
User->>handleBulkPurchase: Select recs (same provider/service/term, different payments)
handleBulkPurchase->>Buckets: Create bucket key (provider,service,term,normalized payment)
Buckets->>Buckets: Split into separate buckets per payment
Buckets->>resolveSeed: For each bucket, request payment seed
resolveSeed->>resolveSeed: If uniform non-empty cloud_account_id -> lookup override
resolveSeed->>resolveSeed: If override supports (provider,service,term,payment) -> use override
resolveSeed->>resolveSeed: Else if recs[0].payment normalized & supported -> use recs[0].payment
resolveSeed->>Modal: Return payment + paymentSource for bucket
Modal->>User: Show Configure Purchase modal seeded per bucket
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
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 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 3157-3158: Normalize incoming payment synonyms before building
bucket keys and seeding: ensure the payment value (r.payment / rec.payment) is
mapped so that 'upfront' becomes 'all-upfront' (the form used by
BulkPurchasePayment/paymentOptionsFor) prior to computing bucketService/key and
any seeding logic; update the code around bucketService/key (and the analogous
block at lines ~3319-3323) to use the normalized payment variable when
constructing the key and when storing/returning bucket seeds, keeping references
to isSavingsPlanService and SAVINGS_PLANS_BUCKET_KEY unchanged.
🪄 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: da979f9e-402e-44a8-bb27-1858f7615623
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
Use normalizeBulkPayment() to map upstream payment synonyms ('upfront'
-> 'all-upfront') before building the fan-out bucket key and before
seeding resolveBucketPaymentSeed. Without this, two recs that share the
same effective payment but differ only in the synonym form ('upfront' vs
'all-upfront') were keyed into separate buckets, defeating the #699 fix.
Closes: CR Major finding on PR #710 (coderabbitai comment 3299769088).
CR Major finding resolved (commit 8e21a1b)Finding: Fix: Added
Test added: test Note: Full suite: 1933 passed, 1 skipped, 0 failed. TypeScript: no errors. @coderabbitai review |
|
@coderabbitai review |
|
Triggering a review of the updated commits now. ✅ Actions performedReview triggered.
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/recommendations.ts (1)
3324-3335:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize override payments before seeding the bucket.
match.paymentstill skipsnormalizeBulkPayment(). If an account override carries Azure/GCP's'upfront'synonym, this branch seedsFanOutBucket.paymentwith a value the fan-out dropdown never renders, so the modal can displayall-upfrontwhile the backing bucket state/post body still holdsupfront.Suggested fix
- if ( - match - && match.payment - // Defensive: only honour the override when the (provider, service, - // term, payment) combo is actually supported. A stale or hand-saved - // override pointing at an unsupported payment for this term shouldn't - // poison the dropdown — fall through to rec.payment seed. - && isPaymentSupported(provider, r0.service, term, match.payment as CompatPayment) - ) { + const overridePayment = normalizeBulkPayment(match?.payment); + if ( + overridePayment + // Defensive: only honour the override when the (provider, service, + // term, payment) combo is actually supported. A stale or hand-saved + // override pointing at an unsupported payment for this term shouldn't + // poison the dropdown — fall through to rec.payment seed. + && isPaymentSupported(provider, r0.service, term, overridePayment) + ) { return { - payment: match.payment as BulkPurchaseToolbarState['payment'], + payment: overridePayment, source: 'override', }; }🤖 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/recommendations.ts` around lines 3324 - 3335, The override branch currently returns match.payment directly without running it through normalizeBulkPayment, allowing synonyms like 'upfront' to persist; update the branch so you first compute const normalized = normalizeBulkPayment(match.payment) (or similar), use normalized when calling isPaymentSupported(provider, r0.service, term, normalized) and return payment: normalized (as BulkPurchaseToolbarState['payment']) and source: 'override' so the FanOutBucket.payment is seeded with the normalized value the UI expects.
🤖 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.
Outside diff comments:
In `@frontend/src/recommendations.ts`:
- Around line 3324-3335: The override branch currently returns match.payment
directly without running it through normalizeBulkPayment, allowing synonyms like
'upfront' to persist; update the branch so you first compute const normalized =
normalizeBulkPayment(match.payment) (or similar), use normalized when calling
isPaymentSupported(provider, r0.service, term, normalized) and return payment:
normalized (as BulkPurchaseToolbarState['payment']) and source: 'override' so
the FanOutBucket.payment is seeded with the normalized value the UI expects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 710f2ace-b70b-4bd7-8f1b-d4810149cd42
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
In resolveBucketPaymentSeed, match.payment was passed raw to isPaymentSupported and returned directly. An account override carrying Azure's 'upfront' synonym would seed FanOutBucket.payment = 'upfront', a value the fan-out dropdown never renders (it only lists canonical BulkPurchasePayment values), while the backing state still held 'upfront'. Fix: compute overridePayment = normalizeBulkPayment(match?.payment) first, then use overridePayment in both the isPaymentSupported guard and the return value. Synonyms like 'upfront' now normalize to 'all-upfront' before reaching the bucket, keeping the displayed value and submitted value in sync. Add test (f): azure/vm override carrying 'upfront' produces two fan-out buckets each seeded with payment='all-upfront' and source='override'. Rename previous test (f) to (g) to preserve ordering. Resolves CR outside-diff finding on PR #710.
|
CR outside-diff finding resolved (commit 40f2826) Finding: Fix ( Test added ( Counts: 1935 tests pass (1 pre-existing skip), @coderabbitai review |
|
The new test (f) covering the azure/override → Triggering a review of the updated commits now. ✅ Actions performedReview triggered.
|
1 participant
Closes #699. Multi-bucket Configure Purchase modal was seeding all selected recs with the toolbar default payment (
all-upfront) regardless of each rec's actual payment option, silently mis-billing on bulk purchases.Changes
frontend/src/recommendations.ts:3155) — added|${r.payment ?? ''}so recs with different payments separate into payment-uniform buckets, mirroring the existing term-based fan-out.resolveBucketPaymentSeed— after the override lookup (account override still takes priority), falls through torecs[0].paymentwhen it is supported for the(provider, service, term)cell rather than unconditionally seeding fromtoolbar.payment. Multi-account buckets, missing-payment, or unsupported-payment values still fall back to toolbar.Tests
(e)inIssue #111: per-bucket Payment seed...: two AWS EC2 1yr recs withpartial-upfront+no-upfrontnow produce 2 buckets with payments['no-upfront', 'partial-upfront']instead of one collapsed bucket seeded toall-upfront. Test failed pre-fix, passes post-fix.Summary by CodeRabbit
Bug Fixes
Tests