diff --git a/frontend/src/__tests__/chart-upsert.test.ts b/frontend/src/__tests__/chart-upsert.test.ts new file mode 100644 index 00000000..922a4366 --- /dev/null +++ b/frontend/src/__tests__/chart-upsert.test.ts @@ -0,0 +1,76 @@ +/** + * Issue #747: charts are updated in place instead of destroyed and rebuilt, + * so the entry animation doesn't replay on every filter or range change. + */ +jest.mock('chart.js', () => { + const MockChart = jest.fn().mockImplementation((canvas: HTMLCanvasElement, config: { type: string; data: unknown; options?: unknown }) => ({ + canvas, + config: { type: config.type }, + data: config.data, + options: config.options, + update: jest.fn(), + destroy: jest.fn(), + })); + return { Chart: MockChart, registerables: [] }; +}); + +import { Chart, type ChartConfiguration } from 'chart.js'; +import { upsertChart } from '../modules/chart-utils'; + +type MockInstance = { canvas: HTMLCanvasElement; data: unknown; options: unknown; update: jest.Mock; destroy: jest.Mock }; + +const config = (value: number, type: 'line' | 'bar' = 'line'): ChartConfiguration => ({ + type, + data: { datasets: [{ label: 'x', data: [value] }] }, + options: { responsive: true }, +}); + +describe('upsertChart', () => { + beforeEach(() => (Chart as unknown as jest.Mock).mockClear()); + + test('creates a chart on first render', () => { + const canvas = document.createElement('canvas'); + const chart = upsertChart(null, canvas, config(1)); + expect(Chart).toHaveBeenCalledTimes(1); + expect((chart as unknown as MockInstance).canvas).toBe(canvas); + }); + + test('updates an existing chart in place without animation', () => { + const canvas = document.createElement('canvas'); + const first = upsertChart(null, canvas, config(1)); + const next = config(2); + const second = upsertChart(first, canvas, next); + + expect(second).toBe(first); + expect(Chart).toHaveBeenCalledTimes(1); + const inst = second as unknown as MockInstance; + expect(inst.data).toBe(next.data); + expect(inst.options).toBe(next.options); + expect(inst.update).toHaveBeenCalledWith('none'); + expect(inst.destroy).not.toHaveBeenCalled(); + }); + + test('recreates the chart when its canvas was replaced', () => { + const first = upsertChart(null, document.createElement('canvas'), config(1)); + const fresh = document.createElement('canvas'); + const second = upsertChart(first, fresh, config(2)); + + expect((first as unknown as MockInstance).destroy).toHaveBeenCalledTimes(1); + expect(second).not.toBe(first); + expect((second as unknown as MockInstance).canvas).toBe(fresh); + expect(Chart).toHaveBeenCalledTimes(2); + }); + + // #749's Purchases series toggle switches between a line and a bar chart; + // Chart.js can't change the chart type of an instance in place. + test('recreates the chart when the chart type changes', () => { + const canvas = document.createElement('canvas'); + const first = upsertChart(null, canvas, config(1, 'bar')); + const second = upsertChart(first, canvas, config(2, 'line')); + + expect((first as unknown as MockInstance).destroy).toHaveBeenCalledTimes(1); + expect((first as unknown as MockInstance).update).not.toHaveBeenCalled(); + expect(second).not.toBe(first); + expect(Chart).toHaveBeenCalledTimes(2); + }); +}); diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index 8996d884..e4038d31 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -1058,6 +1058,84 @@ describe('Dashboard Module', () => { (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); }); + // #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 }; + const deferAnalytics = (): Deferred => { + const d = {} as Deferred; + (api.getSavingsAnalytics as jest.Mock).mockImplementationOnce(() => new Promise((resolve, reject) => { + d.resolve = resolve; d.reject = reject; + })); + return d; + }; + const plotted = (): number[] => { + const calls = (Chart as unknown as jest.Mock).mock.calls; + const cfg = calls[calls.length - 1]?.[1] as { data: { datasets: { data: { y: number }[] }[] } } | undefined; + return cfg?.data.datasets[0]?.data.map((p) => p.y) ?? []; + }; + // Two points: one point is the sparse-data state (#188), which draws no chart. + const point = (y: number) => ({ data_points: [ + { timestamp: '2025-12-31T00:00:00Z', cumulative_savings: 0 }, + { timestamp: '2026-01-01T00:00:00Z', cumulative_savings: y }, + ] }); + + test('an older response landing last is ignored', async () => { + const older = deferAnalytics(); + const newer = deferAnalytics(); + const first = loadSavingsTrendChart(); + const second = loadSavingsTrendChart(); + + newer.resolve(point(200)); + await second; + older.resolve(point(100)); + await first; + + expect(Chart).toHaveBeenCalledTimes(1); + expect(plotted()).toEqual([0, 200]); + }); + + test('an older request failing last does not replace the newer chart with an error', async () => { + const older = deferAnalytics(); + const newer = deferAnalytics(); + const first = loadSavingsTrendChart(); + const second = loadSavingsTrendChart(); + + newer.resolve(point(200)); + await second; + older.reject(new Error('503')); + await first; + + expect(document.getElementById('savings-trend-chart')?.classList.contains('hidden')).toBe(false); + expect(document.getElementById('savings-trend-empty')?.classList.contains('hidden')).toBe(true); + }); + }); + + // #747: a range change updates the existing chart instead of rebuilding it. + test('a second load updates the trend chart in place without animation (#747)', async () => { + const update = jest.fn(); + const destroy = jest.fn(); + (Chart as unknown as jest.Mock).mockImplementationOnce((canvas: HTMLCanvasElement, config: { type: string }) => ({ canvas, config, update, destroy })); + (api.getSavingsAnalytics as jest.Mock) + .mockResolvedValueOnce({ data_points: [ + { timestamp: '2025-12-31T00:00:00Z', cumulative_savings: 0 }, + { timestamp: '2026-01-01T00:00:00Z', cumulative_savings: 100 }, + ] }) + .mockResolvedValueOnce({ data_points: [ + { timestamp: '2025-12-31T00:00:00Z', cumulative_savings: 0 }, + { timestamp: '2026-01-01T00:00:00Z', cumulative_savings: 300 }, + ] }); + + await loadSavingsTrendChart(); + await loadSavingsTrendChart(); + + expect(Chart).toHaveBeenCalledTimes(1); + expect(destroy).not.toHaveBeenCalled(); + expect(update).toHaveBeenCalledWith('none'); + // The trend chart is module state; drop it so later tests start fresh. + (api.getSavingsAnalytics as jest.Mock).mockResolvedValueOnce({ data_points: [] }); + await loadSavingsTrendChart(); + }); + // #744: summary label + table for the trend chart. test('describes the plotted series and lists every point as a table (#744)', async () => { (api.getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [ @@ -1837,6 +1915,24 @@ describe('Dashboard Module', () => { // renderSavingsByService DOM behaviour tests (now driven by recommendation fixtures). describe('DOM behaviour', () => { + // #747: re-rendering with new data updates the chart in place. + test('re-rendering updates the existing chart instead of rebuilding it (#747)', () => { + buildDOM(); + const update = jest.fn(); + const destroy = jest.fn(); + (Chart as unknown as jest.Mock).mockImplementationOnce((canvas: HTMLCanvasElement, config: { type: string }) => ({ canvas, config, update, destroy })); + renderSavingsByService([rec('ec2', 100)]); + renderSavingsByService([rec('ec2', 100), rec('rds', 50)]); + + expect(Chart).toHaveBeenCalledTimes(1); + expect(destroy).not.toHaveBeenCalled(); + expect(update).toHaveBeenCalledWith('none'); + + // The empty state still tears the chart down. + renderSavingsByService([]); + expect(destroy).toHaveBeenCalledTimes(1); + }); + // #744: the role="img" canvas gets a value summary and a table. describe('text alternative (#744)', () => { const tableRows = (): string[][] => diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index 96999e1e..84b75750 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -18,7 +18,7 @@ import { buildFirstRunEmptyState, isScopeFiltered } from './first-run-empty-stat import { showSkeletonTiles, showSkeletonBlock, teardownSkeleton } from './lib/skeleton'; import { buildKpiTile } from './lib/kpi-tile'; import { scopeUpcomingToProvider, upcomingEmptyMessage, UPCOMING_ACCOUNT_SCOPE_NOTE } from './lib/upcoming-scope'; -import { formatTrendAxisTick, formatTrendTooltipTitle, formatSparseTrendMessage, MIN_TREND_POINTS } from './modules/chart-utils'; +import { formatTrendAxisTick, formatTrendTooltipTitle, formatSparseTrendMessage, MIN_TREND_POINTS, upsertChart } from './modules/chart-utils'; import { setChartSummary, renderChartTable, removeChartTable } from './lib/chart-a11y'; // Re-export for backward compatibility (tests and other callers import from dashboard.ts). export { formatTrendAxisTick } from './modules/chart-utils'; @@ -30,6 +30,8 @@ Chart.register(...registerables); // the per-service savings chart instance below. let savingsTrendChart: Chart | null = null; let savingsTrendRange: '7' | '30' | '90' | 'all' = '90'; +// Request generation for loadSavingsTrendChart; see the stale guard there (#747). +let savingsTrendGeneration = 0; // Chart instance for the per-service savings-range bar chart (issue #765). let savingsByServiceChart: Chart | null = null; @@ -881,12 +883,6 @@ export function renderSavingsByService( const section = document.getElementById('savings-by-service-section'); if (!canvas) return; - // Destroy existing chart before rebuilding. - if (savingsByServiceChart) { - savingsByServiceChart.destroy(); - savingsByServiceChart = null; - } - const stats = computeServiceStatsFromRecs(recs); const heading = section?.querySelector('h3'); @@ -933,6 +929,8 @@ export function renderSavingsByService( if (positive.length === 0) { if (heading) heading.textContent = 'Potential savings range per service'; + savingsByServiceChart?.destroy(); + savingsByServiceChart = null; canvas.classList.add('hidden'); removeChartTable(SAVINGS_BY_SERVICE_TABLE_ID); if (emptyEl) { @@ -988,7 +986,8 @@ export function renderSavingsByService( formatCurrency(e.upside), formatCurrency(e.visibleTotal)]), }); - savingsByServiceChart = new Chart(canvas, { + // #747: update in place so a filter change doesn't replay the entry animation. + savingsByServiceChart = upsertChart(savingsByServiceChart, canvas, { type: 'bar', data: { labels, @@ -1102,6 +1101,9 @@ export async function loadSavingsTrendChart(): Promise { const canvas = document.getElementById('savings-trend-chart') as HTMLCanvasElement | null; const empty = document.getElementById('savings-trend-empty'); if (!canvas) return; + // #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 now = new Date(); const nowMs = now.getTime(); @@ -1135,6 +1137,7 @@ export async function loadSavingsTrendChart(): Promise { ...(provider ? { provider } : {}), ...(accountIDs.length > 0 ? { account_ids: accountIDs } : {}), }); + if (generation !== savingsTrendGeneration) return; const points = data.data_points ?? []; @@ -1198,8 +1201,6 @@ export async function loadSavingsTrendChart(): Promise { canvas.classList.remove('hidden'); empty?.classList.add('hidden'); - if (savingsTrendChart) savingsTrendChart.destroy(); - // #744: summarize the plotted series and list every point as text. const rangeText = isAllRange ? 'the last year' : `the last ${days} days`; const latest = chartData[chartData.length - 1]!; @@ -1213,7 +1214,8 @@ export async function loadSavingsTrendChart(): Promise { rows: chartData.map(p => [formatTrendAxisTick(p.x, interval), formatCurrency(p.y)]), }); - savingsTrendChart = new Chart(canvas, { + // #747: update in place so a range click doesn't replay the entry animation. + savingsTrendChart = upsertChart(savingsTrendChart, canvas, { type: 'line', data: { datasets: [{ @@ -1263,6 +1265,7 @@ export async function loadSavingsTrendChart(): Promise { }, }); } catch (err) { + 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. diff --git a/frontend/src/modules/chart-utils.ts b/frontend/src/modules/chart-utils.ts index a28a710c..84e65bce 100644 --- a/frontend/src/modules/chart-utils.ts +++ b/frontend/src/modules/chart-utils.ts @@ -2,6 +2,7 @@ * Shared chart utility helpers used by multiple chart modules. */ +import { Chart, type ChartConfiguration } from 'chart.js'; import { formatCurrency } from '../utils'; /** @@ -48,3 +49,28 @@ export function formatTrendTooltipTitle(tsMs: number, intervalHint: 'hourly' | ' } return d.toLocaleDateString('en-US', { year: 'numeric', month: 'short', day: 'numeric' }); } + +/** + * Create a chart, or update an existing one in place (issue #747). + * + * Destroying and recreating a chart on every filter or range change replays + * Chart.js's grow-from-zero entry animation each time. When `existing` is + * still drawing on `canvas`, swap in the new data and options and redraw + * without animation; otherwise (first render, the canvas was replaced, or the + * chart type changed, e.g. the Purchases chart's line/bar series toggle from + * #749) destroy any stale instance and create a fresh one. + */ +export function upsertChart(existing: Chart | null, canvas: HTMLCanvasElement, config: ChartConfiguration): Chart { + if (existing && existing.canvas === canvas && chartType(existing) === config.type) { + existing.data = config.data; + existing.options = config.options ?? {}; + existing.update('none'); + return existing; + } + existing?.destroy(); + return new Chart(canvas, config); +} + +function chartType(chart: Chart): string | undefined { + return 'type' in chart.config ? chart.config.type : undefined; +} diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index 8ecdb037..f45b7fc2 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -5,7 +5,7 @@ import { Chart, registerables } from 'chart.js'; import { getSavingsAnalytics, type SavingsAnalyticsResponse, type SavingsDataPoint } from '../api'; import * as state from '../state'; -import { formatTrendAxisTick } from './chart-utils'; +import { formatTrendAxisTick, upsertChart } from './chart-utils'; import { formatCompactCurrency, formatCurrency } from '../utils'; import { setChartSummary, renderChartTable, removeChartTable } from '../lib/chart-a11y'; @@ -401,10 +401,6 @@ function renderSavingsChart( const axisMinMs = periodStart ? periodStart.getTime() : nowMs - 7 * 86400_000; const axisMaxMs = periodEnd ? periodEnd.getTime() : nowMs; - if (savingsChart) { - savingsChart.destroy(); - } - const label = cumulative ? 'Cumulative Savings' : 'Period Savings'; const dataset = cumulative ? { @@ -449,7 +445,10 @@ function renderSavingsChart( ]), }); - savingsChart = new Chart(ctx, { + // #747: update in place so a period or unit change doesn't replay the + // entry animation. Switching the series (#749) changes the chart type, + // which upsertChart handles by recreating the chart. + savingsChart = upsertChart(savingsChart, ctx, { type: cumulative ? 'line' : 'bar', data: { datasets: [dataset], diff --git a/frontend/src/trends.ts b/frontend/src/trends.ts index da8f8803..3493aed0 100644 --- a/frontend/src/trends.ts +++ b/frontend/src/trends.ts @@ -41,7 +41,11 @@ const CHART_ID = 'trends-savings-chart'; const PROVIDER_LABELS: Record = { aws: 'AWS', azure: 'Azure', gcp: 'GCP' }; -let trendsChart: Chart | null = null; +// Typed as the bar chart it is: assigning Chart<'bar'> to the default +// Chart trips a Chart.js variance check whose +// result depends on ts-loader's check order (it broke the webpack build +// once chart-utils gained upsertChart, #747). +let trendsChart: Chart<'bar', (number | null)[], string> | null = null; // Each load takes a ticket; a response that lands after a newer request was // issued is dropped so a slow reply can't overwrite the current scope. let loadSeq = 0;