From a18ba161421aa904525c9a0ba7412f0c6b41ebd0 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 23:08:15 +0000 Subject: [PATCH 1/3] fix(frontend): update savings charts in place and drop stale trend responses The Home per-service and "Savings over time" charts and the Purchases savings-history chart were destroyed and recreated on every filter, range, period or unit change, so Chart.js replayed its grow-from-zero entry animation each time. A new upsertChart helper (modules/chart-utils.ts) swaps the data and options into the existing instance and redraws with update('none'); it only creates a chart on first render or when the canvas was replaced, and the empty states still destroy the chart. loadSavingsTrendChart had no request-generation guard, so clicking one range and then another could let the older, slower response paint last and show its data under the newer range button. It now takes a generation number per call and ignores responses (and errors) from superseded calls, the same pattern riexchange.ts uses. The ladder timeline item from the issue was already fixed in #823, which updates that chart in place. Refs #747 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/chart-upsert.test.ts | 62 +++++++++++++++ frontend/src/__tests__/dashboard.test.ts | 86 +++++++++++++++++++++ frontend/src/dashboard.ts | 25 +++--- frontend/src/modules/chart-utils.ts | 22 ++++++ frontend/src/modules/savings-history.ts | 10 +-- 5 files changed, 188 insertions(+), 17 deletions(-) create mode 100644 frontend/src/__tests__/chart-upsert.test.ts diff --git a/frontend/src/__tests__/chart-upsert.test.ts b/frontend/src/__tests__/chart-upsert.test.ts new file mode 100644 index 000000000..b771fd5fd --- /dev/null +++ b/frontend/src/__tests__/chart-upsert.test.ts @@ -0,0 +1,62 @@ +/** + * 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: { data: unknown; options?: unknown }) => ({ + canvas, + 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): ChartConfiguration => ({ + type: 'line', + 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); + }); +}); diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index 9a296bec6..9f3216a7c 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -964,6 +964,74 @@ 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) ?? []; + }; + const point = (y: number) => ({ data_points: [{ 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([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) => ({ canvas, update, destroy })); + (api.getSavingsAnalytics as jest.Mock) + .mockResolvedValueOnce({ data_points: [{ timestamp: '2026-01-01T00:00:00Z', cumulative_savings: 100 }] }) + .mockResolvedValueOnce({ data_points: [{ 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(); + }); + test('uses daily interval for the default 90-day range', async () => { await loadSavingsTrendChart(); @@ -1625,6 +1693,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) => ({ canvas, 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); + }); + test('shows empty state and hides canvas when no recommendations', () => { buildDOM(); renderSavingsByService([]); diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index 2a8e61716..7ad2334f1 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -13,7 +13,7 @@ import { confirmDialog } from './confirmDialog'; import { canAccess } from './permissions'; import { groupRecsByCell, pageLevelRange, formatSavingsRange, triggerAutoRefreshIfStale } from './recommendations'; import { showSkeletonTiles, showSkeletonBlock, teardownSkeleton } from './lib/skeleton'; -import { formatTrendAxisTick } from './modules/chart-utils'; +import { formatTrendAxisTick, upsertChart } from './modules/chart-utils'; // Re-export for backward compatibility (tests and other callers import from dashboard.ts). export { formatTrendAxisTick } from './modules/chart-utils'; @@ -24,6 +24,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; @@ -894,12 +896,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'); @@ -946,6 +942,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'); if (emptyEl) { // When a filter is active and excluded all results, surface it so the @@ -1000,7 +998,8 @@ export function renderSavingsByService( return `rgba(${r},${g},${b},0.35)`; }); - 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, @@ -1118,6 +1117,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(); @@ -1150,6 +1152,7 @@ export async function loadSavingsTrendChart(): Promise { ...(provider ? { provider } : {}), ...(accountIDs.length > 0 ? { account_ids: accountIDs } : {}), }); + if (generation !== savingsTrendGeneration) return; const points = data.data_points ?? []; @@ -1204,9 +1207,8 @@ export async function loadSavingsTrendChart(): Promise { canvas.classList.remove('hidden'); empty?.classList.add('hidden'); - if (savingsTrendChart) savingsTrendChart.destroy(); - - 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: [{ @@ -1255,6 +1257,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 ab11e3fcc..05b1c6599 100644 --- a/frontend/src/modules/chart-utils.ts +++ b/frontend/src/modules/chart-utils.ts @@ -2,6 +2,8 @@ * Shared chart utility helpers used by multiple chart modules. */ +import { Chart, type ChartConfiguration } from 'chart.js'; + /** * Format a millisecond timestamp for a savings-trend x-axis tick label. * The intervalHint drives granularity: hourly shows date+time, everything else shows date only. @@ -14,3 +16,23 @@ export function formatTrendAxisTick(tsMs: number, intervalHint: 'hourly' | 'dail } return d.toLocaleDateString('en-US', { 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, or the canvas was replaced) + * 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) { + existing.data = config.data; + existing.options = config.options ?? {}; + existing.update('none'); + return existing; + } + existing?.destroy(); + return new Chart(canvas, config); +} diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index 60a4332f5..b30d804b4 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'; // Register Chart.js components Chart.register(...registerables); @@ -397,11 +397,9 @@ function renderSavingsChart( const axisMinMs = periodStart ? periodStart.getTime() : nowMs - 7 * 86400_000; const axisMaxMs = periodEnd ? periodEnd.getTime() : nowMs; - if (savingsChart) { - savingsChart.destroy(); - } - - savingsChart = new Chart(ctx, { + // #747: update in place so a period or unit change doesn't replay the + // entry animation. + savingsChart = upsertChart(savingsChart, ctx, { type: 'line', data: { datasets: [ From e96e8fa6f7a71a80bc3ef4ecf1090da563ab1f22 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 24 Oct 2026 17:42:13 +0000 Subject: [PATCH 2/3] fix(frontend): type the Trends chart handle as a bar chart After merging main, webpack's ts-loader rejected trends.ts (new on main) with TS2322 assigning Chart<'bar', (number | null)[], string> to the default Chart type. tsc --noEmit still passed: Chart.js's generic variance check depends on the order files are checked in, and adding upsertChart (which takes the default ChartConfiguration) to chart-utils changed that order for ts-loader. Declaring trendsChart with the exact type it holds removes the cross-type assignment. Refs #747 Co-Authored-By: Claude Opus 5.5 --- frontend/src/trends.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frontend/src/trends.ts b/frontend/src/trends.ts index 34bce6750..786b1b158 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; From 49a12791dbcc1b9b1fca5d0706dc1536e120bed2 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 24 Oct 2026 21:16:34 +0000 Subject: [PATCH 3/3] fix(frontend): give the #747 trend tests two data points Main's #1334 (issue #188) shows a sparse-data message instead of a chart when the trend has fewer than MIN_TREND_POINTS (2) points. The stale-response and update-in-place tests used one point, so no chart was drawn. Use two points so they still exercise the chart path. Refs #747 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/dashboard.test.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index 2762d614c..32c544cd8 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -1073,7 +1073,11 @@ describe('Dashboard Module', () => { const cfg = calls[calls.length - 1]?.[1] as { data: { datasets: { data: { y: number }[] }[] } } | undefined; return cfg?.data.datasets[0]?.data.map((p) => p.y) ?? []; }; - const point = (y: number) => ({ data_points: [{ timestamp: '2026-01-01T00:00:00Z', cumulative_savings: 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(); @@ -1087,7 +1091,7 @@ describe('Dashboard Module', () => { await first; expect(Chart).toHaveBeenCalledTimes(1); - expect(plotted()).toEqual([200]); + expect(plotted()).toEqual([0, 200]); }); test('an older request failing last does not replace the newer chart with an error', async () => { @@ -1112,8 +1116,14 @@ describe('Dashboard Module', () => { const destroy = jest.fn(); (Chart as unknown as jest.Mock).mockImplementationOnce((canvas: HTMLCanvasElement) => ({ canvas, update, destroy })); (api.getSavingsAnalytics as jest.Mock) - .mockResolvedValueOnce({ data_points: [{ timestamp: '2026-01-01T00:00:00Z', cumulative_savings: 100 }] }) - .mockResolvedValueOnce({ data_points: [{ timestamp: '2026-01-01T00:00:00Z', cumulative_savings: 300 }] }); + .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();