From 9a54389b0c1af290ce31a6f4d1f84a2324137caf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 26 May 2026 17:57:04 +0200 Subject: [PATCH 1/2] fix(purchases): wire topbar filter chips to all three Purchases consumers (#701) Three sub-flows of the global filter on the Purchases page still failed after PR #716: 1. Provider filter (4.2): selecting a provider did nothing to Purchase History or Approval Queue because setupHistoryHandlers() was exported from history.ts but never called in app.ts. 2. Account filter (4.4): the Savings History chart disappeared when an account UUID was selected because the analytics SQL only matched the legacy account_id VARCHAR(20) column, not cloud_account_id UUID FK. The topbar chip always sends cloud_accounts.id (a UUID), so the WHERE clause returned 0 rows and the chart fell into the empty-state branch. 3. Empty-state UX (4.4/4.5): showEmptyState() always showed "No savings history data available yet." regardless of whether a filter caused the empty result, giving no signal to the user. Changes: - frontend/src/app.ts: import and call setupHistoryHandlers() after initSavingsHistory() so provider/account chip changes reload Purchase History and Approval Queue. - internal/api/analytics_postgres.go: add OR cloud_account_id::text = $3 to the WHERE clause in QueryHistory and QueryBreakdown so UUID-based account filtering includes rows written by current purchase flows. - frontend/src/modules/savings-history.ts: add buildFilterDesc() helper and update showEmptyState() to show "No savings data for the selected filter (AWS)" when a filter is active vs the original message when not. - Tests: new Go tests for UUID account filter path; new TS tests for setupHistoryHandlers subscriptions and filter-aware empty-state copy; update history mocks in index/sidebar-anchors/purchase-execution-toast tests to include setupHistoryHandlers. --- frontend/src/__tests__/history.test.ts | 49 +++++++++- frontend/src/__tests__/index.test.ts | 3 +- .../purchase-execution-toast.test.ts | 1 + .../src/__tests__/savings-history.test.ts | 89 ++++++++++++++++++- .../src/__tests__/sidebar-anchors.test.ts | 2 +- frontend/src/app.ts | 8 +- frontend/src/modules/savings-history.ts | 54 +++++++++-- internal/api/analytics_postgres.go | 14 ++- internal/api/analytics_postgres_test.go | 53 +++++++++++ 9 files changed, 261 insertions(+), 12 deletions(-) diff --git a/frontend/src/__tests__/history.test.ts b/frontend/src/__tests__/history.test.ts index 817870dd4..6c0bd9bc5 100644 --- a/frontend/src/__tests__/history.test.ts +++ b/frontend/src/__tests__/history.test.ts @@ -1,7 +1,7 @@ /** * History module tests */ -import { initHistoryDateRange, viewPlanHistory, loadHistory } from '../history'; +import { initHistoryDateRange, viewPlanHistory, loadHistory, setupHistoryHandlers } from '../history'; // Mock the dependent modules jest.mock('../api', () => ({ @@ -316,4 +316,51 @@ describe('History Module', () => { }); }); }); + + // Issue #701: setupHistoryHandlers must subscribe to the global topbar + // provider/account filter chips so the Purchase History table and + // Approval Queue reload when a chip changes. PR #716 fixed the backend + // filter params but the frontend subscription was never registered in + // app.ts -- adding this suite guards against a regression. + describe('setupHistoryHandlers (issue #701)', () => { + beforeEach(() => { + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [] }); + }); + + test('registers a callback with state.subscribeProvider', () => { + setupHistoryHandlers(); + expect((require('../state').subscribeProvider as jest.Mock)).toHaveBeenCalledTimes(1); + }); + + test('registers a callback with state.subscribeAccount', () => { + setupHistoryHandlers(); + expect((require('../state').subscribeAccount as jest.Mock)).toHaveBeenCalledTimes(1); + }); + + test('provider change triggers loadHistory', async () => { + setupHistoryHandlers(); + const stateModule = require('../state'); + const providerCb = (stateModule.subscribeProvider as jest.Mock).mock.calls[0]?.[0] as () => void; + expect(typeof providerCb).toBe('function'); + + (api.getHistory as jest.Mock).mockClear(); + providerCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.getHistory).toHaveBeenCalledTimes(1); + }); + + test('account change triggers loadHistory', async () => { + setupHistoryHandlers(); + const stateModule = require('../state'); + const accountCb = (stateModule.subscribeAccount as jest.Mock).mock.calls[0]?.[0] as () => void; + expect(typeof accountCb).toBe('function'); + + (api.getHistory as jest.Mock).mockClear(); + accountCb(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.getHistory).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/frontend/src/__tests__/index.test.ts b/frontend/src/__tests__/index.test.ts index 8f9baebfc..015724152 100644 --- a/frontend/src/__tests__/index.test.ts +++ b/frontend/src/__tests__/index.test.ts @@ -19,7 +19,8 @@ jest.mock('../recommendations', () => ({ })); jest.mock('../history', () => ({ - loadHistory: jest.fn() + loadHistory: jest.fn(), + setupHistoryHandlers: jest.fn(), })); jest.mock('../settings', () => ({ diff --git a/frontend/src/__tests__/purchase-execution-toast.test.ts b/frontend/src/__tests__/purchase-execution-toast.test.ts index 6c2317f7e..b4edce4df 100644 --- a/frontend/src/__tests__/purchase-execution-toast.test.ts +++ b/frontend/src/__tests__/purchase-execution-toast.test.ts @@ -77,6 +77,7 @@ jest.mock('../apikeys', () => ({ jest.mock('../history', () => ({ loadHistory: jest.fn(), + setupHistoryHandlers: jest.fn(), })); jest.mock('../modules/savings-history', () => ({ diff --git a/frontend/src/__tests__/savings-history.test.ts b/frontend/src/__tests__/savings-history.test.ts index 0939323d9..58d031bbd 100644 --- a/frontend/src/__tests__/savings-history.test.ts +++ b/frontend/src/__tests__/savings-history.test.ts @@ -55,7 +55,10 @@ describe('Savings History Module', () => {
- +
$0 $0/hr @@ -975,4 +978,88 @@ describe('Savings History Module', () => { expect(mockChartInstance.destroy).toHaveBeenCalled(); }); }); + + // Issue #701: when the chart returns no data because a filter is active, the + // empty-state message must distinguish "nothing in this scope" from "no data + // at all". Without a filter the original message applies; with a filter the + // message should name the active filter so the user understands why the chart + // is blank. + describe('empty-state copy with active filter (issue #701)', () => { + test('shows filter name in empty-state heading when provider chip is set', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('AWS'); + expect(heading?.textContent).not.toContain('available yet'); + }); + + test('shows help text asking to broaden filter when a filter is active', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('gcp'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const help = emptyEl?.querySelector('p.help-text'); + expect(help?.textContent).toMatch(/broaden/i); + }); + + test('shows original message when no filter is active', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('available yet'); + }); + + test('shows filter name in empty-state heading when account chip is set', async () => { + const testUUID = 'aabbccdd-1234-5678-abcd-aabbccddee00'; + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([testUUID]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + expect(heading?.textContent).toContain(testUUID); + }); + + test('shows provider and account in heading when both chips are set', async () => { + const testUUID = 'aabbccdd-1234-5678-abcd-aabbccddee00'; + (state.getCurrentProvider as jest.Mock).mockReturnValue('azure'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([testUUID]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('AZURE'); + expect(heading?.textContent).toContain(testUUID); + }); + + test('shows filter-aware message on API error with active filter', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockRejectedValue(new Error('network failure')); + console.error = jest.fn(); + + await loadSavingsHistory(); + + const emptyEl = document.getElementById('savings-history-empty'); + const heading = emptyEl?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('AWS'); + }); + }); }); diff --git a/frontend/src/__tests__/sidebar-anchors.test.ts b/frontend/src/__tests__/sidebar-anchors.test.ts index f5abd9e07..506b7c3e9 100644 --- a/frontend/src/__tests__/sidebar-anchors.test.ts +++ b/frontend/src/__tests__/sidebar-anchors.test.ts @@ -49,7 +49,7 @@ jest.mock('../settings', () => ({ })); jest.mock('../users', () => ({ setupUserHandlers: jest.fn() })); jest.mock('../apikeys', () => ({ initApiKeys: jest.fn() })); -jest.mock('../history', () => ({ loadHistory: jest.fn() })); +jest.mock('../history', () => ({ loadHistory: jest.fn(), setupHistoryHandlers: jest.fn() })); jest.mock('../modules/savings-history', () => ({ initSavingsHistory: jest.fn() })); jest.mock('../riexchange', () => ({ setupRIExchangeHandlers: jest.fn(), diff --git a/frontend/src/app.ts b/frontend/src/app.ts index 242d190d5..124b7e0dc 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -12,7 +12,7 @@ import { savePlan, setupPlanHandlers, closePlanModal, openNewPlanModal, closePur import { saveGlobalSettings, setupSettingsHandlers, resetSettings } from './settings'; import { setupUserHandlers } from './users'; import { initApiKeys } from './apikeys'; -import { loadHistory } from './history'; +import { loadHistory, setupHistoryHandlers } from './history'; import { initSavingsHistory } from './modules/savings-history'; import { setupRIExchangeHandlers, saveAutomationSettings } from './riexchange'; import { showToast } from './toast'; @@ -167,6 +167,12 @@ export function setupEventListeners(): void { // Setup savings history charts initSavingsHistory(); + // Wire provider/account topbar filter chips to Purchase History + + // Approval Queue (issue #701). initSavingsHistory above subscribes + // the chart; setupHistoryHandlers subscribes the two list consumers + // so all three reload together when a chip changes. + setupHistoryHandlers(); + // Setup feedback link setupFeedbackLink(); diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index 7b3926916..200da7314 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -35,6 +35,11 @@ export async function loadSavingsHistory(): Promise { const currentProvider = state.getCurrentProvider(); const currentAccountIDs = state.getCurrentAccountIDs(); + // Build a human-readable description of the active filter so the + // empty-state message distinguishes "no data yet" from "nothing in + // the selected scope". Used when the API returns 0 data points. + const filterDesc = buildFilterDesc(currentProvider, currentAccountIDs); + try { const data = await getSavingsAnalytics({ start: start.toISOString(), @@ -45,7 +50,7 @@ export async function loadSavingsHistory(): Promise { }); if (!data.data_points || data.data_points.length === 0) { - showEmptyState(chartContainer, emptyEl, statsEl); + showEmptyState(chartContainer, emptyEl, statsEl, filterDesc); return; } @@ -58,20 +63,46 @@ export async function loadSavingsHistory(): Promise { renderSavingsChart(data.data_points, interval); } catch (error) { console.error('Failed to load savings history:', error instanceof Error ? error.message : 'Unknown error'); - showEmptyState(chartContainer, emptyEl, statsEl); + showEmptyState(chartContainer, emptyEl, statsEl, filterDesc); } } /** - * Show empty state when no data is available + * Show empty state when no data is available. + * + * filterDesc is a short human-readable description of the active + * topbar filter (e.g. "AWS" or "account abc-123"). When non-empty + * the empty-state copy says "no data for the selected filter" so + * the user understands the chart is hidden because of scoping, not + * because no purchases exist at all (issue #701). */ function showEmptyState( chartContainer: HTMLElement | null | undefined, emptyEl: HTMLElement | null, - statsEl: HTMLElement | null + statsEl: HTMLElement | null, + filterDesc: string = '', ): void { if (chartContainer) chartContainer.classList.add('hidden'); - if (emptyEl) emptyEl.classList.remove('hidden'); + if (emptyEl) { + emptyEl.classList.remove('hidden'); + // Update the copy inside the empty-state element so the message + // reflects whether a filter is active. The element is always + // present in the DOM (hidden by CSS class, not removed), so we + // can safely write innerHTML here — this is the only place that + // mutates it and the content is built from trusted constants. + const heading = emptyEl.querySelector('p:first-child'); + const help = emptyEl.querySelector('p.help-text'); + if (heading) { + heading.textContent = filterDesc + ? `No savings data for the selected filter (${filterDesc}).` + : 'No savings history data available yet.'; + } + if (help) { + help.textContent = filterDesc + ? 'Try broadening the filter or selecting a different period.' + : 'Data will be collected hourly once you have active purchases.'; + } + } if (statsEl) statsEl.classList.add('hidden'); // Clear any existing chart @@ -81,6 +112,19 @@ function showEmptyState( } } +/** + * Build a short human-readable description of the active topbar filter + * for use in the Savings History empty-state message. Returns '' when + * no filter is active (no chip selected) so callers can distinguish + * "unfiltered empty" from "filtered empty". + */ +function buildFilterDesc(provider: string, accountIDs: readonly string[]): string { + const parts: string[] = []; + if (provider) parts.push(provider.toUpperCase()); + if (accountIDs.length > 0) parts.push(accountIDs[0] ?? ''); + return parts.join(', '); +} + /** * Get start/end dates and interval based on period selection */ diff --git a/internal/api/analytics_postgres.go b/internal/api/analytics_postgres.go index 747d7e757..1deed6401 100644 --- a/internal/api/analytics_postgres.go +++ b/internal/api/analytics_postgres.go @@ -93,6 +93,13 @@ func (c *PostgresAnalyticsClient) QueryHistory( // both the top-line bucket totals and the by_service / by_provider // breakdowns without a second trip to the DB. // + // account_id is the legacy VARCHAR(20) external-account identifier + // (e.g. a 12-digit AWS account number) written by older code paths. + // cloud_account_id is the UUID FK to cloud_accounts added in migration + // 000011 and written by all current purchase flows. The topbar Account + // chip sends the cloud_accounts.id UUID, so we match BOTH columns so + // that rows written by either path are included (issue #701). + // // #nosec G201 — `unit` is allowlisted by intervalToTruncUnit above. query := fmt.Sprintf(` SELECT date_trunc('%s', timestamp) AS bucket, @@ -104,7 +111,7 @@ func (c *PostgresAnalyticsClient) QueryHistory( FROM purchase_history WHERE timestamp >= $1 AND timestamp <= $2 - AND ($3 = '' OR account_id = $3) + AND ($3 = '' OR account_id = $3 OR cloud_account_id::text = $3) GROUP BY bucket, service, provider ORDER BY bucket ASC `, unit) @@ -186,6 +193,9 @@ func (c *PostgresAnalyticsClient) QueryBreakdown( return nil, err } + // account_id / cloud_account_id dual-column match: see QueryHistory + // comment for rationale (issue #701). + // // #nosec G201 — `column` is allowlisted by dimensionToColumn above. query := fmt.Sprintf(` SELECT %s AS bucket, @@ -195,7 +205,7 @@ func (c *PostgresAnalyticsClient) QueryBreakdown( FROM purchase_history WHERE timestamp >= $1 AND timestamp <= $2 - AND ($3 = '' OR account_id = $3) + AND ($3 = '' OR account_id = $3 OR cloud_account_id::text = $3) GROUP BY bucket ORDER BY savings DESC `, column) diff --git a/internal/api/analytics_postgres_test.go b/internal/api/analytics_postgres_test.go index df4789aab..6f7085ee7 100644 --- a/internal/api/analytics_postgres_test.go +++ b/internal/api/analytics_postgres_test.go @@ -164,3 +164,56 @@ func TestQueryBreakdown_ZeroTotalYieldsZeroPct(t *testing.T) { assert.Equal(t, 0.0, out["ec2"].Percentage, "percentage must be 0 when total savings is 0") assert.NoError(t, mock.ExpectationsWereMet()) } + +// TestQueryHistory_CloudAccountIDFilter verifies that QueryHistory accepts a +// cloud_accounts UUID as the accountID parameter (issue #701). The WHERE +// clause matches on BOTH account_id (legacy VARCHAR(20) external ID) and +// cloud_account_id::text (UUID FK), so rows written by either code path are +// included. pgxmock validates the SQL shape and arg binding. +func TestQueryHistory_CloudAccountIDFilter(t *testing.T) { + client, mock := newMockAnalyticsClient(t) + ctx := context.Background() + uuid := "aabbccdd-1234-5678-abcd-aabbccddee00" + + bucket := time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC) + rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}). + AddRow(bucket, "ec2", "aws", 50.0, 20.0, 1) + + mock.ExpectQuery(`SELECT date_trunc\('day', timestamp\)`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid). + WillReturnRows(rows) + + start := time.Date(2026, 4, 25, 0, 0, 0, 0, time.UTC) + end := time.Date(2026, 5, 5, 0, 0, 0, 0, time.UTC) + points, summary, err := client.QueryHistory(ctx, uuid, start, end, "daily") + require.NoError(t, err) + require.Len(t, points, 1) + assert.InDelta(t, 50.0, points[0].TotalSavings, 1e-9) + require.NotNil(t, summary) + assert.Equal(t, 1, summary.TotalPurchases) + assert.NoError(t, mock.ExpectationsWereMet()) +} + +// TestQueryBreakdown_CloudAccountIDFilter verifies that QueryBreakdown accepts +// a UUID as the accountID (issue #701). Mirrors TestQueryHistory_CloudAccountIDFilter. +func TestQueryBreakdown_CloudAccountIDFilter(t *testing.T) { + client, mock := newMockAnalyticsClient(t) + ctx := context.Background() + uuid := "aabbccdd-1234-5678-abcd-aabbccddee00" + + rows := mock.NewRows([]string{"bucket", "savings", "upfront", "purchases"}). + AddRow("rds", 200.0, 80.0, 3) + + mock.ExpectQuery(`SELECT service AS bucket`). + WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid). + WillReturnRows(rows) + + out, err := client.QueryBreakdown(ctx, uuid, + time.Date(2026, 4, 1, 0, 0, 0, 0, time.UTC), + time.Date(2026, 5, 1, 0, 0, 0, 0, time.UTC), + "service") + require.NoError(t, err) + require.Len(t, out, 1) + assert.InDelta(t, 200.0, out["rds"].TotalSavings, 1e-9) + assert.NoError(t, mock.ExpectationsWereMet()) +} From 2083142174c70305c2191ab8c22d33dd9c9bd5de Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 27 May 2026 11:54:32 +0200 Subject: [PATCH 2/2] fix(savings-history): address CR findings on PR #741 Three CodeRabbit findings addressed: 1. (Major, line 67) Use explicit error state on fetch failure: add showErrorState() that sets distinct heading/help copy ("Failed to load savings history. / Please retry.") instead of the no-data empty state, so users can tell a filter-scope empty from a real outage. Update the catch path to call showErrorState with the extracted error message. 2. (Minor, line 125) Treat the 'all' provider sentinel as unfiltered in buildFilterDesc: skip pushing provider.toUpperCase() when the value is 'all' (case-insensitive), so the empty-state heading reads "No savings history data available yet." rather than "No savings data for the selected filter (ALL)." when no real provider chip is selected. 3. (Minor, line 184) Tighten SQL expectations in TestQueryHistory_CloudAccountIDFilter and TestQueryBreakdown_CloudAccountIDFilter: prefix regexes with (?s) and extend to include the dual-column account filter WHERE fragment, so removing that clause would now cause the mock expectations to fail instead of passing silently. Tests: all 66 savings-history frontend tests pass; Go analytics tests pass (including the two tightened CloudAccountID tests). --- .../src/__tests__/savings-history.test.ts | 80 ++++++++++++++++++- frontend/src/modules/savings-history.ts | 33 +++++++- internal/api/analytics_postgres_test.go | 4 +- 3 files changed, 110 insertions(+), 7 deletions(-) diff --git a/frontend/src/__tests__/savings-history.test.ts b/frontend/src/__tests__/savings-history.test.ts index 58d031bbd..e952f0353 100644 --- a/frontend/src/__tests__/savings-history.test.ts +++ b/frontend/src/__tests__/savings-history.test.ts @@ -1049,7 +1049,12 @@ describe('Savings History Module', () => { expect(heading?.textContent).toContain(testUUID); }); - test('shows filter-aware message on API error with active filter', async () => { + test('shows distinct error copy on API failure (not the filter-aware empty state)', async () => { + // CR feedback on PR #741: a fetch failure used to fall through to the + // "no data for the selected filter" empty state, which masked real + // outages (network/5xx/auth). The error path must now render its own + // copy so the user can tell "filter matched nothing" from "the fetch + // never completed". (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); (getSavingsAnalytics as jest.Mock).mockRejectedValue(new Error('network failure')); @@ -1059,7 +1064,78 @@ describe('Savings History Module', () => { const emptyEl = document.getElementById('savings-history-empty'); const heading = emptyEl?.querySelector('p:first-child'); - expect(heading?.textContent).toContain('AWS'); + const help = emptyEl?.querySelector('p.help-text'); + expect(heading?.textContent).toBe('Failed to load savings history.'); + expect(help?.textContent).toContain('network failure'); + // Error copy is NOT the empty-state copy — it does not name the filter. + expect(heading?.textContent).not.toContain('AWS'); + expect(heading?.textContent).not.toContain('available yet'); + }); + + test('error state hides chart container and stats', async () => { + (getSavingsAnalytics as jest.Mock).mockRejectedValue(new Error('boom')); + console.error = jest.fn(); + + await loadSavingsHistory(); + + const chartContainer = document.getElementById('savings-history-chart')?.parentElement; + const statsEl = document.getElementById('savings-stats'); + expect(chartContainer?.classList.contains('hidden')).toBe(true); + expect(statsEl?.classList.contains('hidden')).toBe(true); + }); + + test('error state handles non-Error rejection with "Unknown error"', async () => { + // Generic reject value (e.g. a thrown string from a non-Error catch path) + // must still surface a helpful message rather than `[object Object]`. + (getSavingsAnalytics as jest.Mock).mockRejectedValue('something broke'); + console.error = jest.fn(); + + await loadSavingsHistory(); + + const help = document.getElementById('savings-history-empty')?.querySelector('p.help-text'); + expect(help?.textContent).toContain('Unknown error'); + }); + }); + + // CR feedback on PR #741 F2: a provider chip set to the "all" sentinel + // is the user explicitly choosing unfiltered, so buildFilterDesc must not + // include "ALL" in the empty-state copy. + describe('buildFilterDesc — "all" sentinel handling (PR #741 F2)', () => { + test('treats provider === "all" as unfiltered (no provider clause in heading)', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('all'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const heading = document.getElementById('savings-history-empty')?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('available yet'); + expect(heading?.textContent).not.toMatch(/all/i); + }); + + test('treats provider === "ALL" (case-insensitive) as unfiltered', async () => { + (state.getCurrentProvider as jest.Mock).mockReturnValue('ALL'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const heading = document.getElementById('savings-history-empty')?.querySelector('p:first-child'); + expect(heading?.textContent).toContain('available yet'); + expect(heading?.textContent).not.toContain('ALL'); + }); + + test('still shows account clause when provider === "all" but account chip is set', async () => { + const testUUID = 'aabbccdd-1234-5678-abcd-aabbccddee00'; + (state.getCurrentProvider as jest.Mock).mockReturnValue('all'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([testUUID]); + (getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadSavingsHistory(); + + const heading = document.getElementById('savings-history-empty')?.querySelector('p:first-child'); + expect(heading?.textContent).toContain(testUUID); + expect(heading?.textContent).not.toMatch(/all,/i); }); }); }); diff --git a/frontend/src/modules/savings-history.ts b/frontend/src/modules/savings-history.ts index 200da7314..928e4e651 100644 --- a/frontend/src/modules/savings-history.ts +++ b/frontend/src/modules/savings-history.ts @@ -62,8 +62,9 @@ export async function loadSavingsHistory(): Promise { renderSavingsStats(data); renderSavingsChart(data.data_points, interval); } catch (error) { - console.error('Failed to load savings history:', error instanceof Error ? error.message : 'Unknown error'); - showEmptyState(chartContainer, emptyEl, statsEl, filterDesc); + const msg = error instanceof Error ? error.message : 'Unknown error'; + console.error('Failed to load savings history:', msg); + showErrorState(chartContainer, emptyEl, statsEl, msg); } } @@ -112,6 +113,32 @@ function showEmptyState( } } +/** + * Show an explicit error state when the API call fails. Reuses the empty-state + * DOM element but sets copy that makes clear this is a fetch failure, not + * simply a lack of data (issue #701 CR finding). + */ +function showErrorState( + chartContainer: HTMLElement | null | undefined, + emptyEl: HTMLElement | null, + statsEl: HTMLElement | null, + message: string, +): void { + if (chartContainer) chartContainer.classList.add('hidden'); + if (statsEl) statsEl.classList.add('hidden'); + if (emptyEl) { + emptyEl.classList.remove('hidden'); + const heading = emptyEl.querySelector('p:first-child'); + const help = emptyEl.querySelector('p.help-text'); + if (heading) heading.textContent = 'Failed to load savings history.'; + if (help) help.textContent = `Please retry. (${message})`; + } + if (savingsChart) { + savingsChart.destroy(); + savingsChart = null; + } +} + /** * Build a short human-readable description of the active topbar filter * for use in the Savings History empty-state message. Returns '' when @@ -120,7 +147,7 @@ function showEmptyState( */ function buildFilterDesc(provider: string, accountIDs: readonly string[]): string { const parts: string[] = []; - if (provider) parts.push(provider.toUpperCase()); + if (provider && provider.toLowerCase() !== 'all') parts.push(provider.toUpperCase()); if (accountIDs.length > 0) parts.push(accountIDs[0] ?? ''); return parts.join(', '); } diff --git a/internal/api/analytics_postgres_test.go b/internal/api/analytics_postgres_test.go index 6f7085ee7..e05bef11e 100644 --- a/internal/api/analytics_postgres_test.go +++ b/internal/api/analytics_postgres_test.go @@ -179,7 +179,7 @@ func TestQueryHistory_CloudAccountIDFilter(t *testing.T) { rows := mock.NewRows([]string{"bucket", "service", "provider", "savings", "upfront", "purchases"}). AddRow(bucket, "ec2", "aws", 50.0, 20.0, 1) - mock.ExpectQuery(`SELECT date_trunc\('day', timestamp\)`). + mock.ExpectQuery(`(?s)SELECT date_trunc\('day', timestamp\).*AND \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`). WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid). WillReturnRows(rows) @@ -204,7 +204,7 @@ func TestQueryBreakdown_CloudAccountIDFilter(t *testing.T) { rows := mock.NewRows([]string{"bucket", "savings", "upfront", "purchases"}). AddRow("rds", 200.0, 80.0, 3) - mock.ExpectQuery(`SELECT service AS bucket`). + mock.ExpectQuery(`(?s)SELECT service AS bucket.*AND \(\$3 = '' OR account_id = \$3 OR cloud_account_id::text = \$3\)`). WithArgs(pgxmock.AnyArg(), pgxmock.AnyArg(), uuid). WillReturnRows(rows)