feat(frontend/recs): On-Demand Monthly uses provider on_demand_cost directly (closes #330)#331
Conversation
…_cost directly (closes #330) The On-Demand Monthly column now returns the provider-supplied `r.on_demand_cost` directly instead of reconstructing the value from `monthly_cost + savings + (upfront_cost / months_in_term)`. When `on_demand_cost` is missing, undefined, or `0`, the cell renders the existing em-dash sentinel. PR #322 originally scoped the column to a *reconstruction-only* helper ("the column's purpose is to display the reconstructed denominator so users can verify the formula against the raw fields visible in the same row"). In practice, post-#277/#312/#324 every supported provider (AWS, Azure, GCP) plumbs `on_demand_cost` through to the frontend, and `effectiveSavingsPct` already prefers the provider value when present (its `hasOnDemand` branch). The two columns therefore disagreed for any row where the formula reconstruction drifted from the provider's billed price (Azure all-upfront RIs, AWS Capacity Reservation discounts, partial-day proration, list-price rounding). The product decision: surface the **authoritative** number from the provider, not a teaching-tool reconstruction. Users who want to verify the formula can still compute `monthly_cost + savings + upfront_cost/(term*12)` from the row's other columns. Aligning this column with `effectiveSavingsPct`'s denominator means the two never disagree. Changes: * `frontend/src/recommendations.ts::onDemandMonthly` simplified to: `return r.on_demand_cost > 0 ? r.on_demand_cost : null` — drops the `monthly_cost`/`term`/`upfront_cost` arithmetic entirely. * Updated the function's doc comment to reflect the new contract (the prior comment explicitly said "does NOT use on_demand_cost" and is now exactly inverted). * Updated the `SORTABLE_NUMERIC_COLUMNS.on_demand_monthly` preamble — the null-rendering condition is now "missing on_demand_cost" rather than "null monthly_cost / term=0". Tests: * Replaced the seven reconstruction-arithmetic tests in the `describe('onDemandMonthly')` block with five tests covering the new contract: provider value used directly, null/undefined/0 return null, supporting fields ignored when on_demand_cost is positive. * Updated three column-rendering tests in `describe('Monthly Cost + Effective % column rendering')` to set `on_demand_cost` on the fixture so the rendered value matches the new logic — the em-dash and filter regression tests still cover the same semantics, just with clearer fixture intent. `effectiveSavingsPct` keeps its existing reconstruction fallback unchanged — it's the canonical Effective Savings % calculation and must stay backward-compatible with legacy cached rows that pre-date #312/#324. Verification: - `npx tsc --noEmit` clean - `npm test` clean (1578 passed / 0 failed across 42 suites)
cristim
added
triaged
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughThe PR replaces ChangesOn-Demand Monthly Provider Alignment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/recommendations.ts (1)
1085-1088: 💤 Low valueOptional: stale parenthetical in comment.
The parenthetical "(null monthly_cost or term=0)" at line 1085 described the old reconstruction logic's null conditions. With the new
onDemandMonthlyimplementation, null is returned whenon_demand_costis missing, null, or 0—independent ofmonthly_costorterm.Consider updating for accuracy:
📝 Suggested comment update
- // Return NaN for null on_demand_monthly (null monthly_cost or term=0) so any + // Return NaN for null on_demand_monthly (missing or zero on_demand_cost) so any // numeric predicate returns false rather than coincidentally matching 0.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/recommendations.ts` around lines 1085 - 1088, Update the stale parenthetical in the comment above the on_demand_monthly case to reflect the current null conditions: mention that onDemandMonthly(r) now returns null when on_demand_cost is missing, null, or 0 (not "monthly_cost or term=0"), and keep the rest of the explanation about returning NaN and scaling via scaleCost(onDemandMonthly(r), period) intact; reference the onDemandMonthly function and the scaleCost call so reviewers can find the exact code to edit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/__tests__/recommendations.test.ts`:
- Around line 3536-3540: The test title claims to cover both "undefined" and
"missing" but only deletes the property; update the test for onDemandMonthly by
additionally creating a record with on_demand_cost explicitly set to undefined
(e.g., const rUndefined = mk({ on_demand_cost: undefined }) ) and asserting
onDemandMonthly(rUndefined) returns null; keep the existing delete-based case
(mk({}) then delete ...) so both missing and explicit undefined are verified for
the onDemandMonthly behavior.
---
Nitpick comments:
In `@frontend/src/recommendations.ts`:
- Around line 1085-1088: Update the stale parenthetical in the comment above the
on_demand_monthly case to reflect the current null conditions: mention that
onDemandMonthly(r) now returns null when on_demand_cost is missing, null, or 0
(not "monthly_cost or term=0"), and keep the rest of the explanation about
returning NaN and scaling via scaleCost(onDemandMonthly(r), period) intact;
reference the onDemandMonthly function and the scaleCost call so reviewers can
find the exact code to edit.
🪄 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: 050082cc-9f11-42be-9f35-726c89f2de1f
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
| test('undefined / missing on_demand_cost returns null', () => { | ||
| const r = mk({}); | ||
| delete (r as { on_demand_cost?: unknown }).on_demand_cost; | ||
| expect(onDemandMonthly(r)).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Add an explicit undefined assertion (current test only proves “missing”).
Line 3536’s test title says “undefined / missing”, but the body only validates the missing-property case via delete. Please add an explicit on_demand_cost: undefined assertion so the contract is fully pinned.
Suggested test tweak
test('undefined / missing on_demand_cost returns null', () => {
+ expect(onDemandMonthly(mk({ on_demand_cost: undefined }))).toBeNull();
const r = mk({});
delete (r as { on_demand_cost?: unknown }).on_demand_cost;
expect(onDemandMonthly(r)).toBeNull();
});📝 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.
| test('undefined / missing on_demand_cost returns null', () => { | |
| const r = mk({}); | |
| delete (r as { on_demand_cost?: unknown }).on_demand_cost; | |
| expect(onDemandMonthly(r)).toBeNull(); | |
| }); | |
| test('undefined / missing on_demand_cost returns null', () => { | |
| expect(onDemandMonthly(mk({ on_demand_cost: undefined }))).toBeNull(); | |
| const r = mk({}); | |
| delete (r as { on_demand_cost?: unknown }).on_demand_cost; | |
| expect(onDemandMonthly(r)).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/__tests__/recommendations.test.ts` around lines 3536 - 3540, The
test title claims to cover both "undefined" and "missing" but only deletes the
property; update the test for onDemandMonthly by additionally creating a record
with on_demand_cost explicitly set to undefined (e.g., const rUndefined = mk({
on_demand_cost: undefined }) ) and asserting onDemandMonthly(rUndefined) returns
null; keep the existing delete-based case (mk({}) then delete ...) so both
missing and explicit undefined are verified for the onDemandMonthly behavior.
… (CR pass on PR #331) Addresses both items from CodeRabbit review 4244506117 on PR #331: 1. **Actionable** — `frontend/src/__tests__/recommendations.test.ts`: the single "undefined / missing" test only exercised the delete-the-property branch. TypeScript callers can also pass `on_demand_cost: undefined` explicitly (strict-null safety pattern), and the type system distinguishes "field absent" from "field present with value undefined" — so a regression that uses `'on_demand_cost' in r` instead of `r.on_demand_cost > 0` would be caught by one but not the other. Split into two tests: one for explicit undefined, one for the deleted-property case, with comments explaining why both matter. 2. **Nitpick** — `frontend/src/recommendations.ts:1085-1088`: the parenthetical "(null monthly_cost or term=0)" described the pre-#330 reconstruction's null conditions. Refresh to match the new contract: "missing or zero on_demand_cost — see onDemandMonthly() for the contract". Adds a back-reference so a future reader can find the source of truth. Verification: - `npx tsc --noEmit` clean - `npm test` clean (1579 passed / 0 failed across 42 suites — +1 test)
|
@coderabbitai — addressed both items from review
Verification: @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
Triggering a fresh review now. [review] |
1 participant
Summary
The On-Demand Monthly column on the recommendations table now returns the
provider-supplied
r.on_demand_costdirectly instead of reconstructing thevalue from
monthly_cost + savings + (upfront_cost / months_in_term).Why
PR #322 originally scoped the column to a reconstruction-only helper
(commit comment: "the column's purpose is to display the reconstructed
denominator so users can verify the formula against the raw fields visible
in the same row"). In practice, post-#277/#312/#324 every supported
provider (AWS, Azure, GCP) plumbs
on_demand_costthrough to the frontend,and
effectiveSavingsPctalready prefers the provider value when present(its
hasOnDemandbranch). The two columns therefore disagreed for any rowwhere the formula reconstruction drifted from the provider's billed price —
Azure all-upfront RIs, AWS Capacity Reservation discounts, partial-day
proration, list-price rounding.
This PR aligns On-Demand Monthly with
effectiveSavingsPct's denominatorso the two columns can never disagree.
What changed
frontend/src/recommendations.ts::onDemandMonthlysimplified toreturn r.on_demand_cost > 0 ? r.on_demand_cost : null— dropsthe arithmetic entirely.
onDemandMonthlyupdated to reflect the new contract."missing
on_demand_cost" rather than "nullmonthly_cost/term=0".What stayed
effectiveSavingsPctkeeps its existing reconstruction fallback forlegacy cached rows that pre-date fix(recommendations/aws): plumb on_demand_cost for canonical Effective Savings % (closes #303) #312/fix(recommendations/aws): flush stale on_demand_cost cache + parser visibility (closes #321) #324.
semantics are unchanged — only the function's null-condition shifted.
Tests
describe('onDemandMonthly')with 5 tests covering the new contract:provider value used directly,
null/undefined/0returnnull,supporting fields ignored when
on_demand_costis positive.describe('Monthly Cost + Effective % column rendering')to seton_demand_coston the fixture so therendered value matches the new logic.
Closes #330.
Test plan
npx tsc --noEmitcleannpm testclean (1578 passed / 0 failed across 42 suites)on-demand price, not the reconstruction. Em-dash for legacy cached
rows lacking
on_demand_cost.Summary by CodeRabbit
Bug Fixes
Tests