[cristim]

Root cause

internal/api/handler_dashboard.go -> summarizeRecommendationsWithCoverage summed scaledSavings for EVERY recommendation into byService[rec.Service].PotentialSavings and the headline total. After PR #195's per-(term, payment) fan-out, a single physical-resource cell produces up to 6 variant recs (2 terms x 3 payments) that are mutually-exclusive ALTERNATIVES (one commitment per resource), not additive entries. Summing them inflated the per-service breakdown and the headline by up to ~6x.

The headline KPI already avoids this on the frontend by grouping per cell (groupRecsByCell / cellKey in frontend/src/recommendations.ts:588, see the dashboard.ts:139-142 comment about "~6x inflation of summing every variant"). The backend reducer never deduped.

Fix

• Dedupe recs to one representative variant per physical-resource cell BEFORE summing, keyed by (provider, cloud_account_id, service, region, resource_type, engine) to mirror the frontend cellKey.
• The representative is the variant with the MAXIMUM coverage-scaled savings (the best realistic single purchase for that resource, matching the per-service potential users expect). Coverage scaling (scaledSavings / resolveCoverageByAccountKey) is applied to the chosen variant.
• New private helpers bestVariantPerCell and recCellKey live in the api package with a cross-reference comment to the frontend cellKey per the project's cross-language-duplication rule. No existing backend cell-grouping helper existed.
• Both the headline total and by_service now dedupe identically, keeping total == sum(by_service) and fixing the documented backend potential_monthly_savings inflation in lockstep.

Verification

• TestSummarizeRecommendationsWithCoverage_DedupesVariantsPerCell (new regression): feeds three term/payment variants of one cell (savings 100/200/300) plus a distinct second cell (50). Asserts by_service and total equal the sum of per-cell MAX (300 + 50 = 350), NOT the sum of all variants (650). Confirmed it FAILS against the pre-fix code (returned 650) and PASSES after.
• Updated TestHandler_getDashboardSummary fixture to give its two rds recs distinct resource_type values so they stay additive (distinct cells) under the new dedupe; the test's aggregation intent is preserved.
• go build ./... OK, go vet ./internal/api/... clean, go test ./internal/api/... 1512 passed (mock/pgxmock; no live Postgres required for this package).
• Changed Go files are gofmt-clean.

Closes #1100
Related: #1031 (current_savings always 0, out of scope here), #908 (per-service chart redesign)

[coderabbitai]

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 55 minutes and 12 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4518faa6-85ec-4d2f-a3d0-96dece2b01d9

📥 Commits

Reviewing files that changed from the base of the PR and between f5632b3 and fa07d88.

📒 Files selected for processing (2)

• internal/api/handler_dashboard.go
• internal/api/handler_dashboard_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/by-service-variant-dedup

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

@coderabbitai full review

---

Generated by Claude Code

[coderabbitai]

@cristim Sure, I'll kick off a full review of PR #1102 right away.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:45:38.069Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/cloudsql/client.go`, `getSQLPricing` had the same per-hour/term-total unit mismatch as memorystore and cloudstorage: `commitmentPrice` (per-hour SKU rate) was passed directly to savings math that expected a term total, producing ~99.99% savings, and `HourlyRate` was set to `per-hour / hoursInTerm` (near-zero). Fixed in PR `#1047` commit 20590c6b4 (issue `#1078` folded in): `commitmentPriceTerm := commitmentPrice * hoursInTerm`; `HourlyRate = commitmentPrice` (raw per-hour). `convertGCPRecommendation` also hardcoded `rec.Term = "1yr"`, ignoring `params.Term`; fixed with same defaulting pattern as memorystore/cloudstorage. Regression tests: `TestGetSQLPricing_CommitmentPriceIsTermTotal` and `TestCloudSQLConvertGCPRecommendation_PropagatesTermFromParams`. PaymentOption was already correct (`if paymentOption == "" { paymentOption = "monthly" }`).

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly `providers/gcp/recommendations.go`, `collectRegion` fans out to all four GCP service clients (computeengine, cloudsql, memorystore, cloudstorage) concurrently (fixed in PR `#1047` commit 95a390f, H-2 finding). `regionResult` gains `cache` and `storage` slices; merge is updated accordingly. The decision was to wire (not trim) memorystore and cloudstorage because both have complete `GetRecommendations` implementations and their no-op `PurchaseCommitment` paths are orthogonal to surfacing advisory recommendations. Regression tests: `TestRegionResult_HasCacheAndStorageFields` and `TestShouldIncludeService_Cache_Storage`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 277
File: frontend/src/__tests__/recommendations.test.ts:2472-2476
Timestamp: 2026-05-05T07:46:01.902Z
Learning: In the CUDly frontend (`frontend/src/recommendations.ts`), `effectiveSavingsPct` intentionally has NO plausibility guard (no null return or warning for reconstructed percentages above per-term ceilings). The maintainer explicitly rejected that approach in PR `#277`. The fix for inflated percentages (issue `#274`) is purely a data-path fix: plumbing the provider's canonical `on_demand_cost` through to the frontend so the denominator is not reconstructed from potentially misleading `monthly_cost + savings + amortized`. Tests that pin the reconstructed >80% behavior are intentional documentation of the pre-fix path, not bugs.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-08T04:21:35.802Z
Learning: In LeanerCloud/CUDly `frontend/src/recommendations.ts` (PR `#838`, commit d7c5764), `perRecPayments` in `FanOutBucket` must be seeded **sparsely** (override-only: only recs whose resolved payment differs from the bucket-level default). The execute path in `frontend/src/app.ts` resolves each rec's payment as `perRecPayments.get(rec.id) ?? b.payment`; if the map is fully populated the fallback is dead and changing the bucket-level dropdown has no effect on unedited rows. The per-rec change handler deletes the map entry when the user reverts to the bucket default (resuming tracking via the fallback); the bucket-level change handler re-syncs visible per-rec selects that have no override, skipping rows whose service does not support the new payment (mixed-SP buckets). Tests (i) and (j) cover the override-only contract and bucket-level propagation respectively; 322 tests pass, `tsc --noEmit` clean.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly GCP service converters (`convertGCPRecommendation` in memorystore/client.go and cloudstorage/client.go), `rec.Term` must be derived from `params.Term` with a `"1yr"` default — not hardcoded to `"1yr"`. Without this, 3-year callers always emit 1-year commitments even though the downstream `termYears` derivation from `rec.Term` is correct. Fixed in PR `#1047` commit c6280c390 (F2 for memorystore, F4 for cloudstorage). Regression tests: `TestConvertGCPRecommendation_PropagatesTermFromParams` and `TestCloudStorageConvertGCPRecommendation_PropagatesTermFromParams`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T17:38:19.433Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` must force `paymentOption = "monthly"` unconditionally (logging a WARN when a non-monthly value such as "upfront", "all-upfront", or "partial-upfront" is supplied) because GCP CUDs are monthly-only and any non-monthly value passed through would be a silent misconfiguration. Fixed in PR `#1047` commit c6280c390 (F1). Regression test: `TestConvertGCPRecommendation_NonMonthlyPaymentOptionForcedToMonthly`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.445Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, `convertGCPRecommendation` no longer hardcodes `Term = "1yr"` (fixed in PR `#1047` commit 8f9a787, H-3 audit finding). It propagates `params.Term` with a `"1yr"` default, validates via `termPlan`, and returns nil on unknown term. This is stricter than memorystore/cloudstorage/cloudsql (which default and continue): computeengine returns nil on unknown term because it is on the purchase path. Regression tests: `TestConvertGCPRecommendation_PropagatesParamsTerm`, `TestConvertGCPRecommendation_RejectsUnknownTerm`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:28:28.139Z
Learning: In LeanerCloud/CUDly GCP service clients (computeengine, cloudsql, memorystore, cloudstorage), `GetRecommendations` filters out non-ACTIVE recommendations using the typed `recommenderpb.RecommendationStateInfo_ACTIVE` enum (CLAIMED/SUCCEEDED/FAILED/DISMISSED are skipped). State is accessed nil-safely via the proto getter. Fixed in PR `#1047` commit 95a390f (H-1 finding). Regression tests: `TestGetRecommendations_FiltersNonActiveStates` (5-rec mock, only ACTIVE survives) and `TestGetRecommendations_ActiveRecIncluded`. Existing mock tests in all four services had StateInfo added to satisfy the new filter.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:13:37.445Z
Learning: In LeanerCloud/CUDly `providers/gcp/services/computeengine/client.go`, the `memMBPerVCPU = 4096` const was removed in PR `#1047` commit 8f9a787. MEMORY MB is now extracted directly from the Recommender payload via `memoryMBFromOperationGroups` (sibling of `vcpuCountFromOperationGroups`). `buildInsertRequest` and `GroupCommitments` call `memoryMBFromDetails(rec)` which returns an error (no silent fallback) when `Details.MemoryGB` is absent. This ensures high-memory families (e.g. N2-highmem) get the correct MB from the payload rather than the GENERAL_PURPOSE approximation. Regression test: `TestBuildInsertRequest_RefusesMissingMemory`.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-07T18:34:23.754Z
Learning: In LeanerCloud/CUDly Azure pricing extractors (compute, cache, database, cosmosdb, search, managedredis), Azure's Retail Prices API returns "1 Year" (singular) for 1-year reservation terms and "3 Years" (plural) for 3-year terms. All 6 pricing extractors use `azureTermString(termYears int) string` helper (added in PR `#1045` commit 5e938d9) to produce the correct singular/plural form for term matching. Prior to this fix, "%d Years" format produced "1 Years" which never matched Azure API responses, causing all 1-year reservation pricing lookups to silently fail. Regression tests: `TestExtractVMPricing_SingularOneYear`, `TestExtractVMPricing_PluralThreeYears`, `TestExtractRedisPricing_SingularOneYear`, `TestExtractRedisPricing_PluralThreeYears`.

✅ Action performed

Full review finished.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.