fix(frontend/scheduler): monthly cost shows $0 and effective % shows 100%/negatives (#252)#254
Conversation
Adds a *float64 field to pkg/common.Recommendation so cloud parsers can carry the provider-reported recurring monthly charge through to the DB and frontend. nil means the provider API did not expose a monthly breakdown (renders as "—" not "$0"). Closes the data gap that caused the MonthlyCost column to always show $0 (issue #252).
…hlyCost Reads the AWS Cost Explorer SDK field RecurringStandardMonthlyCost from RI recommendation details and stores it as RecurringMonthlyCost on the common.Recommendation struct. Previously this field was ignored, causing the monthly cost column to show $0 for all RI recommendations.
…hase Savings Plans don't expose a direct monthly recurring field; instead, RecurringMonthlyCost is derived as HourlyCommitmentToPurchase * 730 (the standard AWS billing constant of 730 hours per month). Left nil when the hourly commitment field is absent from the API response.
…nRecord - scheduler: replace hardcoded MonthlyCost: 0 with rec.RecurringMonthlyCost so the provider-populated value is persisted to the DB - types: change RecommendationRecord.MonthlyCost from float64 to *float64 so nil (provider didn't expose data) is distinguishable from 0.0 (explicitly zero recurring charge). Backward-compatible with existing DynamoDB/Postgres rows: absent attrs deserialize as nil, not 0. - execution: derefFloat64 helper when copying to PurchaseHistoryRecord (history records keep float64 since the data was known at purchase time) - exchange_lookup: safe nil dereference before division - tests: update all literal MonthlyCost usages to *float64 (aws.Float64 / float64Ptr helper / nil as appropriate)
…eSavingsPct When the backend returns null for monthly_cost (provider API didn't expose a monthly recurring breakdown), the UI previously fell through to $0 for the Monthly Cost column and denominator-collapse in effectiveSavingsPct produced misleading 100% / negative values (issue #252). Changes: - api/types.ts: monthly_cost: number | null (null = data not provided) - types.ts: LocalRecommendation.monthly_cost: number | null - app.ts: propagate null instead of coercing undefined→0 in purchase builders - recommendations.ts: - effectiveSavingsPct: early-return null when monthly_cost == null (cannot reconstruct on_demand_monthly without it; null sorts to bottom via ??Infinity) - table cell: render "—" instead of $0 when monthly_cost is null - capacity scaling: preserve null through ratio multiplication
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR fixes backend and frontend handling of recurring monthly costs. Previously, the scheduler hardcoded ChangesBackend and Frontend Recurring Cost Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/api/exchange_lookup_test.go (1)
129-132: ⚡ Quick winAdd one explicit nil-
MonthlyCostmapping test.Now that
MonthlyCostis nullable, add a focused test case whereMonthlyCostisnilto pin the nil-path monthly calculation behavior and prevent regressions.Proposed test addition
+func TestPurchaseRecLookupFromStore_NilMonthlyCost_DoesNotPanicAndUsesAmortizedUpfront(t *testing.T) { + t.Parallel() + store := &fakeRecsLister{ + out: []config.RecommendationRecord{ + { + ID: "rec-nil-monthly", + Provider: "aws", + Service: "ec2", + Region: "us-east-1", + ResourceType: "m5.large", + Term: 1, + UpfrontCost: 120, // 10/mo amortized + MonthlyCost: nil, + }, + }, + } + lookup := purchaseRecLookupFromStore(store, "") + got, err := lookup(context.Background(), "us-east-1", "USD") + require.NoError(t, err) + require.Len(t, got, 1) + assert.InDelta(t, 10.0, got[0].EffectiveMonthlyCost, 0.001) +}Also applies to: 142-143, 182-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/exchange_lookup_test.go` around lines 129 - 132, Add a focused unit test case in internal/api/exchange_lookup_test.go that constructs the same input struct used in the other tests but sets MonthlyCost to nil (e.g., MonthlyCost: nil) to explicitly exercise the nil-monthly-cost path; call the same test helper or function used by the existing cases in this file (the test block around Term/UpfrontCost/MonthlyCost) and assert the expected amortised/monthly calculation result for the nil case. Add identical nil-MonthlyCost variants for the other locations mentioned (the other test cases around lines 142-143 and 182-183) so the nil path is pinned across all scenarios. Ensure the new cases use descriptive names (e.g., "monthly cost nil") and mirror the existing assertion style for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/recommendations.ts`:
- Line 1332: The numeric helpers are treating rec.monthly_cost nulls as 0 which
makes unknown costs appear in "=0" filters and sorts; update the numeric
accessor and helper logic so rec.monthly_cost is propagated as null/undefined
(not coerced to 0) and have the numeric helpers (e.g., any
toNumber/parseNumber/numericSort/numericFilter utilities) return null/NaN for
null/undefined inputs and treat those values as “missing” (place at end when
sorting and exclude from equality filters). Specifically, change the monthly
cost column/accessor that currently reads rec.monthly_cost (and any place that
coerces it before calling formatCurrency) to return null when rec.monthly_cost
is null, and update the numeric helper functions to explicitly check for
null/undefined and handle them as missing rather than 0.
In `@internal/purchase/execution.go`:
- Line 271: The code currently calls derefFloat64(rec.MonthlyCost) when
constructing a PurchaseHistoryRecord which converts a nil monthly-cost into 0,
losing the "unknown" semantic; instead preserve the nullable type by changing
PurchaseHistoryRecord.MonthlyCost to a *float64 (or equivalent nullable type)
and pass rec.MonthlyCost directly (do not call derefFloat64) wherever
PurchaseHistoryRecord is constructed (e.g., in the code building
config.PurchaseHistoryRecord and the other sites around lines referenced), and
update downstream API/frontend history models to accept the nullable value so
the distinction between nil (unknown) and 0.0 (explicit zero) is retained.
In `@providers/aws/recommendations/parser_sp.go`:
- Around line 149-160: The code currently writes an amortized all‑upfront
Savings Plan rate into RecurringMonthlyCost which misrepresents all‑upfront
plans; change the logic so that recurringMonthlyCost is only populated when
detail.HourlyCommitmentToPurchase != nil AND the plan is not an all‑upfront
payment option, and for the all‑upfront payment option explicitly set
recurringMonthlyCost to a pointer to 0.0; leave recurringMonthlyCost nil when
HourlyCommitmentToPurchase is absent. Use the existing symbols
detail.HourlyCommitmentToPurchase, recurringMonthlyCost and hourlyCommitment and
check the payment option flag (the field used to indicate all‑upfront in the
request/response) to decide whether to compute monthly := hourlyCommitment *
hoursPerMonth or set it to &zero.
---
Nitpick comments:
In `@internal/api/exchange_lookup_test.go`:
- Around line 129-132: Add a focused unit test case in
internal/api/exchange_lookup_test.go that constructs the same input struct used
in the other tests but sets MonthlyCost to nil (e.g., MonthlyCost: nil) to
explicitly exercise the nil-monthly-cost path; call the same test helper or
function used by the existing cases in this file (the test block around
Term/UpfrontCost/MonthlyCost) and assert the expected amortised/monthly
calculation result for the nil case. Add identical nil-MonthlyCost variants for
the other locations mentioned (the other test cases around lines 142-143 and
182-183) so the nil path is pinned across all scenarios. Ensure the new cases
use descriptive names (e.g., "monthly cost nil") and mirror the existing
assertion style for consistency.
🪄 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: 62c78ef1-17f4-4dbd-a9f8-c17612761f91
📒 Files selected for processing (16)
frontend/src/__tests__/recommendations.test.tsfrontend/src/api/types.tsfrontend/src/app.tsfrontend/src/recommendations.tsfrontend/src/types.tsinternal/api/exchange_lookup.gointernal/api/exchange_lookup_test.gointernal/api/handler_ri_exchange_integration_test.gointernal/config/store_postgres_recommendations_test.gointernal/config/types.gointernal/config/types_test.gointernal/purchase/execution.gointernal/scheduler/scheduler.gopkg/common/types.goproviders/aws/recommendations/parser_ri.goproviders/aws/recommendations/parser_sp.go
| <td class="savings">${formatCurrency(rec.savings)}</td> | ||
| <td>${formatCurrency(rec.upfront_cost)}</td> | ||
| <td>${formatCurrency(rec.monthly_cost ?? 0)}</td> | ||
| <td>${rec.monthly_cost != null ? formatCurrency(rec.monthly_cost) : '—'}</td> |
There was a problem hiding this comment.
Unknown monthly cost is still conflated with $0 in numeric sort/filter paths.
Line 1332 correctly renders —, but numeric helpers still treat null as 0, so unknown rows can appear in “=0” filters and sort alongside explicit zero-cost rows.
Proposed alignment patch
const SORTABLE_NUMERIC_COLUMNS: Record<string, (r: LocalRecommendation) => number> = {
savings: (r) => r.savings,
upfront_cost: (r) => r.upfront_cost,
- monthly_cost: (r) => r.monthly_cost ?? 0,
+ monthly_cost: (r) => r.monthly_cost ?? Number.POSITIVE_INFINITY,
// effectiveSavingsPct returns null for term=0 / on_demand=0 edge cases.
// POSITIVE_INFINITY places null rows at the bottom in ascending order and
// at the top in descending — the least surprising behaviour for a savings
// column where "no data" rows should be de-emphasised.
@@
function numericCellValue(r: LocalRecommendation, col: state.RecommendationsColumnId): number {
switch (col) {
@@
- case 'monthly_cost': return r.monthly_cost ?? 0;
+ case 'monthly_cost': return r.monthly_cost ?? Number.NaN;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/recommendations.ts` at line 1332, The numeric helpers are
treating rec.monthly_cost nulls as 0 which makes unknown costs appear in "=0"
filters and sorts; update the numeric accessor and helper logic so
rec.monthly_cost is propagated as null/undefined (not coerced to 0) and have the
numeric helpers (e.g., any toNumber/parseNumber/numericSort/numericFilter
utilities) return null/NaN for null/undefined inputs and treat those values as
“missing” (place at end when sorting and exclude from equality filters).
Specifically, change the monthly cost column/accessor that currently reads
rec.monthly_cost (and any place that coerces it before calling formatCurrency)
to return null when rec.monthly_cost is null, and update the numeric helper
functions to explicitly check for null/undefined and handle them as missing
rather than 0.
| Payment: rec.Payment, | ||
| UpfrontCost: result.Cost, | ||
| MonthlyCost: rec.MonthlyCost, | ||
| MonthlyCost: derefFloat64(rec.MonthlyCost), |
There was a problem hiding this comment.
Don't collapse unknown monthly cost back to 0 when writing history.
rec.MonthlyCost == nil now means "provider didn't expose a monthly breakdown", but this helper stores that as a real zero in PurchaseHistoryRecord. That loses the distinction again for any history/analytics path and will make unknown monthly charges look like explicit $0 after purchase.
Suggested direction
- MonthlyCost: derefFloat64(rec.MonthlyCost),
+ MonthlyCost: rec.MonthlyCost,That requires carrying the same nullable type through config.PurchaseHistoryRecord and the corresponding API/frontend history models instead of flattening it here.
Also applies to: 472-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/purchase/execution.go` at line 271, The code currently calls
derefFloat64(rec.MonthlyCost) when constructing a PurchaseHistoryRecord which
converts a nil monthly-cost into 0, losing the "unknown" semantic; instead
preserve the nullable type by changing PurchaseHistoryRecord.MonthlyCost to a
*float64 (or equivalent nullable type) and pass rec.MonthlyCost directly (do not
call derefFloat64) wherever PurchaseHistoryRecord is constructed (e.g., in the
code building config.PurchaseHistoryRecord and the other sites around lines
referenced), and update downstream API/frontend history models to accept the
nullable value so the distinction between nil (unknown) and 0.0 (explicit zero)
is retained.
| // RecurringMonthlyCost for a Savings Plan is the hourly commitment rate | ||
| // multiplied by the standard 730 hours/month billing constant. This is | ||
| // non-nil only when HourlyCommitmentToPurchase is present in the API | ||
| // response; for all-upfront SPs the field is present but the value is | ||
| // the full amortised hourly rate (there is no additional recurring | ||
| // charge after upfront — the "recurring" is already captured in upfront). | ||
| // We populate it regardless of payment option so the frontend can display | ||
| // the amortised monthly equivalent for all payment variants. | ||
| var recurringMonthlyCost *float64 | ||
| if detail.HourlyCommitmentToPurchase != nil { | ||
| monthly := hourlyCommitment * hoursPerMonth | ||
| recurringMonthlyCost = &monthly |
There was a problem hiding this comment.
Don't write amortized all-upfront SP cost into RecurringMonthlyCost.
This block conflicts with the new field contract. Your own comment says HourlyCommitmentToPurchase is just the amortized effective rate for all-upfront plans, not a post-purchase recurring charge, so storing it here will make all-upfront SPs show a fake monthly bill and skew the frontend percentage math. For all-upfront, this should be an explicit 0; keep nil for genuinely unavailable breakdowns.
Suggested direction
var recurringMonthlyCost *float64
-if detail.HourlyCommitmentToPurchase != nil {
- monthly := hourlyCommitment * hoursPerMonth
- recurringMonthlyCost = &monthly
+switch params.PaymentOption {
+case "all-upfront":
+ monthly := 0.0
+ recurringMonthlyCost = &monthly
+case "partial-upfront", "no-upfront":
+ if detail.HourlyCommitmentToPurchase != nil {
+ monthly := hourlyCommitment * hoursPerMonth
+ recurringMonthlyCost = &monthly
+ }
}📝 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.
| // RecurringMonthlyCost for a Savings Plan is the hourly commitment rate | |
| // multiplied by the standard 730 hours/month billing constant. This is | |
| // non-nil only when HourlyCommitmentToPurchase is present in the API | |
| // response; for all-upfront SPs the field is present but the value is | |
| // the full amortised hourly rate (there is no additional recurring | |
| // charge after upfront — the "recurring" is already captured in upfront). | |
| // We populate it regardless of payment option so the frontend can display | |
| // the amortised monthly equivalent for all payment variants. | |
| var recurringMonthlyCost *float64 | |
| if detail.HourlyCommitmentToPurchase != nil { | |
| monthly := hourlyCommitment * hoursPerMonth | |
| recurringMonthlyCost = &monthly | |
| // RecurringMonthlyCost for a Savings Plan is the hourly commitment rate | |
| // multiplied by the standard 730 hours/month billing constant. This is | |
| // non-nil only when HourlyCommitmentToPurchase is present in the API | |
| // response; for all-upfront SPs the field is present but the value is | |
| // the full amortised hourly rate (there is no additional recurring | |
| // charge after upfront — the "recurring" is already captured in upfront). | |
| // We populate it regardless of payment option so the frontend can display | |
| // the amortised monthly equivalent for all payment variants. | |
| var recurringMonthlyCost *float64 | |
| switch params.PaymentOption { | |
| case "all-upfront": | |
| monthly := 0.0 | |
| recurringMonthlyCost = &monthly | |
| case "partial-upfront", "no-upfront": | |
| if detail.HourlyCommitmentToPurchase != nil { | |
| monthly := hourlyCommitment * hoursPerMonth | |
| recurringMonthlyCost = &monthly | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@providers/aws/recommendations/parser_sp.go` around lines 149 - 160, The code
currently writes an amortized all‑upfront Savings Plan rate into
RecurringMonthlyCost which misrepresents all‑upfront plans; change the logic so
that recurringMonthlyCost is only populated when
detail.HourlyCommitmentToPurchase != nil AND the plan is not an all‑upfront
payment option, and for the all‑upfront payment option explicitly set
recurringMonthlyCost to a pointer to 0.0; leave recurringMonthlyCost nil when
HourlyCommitmentToPurchase is absent. Use the existing symbols
detail.HourlyCommitmentToPurchase, recurringMonthlyCost and hourlyCommitment and
check the payment option flag (the field used to indicate all‑upfront in the
request/response) to decide whether to compute monthly := hourlyCommitment *
hoursPerMonth or set it to &zero.
- providers/aws/recommendations/parser_sp.go: for all-upfront SPs, RecurringMonthlyCost is 0 (not HourlyCommitmentToPurchase * 730); the latter is an amortised effective rate, not a post-purchase recurring charge. For partial/no-upfront, keep the existing approximation. Clarify the comment accordingly. - frontend/src/recommendations.ts: use NaN (not 0) for null monthly_cost in sort and filter numeric-value helpers so that "data not provided" rows don't appear in "= $0" filters and don't sort alongside explicit $0 (all-upfront) rows. - internal/api/exchange_lookup_test.go: add focused nil-MonthlyCost test case to pin the nil-path monthly calculation and prevent regressions.
CodeRabbit review — pass 1 responseFixed (pushed in follow-up commit b73cbb5):
Dismissed (with justification):
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
The three fixes look well-reasoned:
The dismissal of the Triggering a review of the updated changes now. [review] |
…write
Azure recommendation collection alone takes >60s and exceeds the API Lambda
timeout — a forced cold-start collect (triggered by TRUNCATE + resetting
last_collected_at) would cause a 502 on the first request after deploy.
Switch to a targeted in-place payload rewrite:
UPDATE recommendations
SET payload = jsonb_set(payload, '{monthly_cost}', 'null'::jsonb)
WHERE (payload->>'monthly_cost')::text = '0';
This converts stale pre-PR-#254 zeros to null so the frontend renders "—"
(honest: data not yet collected) rather than "$0" (misleading: implies $0
recurring). Existing rows stay in place; the daily scheduler tick writes
correct values on its next run.
…loses #252) (#256) * fix(db): flush stale monthly_cost=0 cache rows on deploy (closes #252 gap 1) Pre-PR-#254 rows in the recommendations table store monthly_cost as a literal JSON 0 (old float64 field). After PR #254 changed the field to *float64, reading those rows produces a pointer-to-zero, which the API re-emits as 0 — indistinguishable from a legitimate all-upfront zero. Migration 000046 truncates the recommendations table and resets last_collected_at to NULL, which triggers a synchronous cold-start collect on the next ListRecommendations call. The table is a pure cache (scheduler re-fetches from cloud APIs on every collect), so TRUNCATE has no data-loss risk. * fix(azure): populate RecurringMonthlyCost=0 for all-upfront reservations (closes #252) Azure Reservation recommendations are always all-upfront (single payment, no recurring monthly charge). PR #254 added RecurringMonthlyCost to the common.Recommendation struct and wired it through the scheduler but never updated the Azure parsers, so all 16 Azure recs still returned monthly_cost null (renders as "—" after cache refresh) instead of the accurate value 0 ("$0 — no recurring charge"). Changes: - providers/azure/internal/recommendations: add RecurringMonthlyCost *float64 to ExtractedFields with float64Ptr(0) set in both extractLegacy and extractModern. All Azure recs are all-upfront by API convention. - providers/azure/services/{compute,cache,database,cosmosdb}: pass RecurringMonthlyCost: f.RecurringMonthlyCost through to common.Recommendation. - Tests: assert RecurringMonthlyCost is non-nil pointer-to-zero for both Legacy and Modern fixtures in converter_test.go and for the compute converter in client_test.go. GCP CUDs: RecurringMonthlyCost left nil — the GCP Recommender API does not expose commitment cost per period (only savings), so nil ("—") is the accurate frontend rendering. * fix(db): switch migration 000046 from TRUNCATE to targeted payload rewrite Azure recommendation collection alone takes >60s and exceeds the API Lambda timeout — a forced cold-start collect (triggered by TRUNCATE + resetting last_collected_at) would cause a 502 on the first request after deploy. Switch to a targeted in-place payload rewrite: UPDATE recommendations SET payload = jsonb_set(payload, '{monthly_cost}', 'null'::jsonb) WHERE (payload->>'monthly_cost')::text = '0'; This converts stale pre-PR-#254 zeros to null so the frontend renders "—" (honest: data not yet collected) rather than "$0" (misleading: implies $0 recurring). Existing rows stay in place; the daily scheduler tick writes correct values on its next run.
Problem
Monthly Cost column always displays $0 and Effective Savings % shows
100% (or negative values) for all recommendations. Root cause: the
scheduler hardcoded
MonthlyCost: 0becausepkg/common.Recommendationhad no field to carry the provider-reported recurring monthly charge, so
parsers had no way to populate it.
Closes #252.
Changes
pkg/common
RecurringMonthlyCost *float64toRecommendationstruct.nil= provider API did not expose a monthly breakdown (renders as—in the UI, not\$0); non-nil0= explicitly zero recurringcharge (e.g. all-upfront RI).
AWS RI parser
RecurringStandardMonthlyCostfrom the Cost Explorer SDKresponse and store it in
RecurringMonthlyCost.AWS SP parser
RecurringMonthlyCost = HourlyCommitmentToPurchase × 730(standard AWS billing hours/month). Left
nilwhen the field isabsent from the response.
Azure / GCP parsers
breakdown;
RecurringMonthlyCostis leftnil, which renderstruthfully as
—.Scheduler
MonthlyCost: 0withMonthlyCost: rec.RecurringMonthlyCost(propagates the pointer;
nilis persisted as a JSONnull).DB types
RecommendationRecord.MonthlyCost:float64→*float64.Backward-compatible: existing DynamoDB/Postgres rows with numeric
0deserialize as a pointer to
0.0; absent attributes deserialize asnil. No migration required.Frontend
monthly_cost: number | nullinRecommendation(API type) andLocalRecommendation.effectiveSavingsPct: early-returnnullwhenmonthly_costisnull/undefined(cannot reconstructon_demand_monthlywithout it;the previous
?? 0collapsed the denominator tosavingsalone,producing the misleading 100%/negative values).
—instead of\$0whennull.nullthrough ratio multiplication.Test plan
go test ./...in root andgo test ./...inpkg/npm testinfrontend/(1440 tests)npm run buildinfrontend/Monthly Cost column (not $0)
for all, not negative)
—in Monthly Cost (data notavailable from those provider APIs)
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features