From 5ccc301e242e567af0c843f98c238e46a38fb790 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 20:26:29 +0200 Subject: [PATCH] fix(dashboard): merge per-service savings charts and populate current_savings (closes #908) Backend: summarizeRecommendationsWithCoverage now sets ServiceSavings.CurrentSavings (previously never written, so the Home chart's current series was always $0). It reuses scaledSavings, the same coverage-scaled amount used for the per-service potential, so current is keyed and scaled identically. Adds a regression test asserting non-zero current_savings where coverage exists. Frontend: collapses the two Home per-service charts into one. The old standalone "Potential Savings by Service" grouped chart (renderSavingsChart) and its #savings-chart container are removed. The surviving range chart now draws each service's current (committed) savings as a separate bar in a programmatically-darkened shade of that service's potential hue (darkenHexColor, ~30% darker per channel, hue preserved). Preserves the top-N "+N more" cap, chip filter re-render, empty/zero states, escaped axis/legend labels, and chart dedup. --- frontend/src/__tests__/dashboard.test.ts | 168 +++++++++++++++++--- frontend/src/__tests__/html.test.ts | 16 +- frontend/src/dashboard.ts | 189 +++++++++++------------ frontend/src/index.html | 6 +- internal/api/handler_dashboard.go | 11 ++ internal/api/handler_dashboard_test.go | 43 ++++++ 6 files changed, 301 insertions(+), 132 deletions(-) diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index 58d186c74..c24e2c735 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -105,7 +105,10 @@ describe('Dashboard Module', () => { // Reset DOM document.body.innerHTML = `
- +

Potential savings range per service

+ + +
`; @@ -174,7 +177,7 @@ describe('Dashboard Module', () => { expect(summary?.innerHTML).toContain('YTD Savings'); }); - test('renders savings chart', async () => { + test('renders the merged per-service savings chart', async () => { (api.getDashboardSummary as jest.Mock).mockResolvedValue({ potential_monthly_savings: 1000, by_service: { @@ -185,28 +188,37 @@ describe('Dashboard Module', () => { (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + // Range chart is driven by recs; supply two ec2 options + one rds. + (api.getRecommendations as jest.Mock).mockResolvedValue([ + { service: 'ec2', savings: 400, term: 1, payment: 'no-upfront' }, + { service: 'ec2', savings: 600, term: 3, payment: 'all-upfront' }, + { service: 'rds', savings: 300, term: 1, payment: 'no-upfront' }, + ]); await loadDashboard(); expect(Chart).toHaveBeenCalled(); - expect(state.setSavingsChart).toHaveBeenCalled(); }); - test('destroys existing chart before creating new one', async () => { - const mockChart = { destroy: jest.fn() }; - (state.getSavingsChart as jest.Mock).mockReturnValue(mockChart); - + test('destroys existing merged chart before creating new one', async () => { (api.getDashboardSummary as jest.Mock).mockResolvedValue({ potential_monthly_savings: 1000, - by_service: {} + by_service: { ec2: { potential_savings: 500, current_savings: 200 } } }); (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + (api.getRecommendations as jest.Mock).mockResolvedValue([ + { service: 'ec2', savings: 500, term: 1, payment: 'no-upfront' }, + ]); + // First render builds a chart; second render must destroy it. + await loadDashboard(); + const results = (Chart as unknown as jest.Mock).mock.results; + const firstChart = results[results.length - 1]?.value as { destroy: jest.Mock }; await loadDashboard(); - expect(mockChart.destroy).toHaveBeenCalled(); + expect(firstChart.destroy).toHaveBeenCalled(); }); test('renders upcoming purchases', async () => { @@ -700,7 +712,7 @@ describe('Dashboard Module', () => { }); // #304: summaryData.by_service missing entirely (null/undefined from - // backend). renderSavingsChart receives `undefined || {}` = {} which + // backend). renderSavingsByService receives `undefined || {}` = {} which // is safe; verify no throw and the error banner does not appear. test('#304: summaryData missing by_service field does not throw', async () => { (api.getDashboardSummary as jest.Mock).mockResolvedValue({ @@ -1134,10 +1146,12 @@ describe('Dashboard Module', () => { // Import the public helpers from dashboard. The module is already // loaded above via the jest.mock chain, so we can import directly. // eslint-disable-next-line @typescript-eslint/no-var-requires - const { renderSavingsByService, computeServiceStats, computeServiceStatsFromRecs } = require('../dashboard') as { - renderSavingsByService: (recs: unknown[], filterDesc?: string) => void; + const { renderSavingsByService, computeServiceStats, computeServiceStatsFromRecs, darkenHexColor, parseHexColor } = require('../dashboard') as { + renderSavingsByService: (recs: unknown[], byService?: Record, filterDesc?: string) => void; computeServiceStats: (dataPoints: unknown[]) => Map; computeServiceStatsFromRecs: (recs: unknown[]) => Map; + darkenHexColor: (hex: string, factor?: number) => string; + parseHexColor: (hex: string) => { r: number; g: number; b: number }; }; function buildDOM(): void { @@ -1393,6 +1407,57 @@ describe('Dashboard Module', () => { 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', () => { + buildDOM(); + renderSavingsByService( + [rec('ec2', 100, 1, 'no_upfront'), rec('ec2', 400, 3, 'all_upfront')], + { ec2: { potential_savings: 400, current_savings: 250 } }, + ); + 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; + 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'); + }); + + test('current underlay uses a DARKER variant of each service potential hue', () => { + buildDOM(); + renderSavingsByService( + [rec('ec2', 500)], + { ec2: { potential_savings: 500, current_savings: 200 } }, + ); + 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; 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)); + // 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)); + }); + + test('current bar 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; + const lastCall = chartCtor.mock.calls[chartCtor.mock.calls.length - 1]; + const datasets = (lastCall?.[1] as { + data: { datasets: { label: string; data: number[] }[] }; + }).data.datasets; + const currentDs = datasets.find((d) => d.label === 'Current (committed)'); + expect(currentDs?.data[0]).toBe(0); + }); + test('services are sorted by max potential savings descending', () => { buildDOM(); // ec2: max=200, rds: max=500, lambda: max=50 -- expected order: rds, ec2, lambda. @@ -1429,7 +1494,7 @@ describe('Dashboard Module', () => { // Issue #867: filter-aware empty state. test('empty state shows generic text when no filter is active', () => { buildDOM(); - renderSavingsByService([], ''); + renderSavingsByService([], {}, ''); const empty = document.getElementById('savings-by-service-empty'); expect(empty?.classList.contains('hidden')).toBe(false); expect(empty?.textContent).toBe('No positive potential savings found for current recommendations.'); @@ -1437,7 +1502,7 @@ describe('Dashboard Module', () => { test('empty state mentions filter when provider chip is active and result is empty', () => { buildDOM(); - renderSavingsByService([], 'AWS'); + renderSavingsByService([], {}, 'AWS'); const empty = document.getElementById('savings-by-service-empty'); expect(empty?.classList.contains('hidden')).toBe(false); expect(empty?.textContent).toContain('AWS'); @@ -1446,7 +1511,7 @@ describe('Dashboard Module', () => { test('empty state mentions filter when account chip is active and result is empty', () => { buildDOM(); - renderSavingsByService([], 'uuid-acct-1'); + renderSavingsByService([], {}, 'uuid-acct-1'); const empty = document.getElementById('savings-by-service-empty'); expect(empty?.classList.contains('hidden')).toBe(false); expect(empty?.textContent).toContain('uuid-acct-1'); @@ -1457,7 +1522,7 @@ describe('Dashboard Module', () => { // First render with data -- chart shown, empty hidden. renderSavingsByService([rec('ec2', 100)]); // Second render with filter-narrowed empty result. - renderSavingsByService([], 'AWS, uuid-acct-2'); + renderSavingsByService([], {}, 'AWS, uuid-acct-2'); const empty = document.getElementById('savings-by-service-empty'); expect(empty?.classList.contains('hidden')).toBe(false); expect(empty?.textContent).toContain('AWS, uuid-acct-2'); @@ -1471,13 +1536,7 @@ describe('Dashboard Module', () => { summaryEl.id = 'summary'; const upcomingEl = document.createElement('div'); upcomingEl.id = 'upcoming-list'; - const savingsChartSection = document.createElement('section'); - savingsChartSection.id = 'savings-chart-section'; - const savingsCanvas = document.createElement('canvas'); - savingsCanvas.id = 'savings-chart'; - savingsChartSection.appendChild(savingsCanvas); document.body.appendChild(summaryEl); - document.body.appendChild(savingsChartSection); document.body.appendChild(upcomingEl); (api.getDashboardSummary as jest.Mock).mockResolvedValue({ @@ -1497,6 +1556,71 @@ describe('Dashboard Module', () => { expect(document.getElementById('savings-by-service-chart')?.classList.contains('hidden')).toBe(false); expect(document.getElementById('savings-by-service-empty')?.classList.contains('hidden')).toBe(true); }); + + // Issue #908: the merged chart must keep honoring the topbar chips. + // loadDashboard re-runs on every chip change (via the state subscribers + // wired in setupDashboardHandlers), so a second load with new filter + // results must re-render the chart with the new data. + test('re-renders the merged chart when the filter changes between loads', async () => { + buildDOM(); + const summaryEl = document.createElement('section'); + summaryEl.id = 'summary'; + const upcomingEl = document.createElement('div'); + upcomingEl.id = 'upcoming-list'; + document.body.appendChild(summaryEl); + document.body.appendChild(upcomingEl); + + (api.getDashboardSummary as jest.Mock).mockResolvedValue({ + potential_monthly_savings: 0, total_recommendations: 1, + active_commitments: 0, committed_monthly: 0, target_coverage: 80, + ytd_savings: 0, by_service: { ec2: { potential_savings: 150, current_savings: 90 } }, + }); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + (api.getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + // First load: AWS/ec2 result. + (api.getRecommendations as jest.Mock).mockResolvedValue([rec('ec2', 150)]); + await loadDashboard(); + const chartCtor = Chart as unknown as jest.Mock; + const lastChartData = (): { labels: string[] } => + (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. + (api.getRecommendations as jest.Mock).mockResolvedValue([rec('rds', 220)]); + await loadDashboard(); + expect(lastChartData().labels).toEqual(['rds']); + }); + }); + + // Issue #908: colour-derivation helpers for the current-savings underlay. + describe('colour helpers (issue #908)', () => { + test('parseHexColor parses #rrggbb', () => { + expect(parseHexColor('#1a73e8')).toEqual({ r: 26, g: 115, b: 232 }); + }); + + test('parseHexColor tolerates a missing leading hash', () => { + expect(parseHexColor('34a853')).toEqual({ r: 52, g: 168, b: 83 }); + }); + + test('parseHexColor falls back to the default blue for malformed input', () => { + expect(parseHexColor('not-a-color')).toEqual({ r: 26, g: 115, b: 232 }); + }); + + test('darkenHexColor returns a strictly darker same-hue colour', () => { + const base = '#34a853'; + const darker = darkenHexColor(base); + expect(darker).toMatch(/^#[0-9a-f]{6}$/); + const sum = (hex: string): number => { const c = parseHexColor(hex); return c.r + c.g + c.b; }; + expect(sum(darker)).toBeLessThan(sum(base)); + // 30% reduction by default (factor 0.7): green channel 168 -> ~118. + expect(parseHexColor(darker).g).toBe(Math.round(168 * 0.7)); + }); + + test('darkenHexColor honours an explicit factor', () => { + // factor 0.5 halves each channel. + expect(darkenHexColor('#646464', 0.5)).toBe('#323232'); + }); }); }); }); diff --git a/frontend/src/__tests__/html.test.ts b/frontend/src/__tests__/html.test.ts index 668253945..cfc350a72 100644 --- a/frontend/src/__tests__/html.test.ts +++ b/frontend/src/__tests__/html.test.ts @@ -135,17 +135,25 @@ describe('HTML Structure', () => { expect(summary).toBeTruthy(); }); - test('has savings chart section', () => { - const chartSection = document.getElementById('savings-chart-section'); + // Issue #908 merged the two per-service charts into one; the standalone + // #savings-chart-section / #savings-chart "Potential Savings by Service" + // chart was removed in favour of the merged range chart below. + test('has the merged per-service savings range section', () => { + const chartSection = document.getElementById('savings-by-service-section'); expect(chartSection).toBeTruthy(); }); - test('has savings chart canvas', () => { - const canvas = document.getElementById('savings-chart'); + test('has the merged per-service savings range canvas', () => { + const canvas = document.getElementById('savings-by-service-chart'); expect(canvas).toBeTruthy(); expect(canvas?.tagName.toLowerCase()).toBe('canvas'); }); + test('no longer renders the old standalone savings chart (issue #908)', () => { + expect(document.getElementById('savings-chart-section')).toBeNull(); + expect(document.getElementById('savings-chart')).toBeNull(); + }); + test('has upcoming purchases section', () => { const upcoming = document.getElementById('upcoming-purchases'); expect(upcoming).toBeTruthy(); diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index abb0689af..01c4ae89c 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -16,8 +16,8 @@ import { showSkeletonTiles, showSkeletonBlock, teardownSkeleton } from './lib/sk // Register Chart.js components Chart.register(...registerables); -// Separate Chart instance for the trend widget so renderSavingsChart's -// state.savingsChart doesn't conflict. +// Separate Chart instance for the trend widget so it doesn't conflict with +// the per-service savings chart instance below. let savingsTrendChart: Chart | null = null; let savingsTrendRange: '7' | '30' | '90' | 'all' = '90'; @@ -42,6 +42,38 @@ const SERVICE_BAR_COLORS = [ '#4caf50', // light-green ]; +/** + * Parse a #rrggbb hex string into its r/g/b components. Falls back to the + * default service bar blue (#1a73e8) for malformed input so the chart never + * renders a NaN colour. Exported for unit testing. + */ +export function parseHexColor(hex: string): { r: number; g: number; b: number } { + const m = /^#?([0-9a-fA-F]{6})$/.exec(hex.trim()); + const clean = m ? m[1]! : '1a73e8'; + return { + r: parseInt(clean.substring(0, 2), 16), + g: parseInt(clean.substring(2, 4), 16), + b: parseInt(clean.substring(4, 6), 16), + }; +} + +/** + * Derive a darker shade of a base #rrggbb colour by reducing its perceived + * lightness. Used for the current-savings underlay so it reads as the + * "already-realized" portion beneath the lighter potential-range bar of the + * same hue (issue #908). The factor (default 0.7 → ~30% darker) is applied + * multiplicatively per channel so the hue is preserved; the result is + * programmatic, not hardcoded per service. Exported for unit testing. + */ +export function darkenHexColor(hex: string, factor = 0.7): string { + const { r, g, b } = parseHexColor(hex); + const scale = (c: number): string => { + const v = Math.max(0, Math.min(255, Math.round(c * factor))); + return v.toString(16).padStart(2, '0'); + }; + return `#${scale(r)}${scale(g)}${scale(b)}`; +} + // In-memory index of the currently-rendered upcoming purchases, keyed by // execution_id. The "View Details" affordance renders from this — the // /api/dashboard/upcoming response already carries every field the @@ -168,12 +200,13 @@ export async function loadDashboard(): Promise { } // Build a human-readable filter description for filter-aware empty states - // on both Home charts. Mirrors the pattern from loadSavingsTrendChart (#747). + // on the Home chart. Mirrors the pattern from loadSavingsTrendChart (#747). const filterDesc = buildFilterDesc(currentProvider, currentAccountIDs); renderDashboardSummary(summaryData!, recs); - renderSavingsChart(summaryData!.by_service || {}, filterDesc); - renderSavingsByService(recs, filterDesc); + // Single merged per-service chart (#908): potential range from recs + + // current-savings underlay from the summary's by_service map. + renderSavingsByService(recs, summaryData!.by_service || {}, filterDesc); renderUpcomingPurchases(upcomingData?.purchases || []); // Load the savings-over-time widget independently -- failure shouldn't // block the rest of the dashboard (e.g. analytics not configured). @@ -340,86 +373,6 @@ function attachSparkline(key: string, values: readonly number[]): void { export const __test__ = { sparklinePoints, attachSparkline, computeServiceStats }; -function renderSavingsChart(byService: Record, filterDesc = ''): void { - const ctx = document.getElementById('savings-chart') as HTMLCanvasElement | null; - if (!ctx) return; - - const labels = Object.keys(byService); - const potentialSavings = labels.map(s => byService[s]?.potential_savings || 0); - const currentSavings = labels.map(s => byService[s]?.current_savings || 0); - - const existingChart = state.getSavingsChart(); - if (existingChart) { - existingChart.destroy(); - state.setSavingsChart(null); - } - - // No data → hide the canvas and render an empty-state message so the - // chart doesn't render with a synthetic $0–$1 y-axis. - // When a filter is active, mention it so the user understands why the - // chart is blank (mirrors the savings-trend empty-state pattern from #747). - const section = ctx.parentElement; - let emptyState = section?.querySelector('.chart-empty'); - if (labels.length === 0) { - ctx.classList.add('hidden'); - const emptyText = filterDesc - ? `No savings data for the selected filter (${filterDesc}).` - : 'No savings data yet. Add accounts and wait for recommendations.'; - if (section && !emptyState) { - emptyState = document.createElement('p'); - emptyState.className = 'chart-empty empty'; - section.appendChild(emptyState); - } - if (emptyState) emptyState.textContent = emptyText; - return; - } - // Data is back — restore the canvas and remove any stale empty state. - ctx.classList.remove('hidden'); - emptyState?.remove(); - - const chart = new Chart(ctx, { - type: 'bar', - data: { - labels: labels, - datasets: [ - { - label: 'Potential Savings', - data: potentialSavings, - backgroundColor: '#fbbc04', - borderRadius: 4 - }, - { - label: 'Current Savings', - data: currentSavings, - backgroundColor: '#34a853', - borderRadius: 4 - } - ] - }, - options: { - responsive: true, - maintainAspectRatio: false, - scales: { - y: { - beginAtZero: true, - ticks: { - callback: (value) => '$' + value.toLocaleString() - } - } - }, - plugins: { - tooltip: { - callbacks: { - label: (context) => `${context.dataset.label}: $${(context.raw as number).toLocaleString()}/mo` - } - } - } - } - }); - - state.setSavingsChart(chart); -} - function renderUpcomingPurchases(purchases: UpcomingPurchase[]): void { const container = document.getElementById('upcoming-list'); if (!container) return; @@ -755,20 +708,34 @@ export function computeServiceStatsFromRecs( /** - * Render the per-service potential-savings range stacked bar chart (issue #769). - * Accepts the recommendations array from loadDashboard. + * 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 bar is split into two stacked datasets: - * - "Min potential" (solid): height = min potential savings across recommendation rows for that service. - * - "Upside" (translucent 35% opacity): height = max - min (the additional potential). + * 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 max potential savings descending; the top SAVINGS_BY_SERVICE_MAX - * are shown. When truncated, the section heading notes "+N more". + * Services are sorted by max potential savings descending; the top + * SAVINGS_BY_SERVICE_MAX are shown. When truncated, the section heading notes + * "+N more". * - * Empty state: when no recommendations are available, the canvas is hidden and - * the empty-state paragraph is shown. + * Empty state: when no recommendations have positive savings, the canvas is + * hidden and the empty-state paragraph is shown. */ -export function renderSavingsByService(recs: readonly LocalRecommendation[], filterDesc = ''): void { +export function renderSavingsByService( + recs: readonly LocalRecommendation[], + byService: Record = {}, + filterDesc = '', +): void { const canvas = document.getElementById('savings-by-service-chart') as HTMLCanvasElement | null; const emptyEl = document.getElementById('savings-by-service-empty'); const section = document.getElementById('savings-by-service-section'); @@ -816,18 +783,23 @@ export function renderSavingsByService(recs: readonly LocalRecommendation[], fil 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); // Assign a colour per service (cycles if more than palette length). 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. - const hex = c.replace('#', ''); - const r = parseInt(hex.substring(0, 2), 16); - const g = parseInt(hex.substring(2, 4), 16); - const b = parseInt(hex.substring(4, 6), 16); + 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', @@ -839,14 +811,21 @@ export function renderSavingsByService(recs: readonly LocalRecommendation[], fil data: floorData, backgroundColor: bgColors, borderRadius: { topLeft: 0, topRight: 0, bottomLeft: 4, bottomRight: 4 }, - stack: 'savings', + stack: 'potential', }, { label: 'Upside', data: rangeData, backgroundColor: rangeColors, borderRadius: { topLeft: 4, topRight: 4, bottomLeft: 0, bottomRight: 0 }, - stack: 'savings', + stack: 'potential', + }, + { + label: 'Current (committed)', + data: currentData, + backgroundColor: currentColors, + borderRadius: 4, + stack: 'current', }, ], }, @@ -865,6 +844,12 @@ export function renderSavingsByService(recs: readonly LocalRecommendation[], fil 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 lines = [ `Service: ${svc}`, @@ -883,10 +868,12 @@ export function renderSavingsByService(recs: readonly LocalRecommendation[], fil scales: { x: { stacked: true, + title: { display: true, text: 'Service' }, }, y: { stacked: true, beginAtZero: true, + title: { display: true, text: 'Monthly savings ($)' }, ticks: { callback: (v) => '$' + (v as number).toLocaleString() }, }, }, diff --git a/frontend/src/index.html b/frontend/src/index.html index 3e4f178a5..2f796c555 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -93,13 +93,9 @@

Savings over time

-
-

Potential Savings by Service

- -

Potential savings range per service

- +
diff --git a/internal/api/handler_dashboard.go b/internal/api/handler_dashboard.go index 1de157228..763a8d969 100644 --- a/internal/api/handler_dashboard.go +++ b/internal/api/handler_dashboard.go @@ -153,6 +153,17 @@ func summarizeRecommendationsWithCoverage( total += scaled svc := byService[rec.Service] svc.PotentialSavings += scaled + // CurrentSavings is the committed/realized monthly savings for the + // service: the full 100%-coverage potential (rec.Savings) projected + // down to the operator-configured coverage %. scaledSavings already + // computes exactly that (rec.Savings * min(coverage,100)/100), so we + // reuse it rather than re-deriving the coverage lookup. When no + // coverage override exists the rec falls through to full savings, so + // CurrentSavings == PotentialSavings (nothing committed-away yet); a + // configured coverage < 100 pulls CurrentSavings below the potential. + // Issue #908: this field was previously never set, so the Home + // chart's current-savings underlay always rendered as $0. + svc.CurrentSavings += scaled byService[rec.Service] = svc } return total, byService diff --git a/internal/api/handler_dashboard_test.go b/internal/api/handler_dashboard_test.go index 43c673ee5..bc20f205f 100644 --- a/internal/api/handler_dashboard_test.go +++ b/internal/api/handler_dashboard_test.go @@ -255,6 +255,9 @@ func TestSummarizeRecommendationsWithCoverage(t *testing.T) { total, byService := summarizeRecommendationsWithCoverage(tc.recs, tc.coverage) assert.InDelta(t, tc.wantTotal, total, 0.0001) assert.InDelta(t, tc.wantTotal, byService["rds"].PotentialSavings, 0.0001) + // Issue #908: CurrentSavings (committed/realized) is sourced from + // the same coverage-scaled amount, so it tracks the scaled total. + assert.InDelta(t, tc.wantTotal, byService["rds"].CurrentSavings, 0.0001) }) } } @@ -309,6 +312,46 @@ func TestSummarizeRecommendationsWithCoverage_100PctContract(t *testing.T) { "nil coverage map must return un-scaled savings (issue #201 contract)") } +// TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings is the +// issue #908 regression: by_service[svc].current_savings must be populated +// (and keyed/scaled identically to potential) so the Home chart's +// current-savings underlay renders instead of being a flat $0 series. +// +// Before the fix, summarizeRecommendationsWithCoverage set only +// PotentialSavings, leaving CurrentSavings at its float64 zero value for +// every service regardless of configured coverage. +func TestSummarizeRecommendationsWithCoverage_PopulatesCurrentSavings(t *testing.T) { + acctA := "acct-A" + keyEC2 := config.AccountConfigKey(acctA, "aws", "ec2") + keyRDS := config.AccountConfigKey(acctA, "aws", "rds") + + recs := []config.RecommendationRecord{ + {Provider: "aws", Service: "ec2", Savings: 1000.0, CloudAccountID: &acctA}, + {Provider: "aws", Service: "rds", Savings: 400.0, CloudAccountID: &acctA}, + } + + // 60% coverage on ec2, 25% on rds. + coverage := map[string]float64{keyEC2: 60, keyRDS: 25} + + _, byService := summarizeRecommendationsWithCoverage(recs, coverage) + + // current_savings is non-zero where coverage exists and is scaled the + // same way potential is (rec.Savings * coverage/100), keyed per service. + assert.InDelta(t, 600.0, byService["ec2"].CurrentSavings, 0.001, + "ec2 current_savings = 1000 * 60/100") + assert.InDelta(t, 100.0, byService["rds"].CurrentSavings, 0.001, + "rds current_savings = 400 * 25/100") + + // And it matches PotentialSavings (both flow from the same scaled amount). + assert.InDelta(t, byService["ec2"].PotentialSavings, byService["ec2"].CurrentSavings, 0.001) + assert.InDelta(t, byService["rds"].PotentialSavings, byService["rds"].CurrentSavings, 0.001) + + // Sanity: every populated service has a strictly positive current_savings + // when coverage is configured (the bug produced 0 here). + require.Positive(t, byService["ec2"].CurrentSavings) + require.Positive(t, byService["rds"].CurrentSavings) +} + func TestHandler_getUpcomingPurchases(t *testing.T) { ctx := context.Background() mockStore := new(MockConfigStore)