diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index f6efc4b6e..1189b0d3e 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -193,6 +193,54 @@ describe('Dashboard Module', () => { expect(api.getUpcomingPurchases).toHaveBeenCalled(); }); + // #1686: the YTD figure is monthly estimated savings times elapsed months, + // not recorded savings, so its caption must say so. + test('labels YTD Savings as an estimate', async () => { + (api.getDashboardSummary as jest.Mock).mockResolvedValue({ + total_recommendations: 0, active_commitments: 1, committed_monthly: 10, + current_coverage: 50, target_coverage: 80, ytd_savings: 5000, by_service: {}, + }); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + await loadDashboard(); + + const ytd = document.querySelector('#summary [data-kpi="ytd"]') ?? document.getElementById('summary'); + expect(ytd?.textContent).toContain('Estimated from active commitments'); + expect(ytd?.textContent).not.toContain('From commitment purchases'); + }); + + // #1686: the per-service card shares the KPI cards' loading treatment. + test('shows a per-service loading skeleton until the chart renders', async () => { + const loading = document.createElement('div'); + loading.id = 'savings-by-service-loading'; + document.getElementById('savings-by-service-section')?.appendChild(loading); + let resolve!: (v: unknown) => void; + (api.getDashboardSummary as jest.Mock).mockReturnValue(new Promise(r => { resolve = r; })); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + const done = loadDashboard(); + expect(loading.querySelector('.skeleton')).not.toBeNull(); + + resolve({ total_recommendations: 0, active_commitments: 0, by_service: {} }); + await done; + expect(loading.querySelector('.skeleton')).toBeNull(); + }); + + test('clears the per-service loading skeleton when the dashboard fails to load', async () => { + const loading = document.createElement('div'); + loading.id = 'savings-by-service-loading'; + document.getElementById('savings-by-service-section')?.appendChild(loading); + (api.getDashboardSummary as jest.Mock).mockRejectedValue(new Error('boom')); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + + await loadDashboard(); + + expect(loading.querySelector('.skeleton')).toBeNull(); + const empty = document.getElementById('savings-by-service-empty'); + expect(empty?.classList.contains('hidden')).toBe(false); + expect(empty?.textContent).toBe('Potential savings could not be loaded. Reload the page to try again.'); + }); + test('renders dashboard summary', async () => { (api.getDashboardSummary as jest.Mock).mockResolvedValue({ potential_monthly_savings: 1000, @@ -1147,6 +1195,138 @@ describe('Dashboard Module', () => { (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); }); + // #1686: the empty state is about the selected range, not the account's + // whole history, and a failed fetch must not read as "no history". + describe('empty and error states name what happened (#1686)', () => { + const selectRange = async (range: string): Promise => { + const btn = document.createElement('button'); + btn.className = 'trend-range'; + btn.dataset['range'] = range; + document.body.appendChild(btn); + setupSavingsTrendHandlers(); + btn.click(); + await new Promise(r => setTimeout(r, 0)); + }; + const emptyText = (): string => document.getElementById('savings-trend-empty')?.textContent ?? ''; + + test('a short range with no points names the range and points to 1y', async () => { + await selectRange('90'); + expect(emptyText()).toContain('No savings recorded in the last 90 days.'); + expect(emptyText()).toContain('1y'); + expect(emptyText()).not.toContain('No purchase history yet'); + }); + + test('the 1y range with no points names the year and offers no longer range', async () => { + await selectRange('all'); + expect(emptyText()).toContain('No savings recorded in the last year.'); + expect(emptyText()).not.toContain('1y'); + }); + + test('an active filter is named alongside the range', async () => { + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['uuid-acct-3']); + await selectRange('30'); + expect(emptyText()).toContain('No savings recorded in the last 30 days for'); + expect(emptyText()).toContain('uuid-acct-3'); + }); + + test('a fetch error shows an error, not an empty or "not yet" state', async () => { + (api.getSavingsAnalytics as jest.Mock).mockRejectedValue(new Error('503')); + await loadSavingsTrendChart(); + expect(emptyText()).toBe('Savings history could not be loaded. Reload the page to try again.'); + }); + + test('shows a loading skeleton until the fetch settles', async () => { + const loading = document.createElement('div'); + loading.id = 'savings-trend-loading'; + document.body.appendChild(loading); + let resolve!: (v: { data_points: [] }) => void; + (api.getSavingsAnalytics as jest.Mock).mockReturnValue(new Promise(r => { resolve = r; })); + + const done = loadSavingsTrendChart(); + expect(loading.querySelector('.skeleton')).not.toBeNull(); + // The skeleton stands in for the canvas rather than stacking above it. + expect(document.getElementById('savings-trend-chart')?.classList.contains('hidden')).toBe(true); + + resolve({ data_points: [] }); + await done; + expect(loading.querySelector('.skeleton')).toBeNull(); + }); + + const addLoading = (): HTMLElement => { + const loading = document.createElement('div'); + loading.id = 'savings-trend-loading'; + document.body.appendChild(loading); + return loading; + }; + const pending = (): ((v: unknown) => void) => { + let resolve!: (v: unknown) => void; + (api.getSavingsAnalytics as jest.Mock).mockReturnValueOnce(new Promise(r => { resolve = r; })); + return (v) => resolve(v); + }; + const points = [ + { timestamp: '2026-01-01T00:00:00Z', cumulative_savings: 100 }, + { timestamp: '2026-01-02T00:00:00Z', cumulative_savings: 150 }, + ]; + + test('a reload with a chart already drawn keeps the chart and shows no skeleton', async () => { + const loading = addLoading(); + (api.getSavingsAnalytics as jest.Mock).mockResolvedValueOnce({ data_points: points }); + await loadSavingsTrendChart(); + expect(document.getElementById('savings-trend-chart')?.classList.contains('hidden')).toBe(false); + + const resolve = pending(); + const done = loadSavingsTrendChart(); + expect(loading.querySelector('.skeleton')).toBeNull(); + expect(document.getElementById('savings-trend-chart')?.classList.contains('hidden')).toBe(false); + resolve({ data_points: points }); + await done; + }); + + test('an older load settling after a newer one leaves the newer load\'s skeleton', async () => { + const loading = addLoading(); + await loadSavingsTrendChart(); // empty: no chart drawn + const resolveOld = pending(); + const older = loadSavingsTrendChart(); + const resolveNew = pending(); + const newer = loadSavingsTrendChart(); + + resolveOld({ data_points: [] }); + await older; + expect(loading.querySelector('.skeleton')).not.toBeNull(); + + resolveNew({ data_points: [] }); + await newer; + expect(loading.querySelector('.skeleton')).toBeNull(); + }); + + test('the previous range\'s empty text is hidden while the next range loads', async () => { + addLoading(); + await loadSavingsTrendChart(); // empty state shown + expect(document.getElementById('savings-trend-empty')?.classList.contains('hidden')).toBe(false); + + const resolve = pending(); + const done = loadSavingsTrendChart(); + expect(document.getElementById('savings-trend-empty')?.classList.contains('hidden')).toBe(true); + resolve({ data_points: [] }); + await done; + }); + + test('a failed dashboard load clears the trend skeleton and says the load failed', async () => { + const loading = addLoading(); + await loadSavingsTrendChart(); // no chart drawn + (api.getDashboardSummary as jest.Mock).mockRejectedValue(new Error('boom')); + (api.getUpcomingPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); + const summary = document.createElement('div'); + summary.id = 'summary'; + document.body.appendChild(summary); + + await loadDashboard(); + + expect(loading.querySelector('.skeleton')).toBeNull(); + expect(emptyText()).toBe('Savings history could not be loaded. Reload the page to try again.'); + }); + }); + // #747: a slower, older response must not overwrite a newer one. describe('stale-response guard (#747)', () => { type Deferred = { resolve: (v: unknown) => void; reject: (e: unknown) => void }; @@ -1351,7 +1531,7 @@ describe('Dashboard Module', () => { const empty = document.getElementById('savings-trend-empty'); expect(canvas?.classList.contains('hidden')).toBe(true); expect(empty?.classList.contains('hidden')).toBe(false); - expect(empty?.textContent).toContain('No purchase history yet'); + expect(empty?.textContent).toContain('No savings recorded in the last'); }); test('x-axis min/max spans the selected window regardless of data point dates (QA 3.1)', async () => { @@ -1679,7 +1859,7 @@ describe('Dashboard Module', () => { const empty = document.getElementById('savings-trend-empty'); expect(empty?.classList.contains('hidden')).toBe(false); - expect(empty?.textContent).toContain('No purchase history yet'); + expect(empty?.textContent).toContain('No savings recorded in the last'); }); test('empty-state names the provider when a provider filter is active (issue #498, QA 2.3)', async () => { @@ -1696,7 +1876,7 @@ describe('Dashboard Module', () => { expect(empty?.classList.contains('hidden')).toBe(false); // Provider name (uppercased by buildFilterDesc) appears in the message. expect(empty?.textContent).toContain('AWS'); - expect(empty?.textContent).toContain('No savings history for'); + expect(empty?.textContent).toMatch(/No savings recorded in the last (\d+ days|year) for /); }); test('loadSavingsTrendChart forwards the provider chip to the analytics API (issue #498, QA 2.3)', async () => { @@ -2037,9 +2217,13 @@ describe('Dashboard Module', () => { const label = document.getElementById('savings-by-service-chart')?.getAttribute('aria-label') ?? ''; // ec2: 40 current + 60 lowest option + 200 upside = 300; rds: 50. Total 350. expect(label).toContain('2 services'); - expect(label).toContain('$350 in total'); // #1649: the text alternative uses the same names as the axis. expect(label).toContain('Largest: EC2 at $300'); + // #1686: the bar total includes committed savings and each service's + // highest option, so say so instead of a bare "in total" that reads + // as a third "potential savings" figure next to the KPI range. + expect(label).toContain('The bars add up to $350'); + expect(label).toContain('not the Potential Monthly Savings range'); }); test('lists every layer value per service in a "View as table" disclosure', () => { diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index 5785734f4..aeaa158dc 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -16,7 +16,7 @@ import { groupRecsByCell, pageLevelRange, formatSavingsRange, triggerAutoRefresh import { getRecommendationsFreshness } from './api/recommendations'; import { buildFirstRunEmptyState, isScopeFiltered } from './first-run-empty-state'; import { serviceLabel } from './lib/service-labels'; -import { showSkeletonTiles, showSkeletonBlock, teardownSkeleton } from './lib/skeleton'; +import { showSkeletonTiles, showSkeletonBlock, teardownSkeleton, isSkeletonActive } from './lib/skeleton'; import { buildErrorCallout } from './lib/error-callout'; import { filterByEnabledProviders } from './lib/enabled-providers'; import { buildKpiTile } from './lib/kpi-tile'; @@ -62,6 +62,32 @@ const SERVICE_CHART_CHROME_PX = 84; // Ids of the "View as table" text alternatives for the Home charts (#744). const SAVINGS_BY_SERVICE_TABLE_ID = 'savings-by-service-table'; const SAVINGS_TREND_TABLE_ID = 'savings-trend-table'; +const SAVINGS_TREND_LOADING_ID = 'savings-trend-loading'; +const TREND_LOAD_FAILED_TEXT = 'Savings history could not be loaded. Reload the page to try again.'; + +/** + * #1686: put a skeleton where a chart card's canvas will be, but only before + * its first chart is drawn; later reloads keep the current chart on screen. + * The card's renderer un-hides the canvas once it has data. + */ +function showChartSkeleton(loadingId: string, canvasId: string, emptyId: string, chart: Chart | null): HTMLElement | null { + const loadingEl = document.getElementById(loadingId); + if (!loadingEl || chart || isSkeletonActive(loadingEl)) return loadingEl; + document.getElementById(canvasId)?.classList.add('hidden'); + // A previous range's empty-state text must not sit next to this load. + document.getElementById(emptyId)?.classList.add('hidden'); + showSkeletonBlock(loadingEl, '100%', '12rem'); + return loadingEl; +} + +// Shown in a chart card whose data never arrived because the dashboard load +// failed, so the card is not left as a bare heading (#1686). +function showChartLoadFailed(emptyId: string, text: string): void { + const empty = document.getElementById(emptyId); + if (!empty) return; + empty.textContent = text; + empty.classList.remove('hidden'); +} // One colour per stacked layer, the same on every bar, so the legend swatches // are exactly the colours drawn (#189). The x axis already names each service, @@ -160,6 +186,10 @@ export async function loadDashboard(): Promise { if (summaryEl) showSkeletonTiles(summaryEl, 4); const upcomingEl = document.getElementById('upcoming-list'); if (upcomingEl) showSkeletonBlock(upcomingEl, '100%', '6rem'); + // #1686: the two chart cards share the same loading treatment. The trend + // loader clears its own; the per-service one is cleared in finally below. + const trendLoadingEl = showChartSkeleton(SAVINGS_TREND_LOADING_ID, 'savings-trend-chart', 'savings-trend-empty', savingsTrendChart); + const byServiceLoadingEl = showChartSkeleton('savings-by-service-loading', 'savings-by-service-chart', 'savings-by-service-empty', savingsByServiceChart); try { const currentProvider = state.getCurrentProvider(); @@ -275,6 +305,14 @@ export async function loadDashboard(): Promise { // error reads as a fresh fetch in-flight, which is misleading. const upcoming = document.getElementById('upcoming-list'); if (upcoming) teardownSkeleton(upcoming); + // The trend loader never runs on this path, so clear its skeleton here. + if (trendLoadingEl) teardownSkeleton(trendLoadingEl); + if (!savingsTrendChart) showChartLoadFailed('savings-trend-empty', TREND_LOAD_FAILED_TEXT); + if (!savingsByServiceChart) { + showChartLoadFailed('savings-by-service-empty', 'Potential savings could not be loaded. Reload the page to try again.'); + } + } finally { + if (byServiceLoadingEl) teardownSkeleton(byServiceLoadingEl); } } @@ -383,8 +421,9 @@ function renderDashboardSummary( { kpi: 'commitments', title: 'Active Commitments', value: data.active_commitments != null ? String(data.active_commitments) : NO_VALUE, detail: `${formatCurrency(data.committed_monthly)}/mo estimated savings` }, { kpi: 'coverage', title: 'Current Coverage', value: coverageValue, detail: coverageDetail }, + // #1686: monthly estimated savings times elapsed months, not recorded savings. { kpi: 'ytd', title: 'YTD Savings', value: formatCurrency(data.ytd_savings), valueSavings: true, - detail: 'From commitment purchases' }, + detail: 'Estimated from active commitments' }, ]; // Shared with the Opportunities summary (issue #743). buildKpiTile keeps an // absent (NO_VALUE) figure neutral rather than green (issue #739). @@ -1040,8 +1079,10 @@ export function renderSavingsByService( // including any beyond the SAVINGS_BY_SERVICE_MAX bars drawn. const top = positive[0]!; setChartSummary(canvas, - `Stacked bar chart of monthly savings for ${positive.length} service${positive.length === 1 ? '' : 's'}, ` - + `${formatCurrency(totalSavings)} in total. Largest: ${serviceLabel(top.svc)} at ${formatCurrency(top.visibleTotal)}. ` + `Stacked bar chart of monthly savings for ${positive.length} service${positive.length === 1 ? '' : 's'}. ` + + `The bars add up to ${formatCurrency(totalSavings)}: current committed savings plus each service's ` + + 'highest-savings option, so this is not the Potential Monthly Savings range. ' + + `Largest: ${serviceLabel(top.svc)} at ${formatCurrency(top.visibleTotal)}. ` + 'Layers: Current / Committed, Lowest option, Upside. Values are in the table below the chart.'); renderChartTable(canvas, SAVINGS_BY_SERVICE_TABLE_ID, { caption: 'Potential savings range per service (monthly)', @@ -1171,12 +1212,11 @@ export function renderSavingsByService( * a line chart of cumulative savings spanning the full selected window on * the x-axis (QA row 405, step 3.1). When the response is empty (QA 2.3 * supersedes QA 3.1's earlier empty-axes suggestion), the canvas is hidden - * and an empty-state banner is shown: a filter-aware "No savings history - * for " when a provider or account chip is active, otherwise the - * generic "No purchase history yet." A single data point is too sparse to - * draw a trend, so it gets a "Not enough history yet" message carrying that - * point's value instead of a lone dot on a blank canvas (#188). Fetch failures - * hide the canvas and show the "Savings history is not available yet." stub. + * and an empty-state banner names the range (and the active provider or + * account chip): "No savings recorded in the last 90 days." (#1686). A single + * data point is too sparse to draw a trend, so it gets a "Not enough history + * yet" message carrying that point's value instead of a lone dot on a blank + * canvas (#188). Fetch failures hide the canvas and say the load failed. */ export async function loadSavingsTrendChart(): Promise { const canvas = document.getElementById('savings-trend-chart') as HTMLCanvasElement | null; @@ -1185,6 +1225,7 @@ export async function loadSavingsTrendChart(): Promise { // #747: quick range or filter changes overlap requests; only the latest // may paint, or an older response landing last would show the wrong range. const generation = ++savingsTrendGeneration; + const loadingEl = showChartSkeleton(SAVINGS_TREND_LOADING_ID, 'savings-trend-chart', 'savings-trend-empty', savingsTrendChart); const now = new Date(); const nowMs = now.getTime(); @@ -1244,8 +1285,8 @@ export async function loadSavingsTrendChart(): Promise { // Policy (QA 2.3, supersedes QA 3.1 empty-axis approach): // - No data + active account filter: show empty-state with the filter // name so the user understands why the chart is blank. - // - No data + no filter active: show a generic "No purchase history yet" - // message rather than blank axes. + // - No data + no filter active: name the empty range rather than show + // blank axes; it says nothing about older history (#1686). // - Data present: always show the chart (hide empty-state). // The stub is also shown on fetch errors (catch block below). if (points.length === 0) { @@ -1255,9 +1296,9 @@ export async function loadSavingsTrendChart(): Promise { // above), so naming the active filter in the empty-state is accurate // (issue #498). buildFilterDesc returns '' when no filter is active. const filterDesc = buildFilterDesc(provider, accountIDs); - empty.textContent = filterDesc - ? `No savings history for ${filterDesc}.` - : 'No purchase history yet.'; + const rangeDesc = isAllRange ? 'the last year' : `the last ${days} days`; + empty.textContent = `No savings recorded in ${rangeDesc}${filterDesc ? ` for ${filterDesc}` : ''}.` + + (isAllRange ? '' : ' Older savings may show in the 1y range.'); empty.classList.remove('hidden'); } if (savingsTrendChart) { savingsTrendChart.destroy(); savingsTrendChart = null; } @@ -1354,15 +1395,18 @@ export async function loadSavingsTrendChart(): Promise { if (generation !== savingsTrendGeneration) return; // Analytics is Postgres-backed now, but the endpoint still guards with // a 503 when the analytics client isn't wired. Don't break the rest of - // the dashboard — hide the widget and fall back to a neutral message. + // the dashboard — hide the widget and say the load failed, so it does not + // read as "no history" (#1686). console.warn('Savings trend chart unavailable:', err); if (savingsTrendChart) { savingsTrendChart.destroy(); savingsTrendChart = null; } removeChartTable(SAVINGS_TREND_TABLE_ID); canvas.classList.add('hidden'); if (empty) { - empty.textContent = 'Savings history is not available yet.'; + empty.textContent = TREND_LOAD_FAILED_TEXT; empty.classList.remove('hidden'); } + } finally { + if (loadingEl && generation === savingsTrendGeneration) teardownSkeleton(loadingEl); } } diff --git a/frontend/src/index.html b/frontend/src/index.html index a2631a160..346c2147d 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -128,12 +128,14 @@

Savings over time

+
- +

Potential savings range per service

+