diff --git a/frontend/src/__tests__/savings-history.test.ts b/frontend/src/__tests__/savings-history.test.ts index 2c95e8ac5..14d842174 100644 --- a/frontend/src/__tests__/savings-history.test.ts +++ b/frontend/src/__tests__/savings-history.test.ts @@ -56,6 +56,10 @@ describe('Savings History Module', () => { +
@@ -332,11 +336,11 @@ describe('Savings History Module', () => { jest.useRealTimers(); } - expect(document.getElementById('period-savings')?.textContent).toBe('$1.80K'); + expect(document.getElementById('period-savings')?.textContent).toBe('$1,800.00'); // 90d window: the average spreads the authoritative total over 90 days. expect(document.getElementById('avg-hourly-savings')?.textContent).toBe('$20.00/mo'); // Peak has no wire counterpart, so it comes from the buckets. - expect(document.getElementById('peak-savings')?.textContent).toBe('$2.00K/mo'); + expect(document.getElementById('peak-savings')?.textContent).toBe('$2,000.00/mo'); }); test('ignores the never-sent legacy summary keys (issue #276)', async () => { @@ -393,7 +397,7 @@ describe('Savings History Module', () => { expect(mockChartInstance.destroy).toHaveBeenCalled(); }); - test('formats currency values over 1000 with K suffix', async () => { + test('formats currency values over 1000 with the shared formatter (#749)', async () => { const mockData = { summary: { total_monthly_savings: 5000, total_annual_savings: 60000 }, data_points: [ @@ -405,7 +409,7 @@ describe('Savings History Module', () => { await loadSavingsHistory(); const periodSavingsEl = document.getElementById('period-savings'); - expect(periodSavingsEl?.textContent).toBe('$5.00K'); + expect(periodSavingsEl?.textContent).toBe('$5,000.00'); }); test('formats currency values under 1000 without suffix', async () => { @@ -476,19 +480,18 @@ describe('Savings History Module', () => { await loadSavingsHistory(); + // Issue #749: one series (period savings as bars) on one y-axis. expect(Chart).toHaveBeenCalledWith( expect.any(HTMLCanvasElement), expect.objectContaining({ - type: 'line', + type: 'bar', data: expect.objectContaining({ - datasets: expect.arrayContaining([ + datasets: [ expect.objectContaining({ + type: 'bar', label: 'Period Savings' - }), - expect.objectContaining({ - label: 'Cumulative Savings' }) - ]) + ] }), options: expect.objectContaining({ responsive: true, @@ -794,6 +797,7 @@ describe('Savings History Module', () => { (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: manyDataPoints }); + (document.getElementById('savings-series') as HTMLSelectElement).value = 'cumulative'; await loadSavingsHistory(); @@ -812,6 +816,7 @@ describe('Savings History Module', () => { (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: fewDataPoints }); + (document.getElementById('savings-series') as HTMLSelectElement).value = 'cumulative'; await loadSavingsHistory(); @@ -843,9 +848,11 @@ describe('Savings History Module', () => { const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; const yAxisCallback = chartCall[1].options.scales.y.ticks.callback; - // Test with number value - expect(yAxisCallback(50)).toBe('$50.00'); + // Compact shared formatter, not `$${x.toFixed(2)}` (issue #749) + expect(yAxisCallback(50)).toBe('$50'); expect(yAxisCallback(123.456)).toBe('$123.46'); + expect(yAxisCallback(1500)).toBe('$1.5K'); + expect(yAxisCallback(12500)).toBe('$12.5K'); }); test('y-axis tick callback formats currency for string values', async () => { @@ -862,64 +869,9 @@ describe('Savings History Module', () => { const yAxisCallback = chartCall[1].options.scales.y.ticks.callback; // Test with string value - expect(yAxisCallback('75.5')).toBe('$75.50'); - expect(yAxisCallback('100')).toBe('$100.00'); - }); - - test('y1-axis tick callback formats large values with K suffix', 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 y1AxisCallback = chartCall[1].options.scales.y1.ticks.callback; - - // Test with large number values (>= 1000) - expect(y1AxisCallback(1000)).toBe('$1.0K'); - expect(y1AxisCallback(5000)).toBe('$5.0K'); - expect(y1AxisCallback(12500)).toBe('$12.5K'); - }); - - test('y1-axis tick callback formats small values without K suffix', 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 y1AxisCallback = chartCall[1].options.scales.y1.ticks.callback; - - // Test with small number values (< 1000) - expect(y1AxisCallback(50)).toBe('$50'); - expect(y1AxisCallback(999)).toBe('$999'); - expect(y1AxisCallback(0)).toBe('$0'); - }); - - test('y1-axis tick callback handles string values', 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 y1AxisCallback = chartCall[1].options.scales.y1.ticks.callback; - - // Test with string values - expect(y1AxisCallback('2500')).toBe('$2.5K'); - expect(y1AxisCallback('500')).toBe('$500'); + expect(yAxisCallback('75.5')).toBe('$75.5'); + expect(yAxisCallback('100')).toBe('$100'); + expect(yAxisCallback('2500')).toBe('$2.5K'); }); test('tooltip label callback formats period savings with unit suffix (default monthly)', async () => { @@ -951,19 +903,19 @@ describe('Savings History Module', () => { ] }; (getSavingsAnalytics as jest.Mock).mockResolvedValue(mockData); + (document.getElementById('savings-series') as HTMLSelectElement).value = 'cumulative'; await loadSavingsHistory(); const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; const tooltipLabelCallback = chartCall[1].options.plugins.tooltip.callbacks.label; - // Test cumulative savings (datasetIndex 1) const cumulativeContext = { raw: 1234.56, - datasetIndex: 1, + datasetIndex: 0, dataset: { label: 'Cumulative Savings' } }; - expect(tooltipLabelCallback(cumulativeContext)).toBe('Cumulative Savings: $1234.56'); + expect(tooltipLabelCallback(cumulativeContext)).toBe('Cumulative Savings: $1,234.56'); }); test('tooltip label callback handles null/undefined raw value', async () => { @@ -989,10 +941,10 @@ describe('Savings History Module', () => { const undefinedContext = { raw: undefined, - datasetIndex: 1, - dataset: { label: 'Cumulative Savings' } + datasetIndex: 0, + dataset: { label: 'Period Savings' } }; - expect(tooltipLabelCallback(undefinedContext)).toBe('Cumulative Savings: $0.00'); + expect(tooltipLabelCallback(undefinedContext)).toBe('Period Savings: $0.00/mo'); }); // Regression tests for QA 2.2 / 2.3 / 2.4 (do not remove) @@ -1060,7 +1012,7 @@ describe('Savings History Module', () => { expect(chartCall[1].data.labels).toBeUndefined(); }); - test('QA 2.4: y and y1 axes both have maxTicksLimit to prevent tick instability', async () => { + test('QA 2.4: the y-axis has 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 } @@ -1072,15 +1024,12 @@ describe('Savings History Module', () => { 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 () => { + test('QA 2.5: the y-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 } @@ -1091,7 +1040,7 @@ describe('Savings History Module', () => { await loadSavingsHistory(); const chartCall = (Chart as unknown as jest.Mock).mock.calls[0]; - const y1Cb = chartCall[1].options.scales.y1.ticks.callback; + const y1Cb = chartCall[1].options.scales.y.ticks.callback; // Two floats that differ in decimal but share the same integer part must // produce distinct labels (QA 2.5). @@ -1452,8 +1401,8 @@ describe('Savings History Module', () => { const avgEl = document.getElementById('avg-hourly-savings'); expect(avgEl?.textContent).toContain('/yr'); - // 730 * 12 = $8760.00 -> formatted as $8.76K - expect(avgEl?.textContent).toContain('$8.76K'); + // 730 * 12 = $8,760.00 + expect(avgEl?.textContent).toContain('$8,760.00'); }); test('avg-savings-label heading changes to reflect selected unit', async () => { @@ -1518,10 +1467,10 @@ describe('Savings History Module', () => { expect(document.getElementById('period-savings')?.textContent).toBe('$1.00'); expect(document.getElementById('period-savings-unit')?.textContent).toContain('hourly'); - // Yearly: 730 * 12 = 8760 -> $8.76K; sub-line updates to 'yearly' + // Yearly: 730 * 12 = 8760 -> $8,760.00; sub-line updates to 'yearly' unitSelect.value = 'yearly'; await loadSavingsHistory(); - expect(document.getElementById('period-savings')?.textContent).toBe('$8.76K'); + expect(document.getElementById('period-savings')?.textContent).toBe('$8,760.00'); expect(document.getElementById('period-savings-unit')?.textContent).toContain('yearly'); }); @@ -1537,4 +1486,51 @@ describe('Savings History Module', () => { expect(tooltipCb(ctx)).toContain('/hr'); }); }); + + describe('one series on one axis (issue #749)', () => { + const mockData = { + data_points: [ + { timestamp: '2024-01-01T00:00:00Z', total_savings: 100, cumulative_savings: 100, total_upfront: 0, purchase_count: 1 }, + { timestamp: '2024-01-02T00:00:00Z', total_savings: 300, cumulative_savings: 400, total_upfront: 0, purchase_count: 1 }, + ], + }; + + test('period view draws bars on a single y-axis', async () => { + (getSavingsAnalytics as jest.Mock).mockResolvedValue(mockData); + await loadSavingsHistory(); + const config = (Chart as unknown as jest.Mock).mock.calls[0][1]; + expect(Object.keys(config.options.scales)).toEqual(['x', 'y']); + expect(config.data.datasets).toHaveLength(1); + expect(config.data.datasets[0].type).toBe('bar'); + expect(config.data.datasets[0].data.map((d: { y: number }) => d.y)).toEqual([100, 300]); + expect(config.options.scales.y.title.text).toBe('Savings per period (Monthly)'); + }); + + test('cumulative view plots the running total alone, without a rate suffix', async () => { + (getSavingsAnalytics as jest.Mock).mockResolvedValue(mockData); + (document.getElementById('savings-series') as HTMLSelectElement).value = 'cumulative'; + await loadSavingsHistory(); + const config = (Chart as unknown as jest.Mock).mock.calls[0][1]; + expect(Object.keys(config.options.scales)).toEqual(['x', 'y']); + expect(config.type).toBe('line'); + expect(config.data.datasets).toHaveLength(1); + expect(config.data.datasets[0].label).toBe('Cumulative Savings'); + expect(config.data.datasets[0].data.map((d: { y: number }) => d.y)).toEqual([100, 400]); + expect(config.options.scales.y.title.text).toBe('Cumulative savings'); + const tooltip = config.options.plugins.tooltip.callbacks.label; + expect(tooltip({ raw: { x: 0, y: 400 }, dataset: { label: 'Cumulative Savings' } })).toBe('Cumulative Savings: $400.00'); + }); + + test('changing the series reloads the chart once, even after a second init', async () => { + (getSavingsAnalytics as jest.Mock).mockResolvedValue(mockData); + initSavingsHistory(); + initSavingsHistory(); + (getSavingsAnalytics as jest.Mock).mockClear(); + const series = document.getElementById('savings-series') as HTMLSelectElement; + series.value = 'cumulative'; + series.dispatchEvent(new Event('change')); + await new Promise(resolve => setTimeout(resolve, 50)); + expect(getSavingsAnalytics).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/frontend/src/__tests__/utils.test.ts b/frontend/src/__tests__/utils.test.ts index 572235f8b..6e199a7e5 100644 --- a/frontend/src/__tests__/utils.test.ts +++ b/frontend/src/__tests__/utils.test.ts @@ -2,6 +2,7 @@ * Unit tests for utility functions */ import { + formatCompactCurrency, formatCurrency, formatDate, formatDateTime, @@ -640,6 +641,26 @@ describe('jsonClone (explicit JSON-serialisation variant)', () => { // --------------------------------------------------------------------------- // Regression tests for finding 11-N2: formatCurrency distinguishes absent from $0. // --------------------------------------------------------------------------- +describe('formatCompactCurrency (issue #749)', () => { + test('abbreviates thousands and millions like a chart axis', () => { + expect(formatCompactCurrency(0)).toBe('$0'); + expect(formatCompactCurrency(950)).toBe('$950'); + expect(formatCompactCurrency(1500)).toBe('$1.5K'); + expect(formatCompactCurrency(12345)).toBe('$12.35K'); + expect(formatCompactCurrency(1234567)).toBe('$1.23M'); + expect(formatCompactCurrency(12345, 1)).toBe('$12.3K'); + }); + + test('keeps close fractional ticks distinct', () => { + expect(formatCompactCurrency(0.1)).not.toBe(formatCompactCurrency(0.2)); + }); + + test('uses the same absent marker as formatCurrency', () => { + expect(formatCompactCurrency(null)).toBe(formatCurrency(null)); + expect(formatCompactCurrency(NaN)).toBe(formatCurrency(NaN)); + }); +}); + describe('formatCurrency (11-N2: absent vs real zero)', () => { test('null returns an em dash, not $0', () => { expect(formatCurrency(null as unknown as number)).toBe('—'); diff --git a/frontend/src/index.html b/frontend/src/index.html index 8e16563d8..ddaa7db8a 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -210,6 +210,12 @@

Savings History

+
diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index 3536ef0c6..8ecdb037d 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -6,6 +6,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 { formatCompactCurrency, formatCurrency } from '../utils'; import { setChartSummary, renderChartTable, removeChartTable } from '../lib/chart-a11y'; // Register Chart.js components @@ -74,6 +75,18 @@ function getSelectedUnit(): SavingsUnit { return API_UNIT; } +/** + * Which series the chart plots (issue #749). Period savings and the running + * total have different scales and units, so they are shown one at a time on + * a single axis instead of on two axes whose line crossings mean nothing. + */ +export type SavingsSeries = 'period' | 'cumulative'; + +function getSelectedSeries(): SavingsSeries { + const el = document.getElementById('savings-series') as HTMLSelectElement | null; + return el?.value === 'cumulative' ? 'cumulative' : 'period'; +} + /** * Load savings history data based on selected period */ @@ -122,7 +135,7 @@ export async function loadSavingsHistory(): Promise { if (statsEl) statsEl.classList.remove('hidden'); renderSavingsStats(data, start, end, interval); - renderSavingsChart(data.data_points, interval, getSelectedUnit(), start, end); + renderSavingsChart(data.data_points, interval, getSelectedUnit(), start, end, getSelectedSeries()); } catch (error) { const msg = error instanceof Error ? error.message : 'Unknown error'; console.error('Failed to load savings history:', msg); @@ -310,7 +323,7 @@ function renderSavingsStats(data: SavingsAnalyticsResponse, start: Date, end: Da if (periodSavingsEl) { // Period Savings is the cumulative total over the selected date range // (no per-unit rate suffix -- it is already a dollar total). - periodSavingsEl.textContent = formatCurrency(displayTotal); + periodSavingsEl.textContent = formatCurrency(displayTotal, '$', 2); } if (periodSavingsLabelEl) { periodSavingsLabelEl.textContent = 'Period Savings'; @@ -322,44 +335,30 @@ function renderSavingsStats(data: SavingsAnalyticsResponse, start: Date, end: Da avgLabelEl.textContent = `Avg ${adjective} Savings`; } if (avgSavingsEl) { - avgSavingsEl.textContent = `${formatCurrency(displayAvg)}${suffix}`; + avgSavingsEl.textContent = `${formatCurrency(displayAvg, '$', 2)}${suffix}`; } if (peakSavingsEl) { - peakSavingsEl.textContent = `${formatCurrency(displayPeak)}${suffix}`; - } -} - -/** - * Format currency value for chart contexts (abbreviated K for >= $1000). - * - * M-7: add a null/non-finite guard so a future API change that introduces - * null on total_savings or cumulative_savings doesn't crash the chart with - * a TypeError on .toFixed(). Returns '--' for absent/non-finite values, - * matching the shared formatCurrency sentinel in utils.ts. - */ -function formatCurrency(value: number | null | undefined): string { - if (value == null || !Number.isFinite(value)) { - return '--'; - } - if (value >= 1000) { - return `$${(value / 1000).toFixed(2)}K`; + peakSavingsEl.textContent = `${formatCurrency(displayPeak, '$', 2)}${suffix}`; } - return `$${value.toFixed(2)}`; } /** * Render savings chart using Chart.js. * - * BUG FIX (QA 2.2): the x-axis is now `type:'linear'` anchored to the selected + * Issue #749: one series on one y-axis. Period savings are bars (each bar is + * one interval's projected monthly additions, converted to the chosen unit); + * the running total is a separate view picked with #savings-series. The old + * chart drew both on two y-axes, so the points where the lines crossed had + * no meaning. Ticks and tooltips use the shared formatters ("$1.5K" on the + * axis, "$1,500.00" in the tooltip) instead of `$${x.toFixed(2)}`. + * + * BUG FIX (QA 2.2): the x-axis is `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. + * BUG FIX (QA 2.4/2.5): the y-axis has maxTicksLimit:6 to prevent tick + * instability; the compact formatter keeps up to two fraction digits so + * close float ticks stay distinct. */ function renderSavingsChart( dataPoints: SavingsDataPoint[], @@ -367,6 +366,7 @@ function renderSavingsChart( unit: SavingsUnit = 'monthly', periodStart?: Date, periodEnd?: Date, + series: SavingsSeries = 'period', ): void { const ctx = document.getElementById('savings-history-chart') as HTMLCanvasElement; @@ -375,7 +375,10 @@ function renderSavingsChart( return; } - const suffix = unitSuffix(unit); + const cumulative = series === 'cumulative'; + // The running total is a dollar amount, not a rate, so it takes no + // unit suffix (its unit conversion is tracked separately in #134). + const suffix = cumulative ? '' : unitSuffix(unit); // Determine the interval hint for the x-axis tick formatter. // The interval parameter can be 'hourly', 'daily', 'weekly', or 'monthly'; @@ -387,13 +390,9 @@ 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 periodSavingsData = dataPoints.map(dp => ({ + const points = 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, + y: cumulative ? dp.cumulative_savings ?? 0 : convertFromMonthly(dp.total_savings ?? 0, unit), })); // Axis bounds: anchor to the selected period so the leftmost tick is the @@ -406,51 +405,54 @@ function renderSavingsChart( savingsChart.destroy(); } + const label = cumulative ? 'Cumulative Savings' : 'Period Savings'; + const dataset = cumulative + ? { + type: 'line' as const, + label, + data: points, + borderColor: '#4285f4', + backgroundColor: 'rgba(66, 133, 244, 0.1)', + fill: true, + tension: 0.3, + pointRadius: dataPoints.length > 50 ? 0 : 3, + pointHoverRadius: 5, + } + : { + type: 'bar' as const, + label, + data: points, + backgroundColor: 'rgba(52, 168, 83, 0.7)', + borderColor: '#34a853', + borderWidth: 1, + maxBarThickness: 24, + }; + // #744: a value summary for the role="img" canvas plus every point as a // table. The table goes after the sizing container, not inside it. + // Merged with #749: the summary names the series currently drawn, and the + // table keeps both columns (period in the chosen unit, running total) + // whichever series the chart shows, so no value is hidden by the toggle. const latest = dataPoints[dataPoints.length - 1]; setChartSummary(ctx, - `Line chart of savings over time: ${dataPoints.length} data point${dataPoints.length === 1 ? '' : 's'}, ` - + `cumulative savings ${formatCurrency(latest?.cumulative_savings ?? 0)} at the latest point. ` + `${cumulative ? 'Line chart of cumulative savings' : 'Bar chart of savings per period'}: ` + + `${dataPoints.length} data point${dataPoints.length === 1 ? '' : 's'}, ` + + `cumulative savings ${formatCurrency(latest?.cumulative_savings ?? 0, '$', 2)} at the latest point. ` + 'Values are in the table below the chart.'); renderChartTable(ctx.parentElement ?? ctx, SAVINGS_HISTORY_TABLE_ID, { caption: 'Savings history', - columns: ['Date', `Period savings (${suffix})`, 'Cumulative savings'], - rows: periodSavingsData.map((p, i) => [ - formatTrendAxisTick(p.x, tickIntervalHint), - formatCurrency(p.y), - formatCurrency(cumulativeSavingsData[i]?.y), + columns: ['Date', `Period savings (${unitSuffix(unit)})`, 'Cumulative savings'], + rows: dataPoints.map((dp) => [ + formatTrendAxisTick(new Date(dp.timestamp).getTime(), tickIntervalHint), + formatCurrency(convertFromMonthly(dp.total_savings ?? 0, unit), '$', 2), + formatCurrency(dp.cumulative_savings ?? 0, '$', 2), ]), }); savingsChart = new Chart(ctx, { - type: 'line', + type: cumulative ? 'line' : 'bar', data: { - datasets: [ - { - label: 'Period Savings', - data: periodSavingsData, - borderColor: '#34a853', - backgroundColor: 'rgba(52, 168, 83, 0.1)', - fill: true, - tension: 0.3, - pointRadius: dataPoints.length > 50 ? 0 : 3, - pointHoverRadius: 5, - yAxisID: 'y', - }, - { - label: 'Cumulative Savings', - data: cumulativeSavingsData, - borderColor: '#4285f4', - backgroundColor: 'rgba(66, 133, 244, 0.05)', - fill: false, - tension: 0.3, - borderDash: [5, 5], - pointRadius: dataPoints.length > 50 ? 0 : 2, - pointHoverRadius: 4, - yAxisID: 'y1', - }, - ], + datasets: [dataset], }, options: { responsive: true, @@ -465,6 +467,9 @@ function renderSavingsChart( display: true, min: axisMinMs, max: axisMaxMs, + // A bar chart pads the x-axis by default, which would + // push the period start off the axis edge (QA 2.2). + offset: false, grid: { display: false, }, @@ -486,62 +491,25 @@ function renderSavingsChart( maxTicksLimit: 6, callback: function(value: number | string) { const numValue = typeof value === 'string' ? parseFloat(value) : value; - return `$${numValue.toFixed(2)}`; - }, - }, - title: { - display: true, - text: `Savings per Period (${unitLabel(unit)})`, - }, - }, - y1: { - type: 'linear', - display: true, - position: 'right', - beginAtZero: true, - grid: { - 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`; - } - // 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)}`; + return formatCompactCurrency(numValue); }, }, title: { display: true, - text: 'Cumulative Savings', + text: cumulative ? 'Cumulative savings' : `Savings per period (${unitLabel(unit)})`, }, }, }, plugins: { legend: { - position: 'top', - labels: { - usePointStyle: true, - boxWidth: 8, - }, + display: false, }, tooltip: { callbacks: { label: function(context) { 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)}`; - } - // Period Savings: 2 decimal places matching the KPI box (QA 2.3) - return `${context.dataset.label}: $${value.toFixed(2)}${suffix}`; + return `${context.dataset.label}: ${formatCurrency(value, '$', 2)}${suffix}`; }, }, }, @@ -595,6 +563,16 @@ export function initSavingsHistory(): void { refreshBtn.addEventListener('click', loadSavingsHistory); } + // Series toggle (issue #749): same dedup pattern as the unit dropdown. + const seriesSelect = document.getElementById('savings-series') as (HTMLSelectElement & { _seriesChangeHandler?: () => void }) | null; + if (seriesSelect) { + if (seriesSelect._seriesChangeHandler) { + seriesSelect.removeEventListener('change', seriesSelect._seriesChangeHandler); + } + seriesSelect._seriesChangeHandler = (): void => { void loadSavingsHistory(); }; + seriesSelect.addEventListener('change', seriesSelect._seriesChangeHandler); + } + // Wire unit dropdown. Using a named handler stored as a property so that // repeated calls to initSavingsHistory() don't stack duplicate listeners // (feedback_event_listener_dedup pattern). diff --git a/frontend/src/utils.ts b/frontend/src/utils.ts index e1f62f5fa..c844608d2 100644 --- a/frontend/src/utils.ts +++ b/frontend/src/utils.ts @@ -62,6 +62,25 @@ export function formatCurrency( })}`; } +/** + * Format a number as compact currency ("$950", "$1.5K", "$12.35K", "$3M") + * for chart axes, where full figures would crowd the ticks. Same en-US + * pinning and absent-value marker as formatCurrency; `digits` caps the + * fraction digits so close ticks (0.1 vs 0.2) stay distinct. + */ +export function formatCompactCurrency(value: number | null | undefined, digits = 2): string { + if (value === null || value === undefined || !Number.isFinite(value)) { + // Delegate so both formatters always share the absent marker (NO_VALUE). + return formatCurrency(value); + } + return new Intl.NumberFormat('en-US', { + style: 'currency', + currency: 'USD', + notation: 'compact', + maximumFractionDigits: digits, + }).format(value); +} + /** * Parse a date input into a Date, treating bare calendar dates correctly. * diff --git a/frontend/tests-e2e/savings-history-average.spec.ts b/frontend/tests-e2e/savings-history-average.spec.ts index 815de8075..b3e01225d 100644 --- a/frontend/tests-e2e/savings-history-average.spec.ts +++ b/frontend/tests-e2e/savings-history-average.spec.ts @@ -52,7 +52,7 @@ async function changePeriod(page: Page, period: string) { test.use({ timezoneId: 'UTC' }); for (const [period, count, average, totalFull] of [ - ['24h', 24, '$70.00/mo', '$240.00'], ['7d', 168, '$10.00/mo', '$1.68K'], + ['24h', 24, '$70.00/mo', '$240.00'], ['7d', 168, '$10.00/mo', '$1,680.00'], ['30d', 30, '$56.00/mo', '$300.00'], ['90d', 90, '$18.67/mo', '$900.00'], ] as const) { test(period + ' averages the requested window regardless of missing zero buckets', async ({ page }) => { @@ -66,7 +66,7 @@ for (const [period, count, average, totalFull] of [ await changePeriod(page, period); await expect(page.locator('#avg-savings-label')).toHaveText('Avg Monthly Savings'); await expect(page.locator('#avg-hourly-savings')).toHaveText(mode === 'full' ? '$10.00/mo' : average); - await expect(page.locator('#period-savings')).toHaveText(mode === 'full' ? totalFull : '$1.68K'); + await expect(page.locator('#period-savings')).toHaveText(mode === 'full' ? totalFull : '$1,680.00'); await expect(page.locator('#peak-savings')).toHaveText(mode === 'full' ? '$10.00/mo' : '$560.00/mo'); const request = fixture.requests[fixture.requests.length - 1]!; const ms = period === '24h' || period === '7d' ? 3600000 : 86400000; @@ -84,8 +84,8 @@ test('unit switches, refresh and empty/error states work on a narrow screen', as await changePeriod(page, '7d'); for (const [unit, label, average, total, peak] of [ ['hourly', 'Hourly', '$0.01/hr', '$2.30', '$0.77/hr'], - ['yearly', 'Yearly', '$120.00/yr', '$20.16K', '$6.72K/yr'], - ['monthly', 'Monthly', '$10.00/mo', '$1.68K', '$560.00/mo'], + ['yearly', 'Yearly', '$120.00/yr', '$20,160.00', '$6,720.00/yr'], + ['monthly', 'Monthly', '$10.00/mo', '$1,680.00', '$560.00/mo'], ]) { await page.locator('#savings-unit').selectOption(unit!); await expect(page.locator('#avg-savings-label')).toHaveText('Avg ' + label + ' Savings'); @@ -93,6 +93,12 @@ test('unit switches, refresh and empty/error states work on a narrow screen', as await expect(page.locator('#period-savings')).toHaveText(total!); await expect(page.locator('#peak-savings')).toHaveText(peak!); } + // Issue #749: the running total is its own view, re-queried on change. + const reload = page.waitForResponse(value => value.url().includes('/history/analytics?')); + await page.locator('#savings-series').selectOption('cumulative'); + await reload; + await expect(page.locator('#savings-history-chart')).toBeVisible(); + await page.locator('#savings-series').selectOption('period'); fixture.mode = 'zero'; await page.locator('#refresh-savings-btn').click(); await expect(page.locator('#avg-hourly-savings')).toHaveText('$0.00/mo');