feat(recs): plumb provider SavingsPercentage to GUI for CLI/GUI parity (closes #1106) - #1104
Merged
cristim merged 1 commit intoJun 8, 2026
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 35 minutes and 32 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 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
cristim
force-pushed
the
fix/effective-savings-pct-amortization
branch
from
June 7, 2026 22:53
bd1d139 to
a6f4069
Compare
Member
Author
|
Scope expanded with the CLI/GUI savings-% parity fix (new commit
Verification: @coderabbitai review |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review Generated by Claude Code |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
The CLI/reporter prints AWS's authoritative SavingsPercentage (EstimatedMonthlySavingsPercentage; Azure/GCP converters also set it) directly, but the field was dropped at the API boundary, so the GUI had to re-derive the % via effectiveSavingsPct. That recomputation could drift from the provider's number and returned a blank em-dash for AWS recs missing on_demand_cost even when the provider reported a real % (the #323 case). Plumb SavingsPercentage end-to-end so the GUI shows the same number the CLI shows: - internal/config/types.go: add nullable RecommendationRecord.SavingsPercentage (*float64). Persisted via the recommendations row's JSONB payload column, no DDL needed (same as OnDemandCost). nil = provider did not report a %. - internal/scheduler/scheduler.go: in convertRecommendations (the only builder that maps a common rec to a RecommendationRecord) set SavingsPercentage: nonZeroPtr(rec.SavingsPercentage), mirroring OnDemandCost. A provider value of 0 round-trips as nil, never a fabricated pointer-to-0. - frontend/src/types.ts: add savings_percentage?: number | null to LocalRecommendation (provider-authoritative %, preferred over reconstruction). - frontend/src/recommendations.ts: add displaySavingsPct(r) which prefers a finite, non-null r.savings_percentage and falls back to the (net-savings) effectiveSavingsPct. Route the effective-% column render, the sort accessor, the per-cell variant aggregation, the numeric filter accessor, and the purchase-modal row render through it. effectiveSavingsPct / effectiveMonthlySavings are unchanged (fallback path). Net effect: GUI shows the identical % the CLI shows, and AWS recs missing on_demand_cost now render the provider % instead of an em-dash. Tests (fail pre-change): - Go: TestScheduler_ConvertRecommendations_SavingsPercentage asserts the value is carried (non-nil when present, nil when 0/absent). - TS: displaySavingsPct uses the provider % verbatim and ignores the computed value; falls back to effectiveSavingsPct when null/undefined/non-finite; an AWS rec with a provider % but on_demand_cost=null now renders the % instead of an em-dash. Also closes #1106
cristim
force-pushed
the
fix/effective-savings-pct-amortization
branch
from
June 8, 2026 04:24
a6f4069 to
1fa33c0
Compare
Member
Author
|
Rebased onto the latest What changed in the rebase:
Remaining (parity) work, one commit on top of base:
Verification (post-rebase): |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
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.
CLI/GUI parity for the savings %
The CLI/reporter prints AWS's authoritative
rec.SavingsPercentagedirectly (internal/reporter/reporter.go, sourced from AWSEstimatedMonthlySavingsPercentageinparser_ri.go; Azure/GCP converters also setSavingsPercentage). ButSavingsPercentagewas dropped at the API boundary, so the GUI re-derived the % client-side viaeffectiveSavingsPct. That recomputation (a) could drift from the provider's authoritative number, and (b) returned null / em-dash whenon_demand_costwas missing even though the provider gave a real % (the #323 case). The CLI never had these problems because it used the provider value directly.The fix (full-stack)
internal/config/types.go: added nullableRecommendationRecord.SavingsPercentage *float64. Persisted via the recommendations row's JSONBpayloadcolumn, no DDL/migration needed (same asOnDemandCost). nil = provider did not report a %; a provider 0 round-trips as nil, never a fabricated pointer-to-0.internal/scheduler/scheduler.go: inconvertRecommendations(the only builder mapping a common rec to a record) setSavingsPercentage: nonZeroPtr(rec.SavingsPercentage), parity withOnDemandCost.frontend/src/types.ts: addedsavings_percentage?: number | nulltoLocalRecommendation.frontend/src/recommendations.ts: addeddisplaySavingsPct(r)which prefers a finite, non-nullr.savings_percentageand falls back to the (net-savings)effectiveSavingsPct. Routed the effective-% column render, the sort accessor, the per-cell variant aggregation, the numeric filter accessor, and the purchase-modal row render through it.effectiveSavingsPct/effectiveMonthlySavingsare unchanged (fallback path).Net effect: the GUI shows the identical % the CLI shows, and AWS recs missing
on_demand_costnow render the provider % instead of an em-dash.Tests (fail pre-change)
TestScheduler_ConvertRecommendations_SavingsPercentageasserts the value is carried (non-nil when present, nil when 0/absent).displaySavingsPctuses the provider % verbatim and ignores the computed value; falls back when null/undefined/non-finite; an AWS rec with a provider % buton_demand_cost=nullrenders the % instead of an em-dash.Verification (post-rebase)
go build ./...clean;go vet ./internal/config/... ./internal/scheduler/...clean;go test ./internal/config/... ./internal/scheduler/...-> 663 pass.frontend/:npx jest src/__tests__/recommendations.test.ts-> 386 pass, 0 fail;npx tsc --noEmitclean.Closes #1106
Related: #323, #1035