fix(api): current_savings zero for services with no active commitments (closes #1031)

[cristim]

Summary

• Option chosen: (a) populate CurrentSavings per service from active purchase history. The per-service commitment data is readily available via aggregateActiveCommitmentsPerService, which already runs as part of calculateCommitmentMetrics.

• Root cause: PR fix(api/dashboard): populate ServiceSavings.CurrentSavings from active commitments #926 added svc.CurrentSavings += scaled inside summarizeRecommendationsWithCoverage, intending it to represent coverage-scaled potential savings. But CurrentSavings semantically means committed/realized savings from active purchase history (that's what the frontend chart's green bars show). For services with recommendations but no active purchases, getDashboardSummary never ran the purchase-history overwrite loop, so current_savings incorrectly equalled potential_savings instead of 0.

• Fix: remove the svc.CurrentSavings += scaled assignment from summarizeRecommendationsWithCoverage. CurrentSavings is entirely getDashboardSummary's responsibility, populated after the recommendations reducer runs, via the aggregateActiveCommitmentsPerService overwrite loop (from fix(api/dashboard): populate ServiceSavings.CurrentSavings from active commitments #926). Services with no active purchases now correctly ship current_savings: 0.

Test plan

• TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments (new): service with recommendations but no purchases ships current_savings: 0, not current_savings: potential_savings
• TestSummarizeRecommendationsWithCoverage_DoesNotSetCurrentSavings (renamed from _PopulatesCurrentSavings): the reducer does NOT set CurrentSavings; only PotentialSavings is set
• TestSummarizeRecommendationsWithCoverage table-driven assertions updated: CurrentSavings stays 0 from summarizeRecommendationsWithCoverage
• TestHandler_getDashboardSummary_CurrentSavingsPopulated still passes (services WITH active purchases get the correct non-zero value)
• go build ./... and go vet ./internal/api/... pass
• Full api package: 1493 tests pass (1506 after scope expansion)
• Frontend typecheck: tsc --noEmit passes (JSON schema unchanged)

Closes #1031

Scope expanded

Additional findings from the FOLD-1041 bucket (report 01, internal/api/handler_dashboard.go + handler_purchases.go) folded in as commit a1446dbd5:

ID	Finding	Fix
01-L2	filterDashboardRecommendations used recs[:0] (shared backing array)	Fresh make([]config.RecommendationRecord, 0, len(recs))
01-M3	fetchCommitmentPurchases swallowed store errors silently; dashboard showed zeroed KPIs with no log	logging.Errorf added before returning false
01-M2	YTD month count divided duration by 30-day constant (truncates February, short months)	elapsedWholeMonths helper using AddDate-stepping
01-N1	upcomingFromExecution and buildPlannedPurchase each inlined an identical map-range + break first-service loop	firstServiceConfig(plan) helper with sorted-key traversal for deterministic order

Regression tests added: TestElapsedWholeMonths, TestFilterDashboardRecommendations_FreshSlice, TestFirstServiceConfig.

Also closes #1059

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Fixed dashboard to correctly display $0 current savings for services with recommendations but no active purchases.
  • Improved YTD savings calculation accuracy by counting whole calendar months instead of using fixed 30-day approximations, especially around month boundaries.
  • Enhanced commitment KPI error handling with more specific dashboard messaging when fetch operations fail.
  • Made upcoming purchase projections use deterministic service selection for consistent and predictable behavior.

[cristim]

@coderabbitai review

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 17 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ae17d6b7-8e92-43b1-a350-9c5ca7e441d3

📥 Commits

Reviewing files that changed from the base of the PR and between 2c1169f and 36268e0.

📒 Files selected for processing (3)

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

📝 Walkthrough

Walkthrough

summarizeRecommendationsWithCoverage now only aggregates per-service PotentialSavings and documents that CurrentSavings is populated separately from purchase history. Deterministic first-service selection added for planned/upcoming purchases. Fixed in-place filter allocation, added logging on commitment fetch errors, and replaced 30-day-month heuristic with calendar-month counting. Tests updated/added.

Changes

Dashboard current/potential savings separation and fixes

Layer / File(s)	Summary
Contract, slice, logging, determinism, and YTD helper internal/api/handler_dashboard.go, internal/api/handler_purchases.go	Adds sort import; allocates fresh slice in filterDashboardRecommendations; documents that summarizeRecommendationsWithCoverage does not set CurrentSavings; removes per-service CurrentSavings accumulation; adds firstServiceConfig and uses it in upcoming purchase projection and buildPlannedPurchase; logs store fetch errors for commitments; replaces 30-day month approximation with elapsedWholeMonths.
Tests enforcing contract and helpers internal/api/handler_dashboard_test.go	Updates existing coverage-scaling tests to assert CurrentSavings == 0.0; adds regression TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments; adds TestElapsedWholeMonths, TestFilterDashboardRecommendations_FreshSlice, and TestFirstServiceConfig.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• Merge per-service savings charts into one range chart with current-savings underlay; fix unpopulated current_savings #908 — Changes to summarizeRecommendationsWithCoverage around per-service CurrentSavings handling appear related.
• fix(api): by_service potential_savings sums all term×payment variants (~6x inflation); dedupe per physical-resource cell #1100 — Overlaps on deterministic service selection and deduplication behavior for per-service PotentialSavings.

Possibly related PRs

• LeanerCloud/CUDly#589: Touches summarizeRecommendationsWithCoverage coverage-scaling semantics and tests.
• LeanerCloud/CUDly#277: Modifies OnDemandCost plumbing and related handler/test behavior touching similar chains.

Suggested labels

triaged, priority/p2, severity/medium, urgency/this-sprint, impact/many, effort/m, type/bug

Poem

🐰 I hopped through code with tidy paws,
Split savings clean from commit laws,
Chose first service in the list,
Fixed months and logs that might be missed,
Tests now guard each careful clause.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main change: ensuring current_savings is zero for services without active commitments, which aligns with the primary objective in issue #1031.
Linked Issues check	✅ Passed	All coding requirements from #1031 and #1059 are met: per-service CurrentSavings now populated from active commitments (#1031); YTD month counting via elapsedWholeMonths (#1059-M2); commitment KPI errors logged (#1059-M3); fresh slice allocation in filtering (#1059-L2); firstServiceConfig helper extracted (#1059-N1).
Out of Scope Changes check	✅ Passed	All changes directly support the linked issues: removing CurrentSavings from coverage scaling, adding aggregateActiveCommitmentsPerService integration, implementing YTD month helpers, and extracting deterministic service-config selection. No unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/dashboard-current-savings

---

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

[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]

FOLD-1041 scope expansion (commit a1446db)

Four additional findings from the dashboard/types bucket (report 01) folded in:

01-L2 (filterDashboardRecommendations, line 175): was recs[:0] reusing input backing array; fix: fresh make allocation. Test: TestFilterDashboardRecommendations_FreshSlice.

01-M3 (fetchCommitmentPurchases, line 552): store error returned (nil,false) with no log; fix: logging.Errorf added before return. Existing TestHandler_calculateCommitmentMetrics/purchase-history-error-returns-zeros covers the path.

01-M2 (calculateCommitmentMetrics YTD): elapsed months were int(duration.Hours()/(24*30)), truncating February to 0 months; fix: elapsedWholeMonths helper using AddDate-stepping. Tests: TestElapsedWholeMonths (7 cases including Feb 28-day gap).

01-N1 (upcomingFromExecution + buildPlannedPurchase): both inlined identical map-range+break first-service loops; fix: firstServiceConfig(plan) helper with sorted keys for deterministic order. Test: TestFirstServiceConfig (determinism across 20 iterations).

Build/vet/test: go build ./internal/api/... pass, go vet ./internal/api/... pass, go test ./internal/api/... 1506 passed (1493 pre-fold + 13 new).

@coderabbitai review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the diff for the PR to review the folded changes
gh pr diff 1041 --repo LeanerCloud/CUDly | head -600

Length of output: 21938

---

---

@cristim: All four folded fixes look good. Here's a brief per-finding assessment:

01-L2 (filterDashboardRecommendations) ✅
make([]config.RecommendationRecord, 0, len(recs)) correctly avoids the recs[:0] backing-array aliasing footgun. TestFilterDashboardRecommendations_FreshSlice sets up B-then-A ordering to make the corruption detectable — solid regression.

01-M3 (fetchCommitmentPurchases) ✅
Replacing the dead comment with logging.Errorf(...) including the KPI impact note is the right call.

01-M2 (elapsedWholeMonths) ✅ (one minor nit)
The logic is correct and the February edge case is properly covered. One micro-optimisation: the loop body calls AddDate twice per iteration. The condition is equivalent to !from.AddDate(0, months+1, 0).After(to), which halves the allocations:

for !from.AddDate(0, months+1, 0).After(to) {
    months++
}

Not a bug — just slightly cleaner. Happy to leave it as-is given the negligible loop count (≤ 12 for any realistic YTD window).

01-N1 (firstServiceConfig) ✅
Clean extraction with sorted keys and a proper zero-value guard. The 20-iteration determinism test is convincing. Both call sites in handler_dashboard.go and handler_purchases.go updated consistently.

All 13 new tests cover meaningful edge cases. The scope expansion is well-controlled — nothing feels bolted on. LGTM.

---

🧠 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-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

---

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.

[cristim]

@coderabbitai review

---

Generated by Claude Code

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get recent commits on the PR
gh pr view 1041 --repo LeanerCloud/CUDly --json commits --jq '.commits[-5:] | .[] | {oid: .oid, messageHeadline: .messageHeadline}'

echo "---DIFF---"
# Get full diff
gh pr diff 1041 --repo LeanerCloud/CUDly | head -1000

Length of output: 22383

---

---

@cristim: No new commits since the last review pass — the diff is unchanged. The previous LGTM still stands.

For completeness, here's a quick re-confirmation of all changes in the diff:

Finding	Change	Status
Core (#1031)	svc.CurrentSavings += scaled removed from summarizeRecommendationsWithCoverage; populated solely by purchase-history path	✅
01-L2	recs[:0] → make([]..., 0, len(recs)) — no backing-array aliasing	✅
01-M3	logging.Errorf(...) added before silent return nil, false in fetchCommitmentPurchases	✅
01-M2	elapsedWholeMonths AddDate-stepping replaces 30-day divisor	✅
01-N1	firstServiceConfig with sorted keys replaces non-deterministic map-range+break in two call sites	✅

The only previously noted micro-nit (AddDate called twice per loop iteration in elapsedWholeMonths) remains, but it's non-blocking given the ≤12 iteration bound for any YTD window. All 13 regression tests are correctly scoped. LGTM.

---

🧠 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: 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-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-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

---

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 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-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-05-03T16:07:51.732Z
Learning: In the LeanerCloud/CUDly repository, approximately 50% docstring coverage is a pre-existing project-wide baseline. It should not be flagged as an actionable issue in individual PR reviews, as addressing it is out of scope for focused/surgical fixes.

---

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: 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`.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[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]

Adversarial review notes

Reviewed this as a money-path KPI regression (services with no active commitments displaying non-zero current_savings) plus the piggy-backed #1059 side-fixes. Findings:

No actionable changes needed

The fix is correct, minimal, and the regression test reproduces the real failing scenario. Walked the contract end-to-end:

• Root-cause fix is single-line correct. summarizeRecommendationsWithCoverage no longer assigns svc.CurrentSavings += scaled; getDashboardSummary's overwrite loop (lines 76-80 of handler_dashboard.go) is now the sole writer. For a service absent from currentSavingsByService the map read at entry := byService[svc] yields the zero ServiceSavings and entry.CurrentSavings stays 0 — the exact fix(api): dashboard by_service current_savings is always 0 — populate or remove the field #1031 contract.
• Sibling-KPI parity is preserved. Both committed_monthly (line 676-678) and per-service CurrentSavings (line 75-80) are derived from the same aggregateActiveCommitmentsPerService call, so they share the identical isActiveCommitment gate (status in {"", "completed"} AND !now.After(commitmentExpiry(p))). No risk that the headline KPI and the per-service breakdown diverge on the "active" definition.
• CurrentCoverage is unaffected. Computed from (totalSavings, committedMonthly) at line 87 — neither value flows through the removed assignment.
• Regression test fails pre-fix, passes post-fix. TestHandler_getDashboardSummary_CurrentSavingsZeroWhenNoCommitments seeds two recs (EC2: 500, RDS: 300) with empty purchase history. Pre-fix: the reducer would have written CurrentSavings = 500 / 300 (no coverage key, fallthrough to 100%); post-fix: 0 / 0. The reproducer matches the live failing scenario (services with recommendations but no purchases), not a narrower unit. Coverage triple-stack is sound: positive case in _CurrentSavingsPopulated (line 1102, mixed active+expired), wire-shape positive in _CurrentSavingsJSON (line 1163), new negative here.
• Reducer contract is positively asserted three ways. TestSummarizeRecommendationsWithCoverage (line 263), _DoesNotSetCurrentSavings (line 331), and _DedupesVariantsPerCell (line 406) all now assert CurrentSavings == 0 from the reducer alone — locks in the new boundary.
• No re-amortization. Memory feedback_savings_already_net_no_reamortize.md doesn't trigger: EstimatedSavings is summed once in aggregateActiveCommitmentsPerService and once for committedMonthly. No upfront-cost arithmetic is added or removed here.
• No snapshot/runrate confusion. Memory feedback_snapshot_runrate_avg.md doesn't trigger: aggregation is across SERVICES at one point in time (now), not across TIME. The monthly-unit contract on EstimatedSavings is preserved verbatim.
• No coordination conflict with fix(api): compute active commitments via uncapped SQL active-only read #1221. PR fix(api): compute active commitments via uncapped SQL active-only read #1221 (active-only SQL read replacing the capped GetAll) addresses the truncation surface; fix(api): current_savings zero for services with no active commitments (closes #1031) #1041 addresses the assignment/overwrite contract bug. They're orthogonal — even after fix(api): compute active commitments via uncapped SQL active-only read #1221 ships, the pre-fix(api): current_savings zero for services with no active commitments (closes #1031) #1041 code would still have leaked non-zero current_savings to services without purchases, because the bug was in the reducer, not the fetch. Both fixes are needed; neither subsumes the other.

Considered, no change needed

• Service-key case-sensitivity (rep.rec.Service from recs vs p.Service from purchase history). If the two sources ever disagreed on case (e.g. "EC2" vs "ec2"), the overwrite loop would create a second byService entry rather than overwrite. Pre-existing concern from feat(home): 2x height + stack Current under Potential on savings-by-service chart #925/fix(api/dashboard): populate ServiceSavings.CurrentSavings from active commitments #926, not introduced or worsened by fix(api): current_savings zero for services with no active commitments (closes #1031) #1041. The 4 in-tree tests (_CurrentSavingsPopulated, _CurrentSavingsJSON, _CurrentSavingsZeroWhenNoCommitments, _CurrentSavingsAccountScopedByPurchaseFetch) all use "EC2"/"RDS" consistently across both sources, matching the persisted shape.
• Services with active commitments but no recommendations materialize new byService entries with PotentialSavings = 0, CurrentSavings > 0. Behaviorally correct (the Home chart should surface them), pre-existing from feat(home): 2x height + stack Current under Potential on savings-by-service chart #925/fix(api/dashboard): populate ServiceSavings.CurrentSavings from active commitments #926. Not in scope here.
• Capped GetAllPurchaseHistory(ctx, 1000) at line 640 is the active-only-vs-capped-fetch concern from memory feedback_active_only_not_capped_fetch.md. Pre-existing tech debt; tracked under PR fix(api): compute active commitments via uncapped SQL active-only read #1221's surface. Not in this PR's scope.
• Comment at lines 72-75 of handler_dashboard.go ("Before this fix, CurrentSavings was always zero...") was authored in feat(home): 2x height + stack Current under Potential on savings-by-service chart #925 and remains accurate as a description of the pre-feat(home): 2x height + stack Current under Potential on savings-by-service chart #925 reducer state (which fix(api): current_savings zero for services with no active commitments (closes #1031) #1041 restored). Reading slightly ambiguous in light of the fix(api): dashboard by_service current_savings is always 0 — populate or remove the field #1031 history but not technically wrong — declining to retouch for scope discipline.
• Side-fixes piggy-backed on this PR (commit 2 / fix(api): dashboard YTD calculation, store-error visibility, in-place filter, first-service dedup #1059): recs[:0] -> make(...) (fresh-slice no-input-mutation), fetchCommitmentPurchases now logs the store error before zeroing KPIs, elapsedWholeMonths AddDate-stepping replacing 30-day division, firstServiceConfig lexicographic-determinism helper. Each is independently correct and tightly tested (TestElapsedWholeMonths includes the 28-day-Feb regression, TestFilterDashboardRecommendations_FreshSlice puts the blocked rec at input[0] so the recs[:0] corruption is detectable, TestFirstServiceConfig iterates 20 times to assert determinism). Scope-mix into fix(api): dashboard by_service current_savings is always 0 — populate or remove the field #1031's PR is unusual but the commit boundary makes the split auditable.
• elapsedWholeMonths edge case: a purchase on the 31st of a month rolls forward to the 1st-of-month-after-next via Go's AddDate normalization (Jan 31 + 1mo = Mar 3). For year-spanning gaps this can be off-by-1 vs a strict calendar-month count. Matches the prior 30-day-divisor behaviour for the same input, so no regression vs prior code; only worth fixing if a follow-up issue surfaces real-world drift from it. Not in this PR's scope.
• Mocks in _CurrentSavingsZeroWhenNoCommitments don't register t.Cleanup(func(){ ... .AssertExpectations(t) }) on mockScheduler / mockStore. Memory feedback_mock_assert_expectations.md applies. Pre-existing pattern across this file's _CurrentSavings* family (none of them register the cleanup), not introduced here. Worth a broader sweep but out of scope for the fix(api): dashboard by_service current_savings is always 0 — populate or remove the field #1031 fix.

Out-of-scope follow-ups

None requiring a new issue. The mock.AssertExpectations gap is a file-wide convention drift worth tackling holistically (this file's 1500-line test suite would all benefit), not as a one-off here. The Service-case-normalization question is upstream of byService semantics and predates #925.

CR found no actionable items on the current head; this review concurs.

[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 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 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 review

[cristim]

@coderabbitai review

[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.