diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index c24e2c735..2ee396771 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -1391,25 +1391,44 @@ describe('Dashboard Module', () => { expect(chartData.data.labels).toContain('rds'); }); - test('bar floor dataset uses min potential, upside dataset uses (max - min)', () => { + test('three stacked datasets: Current/Committed (bottom), Lowest option, Upside (top)', () => { buildDOM(); - // ec2: two recs with savings 100 and 400 -> floor=100, upside=300. + // ec2: min rec=100, max rec=400, current=0 -> lowestOption=100, upside=300. renderSavingsByService([ rec('ec2', 100, 1, 'no_upfront'), rec('ec2', 400, 3, 'all_upfront'), ]); const chartCtor = Chart as unknown as jest.Mock; const lastCall = chartCtor.mock.calls[chartCtor.mock.calls.length - 1]; - const datasets = (lastCall?.[1] as { data: { datasets: { label: string; data: number[] }[] } }).data.datasets; - const floorDs = datasets.find((d) => d.label === 'Min potential'); - const rangeDs = datasets.find((d) => d.label === 'Upside'); - expect(floorDs?.data[0]).toBe(100); // min potential savings - expect(rangeDs?.data[0]).toBe(300); // max - min - }); - - // Issue #908: merged chart draws a current-savings underlay per service. - test('renders a current (committed) dataset from byService.current_savings', () => { + const datasets = (lastCall?.[1] as { data: { datasets: { label: string; data: number[]; stack: string }[] } }).data.datasets; + const currentDs = datasets.find((d) => d.label === 'Current / Committed'); + const lowestDs = datasets.find((d) => d.label === 'Lowest option'); + const upsideDs = datasets.find((d) => d.label === 'Upside'); + // All three datasets must be present and in the same stack. + expect(currentDs).toBeDefined(); + expect(lowestDs).toBeDefined(); + expect(upsideDs).toBeDefined(); + expect(currentDs?.stack).toBe('savings'); + expect(lowestDs?.stack).toBe('savings'); + expect(upsideDs?.stack).toBe('savings'); + // Values: current=0 (no byService supplied), lowestOption=100-0=100, upside=400-100=300. + expect(currentDs?.data[0]).toBe(0); + expect(lowestDs?.data[0]).toBe(100); + expect(upsideDs?.data[0]).toBe(300); + // Tooltip Total must equal the sum of the three visible bar layers (0+100+300=400). + const tooltipLabel = (lastCall?.[1] as { + options: { plugins: { tooltip: { callbacks: { label: (ctx: { label: string }) => string[] } } } }; + }).options.plugins.tooltip.callbacks.label({ label: 'ec2' }); + const totalLine = tooltipLabel.find((l) => l.startsWith('Total:')); + expect(totalLine).toMatch(/\$400/); + }); + + // Issue #908: merged chart draws a current-savings bottom layer per service. + test('renders Current / Committed layer from byService.current_savings', () => { buildDOM(); + // current=250, min rec=100, max rec=400 + // lowestOption = max(0, 100-250) = 0 (committed already exceeds min rec) + // upside = max(0, 400-100) = 300 renderSavingsByService( [rec('ec2', 100, 1, 'no_upfront'), rec('ec2', 400, 3, 'all_upfront')], { ec2: { potential_savings: 400, current_savings: 250 } }, @@ -1417,16 +1436,32 @@ describe('Dashboard Module', () => { const chartCtor = Chart as unknown as jest.Mock; const lastCall = chartCtor.mock.calls[chartCtor.mock.calls.length - 1]; const datasets = (lastCall?.[1] as { - data: { datasets: { label: string; data: number[]; backgroundColor: string[]; stack: string }[] }; + data: { datasets: { label: string; data: number[]; stack: string }[] }; }).data.datasets; - const currentDs = datasets.find((d) => d.label === 'Current (committed)'); + const currentDs = datasets.find((d) => d.label === 'Current / Committed'); expect(currentDs).toBeDefined(); expect(currentDs?.data[0]).toBe(250); - // Current bar lives in its own stack so it sits beside the potential range. - expect(currentDs?.stack).toBe('current'); + // Current layer is in the unified savings stack. + expect(currentDs?.stack).toBe('savings'); + }); + + test('Lowest option clamps to 0 when committed savings exceed min rec', () => { + buildDOM(); + // current=300, min rec=100 -> lowestOption = max(0, 100-300) = 0 + renderSavingsByService( + [rec('ec2', 100, 1, 'no_upfront'), rec('ec2', 400, 3, 'all_upfront')], + { ec2: { potential_savings: 400, current_savings: 300 } }, + ); + const chartCtor = Chart as unknown as jest.Mock; + const lastCall = chartCtor.mock.calls[chartCtor.mock.calls.length - 1]; + const datasets = (lastCall?.[1] as { + data: { datasets: { label: string; data: number[] }[] }; + }).data.datasets; + const lowestDs = datasets.find((d) => d.label === 'Lowest option'); + expect(lowestDs?.data[0]).toBe(0); }); - test('current underlay uses a DARKER variant of each service potential hue', () => { + test('current underlay uses a DARKER variant of each service base hue', () => { buildDOM(); renderSavingsByService( [rec('ec2', 500)], @@ -1437,16 +1472,16 @@ describe('Dashboard Module', () => { const datasets = (lastCall?.[1] as { data: { datasets: { label: string; backgroundColor: string[] }[] }; }).data.datasets; - const floorColor = datasets.find((d) => d.label === 'Min potential')?.backgroundColor[0] as string; - const currentColor = datasets.find((d) => d.label === 'Current (committed)')?.backgroundColor[0] as string; - // The current colour is the darkened form of the floor (potential) colour. - expect(currentColor).toBe(darkenHexColor(floorColor)); + const lowestOptionColor = datasets.find((d) => d.label === 'Lowest option')?.backgroundColor[0] as string; + const currentColor = datasets.find((d) => d.label === 'Current / Committed')?.backgroundColor[0] as string; + // The current colour is the darkened form of the base (lowest-option) colour. + expect(currentColor).toBe(darkenHexColor(lowestOptionColor)); // And it is genuinely darker: each channel sum is lower. const sum = (hex: string): number => { const c = parseHexColor(hex); return c.r + c.g + c.b; }; - expect(sum(currentColor)).toBeLessThan(sum(floorColor)); + expect(sum(currentColor)).toBeLessThan(sum(lowestOptionColor)); }); - test('current bar defaults to 0 for a service absent from byService', () => { + test('current layer defaults to 0 for a service absent from byService', () => { buildDOM(); renderSavingsByService([rec('rds', 300)], {}); // no rds entry const chartCtor = Chart as unknown as jest.Mock; @@ -1454,18 +1489,44 @@ describe('Dashboard Module', () => { const datasets = (lastCall?.[1] as { data: { datasets: { label: string; data: number[] }[] }; }).data.datasets; - const currentDs = datasets.find((d) => d.label === 'Current (committed)'); + const currentDs = datasets.find((d) => d.label === 'Current / Committed'); expect(currentDs?.data[0]).toBe(0); }); - test('services are sorted by max potential savings descending', () => { + test('service present in byService but absent from recs renders Current-only bar', () => { buildDOM(); - // ec2: max=200, rds: max=500, lambda: max=50 -- expected order: rds, ec2, lambda. - renderSavingsByService([ - rec('ec2', 200), - rec('rds', 500), - rec('lambda', 50), - ]); + // No recs for 'lambda'; only byService entry -> single Current band, no Lowest/Upside. + renderSavingsByService( + [], + { lambda: { potential_savings: 0, current_savings: 120 } }, + ); + const canvas = document.getElementById('savings-by-service-chart'); + expect(canvas?.classList.contains('hidden')).toBe(false); + const chartCtor = Chart as unknown as jest.Mock; + const lastCall = chartCtor.mock.calls[chartCtor.mock.calls.length - 1]; + const labels = (lastCall?.[1] as { data: { labels: string[] } }).data.labels; + expect(labels).toContain('lambda'); + const datasets = (lastCall?.[1] as { + data: { datasets: { label: string; data: number[] }[] }; + }).data.datasets; + const currentDs = datasets.find((d) => d.label === 'Current / Committed'); + const lowestDs = datasets.find((d) => d.label === 'Lowest option'); + const upsideDs = datasets.find((d) => d.label === 'Upside'); + expect(currentDs?.data[0]).toBe(120); + expect(lowestDs?.data[0]).toBe(0); + expect(upsideDs?.data[0]).toBe(0); + }); + + test('services are sorted by visible total (current + lowestOption + upside) descending', () => { + buildDOM(); + // ec2: current=0, minRec=200, maxRec=200 -> lowestOption=200, upside=0, visibleTotal=200 + // rds: current=150, minRec=500, maxRec=500 -> lowestOption=350, upside=0, visibleTotal=500 + // lambda: current=0, minRec=50, maxRec=50 -> lowestOption=50, upside=0, visibleTotal=50 + // Expected order: rds (500), ec2 (200), lambda (50). + renderSavingsByService( + [rec('ec2', 200), rec('rds', 500), rec('lambda', 50)], + { rds: { potential_savings: 500, current_savings: 150 } }, + ); const chartCtor = Chart as unknown as jest.Mock; const lastCall = chartCtor.mock.calls[chartCtor.mock.calls.length - 1]; const labels = (lastCall?.[1] as { data: { labels: string[] } }).data.labels; @@ -1586,10 +1647,13 @@ describe('Dashboard Module', () => { (chartCtor.mock.calls[chartCtor.mock.calls.length - 1]?.[1] as { data: { labels: string[] } }).data; expect(lastChartData().labels).toEqual(['ec2']); - // Filter changes -> new recs -> second load must re-render with rds. + // Filter changes -> new recs -> second load must re-render. rds appears + // from the new recs (total=220); ec2 still appears from byService + // current_savings=90 even without a rec in this load. rds sorts first. (api.getRecommendations as jest.Mock).mockResolvedValue([rec('rds', 220)]); await loadDashboard(); - expect(lastChartData().labels).toEqual(['rds']); + expect(lastChartData().labels).toContain('rds'); + expect(lastChartData().labels[0]).toBe('rds'); }); }); diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index 01c4ae89c..ffb694564 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -710,26 +710,26 @@ export function computeServiceStatsFromRecs( /** * Render the single merged per-service savings chart (issues #769 + #908). * - * Each service category shows two grouped bars: - * - Potential range (stacked): "Min potential" (solid hue) + "Upside" - * (same hue at 35% opacity, = max - min), derived from the - * recommendations list via computeServiceStatsFromRecs. - * - Current (committed) savings (single bar) in a DARKER shade of the - * same per-service hue, sourced from the dashboard summary's - * by_service[svc].current_savings (now populated by the backend, #908). - * This reads as the already-realized savings sitting beneath the - * potential range of the same colour family. + * Each service bar is a single stack with three layers (bottom to top): + * 1. Current / Committed (darkest shade): already-realized savings from + * active commitments, sourced from by_service[svc].current_savings. + * 2. Lowest option (solid mid-shade): the lowest available recommendation + * option minus what is already committed, i.e. max(0, min(rec.savings) - + * current). Together with the Current layer this represents the floor. + * 3. Upside (lightest / 35% opacity): max(rec.savings) - min(rec.savings), + * the variability between the cheapest and most-aggressive option. * - * This replaces the old standalone grouped "Potential Savings by Service" - * chart (renderSavingsChart) which has been removed; the current-vs-potential - * comparison now lives entirely in this one range chart. + * Services are sorted by (current + max potential) descending so the bar + * with the most total visible value is leftmost. Services present in + * byService but absent from recs still appear with only the Current band + * (no further upside surfaced). Services present in recs but absent from + * byService default to current = 0 (no committed savings yet). * - * Services are sorted by max potential savings descending; the top - * SAVINGS_BY_SERVICE_MAX are shown. When truncated, the section heading notes - * "+N more". + * The top SAVINGS_BY_SERVICE_MAX services are shown; the heading notes + * "+N more" when truncated. * - * Empty state: when no recommendations have positive savings, the canvas is - * hidden and the empty-state paragraph is shown. + * Empty state: shown when no bar (across all three layers) has any positive + * value. */ export function renderSavingsByService( recs: readonly LocalRecommendation[], @@ -750,9 +750,46 @@ export function renderSavingsByService( const stats = computeServiceStatsFromRecs(recs); const heading = section?.querySelector('h3'); - // Filter to services with positive savings, then sort by max desc. - const positive = Array.from(stats.entries()).filter(([, s]) => s.max > 0); - positive.sort((a, b) => b[1].max - a[1].max); + // Build the union of services from both recs and byService. Services in + // byService-only (commitment exists, no new rec) still show a Current band. + const allServices = new Set([ + ...stats.keys(), + ...Object.keys(byService), + ]); + + // Per-service totals used for sort and empty-state detection. + type SvcEntry = { + svc: string; + current: number; + minRec: number; + maxRec: number; + lowestOption: number; + upside: number; + visibleTotal: number; + }; + const entries: SvcEntry[] = Array.from(allServices).map(svc => { + const s = stats.get(svc); + const current = byService[svc]?.current_savings ?? 0; + const minRec = s?.min ?? 0; + const maxRec = s?.max ?? 0; + const lowestOption = Math.max(0, minRec - current); + const upside = Math.max(0, maxRec - minRec); + return { + svc, + current, + minRec, + maxRec, + lowestOption, + upside, + visibleTotal: current + lowestOption + upside, + }; + }); + + // Keep only services with some positive value in any layer. + const positive = entries.filter(e => e.visibleTotal > 0); + + // Sort by visible total desc so the most-valuable bar is leftmost. + positive.sort((a, b) => b.visibleTotal - a.visibleTotal); if (positive.length === 0) { if (heading) heading.textContent = 'Potential savings range per service'; @@ -780,26 +817,35 @@ export function renderSavingsByService( : 'Potential savings range per service'; } - const labels = visible.map(([svc]) => svc); - const floorData = visible.map(([, s]) => s.min); - const rangeData = visible.map(([, s]) => s.max - s.min); - // Current (committed) savings per service from the summary's by_service map. - // Defaults to 0 for services the summary doesn't carry (the bar collapses, - // which is the correct "nothing committed yet" visual). - const currentData = visible.map(([svc]) => byService[svc]?.current_savings ?? 0); - const totalSavings = Array.from(stats.values()).reduce((acc, s) => acc + s.max, 0); + const labels = visible.map(e => e.svc); + + // Layer 1 (bottom): Current / Committed savings. + const currentData = visible.map(e => e.current); + + // Layer 2: Lowest option -- the gap between the cheapest rec and the already- + // committed amount. Clamped to 0 to avoid negative segments when commitments + // exceed the current lowest-option savings figure. + const lowestOptionData = visible.map(e => Math.max(0, e.minRec - e.current)); - // Assign a colour per service (cycles if more than palette length). + // Layer 3 (top): Upside -- the spread between cheapest and most-aggressive option. + const upsideData = visible.map(e => Math.max(0, e.maxRec - e.minRec)); + + const totalSavings = positive.reduce((acc, e) => acc + e.visibleTotal, 0); + + // Colour palette: one base colour per service, then derive lighter/darker shades. const bgColors = visible.map((_, i) => SERVICE_BAR_COLORS[i % SERVICE_BAR_COLORS.length] ?? '#1a73e8'); - const rangeColors = bgColors.map(c => { - // Convert solid hex to rgba at 0.35 opacity for the range segment. + + // Current / Committed: darkest shade (30% darker than base). + const currentColors = bgColors.map(c => darkenHexColor(c)); + + // Lowest option: the base (solid) colour. + const lowestOptionColors = bgColors; + + // Upside: same hue at 35% opacity for the variability band. + const upsideColors = bgColors.map(c => { const { r, g, b } = parseHexColor(c); return `rgba(${r},${g},${b},0.35)`; }); - // Current-savings bar uses a darker variant of the same hue, derived - // programmatically (not hardcoded per service) so the "already-realized" - // bar always matches its service's colour family (#908). - const currentColors = bgColors.map(c => darkenHexColor(c)); savingsByServiceChart = new Chart(canvas, { type: 'bar', @@ -807,25 +853,25 @@ export function renderSavingsByService( labels, datasets: [ { - label: 'Min potential', - data: floorData, - backgroundColor: bgColors, + label: 'Current / Committed', + data: currentData, + backgroundColor: currentColors, borderRadius: { topLeft: 0, topRight: 0, bottomLeft: 4, bottomRight: 4 }, - stack: 'potential', + stack: 'savings', }, { - label: 'Upside', - data: rangeData, - backgroundColor: rangeColors, - borderRadius: { topLeft: 4, topRight: 4, bottomLeft: 0, bottomRight: 0 }, - stack: 'potential', + label: 'Lowest option', + data: lowestOptionData, + backgroundColor: lowestOptionColors, + borderRadius: { topLeft: 0, topRight: 0, bottomLeft: 0, bottomRight: 0 }, + stack: 'savings', }, { - label: 'Current (committed)', - data: currentData, - backgroundColor: currentColors, - borderRadius: 4, - stack: 'current', + label: 'Upside', + data: upsideData, + backgroundColor: upsideColors, + borderRadius: { topLeft: 4, topRight: 4, bottomLeft: 0, bottomRight: 0 }, + stack: 'savings', }, ], }, @@ -843,23 +889,22 @@ export function renderSavingsByService( label: (ctx) => { const svc = ctx.label ?? ''; const s = stats.get(svc); - if (!s) return ''; - // The current-savings dataset gets a focused, single-line - // tooltip; the range datasets share the full breakdown. - if (ctx.dataset.stack === 'current') { - const current = byService[svc]?.current_savings ?? 0; - return `Current (committed): $${current.toLocaleString()}`; - } - const pct = totalSavings > 0 ? ((s.max / totalSavings) * 100).toFixed(1) : '0.0'; + const current = byService[svc]?.current_savings ?? 0; + const maxRec = s?.max ?? 0; + const minRec = s?.min ?? 0; + const lowestOption = Math.max(0, minRec - current); + const upside = Math.max(0, maxRec - minRec); + const total = current + lowestOption + upside; + const pct = totalSavings > 0 ? ((total / totalSavings) * 100).toFixed(1) : '0.0'; const lines = [ `Service: ${svc}`, - `Min potential: $${s.min.toLocaleString()}`, - `Max potential: $${s.max.toLocaleString()}`, - `Options: ${s.count}`, - `% of total: ${pct}%`, + `Total: $${total.toLocaleString()} (${pct}% of all services)`, + `Current / Committed: $${current.toLocaleString()}`, + `Lowest option: $${lowestOption.toLocaleString()}`, + `Upside: $${upside.toLocaleString()}`, ]; - if (s.minLabel) lines.push(`Min option: ${s.minLabel}`); - if (s.maxLabel) lines.push(`Max option: ${s.maxLabel}`); + if (s?.minLabel) lines.push(`Min option: ${s.minLabel}`); + if (s?.maxLabel) lines.push(`Max option: ${s.maxLabel}`); return lines; }, },