From d1c5662b7fdaf14659fe9a587b9a0aed65466444 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 15:47:46 +0200 Subject: [PATCH 1/3] refactor(chart): extract formatTrendAxisTick to shared chart-utils module Move formatTrendAxisTick from dashboard.ts into frontend/src/modules/chart-utils.ts so it can be reused by the Purchases Savings History chart. Re-export from dashboard.ts for backward compatibility with existing tests and callers. --- frontend/src/dashboard.ts | 15 +++------------ frontend/src/modules/chart-utils.ts | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 12 deletions(-) create mode 100644 frontend/src/modules/chart-utils.ts diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index 409114741..9f76f89d4 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -13,6 +13,9 @@ 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'; +// Re-export for backward compatibility (tests and other callers import from dashboard.ts). +export { formatTrendAxisTick } from './modules/chart-utils'; // Register Chart.js components Chart.register(...registerables); @@ -998,18 +1001,6 @@ export function renderSavingsByService( }); } -/** - * Format a millisecond timestamp for the savings-trend x-axis tick label. - * Exported for unit testing. - */ -export function formatTrendAxisTick(tsMs: number, intervalHint: 'hourly' | 'daily' | 'weekly'): string { - const d = new Date(tsMs); - if (intervalHint === 'hourly') { - return d.toLocaleString('en-US', { month: 'short', day: 'numeric', hour: '2-digit', minute: '2-digit', hour12: false }); - } - return d.toLocaleDateString('en-US', { month: 'short', day: 'numeric' }); -} - /** * Load the savings-over-time trend chart for the dashboard. Fetches the * history analytics endpoint with the currently-selected range and renders diff --git a/frontend/src/modules/chart-utils.ts b/frontend/src/modules/chart-utils.ts new file mode 100644 index 000000000..ab11e3fcc --- /dev/null +++ b/frontend/src/modules/chart-utils.ts @@ -0,0 +1,16 @@ +/** + * Shared chart utility helpers used by multiple chart modules. + */ + +/** + * 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. + * Exported for unit testing. + */ +export function formatTrendAxisTick(tsMs: number, intervalHint: 'hourly' | 'daily' | 'weekly'): string { + const d = new Date(tsMs); + if (intervalHint === 'hourly') { + return d.toLocaleString('en-US', { month: 'short', day: 'numeric', hour: '2-digit', minute: '2-digit', hour12: false }); + } + return d.toLocaleDateString('en-US', { month: 'short', day: 'numeric' }); +} From 8dd42d8151d6da5338e9778cf4275b95d8b27c3b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 15:48:34 +0200 Subject: [PATCH 2/3] fix(purchases): correct Savings History chart axis, tooltip decimals, y-axis ticks QA 2.2 -- x-axis starts at period start, not first data point: Convert datasets from scalar arrays to {x: timestamp_ms, y: value} objects and switch the x-axis to type:'linear' with min/max anchored to the selected period window. Mirrors the Home dashboard approach from PR #746. Uses the shared formatTrendAxisTick helper from chart-utils.ts for tick labels. QA 2.3 -- Period Savings tooltip precision matches Cumulative: Change toFixed(4) to toFixed(2) for the Period Savings tooltip label so both series and the KPI box above the chart show 2 decimal places. QA 2.4/2.5 -- y-axis ticks stable when toggling series: Add maxTicksLimit:6 to both y and y1 axis tick configs to cap re-autoscaling on legend toggle. Fix the y1 formatter to emit 2 decimal places for non-integer float ticks, preventing distinct float values from collapsing to the same integer label string. Regression tests added in savings-history.test.ts: - QA 2.2: x-axis type:linear with numeric min equal to period start - QA 2.3: Period Savings tooltip has exactly 2 decimal places - QA 2.4: both y-axes have maxTicksLimit - QA 2.5: y1 formatter does not collapse distinct floats to same label Closes #1252 --- .../src/__tests__/savings-history.test.ts | 142 ++++++++++++++++-- frontend/src/modules/savings-history.ts | 89 +++++++---- 2 files changed, 192 insertions(+), 39 deletions(-) diff --git a/frontend/src/__tests__/savings-history.test.ts b/frontend/src/__tests__/savings-history.test.ts index 70c9f93a9..a24a8c53d 100644 --- a/frontend/src/__tests__/savings-history.test.ts +++ b/frontend/src/__tests__/savings-history.test.ts @@ -373,7 +373,6 @@ describe('Savings History Module', () => { expect.objectContaining({ type: 'line', data: expect.objectContaining({ - labels: expect.any(Array), datasets: expect.arrayContaining([ expect.objectContaining({ label: 'Period Savings' @@ -628,7 +627,7 @@ describe('Savings History Module', () => { }); describe('chart formatting', () => { - test('uses date labels for daily/weekly/monthly interval', async () => { + test('uses linear x-axis type for daily/weekly/monthly interval (QA 2.2)', async () => { const mockData = { data_points: [ { timestamp: '2024-01-15T00:00:00Z', total_savings: 10, cumulative_savings: 10, total_upfront: 100, purchase_count: 1 } @@ -642,12 +641,17 @@ describe('Savings History Module', () => { await loadSavingsHistory(); const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; - const labels = chartCall[1].data.labels; - // Should format as short date (e.g., "Jan 15") - expect(labels[0]).toMatch(/Jan \d+/); + const xAxis = chartCall[1].options.scales.x; + // x-axis must be linear (not category) so leftmost tick is the period start + expect(xAxis.type).toBe('linear'); + expect(typeof xAxis.min).toBe('number'); + expect(typeof xAxis.max).toBe('number'); + // Data points should be {x, y} objects, not scalars + const dataset0 = chartCall[1].data.datasets[0]; + expect(dataset0.data[0]).toMatchObject({ x: expect.any(Number), y: expect.any(Number) }); }); - test('uses datetime labels for hourly interval', async () => { + test('uses linear x-axis with date+time tick format for hourly interval (QA 2.2)', async () => { const mockData = { data_points: [ { timestamp: '2024-01-15T14:00:00Z', total_savings: 10, cumulative_savings: 10, total_upfront: 100, purchase_count: 1 } @@ -661,9 +665,12 @@ describe('Savings History Module', () => { await loadSavingsHistory(); const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; - const labels = chartCall[1].data.labels; - // Should include hour (e.g., "Jan 15, 2 PM") - expect(labels[0]).toMatch(/Jan \d+, \d+ [AP]M/); + const xAxis = chartCall[1].options.scales.x; + expect(xAxis.type).toBe('linear'); + // Tick callback should format timestamps as date strings (not raw ms numbers) + const tickResult = xAxis.ticks.callback(new Date('2024-01-15T14:00:00Z').getTime()); + expect(typeof tickResult).toBe('string'); + expect(tickResult.length).toBeGreaterThan(0); }); test('configures point radius based on data point count', async () => { @@ -820,13 +827,13 @@ describe('Savings History Module', () => { const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; const tooltipLabelCallback = chartCall[1].options.plugins.tooltip.callbacks.label; - // Test period savings (datasetIndex 0) + // Test period savings (datasetIndex 0) -- must be 2 decimal places (QA 2.3) const periodContext = { raw: 25.5678, datasetIndex: 0, dataset: { label: 'Period Savings' } }; - expect(tooltipLabelCallback(periodContext)).toBe('Period Savings: $25.5678/mo'); + expect(tooltipLabelCallback(periodContext)).toBe('Period Savings: $25.57/mo'); }); test('tooltip label callback formats cumulative savings without unit suffix', async () => { @@ -864,13 +871,13 @@ describe('Savings History Module', () => { const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; const tooltipLabelCallback = chartCall[1].options.plugins.tooltip.callbacks.label; - // Test with null/undefined raw value + // Test with null/undefined raw value -- 2 decimal places for period savings (QA 2.3) const nullContext = { raw: null, datasetIndex: 0, dataset: { label: 'Period Savings' } }; - expect(tooltipLabelCallback(nullContext)).toBe('Period Savings: $0.0000/mo'); + expect(tooltipLabelCallback(nullContext)).toBe('Period Savings: $0.00/mo'); const undefinedContext = { raw: undefined, @@ -879,6 +886,115 @@ describe('Savings History Module', () => { }; expect(tooltipLabelCallback(undefinedContext)).toBe('Cumulative Savings: $0.00'); }); + + // Regression tests for QA 2.2 / 2.3 / 2.4 (do not remove) + + test('QA 2.3: Period Savings tooltip has exactly 2 decimal places matching Cumulative', async () => { + const mockData = { + data_points: [ + { timestamp: '2024-01-01T00:00:00Z', total_savings: 10, cumulative_savings: 10, total_upfront: 100, purchase_count: 1 } + ] + }; + (getSavingsAnalytics as jest.Mock).mockResolvedValue(mockData); + + await loadSavingsHistory(); + + const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; + const tooltipCb = chartCall[1].options.plugins.tooltip.callbacks.label; + + // Period Savings: 2 decimal places, not 4 + const periodCtx = { raw: 12.3456, datasetIndex: 0, dataset: { label: 'Period Savings' } }; + const periodLabel = tooltipCb(periodCtx) as string; + // Must render with exactly 2 decimal places, matching Cumulative and the KPI (QA 2.3) + expect(periodLabel).toMatch(/\$\d+\.\d{2}(?!\d)/); + expect(periodLabel).not.toMatch(/\$\d+\.\d{3}/); + + // Cumulative: also 2 decimal places + const cumulCtx = { raw: 99.9876, datasetIndex: 1, dataset: { label: 'Cumulative Savings' } }; + const cumulLabel = tooltipCb(cumulCtx) as string; + expect(cumulLabel).toMatch(/\$\d+\.\d{2}(?!\d)/); + expect(cumulLabel).not.toMatch(/\$\d+\.\d{3}/); + }); + + test('QA 2.2: x-axis is type:linear with numeric min equal to period start', async () => { + const mockData = { + data_points: [ + { timestamp: '2024-03-15T10:00:00Z', total_savings: 50, cumulative_savings: 50, total_upfront: 100, purchase_count: 1 } + ] + }; + (getSavingsAnalytics as jest.Mock).mockResolvedValue(mockData); + + const periodSelect = document.getElementById('savings-period') as HTMLSelectElement; + periodSelect.value = '30d'; + + const before = Date.now(); + await loadSavingsHistory(); + const after = Date.now(); + + const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; + const xAxis = chartCall[1].options.scales.x; + + // Must be linear, not category + expect(xAxis.type).toBe('linear'); + + // min must be a number (the period start timestamp) + expect(typeof xAxis.min).toBe('number'); + // 30d ago: the min should be roughly 30 days before 'after' + const thirtyDaysMs = 30 * 24 * 60 * 60 * 1000; + expect(xAxis.min).toBeGreaterThanOrEqual(before - thirtyDaysMs - 1000); + expect(xAxis.min).toBeLessThanOrEqual(after - thirtyDaysMs + 1000); + + // max should be close to now + expect(xAxis.max).toBeGreaterThanOrEqual(before); + expect(xAxis.max).toBeLessThanOrEqual(after + 1000); + + // No top-level labels array (data is in {x,y} format) + expect(chartCall[1].data.labels).toBeUndefined(); + }); + + test('QA 2.4: y and y1 axes both have maxTicksLimit to prevent tick instability', async () => { + const mockData = { + data_points: [ + { timestamp: '2024-01-01T00:00:00Z', total_savings: 10, cumulative_savings: 10, total_upfront: 100, purchase_count: 1 } + ] + }; + (getSavingsAnalytics as jest.Mock).mockResolvedValue(mockData); + + await loadSavingsHistory(); + + const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; + const yTicks = chartCall[1].options.scales.y.ticks; + const y1Ticks = chartCall[1].options.scales.y1.ticks; + + expect(typeof yTicks.maxTicksLimit).toBe('number'); + expect(yTicks.maxTicksLimit).toBeGreaterThan(0); + expect(typeof y1Ticks.maxTicksLimit).toBe('number'); + expect(y1Ticks.maxTicksLimit).toBeGreaterThan(0); + }); + + test('QA 2.5: y1-axis formatter does not collapse distinct floats to the same integer label', async () => { + const mockData = { + data_points: [ + { timestamp: '2024-01-01T00:00:00Z', total_savings: 10, cumulative_savings: 10, total_upfront: 100, purchase_count: 1 } + ] + }; + (getSavingsAnalytics as jest.Mock).mockResolvedValue(mockData); + + await loadSavingsHistory(); + + const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; + const y1Cb = chartCall[1].options.scales.y1.ticks.callback; + + // Two floats that differ in decimal but share the same integer part must + // produce distinct labels (QA 2.5). + const label1 = y1Cb(0.1) as string; + const label2 = y1Cb(0.2) as string; + expect(label1).not.toBe(label2); + + // True integers should not carry trailing decimals + expect(y1Cb(5)).toBe('$5'); + expect(y1Cb(0)).toBe('$0'); + }); }); describe('edge cases', () => { diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index 4b97ebae0..daeccf830 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -5,6 +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'; // Register Chart.js components Chart.register(...registerables); @@ -118,7 +119,7 @@ export async function loadSavingsHistory(): Promise { if (statsEl) statsEl.classList.remove('hidden'); renderSavingsStats(data); - renderSavingsChart(data.data_points, interval, getSelectedUnit()); + renderSavingsChart(data.data_points, interval, getSelectedUnit(), start, end); } catch (error) { const msg = error instanceof Error ? error.message : 'Unknown error'; console.error('Failed to load savings history:', msg); @@ -339,9 +340,26 @@ function formatCurrency(value: number | null | undefined): string { } /** - * Render savings chart using Chart.js + * Render savings chart using Chart.js. + * + * BUG FIX (QA 2.2): the x-axis is now `type:'linear'` anchored to the selected + * period [start, end] so the leftmost tick is the period start, not the first + * data point. Mirrors the approach used by the Home dashboard chart (PR #746). + * + * BUG FIX (QA 2.3): Period Savings tooltip now uses toFixed(2) matching + * Cumulative and the KPI box above the chart. + * + * BUG FIX (QA 2.4/2.5): Both y-axes have maxTicksLimit:6 to prevent tick + * instability when toggling series; y1 formatter avoids duplicate integer + * labels by showing 2 decimal places for non-integer float ticks. */ -function renderSavingsChart(dataPoints: SavingsDataPoint[], interval: string, unit: SavingsUnit = 'monthly'): void { +function renderSavingsChart( + dataPoints: SavingsDataPoint[], + interval: string, + unit: SavingsUnit = 'monthly', + periodStart?: Date, + periodEnd?: Date, +): void { const ctx = document.getElementById('savings-history-chart') as HTMLCanvasElement; if (!ctx) { @@ -349,25 +367,32 @@ function renderSavingsChart(dataPoints: SavingsDataPoint[], interval: string, un return; } - // Format labels based on interval - const labels = dataPoints.map(dp => { - const date = new Date(dp.timestamp); - if (interval === 'daily' || interval === 'weekly' || interval === 'monthly') { - return date.toLocaleDateString('en-US', { month: 'short', day: 'numeric' }); - } - return date.toLocaleString('en-US', { - month: 'short', - day: 'numeric', - hour: 'numeric', - hour12: true - }); - }); - const suffix = unitSuffix(unit); - // M-6: use ?? 0 (not || 0) to treat a genuine 0-saving bucket correctly; - // || 0 would coerce a legitimate $0 data point the same as a missing one. - const savingsData = dataPoints.map(dp => convertFromMonthly(dp.total_savings ?? 0, unit)); - const cumulativeSavings = dataPoints.map(dp => dp.cumulative_savings ?? 0); + + // Determine the interval hint for the x-axis tick formatter. + // The interval parameter can be 'hourly', 'daily', 'weekly', or 'monthly'; + // formatTrendAxisTick accepts 'hourly' | 'daily' | 'weekly' -- treat + // 'monthly' the same as 'daily' (date-only label). + const tickIntervalHint: 'hourly' | 'daily' | 'weekly' = + interval === 'hourly' ? 'hourly' : interval === 'weekly' ? 'weekly' : 'daily'; + + // Map each data point to {x: timestamp_ms, y: value} so Chart.js positions + // each point at its real date on the linear axis instead of by label index. + // M-6: use ?? 0 (not || 0) to treat a genuine 0-saving bucket correctly. + const savingsData = dataPoints.map(dp => ({ + x: new Date(dp.timestamp).getTime(), + y: convertFromMonthly(dp.total_savings ?? 0, unit), + })); + const cumulativeSavingsData = dataPoints.map(dp => ({ + x: new Date(dp.timestamp).getTime(), + y: dp.cumulative_savings ?? 0, + })); + + // Axis bounds: anchor to the selected period so the leftmost tick is the + // period start, not the first data point (QA 2.2). + const nowMs = Date.now(); + const axisMinMs = periodStart ? periodStart.getTime() : nowMs - 7 * 86400_000; + const axisMaxMs = periodEnd ? periodEnd.getTime() : nowMs; if (savingsChart) { savingsChart.destroy(); @@ -376,7 +401,6 @@ function renderSavingsChart(dataPoints: SavingsDataPoint[], interval: string, un savingsChart = new Chart(ctx, { type: 'line', data: { - labels, datasets: [ { label: 'Period Savings', @@ -391,7 +415,7 @@ function renderSavingsChart(dataPoints: SavingsDataPoint[], interval: string, un }, { label: 'Cumulative Savings', - data: cumulativeSavings, + data: cumulativeSavingsData, borderColor: '#4285f4', backgroundColor: 'rgba(66, 133, 244, 0.05)', fill: false, @@ -412,13 +436,17 @@ function renderSavingsChart(dataPoints: SavingsDataPoint[], interval: string, un }, scales: { x: { + type: 'linear', display: true, + min: axisMinMs, + max: axisMaxMs, grid: { display: false, }, ticks: { maxTicksLimit: 8, maxRotation: 0, + callback: (value) => formatTrendAxisTick(value as number, tickIntervalHint), }, }, y: { @@ -430,6 +458,7 @@ function renderSavingsChart(dataPoints: SavingsDataPoint[], interval: string, un color: 'rgba(0, 0, 0, 0.05)', }, ticks: { + maxTicksLimit: 6, callback: function(value: number | string) { const numValue = typeof value === 'string' ? parseFloat(value) : value; return `$${numValue.toFixed(2)}`; @@ -449,12 +478,18 @@ function renderSavingsChart(dataPoints: SavingsDataPoint[], interval: string, un drawOnChartArea: false, }, ticks: { + maxTicksLimit: 6, callback: function(value: number | string) { const numValue = typeof value === 'string' ? parseFloat(value) : value; if (numValue >= 1000) { return `$${(numValue / 1000).toFixed(1)}K`; } - return `$${numValue.toFixed(0)}`; + // Show 2 decimal places for non-integer float ticks to prevent + // distinct values collapsing to the same integer label when the + // auto-scaled domain is narrow (QA 2.4/2.5). + return Number.isInteger(numValue) + ? `$${numValue}` + : `$${numValue.toFixed(2)}`; }, }, title: { @@ -474,12 +509,14 @@ function renderSavingsChart(dataPoints: SavingsDataPoint[], interval: string, un tooltip: { callbacks: { label: function(context) { - const value = context.raw as number || 0; + const raw = context.raw as ({ x: number; y: number } | number) | null | undefined; + const value = (raw != null && typeof raw === 'object' ? raw.y : raw as number) || 0; if (context.datasetIndex === 1) { // Cumulative savings -- raw total, no rate suffix return `${context.dataset.label}: $${value.toFixed(2)}`; } - return `${context.dataset.label}: $${value.toFixed(4)}${suffix}`; + // Period Savings: 2 decimal places matching the KPI box (QA 2.3) + return `${context.dataset.label}: $${value.toFixed(2)}${suffix}`; }, }, }, From 8be170939c8714907fdee68b622cb65bb42278fd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 20 Jun 2026 12:47:39 +0200 Subject: [PATCH 3/3] refactor(purchases): rename savingsData to periodSavingsData for symmetry Minor readability cleanup: the Period Savings dataset array is now named periodSavingsData, matching its sibling cumulativeSavingsData. No behavior change. --- frontend/src/modules/savings-history.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index daeccf830..e75316c4d 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -379,7 +379,7 @@ function renderSavingsChart( // Map each data point to {x: timestamp_ms, y: value} so Chart.js positions // each point at its real date on the linear axis instead of by label index. // M-6: use ?? 0 (not || 0) to treat a genuine 0-saving bucket correctly. - const savingsData = dataPoints.map(dp => ({ + const periodSavingsData = dataPoints.map(dp => ({ x: new Date(dp.timestamp).getTime(), y: convertFromMonthly(dp.total_savings ?? 0, unit), })); @@ -404,7 +404,7 @@ function renderSavingsChart( datasets: [ { label: 'Period Savings', - data: savingsData, + data: periodSavingsData, borderColor: '#34a853', backgroundColor: 'rgba(52, 168, 83, 0.1)', fill: true,