From b3fb0599674566f492b3bd55027e50730990042e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 7 May 2026 15:39:15 +0200 Subject: [PATCH 1/2] feat(frontend/recs): On-Demand Monthly column uses provider on_demand_cost directly (closes #330) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../src/__tests__/recommendations.test.ts | 94 +++++++++---------- frontend/src/recommendations.ts | 38 ++++---- 2 files changed, 63 insertions(+), 69 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index a3222a58..a9ef68d4 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -3499,6 +3499,9 @@ describe('effectiveSavingsPct', () => { }); describe('onDemandMonthly', () => { + // #330: onDemandMonthly returns the provider-supplied on_demand_cost + // directly. No reconstruction fallback — when the provider didn't + // populate on_demand_cost, the column renders an em-dash. const mk = (overrides: Partial): LocalRecommendation => ({ id: 'r', provider: 'aws', @@ -3513,54 +3516,43 @@ describe('onDemandMonthly', () => { ...overrides, } as unknown as LocalRecommendation); - test('null monthly_cost returns null (data not provided)', () => { - expect(onDemandMonthly(mk({ monthly_cost: null }))).toBeNull(); - expect(onDemandMonthly(mk({ monthly_cost: undefined }))).toBeNull(); + test('returns provider on_demand_cost directly when populated', () => { + // The function must return the raw provider value, not any reconstruction. + const odm = onDemandMonthly(mk({ + on_demand_cost: 500, + savings: 100, + upfront_cost: 0, + monthly_cost: 50, + term: 1, + })); + expect(odm).toBe(500); }); - test('term=0 returns null (anomaly — cannot amortize over zero months)', () => { - expect(onDemandMonthly(mk({ term: 0, monthly_cost: 50 }))).toBeNull(); + test('null on_demand_cost returns null (no reconstruction fallback)', () => { + // Explicit null — common for legacy cached recs that pre-date #277/#312. + expect(onDemandMonthly(mk({ on_demand_cost: null }))).toBeNull(); }); - test('no-upfront: on_demand_monthly = monthly_cost + savings', () => { - // upfront_cost = 0 → amortized = 0 → on_demand = 50 + 100 = 150 - const odm = onDemandMonthly(mk({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 })); - expect(odm).not.toBeNull(); - expect(odm!).toBeCloseTo(150, 2); + 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('partial-upfront: on_demand_monthly includes amortized upfront', () => { - // upfront_cost = 600, term = 1 → amortized = 600/12 = 50/mo - // on_demand = 0 + 50 + 50 = 100 - const odm = onDemandMonthly(mk({ savings: 50, upfront_cost: 600, monthly_cost: 0, term: 1 })); - expect(odm).not.toBeNull(); - expect(odm!).toBeCloseTo(100, 2); + test('on_demand_cost=0 returns null (provider treats 0 as not-populated)', () => { + // Same convention as the on_demand_cost preference branch in + // effectiveSavingsPct: zero means the provider didn't report it. + expect(onDemandMonthly(mk({ on_demand_cost: 0 }))).toBeNull(); }); - test('3yr partial-upfront: on_demand_monthly amortizes upfront over 36 months', () => { - // upfront_cost = 7200, term = 3 → amortized = 7200/36 = 200/mo - // on_demand = 200 + 600 + 200 = 1000 - const odm = onDemandMonthly(mk({ savings: 600, upfront_cost: 7200, monthly_cost: 200, term: 3 })); - expect(odm).not.toBeNull(); - expect(odm!).toBeCloseTo(1000, 2); - }); - - test('monthly_cost=0 (all-upfront): on_demand_monthly = savings + amortized_upfront', () => { - // monthly_cost = 0 is valid known data (all-upfront commitment) - // upfront_cost = 1200, term = 1 → amortized = 100/mo - // on_demand = 0 + 200 + 100 = 300 - const odm = onDemandMonthly(mk({ savings: 200, upfront_cost: 1200, monthly_cost: 0, term: 1 })); - expect(odm).not.toBeNull(); - expect(odm!).toBeCloseTo(300, 2); - }); - - test('identity: on_demand_monthly = monthly_cost + savings + (upfront_cost / (term*12))', () => { - // The value must satisfy the documented formula exactly. - const r = mk({ savings: 120, upfront_cost: 3600, monthly_cost: 80, term: 3 }); - const expected = 80 + 120 + (3600 / (3 * 12)); // = 80 + 120 + 100 = 300 - const odm = onDemandMonthly(r); - expect(odm).not.toBeNull(); - expect(odm!).toBeCloseTo(expected, 5); + test('positive on_demand_cost wins regardless of monthly_cost / term / upfront', () => { + // Whatever the supporting fields say, the provider value is authoritative. + expect(onDemandMonthly(mk({ + on_demand_cost: 1500, + monthly_cost: null, + term: 0, + upfront_cost: 99999, + }))).toBe(1500); }); }); @@ -3705,10 +3697,9 @@ describe('Monthly Cost + Effective % column rendering', () => { ); }); - test('On-Demand Monthly column renders formatCurrency when monthly_cost is non-null', async () => { - // savings=100, upfront_cost=0, monthly_cost=50, term=1 - // on_demand_monthly = 50 + 100 + 0 = 150 → formatCurrency → "$150" - const rec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 }); + test('On-Demand Monthly column renders formatCurrency from provider on_demand_cost (#330)', async () => { + // #330 — column shows the raw provider value, not a reconstruction. + const rec = baseRec({ on_demand_cost: 150, savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 }); (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [rec], @@ -3723,14 +3714,16 @@ describe('Monthly Cost + Effective % column rendering', () => { // Pin the specific on_demand_monthly cell by column index (not a scan of all tds). const odmColIdx = Array.from(document.querySelectorAll('th')) .findIndex((th) => th.getAttribute('data-sort') === 'on_demand_monthly'); + expect(odmColIdx).toBeGreaterThanOrEqual(0); const firstRowCells = Array.from( document.querySelectorAll('tbody tr.recommendation-row td'), ); expect(firstRowCells[odmColIdx]?.textContent).toBe('$150'); }); - test('On-Demand Monthly column renders em-dash when monthly_cost is null', async () => { - const rec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: null, term: 1 }); + test('On-Demand Monthly column renders em-dash when on_demand_cost is missing (#330)', async () => { + // #330 — without provider on_demand_cost, no reconstruction; cell is em-dash. + const rec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 }); (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [rec], @@ -3748,10 +3741,11 @@ describe('Monthly Cost + Effective % column rendering', () => { expect(firstRowCells[odmColIdx]?.textContent).toBe('—'); }); - test('on_demand_monthly numeric filter: null monthly_cost row does not match = 0', async () => { - // When monthly_cost is null, on_demand_monthly returns null → NaN sentinel in numericCellValue. - // A filter expr "0" (equals zero) must NOT match such rows. - const nullRec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: null, term: 1 }); + test('on_demand_monthly numeric filter: row with no on_demand_cost does not match = 0 (#330)', async () => { + // #330 — without provider on_demand_cost, onDemandMonthly returns null, + // which becomes a NaN sentinel in numericCellValue. A filter expr "0" + // (equals zero) must NOT match such rows. + const nullRec = baseRec({ savings: 100, upfront_cost: 0, monthly_cost: 50, term: 1 }); (api.getRecommendations as jest.Mock).mockResolvedValue({ summary: {}, recommendations: [nullRec], diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 06b2c15d..5705d959 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -402,11 +402,11 @@ const SORTABLE_NUMERIC_COLUMNS: Record numbe const period = state.getCostPeriod(); return scaleCost(r.monthly_cost, period) ?? Number.POSITIVE_INFINITY; }, - // onDemandMonthly returns null for null monthly_cost or term=0. - // POSITIVE_INFINITY places null rows at the bottom in ascending order and - // at the top in descending — consistent with monthly_cost. The base value - // is monthly; scale it to the selected display period so sorting matches - // what the user sees in the column. + // onDemandMonthly returns null when the provider didn't report + // on_demand_cost (or reported 0). POSITIVE_INFINITY is the null sentinel; + // groupsInSortOrder keeps nullish rows last in both ascending and descending + // sorts. The base value is monthly; scale it to the selected display period + // so sorting matches what the user sees in the column. on_demand_monthly: (r) => { const period = state.getCostPeriod(); return scaleCost(onDemandMonthly(r), period) ?? Number.POSITIVE_INFINITY; @@ -752,23 +752,23 @@ export function effectiveSavingsPct(r: LocalRecommendation): number | null { } /** - * Computes the equivalent on-demand monthly cost by reversing the savings - * formula: on_demand_monthly = monthly_cost + savings + (upfront_cost / (term * 12)). - * Returns null when monthly_cost is null (provider didn't return a recurring - * breakdown — same semantics as the Monthly Cost column and effectiveSavingsPct). - * Returns null when term is 0 (anomaly — cannot amortize over zero months). + * Returns the provider-reported on-demand monthly cost (`r.on_demand_cost`) + * directly. Issue #330 — earlier behaviour reconstructed the value from + * `monthly_cost + savings + amortized_upfront`, which drifted from the + * provider's billed price for rounding edge cases (Azure all-upfront RIs, + * AWS Capacity Reservation discounts, partial-day proration). Aligning this + * with `effectiveSavingsPct`'s `hasOnDemand` branch makes the column show + * the same denominator the percentage column uses, so the two never disagree. * - * Note: this reconstruction uses only monthly_cost + savings + amortized_upfront. - * It does NOT use on_demand_cost even when available, because 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. For the authoritative - * on-demand baseline used in effectiveSavingsPct, see that function. + * Returns `null` when `on_demand_cost` is missing, undefined, or `0` — + * cell renders `—` (same em-dash convention as the existing Monthly Cost + * column for null `monthly_cost`). */ export function onDemandMonthly(r: LocalRecommendation): number | null { - if (r.monthly_cost == null) return null; - if (!r.term) return null; - const amortized = r.upfront_cost / (r.term * 12); - return r.monthly_cost + r.savings + amortized; + if (r.on_demand_cost != null && r.on_demand_cost > 0) { + return r.on_demand_cost; + } + return null; } // --------------------------------------------------------------------------- From 6244beb6176167ab6a8952cc5f833e80204365bc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 7 May 2026 15:48:15 +0200 Subject: [PATCH 2/2] fix(frontend/recs): cover explicit-undefined + freshen filter comment (CR pass on PR #331) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- frontend/src/__tests__/recommendations.test.ts | 11 ++++++++++- frontend/src/recommendations.ts | 7 ++++--- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/frontend/src/__tests__/recommendations.test.ts b/frontend/src/__tests__/recommendations.test.ts index a9ef68d4..64625c7b 100644 --- a/frontend/src/__tests__/recommendations.test.ts +++ b/frontend/src/__tests__/recommendations.test.ts @@ -3533,7 +3533,16 @@ describe('onDemandMonthly', () => { expect(onDemandMonthly(mk({ on_demand_cost: null }))).toBeNull(); }); - test('undefined / missing on_demand_cost returns null', () => { + test('explicit undefined on_demand_cost returns null', () => { + // TypeScript's strict mode lets callers pass `undefined` rather than + // omitting the field; assert that branch explicitly so a future + // `r.on_demand_cost > 0` guard regression doesn't accept undefined. + expect(onDemandMonthly(mk({ on_demand_cost: undefined }))).toBeNull(); + }); + + test('missing on_demand_cost field returns null', () => { + // Distinct from the explicit-undefined case: the property never + // existed on the object (e.g. legacy cached recs from before #277). const r = mk({}); delete (r as { on_demand_cost?: unknown }).on_demand_cost; expect(onDemandMonthly(r)).toBeNull(); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 5705d959..b45f9f46 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -1082,9 +1082,10 @@ function numericCellValue(r: LocalRecommendation, col: state.RecommendationsColu // Return NaN for null monthly_cost so numeric filter predicates (e.g. "= 0") // don't match rows where the provider simply didn't report a monthly cost. case 'monthly_cost': return scaleCost(r.monthly_cost, period) ?? Number.NaN; - // Return NaN for null on_demand_monthly (null monthly_cost or term=0) so any - // numeric predicate returns false rather than coincidentally matching 0. - // Scale to the active period so a numeric filter targets what the user sees. + // Return NaN for null on_demand_monthly (missing or zero on_demand_cost — see + // onDemandMonthly() for the contract) so any numeric predicate returns false + // rather than coincidentally matching 0. Scale to the active period so a + // numeric filter targets what the user sees. case 'on_demand_monthly': return scaleCost(onDemandMonthly(r), period) ?? Number.NaN; // Return NaN for null effective_savings_pct so any numeric predicate // returns false rather than coincidentally matching 0.