Background
PR #200 / commit b318a75a5 shipped the read-time per-account override filter for #196, including a per-account coverage cap on the dashboard "potential savings" headline.
The cap implementation has a doc/code/UX mismatch around the coverage = 0 case.
Doc / code disagreement
internal/api/handler_dashboard.go:122-125 (docstring on summarizeRecommendationsWithCoverage):
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.
internal/api/handler_dashboard.go:160-162 (scaledSavings):
if coverage <= 0 {
return 0
}
internal/api/handler_dashboard_test.go:174-179 (table case agrees with the code, not the doc):
{
name: "zero coverage scales savings to zero",
recs: []config.RecommendationRecord{rec(acctA, 100)},
coverage: map[string]float64{keyA: 0},
wantTotal: 0,
},
Real-world UX impact
config.ServiceConfig.Coverage is float64 (internal/config/types.go:81) with the zero-value default 0. A user who:
- Creates a global
ServiceConfig for, say, aws/rds to set Term/Payment defaults
- Does NOT explicitly set Coverage on it
…ends up with Coverage = 0 in the resolved per-account configs, which the cap then treats as "scale all RDS savings to 0". Their dashboard "potential monthly savings" headline silently goes to $0 for every (account, RDS) row — without any user-visible signal that this is a configuration gap, not a "you have nothing to save" answer.
Fix options
- Option 1 (small, behaviour-preserving): change
ServiceConfig.Coverage from float64 to *float64. nil = "unset, full weight"; explicit 0 = "scale to zero". The resolver puts only non-nil pointer entries into the coverage map, and scaledSavings keeps its current <= 0 → 0 semantics for explicit zeros.
- Option 2 (smallest diff): invert the cap so
coverage == 0 → return rec.Savings (matches the doc) and explicit zeroing requires the user to set something else (e.g. enabled=false). Tests need updating.
- Option 3 (doc-only): rewrite the docstring to match the code, accept the footgun, and add an explicit "must set Coverage when creating a global ServiceConfig" UI hint.
Option 1 is the most user-friendly; Option 2 is the smallest patch; Option 3 is the cheapest. My read is Option 1 wins on long-term clarity but it's worth a product call.
Acceptance
- The dashboard headline does not silently go to $0 when a user creates a global
ServiceConfig without setting Coverage.
- The doc and the test name agree with each other and with the code.
- A regression test pins whichever interpretation we pick.
References
Background
PR #200 / commit
b318a75a5shipped the read-time per-account override filter for #196, including a per-accountcoveragecap on the dashboard "potential savings" headline.The cap implementation has a doc/code/UX mismatch around the
coverage = 0case.Doc / code disagreement
internal/api/handler_dashboard.go:122-125(docstring onsummarizeRecommendationsWithCoverage):internal/api/handler_dashboard.go:160-162(scaledSavings):internal/api/handler_dashboard_test.go:174-179(table case agrees with the code, not the doc):{ name: "zero coverage scales savings to zero", recs: []config.RecommendationRecord{rec(acctA, 100)}, coverage: map[string]float64{keyA: 0}, wantTotal: 0, },Real-world UX impact
config.ServiceConfig.Coverageisfloat64(internal/config/types.go:81) with the zero-value default0. A user who:ServiceConfigfor, say,aws/rdsto set Term/Payment defaults…ends up with
Coverage = 0in the resolved per-account configs, which the cap then treats as "scale all RDS savings to 0". Their dashboard "potential monthly savings" headline silently goes to $0 for every (account, RDS) row — without any user-visible signal that this is a configuration gap, not a "you have nothing to save" answer.Fix options
ServiceConfig.Coveragefromfloat64to*float64.nil= "unset, full weight"; explicit0= "scale to zero". The resolver puts only non-nil pointer entries into the coverage map, andscaledSavingskeeps its current<= 0 → 0semantics for explicit zeros.coverage == 0 → return rec.Savings(matches the doc) and explicit zeroing requires the user to set something else (e.g.enabled=false). Tests need updating.Option 1 is the most user-friendly; Option 2 is the smallest patch; Option 3 is the cheapest. My read is Option 1 wins on long-term clarity but it's worth a product call.
Acceptance
ServiceConfigwithout settingCoverage.References
internal/api/handler_dashboard.go:50-167internal/api/handler_dashboard_test.go:144-220internal/config/types.go:81(ServiceConfig.Coverage)internal/config/types.go:497(AccountServiceOverride.Coverage *float64— already the right shape on the override side)