From f2ac57b9e811a8a6a0f7302cc9f0987a1ea2b2ea Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 20 May 2026 18:37:01 +0200 Subject: [PATCH] ux(purchases): re-query Savings History chart on filter chip change The Purchases-tab Savings History chart (modules/savings-history.ts) only wired its local period dropdown and refresh button, so changing the global Provider or Account topbar chip did not refresh it (the tab had to be left and re-entered), and it ignored the account filter entirely. Mirror the PR #500 / #488 pattern: - isPurchasesTabActive() guards the reload so a chip change while the user is on another tab defers (switchTab reloads on entry). - queueMicrotask coalesces the account-then-provider subscriber pair fired from one user action (the #185 ordering rule), collapsing two reloads into one and avoiding stale-overwrite races. - loadSavingsHistory() now forwards the selected account_id (single-select, matching the backend) and the provider chip to getSavingsAnalytics. The provider param is honoured by the backend once #502 lands; until then account_ids does the filtering. Adds regression tests covering subscriber wiring, the active-tab guard, coalescing, account_id passthrough, and two consecutive account changes producing distinct fetches. Updates app.test.ts's state mock to expose the new subscribe getters since it exercises the real savings-history module. Closes #503 --- frontend/src/__tests__/app.test.ts | 8 +- .../src/__tests__/savings-history.test.ts | 133 ++++++++++++++++++ frontend/src/modules/savings-history.ts | 55 +++++++- 3 files changed, 194 insertions(+), 2 deletions(-) diff --git a/frontend/src/__tests__/app.test.ts b/frontend/src/__tests__/app.test.ts index e83f017d9..b1b63d41c 100644 --- a/frontend/src/__tests__/app.test.ts +++ b/frontend/src/__tests__/app.test.ts @@ -11,7 +11,13 @@ jest.mock('../api', () => ({ jest.mock('../state', () => ({ setCurrentUser: jest.fn(), - setCurrentProvider: jest.fn() + setCurrentProvider: jest.fn(), + // initSavingsHistory (via setupEventListeners) subscribes to these as of + // issue #503; app.test.ts exercises the real savings-history module. + subscribeProvider: jest.fn(), + subscribeAccount: jest.fn(), + getCurrentProvider: jest.fn(() => ''), + getCurrentAccountIDs: jest.fn(() => []) })); jest.mock('../auth', () => ({ diff --git a/frontend/src/__tests__/savings-history.test.ts b/frontend/src/__tests__/savings-history.test.ts index 3c4461990..0939323d9 100644 --- a/frontend/src/__tests__/savings-history.test.ts +++ b/frontend/src/__tests__/savings-history.test.ts @@ -24,9 +24,21 @@ jest.mock('../api', () => ({ getSavingsAnalytics: jest.fn() })); +// Mock the state module. Defaults (empty provider, no accounts) keep the +// existing loadSavingsHistory tests unchanged: with these defaults +// loadSavingsHistory adds neither `provider` nor `account_ids` to the +// request, so the `objectContaining({ interval })` assertions still hold. +jest.mock('../state', () => ({ + subscribeProvider: jest.fn(), + subscribeAccount: jest.fn(), + getCurrentProvider: jest.fn(() => ''), + getCurrentAccountIDs: jest.fn(() => []) +})); + // Now import after mocking import { loadSavingsHistory, initSavingsHistory, savingsChart } from '../modules/savings-history'; import { getSavingsAnalytics } from '../api'; +import * as state from '../state'; import { Chart } from 'chart.js'; describe('Savings History Module', () => { @@ -483,6 +495,127 @@ describe('Savings History Module', () => { }); }); + // Issue #503: Purchases tab subscriber wiring. Chip changes must re-query + // the Savings History chart with the new filter; inactive-tab guard and + // microtask coalescing mirror PR #488's recommendations.ts pattern. + describe('subscriber wiring (issue #503)', () => { + function addPurchasesTab(active: boolean): void { + const tab = document.createElement('div'); + tab.id = 'purchases-tab'; + if (active) tab.classList.add('active'); + document.body.appendChild(tab); + } + + beforeEach(() => { + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + // Reset filter getters to their defaults; individual tests override. + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + }); + + test('registers callbacks with state.subscribeProvider and state.subscribeAccount', () => { + initSavingsHistory(); + + expect(state.subscribeProvider).toHaveBeenCalledTimes(1); + expect(state.subscribeAccount).toHaveBeenCalledTimes(1); + expect(typeof (state.subscribeProvider as jest.Mock).mock.calls[0]?.[0]).toBe('function'); + expect(typeof (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0]).toBe('function'); + }); + + test('account chip change re-queries the chart when purchases-tab is active', async () => { + addPurchasesTab(true); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-A']); + + initSavingsHistory(); + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + expect(typeof accountCb).toBe('function'); + + (getSavingsAnalytics as jest.Mock).mockClear(); + accountCb(); + // queueMicrotask + the awaited fetch settle within a macrotask flush. + await new Promise((r) => setTimeout(r, 0)); + + expect(getSavingsAnalytics).toHaveBeenCalledTimes(1); + expect(getSavingsAnalytics).toHaveBeenCalledWith( + expect.objectContaining({ account_ids: ['acct-A'] }) + ); + }); + + test('provider chip change re-queries the chart when purchases-tab is active', async () => { + addPurchasesTab(true); + (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); + + initSavingsHistory(); + const providerCb = (state.subscribeProvider as jest.Mock).mock.calls[0]?.[0] as () => void; + expect(typeof providerCb).toBe('function'); + + (getSavingsAnalytics as jest.Mock).mockClear(); + providerCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(getSavingsAnalytics).toHaveBeenCalledTimes(1); + expect(getSavingsAnalytics).toHaveBeenCalledWith( + expect.objectContaining({ provider: 'aws' }) + ); + }); + + test('does NOT re-query when purchases-tab is inactive (active-tab guard)', async () => { + // #purchases-tab present but no .active class: user is on another tab. + addPurchasesTab(false); + + initSavingsHistory(); + const providerCb = (state.subscribeProvider as jest.Mock).mock.calls[0]?.[0] as () => void; + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + + (getSavingsAnalytics as jest.Mock).mockClear(); + providerCb(); + accountCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(getSavingsAnalytics).not.toHaveBeenCalled(); + }); + + test('coalesces back-to-back provider+account fires into one reload', async () => { + addPurchasesTab(true); + + initSavingsHistory(); + const providerCb = (state.subscribeProvider as jest.Mock).mock.calls[0]?.[0] as () => void; + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + + (getSavingsAnalytics as jest.Mock).mockClear(); + // Simulate the topbar provider-change handler firing both subscribers + // synchronously (clear accounts, then set provider, per #185). + accountCb(); + providerCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(getSavingsAnalytics).toHaveBeenCalledTimes(1); + }); + + test('two consecutive account changes produce two fetches with distinct account_ids', async () => { + addPurchasesTab(true); + + initSavingsHistory(); + const accountCb = (state.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + + (getSavingsAnalytics as jest.Mock).mockClear(); + + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-A']); + accountCb(); + await new Promise((r) => setTimeout(r, 0)); + + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['acct-B']); + accountCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(getSavingsAnalytics).toHaveBeenCalledTimes(2); + const firstCall = (getSavingsAnalytics as jest.Mock).mock.calls[0][0]; + const secondCall = (getSavingsAnalytics as jest.Mock).mock.calls[1][0]; + expect(firstCall.account_ids).toEqual(['acct-A']); + expect(secondCall.account_ids).toEqual(['acct-B']); + }); + }); + describe('chart formatting', () => { test('uses date labels for daily/weekly/monthly interval', async () => { const mockData = { diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index 30d4186ef..7b3926916 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -4,6 +4,7 @@ import { Chart, registerables } from 'chart.js'; import { getSavingsAnalytics, type SavingsAnalyticsResponse, type SavingsDataPoint } from '../api'; +import * as state from '../state'; // Register Chart.js components Chart.register(...registerables); @@ -25,11 +26,22 @@ export async function loadSavingsHistory(): Promise { const period = periodSelect.value; const { start, end, interval } = getPeriodDates(period); + // Honour the global topbar filter chips (issue #503). The account chip + // is single-select, and the backend's /history/analytics takes a single + // account_id (see handler_analytics.go), so we forward the only selected + // ID, mirroring dashboard.ts loadSavingsTrendChart. The provider chip is + // forwarded too; the backend honours it once #502 lands (until then it is + // a harmless no-op param and account_ids does the filtering). + const currentProvider = state.getCurrentProvider(); + const currentAccountIDs = state.getCurrentAccountIDs(); + try { const data = await getSavingsAnalytics({ start: start.toISOString(), end: end.toISOString(), interval, + ...(currentProvider ? { provider: currentProvider } : {}), + ...(currentAccountIDs.length === 1 ? { account_ids: currentAccountIDs } : {}), }); if (!data.data_points || data.data_points.length === 0) { @@ -301,7 +313,36 @@ function renderSavingsChart(dataPoints: SavingsDataPoint[], interval: string): v } /** - * Initialize savings history event listeners + * True when the Purchases tab is the currently-visible tab. The reload-on- + * filter-change subscriptions below skip the fetch when this is false so we + * don't burn an API call (and a skeleton flash) for a section the user isn't + * looking at: `switchTab('purchases')` runs loadSavingsHistory() on next + * entry anyway. + */ +function isPurchasesTabActive(): boolean { + return document.getElementById('purchases-tab')?.classList.contains('active') === true; +} + +/** + * Initialize savings history event listeners (issue #503). + * + * Wires the period dropdown + refresh button, and subscribes to the global + * topbar filter chips so a provider/account change re-queries this chart. + * Previously only the local controls were wired, so changing the Account + * chip did nothing until the Purchases tab was left and re-entered. + * + * Mirrors the recommendations.ts pattern from PR #488: + * - Active-tab guard: only fire loadSavingsHistory() when the Purchases + * tab is active. + * - Coalesce duplicate reloads via queueMicrotask: the provider-change + * handler in topbar-filters.ts updates BOTH state slots (clear accounts + * then set provider, per the #185 ordering rule), firing the account- + * AND provider-subscribers from one user action. Without coalescing we'd + * kick off two loadSavingsHistory() calls back-to-back: extra API load + * plus a stale-overwrite risk if the first response lands after the + * second. + * - Re-check active-tab inside the microtask: a tab switch between the + * chip change and the microtask flush cancels the now-unneeded fetch. */ export function initSavingsHistory(): void { const periodSelect = document.getElementById('savings-period'); @@ -314,6 +355,18 @@ export function initSavingsHistory(): void { if (refreshBtn) { refreshBtn.addEventListener('click', loadSavingsHistory); } + + let reloadQueued = false; + const scheduleReload = (): void => { + if (!isPurchasesTabActive() || reloadQueued) return; + reloadQueued = true; + queueMicrotask(() => { + reloadQueued = false; + if (isPurchasesTabActive()) void loadSavingsHistory(); + }); + }; + state.subscribeProvider(scheduleReload); + state.subscribeAccount(scheduleReload); } // Export for use in other modules