feat(recommendations): apply per-account overrides at read time (#196)#200
Conversation
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.
|
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)
📝 WalkthroughWalkthroughBackend and frontend changes wire per-account service overrides into the recommendations read path and dashboard aggregation. Backend resolves per-account configs, filters recommendations at list-time, and scales dashboard savings by coverage; frontend refreshes recommendations after override edits, handling refresh failures as best-effort. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Scheduler
participant ConfigStore
participant FilterLogic
Client->>Scheduler: ListRecommendations(ctx, filter)
Scheduler->>Scheduler: Read cached recommendations
Scheduler->>Scheduler: Apply suppressions
Scheduler->>ConfigStore: ResolveAccountConfigsForRecs(recs)
ConfigStore->>ConfigStore: Deduplicate (account,provider,service) keys
ConfigStore->>ConfigStore: Fetch global ServiceConfig (cached)
ConfigStore->>ConfigStore: Fetch per-account overrides
ConfigStore-->>Scheduler: Resolved config map (or error)
Scheduler->>FilterLogic: applyAccountOverrides(recs, configs)
FilterLogic->>FilterLogic: Filter by Enabled, engines, regions, types
FilterLogic-->>Scheduler: Filtered recs
Scheduler-->>Client: Return recommendations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
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)
Review rate limit: 0/5 reviews remaining, refill in 2 minutes and 53 seconds. Comment |
cristim
added
type/feat
|
@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
🧹 Nitpick comments (2)
frontend/src/__tests__/settings-accounts.test.ts (1)
617-633: Restoreconsole.warneven when the test fails.If an assertion throws before Line 633, the spy leaks into later tests and makes failures noisier to debug.
Suggested cleanup
- const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - - const panel = await openOverridesPanel('acc-1'); - const select = panel.querySelector('select.override-payment-select') as HTMLSelectElement; - select.value = 'all-upfront'; - select.dispatchEvent(new Event('change')); - await new Promise(r => setTimeout(r, 0)); - - expect(api.saveAccountServiceOverride).toHaveBeenCalledTimes(1); - expect(consoleWarnSpy).toHaveBeenCalled(); - // No error toast should have been shown for the refresh failure: the - // success toast from the save path is what the user sees. - const toastCalls = mockShowToast.mock.calls.map(c => c[0]); - const errorToasts = toastCalls.filter(t => (t as { kind?: string }).kind === 'error'); - expect(errorToasts).toHaveLength(0); - - consoleWarnSpy.mockRestore(); + const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const panel = await openOverridesPanel('acc-1'); + const select = panel.querySelector('select.override-payment-select') as HTMLSelectElement; + select.value = 'all-upfront'; + select.dispatchEvent(new Event('change')); + await new Promise(r => setTimeout(r, 0)); + + expect(api.saveAccountServiceOverride).toHaveBeenCalledTimes(1); + expect(consoleWarnSpy).toHaveBeenCalled(); + // No error toast should have been shown for the refresh failure: the + // success toast from the save path is what the user sees. + const toastCalls = mockShowToast.mock.calls.map(c => c[0]); + const errorToasts = toastCalls.filter(t => (t as { kind?: string }).kind === 'error'); + expect(errorToasts).toHaveLength(0); + } finally { + consoleWarnSpy.mockRestore(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/__tests__/settings-accounts.test.ts` around lines 617 - 633, The test spies on console.warn using consoleWarnSpy (jest.spyOn(console, 'warn')) but only calls consoleWarnSpy.mockRestore() at the end, which leaks the spy if an assertion throws; wrap the test logic that sets up consoleWarnSpy, calls openOverridesPanel/select/change, and the subsequent expects in a try/finally so consoleWarnSpy.mockRestore() is executed in the finally block (or move the restore into an afterEach for the spec) to guarantee restoration even on failures.internal/scheduler/scheduler_overrides_test.go (1)
248-261: Add explicit override-lookup error pass-through test.You already verify pass-through for global lookup failure; add the sibling case for
GetAccountServiceOverrideerrors to lock in the same contract.🧪 Suggested test addition
+func TestApplyAccountOverrides_OverrideLookupError_PassesThrough(t *testing.T) { + ctx := context.Background() + store := &mockOverrideStore{ + recs: []config.RecommendationRecord{ + rdsRec("acct-A", "us-east-1", "mysql"), + }, + globals: map[string]*config.ServiceConfig{ + "aws|rds": {Provider: "aws", Service: "rds", Enabled: true}, + }, + getOverrideErr: errors.New("override lookup timeout"), + } + s := &Scheduler{config: store} + + recs, err := s.ListRecommendations(ctx, config.RecommendationFilter{}) + require.NoError(t, err, "ListRecommendations should swallow override resolver errors") + assert.Len(t, recs, 1, "unfiltered list returned on override lookup failure") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduler/scheduler_overrides_test.go` around lines 248 - 261, Add a sibling test mirroring TestApplyAccountOverrides_LookupError_PassesThrough that asserts account-override lookup errors are passed through: instantiate mockOverrideStore with getAccountErr set to a non-nil error (instead of getGlobalErr), include a sample recommendation in recs, create Scheduler{config: store}, call s.ListRecommendations(ctx, config.RecommendationFilter{}), and assert no error from ListRecommendations and that the unfiltered recommendation list is returned; reference mockOverrideStore, getAccountErr, GetAccountServiceOverride, Scheduler and ListRecommendations when locating where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/handler_dashboard.go`:
- Around line 149-163: The scaledSavings function incorrectly treats an explicit
coverage==0 as "no override" because of the `if !ok || coverage <= 0 { return
rec.Savings }` check; change the logic so only a missing entry (`!ok`) returns
rec.Savings, then clamp coverage into [0,100] (e.g., coverage =
min(max(coverage, 0), 100)) and apply the scale; keep the fast path `if coverage
>= 100 { return rec.Savings }` if desired, otherwise return `rec.Savings *
coverage / 100`. Reference: scaledSavings and config.AccountConfigKey to find
the coverage lookup.
---
Nitpick comments:
In `@frontend/src/__tests__/settings-accounts.test.ts`:
- Around line 617-633: The test spies on console.warn using consoleWarnSpy
(jest.spyOn(console, 'warn')) but only calls consoleWarnSpy.mockRestore() at the
end, which leaks the spy if an assertion throws; wrap the test logic that sets
up consoleWarnSpy, calls openOverridesPanel/select/change, and the subsequent
expects in a try/finally so consoleWarnSpy.mockRestore() is executed in the
finally block (or move the restore into an afterEach for the spec) to guarantee
restoration even on failures.
In `@internal/scheduler/scheduler_overrides_test.go`:
- Around line 248-261: Add a sibling test mirroring
TestApplyAccountOverrides_LookupError_PassesThrough that asserts
account-override lookup errors are passed through: instantiate mockOverrideStore
with getAccountErr set to a non-nil error (instead of getGlobalErr), include a
sample recommendation in recs, create Scheduler{config: store}, call
s.ListRecommendations(ctx, config.RecommendationFilter{}), and assert no error
from ListRecommendations and that the unfiltered recommendation list is
returned; reference mockOverrideStore, getAccountErr, GetAccountServiceOverride,
Scheduler and ListRecommendations when locating where to add the test.
🪄 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: 676870fe-3e79-4af0-9009-2b9f2c167f97
📒 Files selected for processing (10)
frontend/src/__tests__/settings-accounts.test.tsfrontend/src/settings.tsinternal/api/handler_dashboard.gointernal/api/handler_dashboard_test.gointernal/config/recommendation_overrides.gointernal/config/recommendation_overrides_test.gointernal/scheduler/scheduler.gointernal/scheduler/scheduler_overrides.gointernal/scheduler/scheduler_overrides_test.gointernal/scheduler/scheduler_suppressions_test.go
… 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>
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.
|
Addressed the latest CodeRabbit pass in commit 63fdd11:\n\n- wrapped the console.warn spy restore in a finally block\n- added override-lookup error pass-through coverage\n- aligned the dashboard zero-coverage test with the explicit 0% cap behavior\n\n@coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/api/handler_dashboard_test.go (1)
91-93: Useconfig.AccountConfigKeyin the test double to avoid key-format drift.This keeps test key construction aligned with production lookup behavior in one place.
Proposed refactor
func (s *dashboardOverrideStore) GetAccountServiceOverride(_ context.Context, accountID, provider, service string) (*config.AccountServiceOverride, error) { - return s.overrides[accountID+"|"+provider+"|"+service], nil + return s.overrides[config.AccountConfigKey(accountID, provider, service)], nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler_dashboard_test.go` around lines 91 - 93, The test double dashboardOverrideStore's GetAccountServiceOverride currently constructs the lookup key by concatenating accountID+"|"+provider+"|"+service; change it to use the same key helper used in production (call config.AccountConfigKey(accountID, provider, service)) for the map lookup, and update any places that populate s.overrides to use config.AccountConfigKey as well so the test keys and lookup logic can't drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/handler_dashboard.go`:
- Around line 122-125: The comment above the reducer that currently states “recs
with a recorded coverage of zero (treated as 'no scaling configured') all count
at full weight” is stale; update that comment to reflect the actual runtime
behavior where zero coverage is scaled to 0. Locate the comment block
referencing CloudAccountID / "no scaling configured" (near the reducer handling
coverage mapping) and change the text to state that records without a
CloudAccountID count at full weight, records missing from the coverage map count
at full weight, but records with an explicit coverage value of 0 are scaled to 0
(no weight).
---
Nitpick comments:
In `@internal/api/handler_dashboard_test.go`:
- Around line 91-93: The test double dashboardOverrideStore's
GetAccountServiceOverride currently constructs the lookup key by concatenating
accountID+"|"+provider+"|"+service; change it to use the same key helper used in
production (call config.AccountConfigKey(accountID, provider, service)) for the
map lookup, and update any places that populate s.overrides to use
config.AccountConfigKey as well so the test keys and lookup logic can't drift.
🪄 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: 68eb26ec-024c-499c-bf0d-fc71a56bdf8d
📒 Files selected for processing (4)
frontend/src/__tests__/settings-accounts.test.tsinternal/api/handler_dashboard.gointernal/api/handler_dashboard_test.gointernal/scheduler/scheduler_overrides_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/tests/settings-accounts.test.ts
- internal/scheduler/scheduler_overrides_test.go
| // Recs without a CloudAccountID, recs whose triple has no entry in the | ||
| // map, and recs with a recorded coverage of zero (treated as "no | ||
| // scaling configured") all count at full weight — this matches the | ||
| // pre-#196 behaviour for un-configured accounts. |
There was a problem hiding this comment.
Fix stale zero-coverage behavior in the reducer comment.
Line 123–125 says zero coverage is treated as “no scaling configured” (full weight), but Line 160–162 now correctly scales it to 0. Please update the comment to match runtime behavior and avoid regressions.
Proposed doc fix
-// Recs without a CloudAccountID, recs whose triple has no entry in the
-// map, and recs with a recorded coverage of zero (treated as "no
-// scaling configured") all count at full weight — this matches the
-// pre-#196 behaviour for un-configured accounts.
+// Recs without a CloudAccountID and recs whose triple has no entry in the
+// map count at full weight (unconfigured fallback).
+// A recorded coverage of zero contributes zero savings.📝 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.
| // Recs without a CloudAccountID, recs whose triple has no entry in the | |
| // map, and recs with a recorded coverage of zero (treated as "no | |
| // scaling configured") all count at full weight — this matches the | |
| // pre-#196 behaviour for un-configured accounts. | |
| // Recs without a CloudAccountID and recs whose triple has no entry in the | |
| // map count at full weight (unconfigured fallback). | |
| // A recorded coverage of zero contributes zero savings. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/api/handler_dashboard.go` around lines 122 - 125, The comment above
the reducer that currently states “recs with a recorded coverage of zero
(treated as 'no scaling configured') all count at full weight” is stale; update
that comment to reflect the actual runtime behavior where zero coverage is
scaled to 0. Locate the comment block referencing CloudAccountID / "no scaling
configured" (near the reducer handling coverage mapping) and change the text to
state that records without a CloudAccountID count at full weight, records
missing from the coverage map count at full weight, but records with an explicit
coverage value of 0 are scaled to 0 (no weight).
Align the dashboard override test double with production key construction so coverage override tests cannot drift from the resolver lookup format.
|
Addressed the latest CodeRabbit nitpick: |
|
✅ Actions performedReview triggered.
|
Move the recommendations engine-key migration to 000043 so the branch no longer collides with the retry-linkage 000042 migration on the target branch.
|
Fixed the failing pre-commit hook: |
|
✅ Actions performedReview triggered.
|
1 participant
Closes #196.
Summary
PR #193 wired
account_service_overridesinto the purchase forms so overrides are visible at commit time. This PR closes the loop on issue #196 by applying the remaining override fields at the read path:enabled = false→ drops that account+service's recs from the listing.include_engines/exclude_engines(and the region / type pairs) → drops non-matching rows.coverage→ caps the dashboard "potential savings" headline per-account, so the widget reflects the user's intended commitment, not the un-overridden total.Plus the frontend cache invalidation the issue called for: every override panel mutation (inline payment edit, Reset/delete, Create modal) now triggers a recommendations refetch.
Architectural choice — post-DB filter, not SQL
The issue text suggested extending
RecommendationFilterwith account-aware predicates. Pushing array-membership joins againstaccount_service_overridesintobuildRecommendationFilter's WHERE clause would force a correlated subquery per (account, provider, service, engine, region, type), and the override table is sparse (most accounts have no row). The existingapplySuppressionshelper already filters post-DB insideScheduler.ListRecommendations— this PR mirrors that pattern via a newapplyAccountOverridesstep.Both filters share the same over-show-vs-under-show degradation: a lookup error logs and passes the un-filtered list through, so a Postgres blip doesn't blank the recommendations page.
Reuse map
config.ResolveServiceConfig(global, override)— already existed, was dead code, now load-bearing.ConfigStore.GetServiceConfig/GetAccountServiceOverride— already exposed via the store interface.Scheduler.applySuppressions— pattern precedent for the new filter.frontend/src/recommendations.ts::loadRecommendations— exported and reused as the refresh trigger.MockConfigStorealready implements both store methods we needed for tests.Files
internal/config/recommendation_overrides.goResolveAccountConfigsForRecshelper that walks recs once, dedupes per(account, provider, service), and merges global ⊕ override viaResolveServiceConfig.internal/scheduler/scheduler_overrides.goScheduler.applyAccountOverridesfilter (engine/region/type include/exclude +enabled=falsedrop).internal/scheduler/scheduler.goapplyAccountOverridesintoListRecommendationsafterapplySuppressions.internal/api/handler_dashboard.gosummarizeRecommendationsWithCoverage+ helperresolveCoverageByAccountKey.frontend/src/settings.tsrefreshRecommendationsAfterOverrideChangehelper, called from the 3 override-mutation callsites (payment edit, Reset, Create modal submit).Out of scope (deliberate; follow-up issues acknowledged in #196)
getRecommendationDetailinherits the filter viaListRecommendations, so a deep link to a now-disabled rec returns 404. That's consistent with "hidden" semantics; a direct by-id store lookup is a separate cleanup.ResolveServiceConfigpanic on nil global — the newResolveAccountConfigsForRecsskips the triple in that case rather than triggering it. Fixing the resolver itself is a separate cleanup.Test plan
Backend (
go test ./...clean across 3 passes):internal/config/recommendation_overrides_test.go— 10 unit tests (dedup, missing-global cached, error propagation, nil-account skip).internal/scheduler/scheduler_overrides_test.go— 9 tests including the issue feat(recommendations): option-B read-time filter for enabled / coverage / include-exclude per-account override fields #196 acceptance criterion mirrored from verify recommendations engine actually consumes per-account service overrides #111 (seed global + per-account override → asserted hidden recs).internal/api/handler_dashboard_test.go— 1 handler integration test for per-account coverage scaling + 7 table-drivensummarizeRecommendationsWithCoveragecases (nil map, missing key, zero/100/>100/fractional, nil CloudAccountID).internal/scheduler/scheduler_suppressions_test.go— small mock extension so existing suppression tests still pass with the new override pass.Frontend (
npm testclean — 1373/1373 pass;npm run buildclean across 3 passes):frontend/src/__tests__/settings-accounts.test.ts— 2 new tests asserting the recs refetch fires after an override save and that a refresh failure is swallowed without a user-facing error toast.Manual UI smoke skipped — local stack not bootstrapped in this session. The frontend test suite covers the wiring; deploy verification will land via the post-push pipelines.
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests