feat(recommendations): per-account service override pre-populated at purchase time (#111, ii+iii)#193
Merged
cristim merged 2 commits intofeat/multicloud-web-frontendfrom Apr 29, 2026
Conversation
|
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 (4)
📝 WalkthroughWalkthroughImplements Issue Changes
Sequence DiagramsequenceDiagram
participant UI as Fan-Out UI
participant Modal as FanOutModal / PurchaseModal
participant API as AccountServiceOverrides API
participant Resolver as PaymentResolver
UI->>Modal: handleBulkPurchaseClick / openPurchaseModal (async)
Modal->>Modal: group recs into buckets / collect distinct accounts
Modal->>API: listAccountServiceOverrides(account_ids...)
API-->>Modal: override lists
Modal->>Resolver: resolveBucket/Row payment (bucket, overrides, toolbar/defaults)
alt matching & supported override
Resolver-->>Modal: {payment, paymentSource: "override"}
else fallback
Resolver-->>Modal: {payment, paymentSource: "toolbar" or rec}
end
Modal->>UI: render modal with per-bucket & per-row Term/Payment dropdowns
UI->>Modal: user changes Term → rebuild payment options for row/bucket
UI->>Modal: user changes Payment → mutate live recommendation/bucket state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 4/5 reviews remaining, refill in 12 minutes. 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.
Actionable comments posted: 2
🤖 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 1668-1676: The dropdown shows a different selected value when
b.payment is unsupported because paymentOptionsFor(b.provider, b.service,
b.term) omits it; fix by ensuring the rendered select matches the seed: either
normalize b.payment to a supported value before building the DOM (update the
bucket seed where used), or if you must preserve the unsupported seed, add an
explicit disabled option into paymentSelect with option.value = b.payment and
option.selected = true (and mark it disabled) before appending supported
options; adjust any state updates to currentFanOutBuckets to reflect the
normalized value if you choose normalization.
- Around line 1528-1558: The modal is blocked waiting for all
listAccountServiceOverrides calls; instead kick off each fetch with a
per-request timeout/AbortController and do not await Promise.all before
rendering: iterate Array.from(eligibleAccountIDs) and for each id start an async
fetch that uses api.listAccountServiceOverrides(id) with an
AbortController/timer (e.g. 2–3s) and on success sets overridesByAccount.set(id,
list) and patches the openFanOutModal UI/state (so toolbar seeds render
immediately and override-derived defaults are applied when available); ensure
errors and aborts are caught and ignored so slow/hung requests don't block
initial render.
🪄 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: a5c8d928-dcba-417d-ac1c-f0842ba59882
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
Comment on lines
+1528
to
+1558
| // Pre-fetch service-overrides for every account that's the SOLE | ||
| // account in any bucket — these are the only buckets eligible for | ||
| // the override seed (multi-account buckets always fall back to | ||
| // toolbar). One fetch per distinct accountID; cached for the | ||
| // lifetime of this openFanOutModal call. Errors are swallowed: the | ||
| // toolbar-seed fallback always works, so a transient API failure | ||
| // shouldn't block the modal. | ||
| const eligibleAccountIDs = new Set<string>(); | ||
| for (const [, recs] of bucketEntries) { | ||
| if (recs.length === 0) continue; | ||
| const ids = new Set<string>(); | ||
| for (const r of recs) { | ||
| if (r.cloud_account_id) ids.add(r.cloud_account_id); | ||
| } | ||
| if (ids.size === 1) { | ||
| const only = recs[0]?.cloud_account_id; | ||
| if (only) eligibleAccountIDs.add(only); | ||
| } | ||
| } | ||
| const overridesByAccount = new Map<string, AccountServiceOverride[]>(); | ||
| await Promise.all( | ||
| Array.from(eligibleAccountIDs).map(async (id) => { | ||
| try { | ||
| const list = await api.listAccountServiceOverrides(id); | ||
| overridesByAccount.set(id, list); | ||
| } catch { | ||
| // Silent fallback to toolbar seed — a network blip shouldn't | ||
| // block the user from purchasing. | ||
| } | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Don't block the fan-out modal on override prefetch.
The modal now waits for every listAccountServiceOverrides call before rendering anything. Rejections fall back, but a slow or hung request still leaves the Purchase click looking dead and prevents the toolbar fallback from ever appearing. Add a timeout/abort per fetch, or render immediately with toolbar seeds and patch override-derived defaults in afterward.
Possible fix
+ const withTimeout = async <T>(promise: Promise<T>, ms: number): Promise<T | null> => {
+ const timeout = new Promise<null>((resolve) => window.setTimeout(() => resolve(null), ms));
+ return Promise.race([promise, timeout]);
+ };
+
await Promise.all(
Array.from(eligibleAccountIDs).map(async (id) => {
try {
- const list = await api.listAccountServiceOverrides(id);
- overridesByAccount.set(id, list);
+ const list = await withTimeout(api.listAccountServiceOverrides(id), 3000);
+ if (list) overridesByAccount.set(id, list);
} catch {
// Silent fallback to toolbar seed — a network blip shouldn't
// block the user from purchasing.
}
}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/recommendations.ts` around lines 1528 - 1558, The modal is
blocked waiting for all listAccountServiceOverrides calls; instead kick off each
fetch with a per-request timeout/AbortController and do not await Promise.all
before rendering: iterate Array.from(eligibleAccountIDs) and for each id start
an async fetch that uses api.listAccountServiceOverrides(id) with an
AbortController/timer (e.g. 2–3s) and on success sets overridesByAccount.set(id,
list) and patches the openFanOutModal UI/state (so toolbar seeds render
immediately and override-derived defaults are applied when available); ensure
errors and aborts are caught and ignored so slow/hung requests don't block
initial render.
…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.
… 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.
cristim
force-pushed
the
fix/issue-111-per-bucket-payment-seed
branch
from
ca9f57a to
ab0ce9c
Compare
cristim
changed the title
Member
Author
|
@coderabbitai review Force-pushed: rebased onto current Please re-review both commits as one PR. |
|
User feedback: Force-pushed rebase onto ✅ Actions performedFull review triggered. |
This was referenced Apr 29, 2026
cristim
added
triaged
5 tasks
cristim
added a commit
that referenced
this pull request
Apr 29, 2026
#200) * feat(recommendations): apply per-account overrides at read time (#196) PR #193 wired account_service_overrides into the purchase forms so overrides are visible at commit time. This change closes the loop on issue #196 by applying the remaining override fields at the read path: - enabled=false hides the account+service's recs from the listing - include/exclude engines, regions, types drop non-matching rows - coverage caps the dashboard "potential savings" headline per-account Filtering is post-DB inside Scheduler.ListRecommendations, mirroring the precedent set by applySuppressions; pushing array-membership joins into the WHERE clause was rejected as messier than the in-Go pass and the override table is sparse. Lookup errors are non-fatal — they log and pass the un-filtered list through (over-show vs. under-show). Coverage scaling lives in handler_dashboard.go's aggregator and assumes recs are generated at 100% coverage; the follow-up dashboard-accuracy issue referenced from #196 will revisit if rec generation becomes coverage-aware. Frontend: every account_service_overrides mutation (inline payment edit, Reset/delete, Create modal) now triggers a recommendations refetch via a tiny refreshRecommendationsAfterOverrideChange helper. The refresh is best-effort: a network blip on the secondary fetch console.warns and yields to the next nav-driven reload rather than surfacing as an error toast for a save that already succeeded. Tests: - internal/config/recommendation_overrides_test.go covers the shared resolver helper (deduping, missing-global, error propagation). - internal/scheduler/scheduler_overrides_test.go covers the per-account filter (enabled=false, include/exclude, nil-account passthrough, engine="" lax rule, lookup-error passthrough) plus the issue #196 acceptance criterion mirrored from #111. - internal/api/handler_dashboard_test.go covers the coverage cap (handler integration + table-driven scaledSavings unit tests). - frontend/src/__tests__/settings-accounts.test.ts asserts the recs refetch fires after an override save and that a refresh failure is swallowed without a user-facing error. * Update internal/api/handler_dashboard.go to treat explicit coverage=0 as a real cap. Right now Line 157 collapses an explicit zero coverage override into "full savings". That breaks the cap semantics: 0 should contribute 0, not 100%. It also contradicts the function comment's min(coverage, 100) / 100 rule, and the UI already accepts 0..100 as valid input. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * test(recommendations): address override review comments Harden the console.warn spy cleanup in the frontend override refresh test, add the missing override-lookup error pass-through regression, and align the dashboard coverage test with explicit zero-coverage semantics. * test(api): use account config key helper in dashboard tests Align the dashboard override test double with production key construction so coverage override tests cannot drift from the resolver lookup format. * fix(db): renumber recommendations engine migration Move the recommendations engine-key migration to 000043 so the branch no longer collides with the retry-linkage 000042 migration on the target branch. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Apr 29, 2026
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.
Closes #111. User-approved design: "default override pre-populated at purchase time, allowing user to select other options if desired" — applied at BOTH purchase entry points.
Summary
The recommendations engine never consumed per-account service overrides — every PUT under
/api/accounts/:id/service-overrides/...landed in the DB and disappeared from the user's perspective. This PR makes overrides visible at every purchase surface:Sub-option (ii) — multi-bucket fan-out modal. Each bucket section in
openFanOutModalnow renders its own Payment<select>, default-seeded from the per-accountAccountServiceOverridewhen all recs share onecloud_account_idand the override matches(provider, service)with a supported payment. Otherwise: toolbar default (today's behavior). Edits round-trip viagetFanOutBuckets()→app.ts::handleFanOutExecute.Sub-option (iii) — single-bucket purchase modal.
openPurchaseModalnow renders editable Term and Payment dropdowns per row. Defaults walk the precedence: account override → rec's own payment (the API stamps it at collection time) →paymentOptionsFor[0]defensive fallback. Term changes rebuild only that row's Payment options.app.ts::handleExecutePurchasereadsr.paymentper rec — replacing a historical hardcoded'all-upfront'that silently dropped the toolbar's Payment for every single-bucket purchase.Recommendations themselves stay unfiltered — overrides are advisory at the purchase surfaces, not a constraint at the listing layer.
Out of scope (deliberately, with follow-up issues filed)
enabled=false/coverage/ include-exclude lists (option B work — those override fields remain decorative)Test plan
openPurchaseModaltests updated to await the now-async functiontsc --noEmitclean across 3 consecutive passesnpm run buildclean across 3 consecutive passesgo test ./internal/api/...clean (no backend changes)Summary by CodeRabbit
Release Notes