From 2c78990223cd9fea1c6754122d3f8be5d86de4fe Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Sat, 24 Oct 2026 01:48:30 +0000 Subject: [PATCH] fix(frontend): sign out on storage clear, own-property display lookups, real YTD sparkline - Cross-tab sign-out ignored the storage event localStorage.clear() fires (key === null), so a tab whose storage was wiped elsewhere kept a signed-in view. A null key now clears in-memory auth and reloads, unless the tab is already signed out (A11-023). - PROVIDER_DISPLAY_NAMES, PAYMENT_ORDER and PAYMENT_DISPLAY_LABELS were indexed by API strings through the prototype chain, so a provider named "constructor" rendered "function Object() { [native code] }" as a card heading. Lookups go through a new lib/own-property ownValue() helper (A12-048). - The YTD Savings tile's sparkline reused the trend chart's points, so the 7d/30d/90d/All toggle reshaped a line labelled YTD. It now plots its own 1 January (UTC, as the backend's ytd_savings uses) to now series with the same chip scope (A12-073). Refs #317 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/crosstab-session.test.ts | 11 ++- frontend/src/__tests__/dashboard.test.ts | 90 +++++++++++++++++-- frontend/src/__tests__/inventory.test.ts | 12 +++ frontend/src/__tests__/own-property.test.ts | 17 ++++ frontend/src/api/client.ts | 17 ++-- frontend/src/dashboard.ts | 43 +++++++-- frontend/src/inventory.ts | 3 +- frontend/src/lib/own-property.ts | 11 +++ frontend/src/recommendations.ts | 11 +-- .../tests-e2e/crosstab-storage-clear.spec.ts | 25 ++++++ 10 files changed, 215 insertions(+), 25 deletions(-) create mode 100644 frontend/src/__tests__/own-property.test.ts create mode 100644 frontend/src/lib/own-property.ts create mode 100644 frontend/tests-e2e/crosstab-storage-clear.spec.ts diff --git a/frontend/src/__tests__/crosstab-session.test.ts b/frontend/src/__tests__/crosstab-session.test.ts index ab35a0bc5..b30c113a0 100644 --- a/frontend/src/__tests__/crosstab-session.test.ts +++ b/frontend/src/__tests__/crosstab-session.test.ts @@ -63,11 +63,18 @@ describe('cross-tab logout sync (issue #493)', () => { expect(reload).not.toHaveBeenCalled(); }); - test('ignores events with null key (e.g. localStorage.clear())', () => { + test('localStorage.clear() in another tab (key === null) signs this tab out (#317)', () => { const { api, handler, reload } = loadAuth(); api.setAuthToken('t'); handler(evt(null, null)); - expect(api.isAuthenticated()).toBe(true); + expect(api.isAuthenticated()).toBe(false); + expect(reload).toHaveBeenCalledTimes(1); + }); + + test('localStorage.clear() does not reload a tab that is already signed out', () => { + const { api, handler, reload } = loadAuth(); + handler(evt(null, null)); + expect(api.isAuthenticated()).toBe(false); expect(reload).not.toHaveBeenCalled(); }); diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index 9a296bec6..7af1d3721 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -71,7 +71,7 @@ jest.mock('../api', () => ({ // value don't need to be updated. getRecommendations: jest.fn().mockResolvedValue([]), })); -import { loadSavingsTrendChart, setupSavingsTrendHandlers, setupDashboardHandlers, formatTrendAxisTick } from '../dashboard'; +import { loadSavingsTrendChart, loadYtdSparkline, setupSavingsTrendHandlers, setupDashboardHandlers, formatTrendAxisTick } from '../dashboard'; // Mock state module jest.mock('../state', () => ({ @@ -1213,10 +1213,11 @@ describe('Dashboard Module', () => { accountCb(); await new Promise((r) => setTimeout(r, 0)); - expect(api.getSavingsAnalytics).toHaveBeenCalledTimes(1); - expect(api.getSavingsAnalytics).toHaveBeenCalledWith( - expect.objectContaining({ account_ids: ['uuid-acct-1'] }) - ); + // Two scoped series: the trend chart and the YTD tile sparkline (#317). + expect(api.getSavingsAnalytics).toHaveBeenCalledTimes(2); + for (const [filters] of (api.getSavingsAnalytics as jest.Mock).mock.calls) { + expect(filters).toEqual(expect.objectContaining({ account_ids: ['uuid-acct-1'] })); + } }); test('does NOT fire when home tab is inactive (active-tab guard)', async () => { @@ -2226,3 +2227,82 @@ describe('Dashboard Module', () => { }); }); }); + +describe('YTD Savings tile sparkline (#317)', () => { + function addYtdTile(): SVGSVGElement { + const svg = document.createElementNS('http://www.w3.org/2000/svg', 'svg'); + svg.classList.add('kpi-tile-spark', 'hidden'); + svg.dataset['sparkKey'] = 'ytd'; + const canvas = document.createElement('canvas'); + canvas.id = 'savings-trend-chart'; + const b7 = document.createElement('button'); + b7.className = 'trend-range'; + b7.dataset['range'] = '7'; + document.body.replaceChildren(svg, canvas, b7); + return svg; + } + + beforeEach(() => { + jest.clearAllMocks(); + (state.getCurrentProvider as jest.Mock).mockReturnValue('aws'); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue(['uuid-acct-1']); + }); + + test('requests 1 January (UTC) to now, scoped by the chips, whatever the trend range', async () => { + addYtdTile(); + (api.getSavingsAnalytics as jest.Mock).mockResolvedValue({ data_points: [] }); + + await loadYtdSparkline(); + + expect(api.getSavingsAnalytics).toHaveBeenCalledTimes(1); + const filters = (api.getSavingsAnalytics as jest.Mock).mock.calls[0][0]; + const start = new Date(filters.start); + expect(start.toISOString()).toBe(new Date(Date.UTC(new Date().getUTCFullYear(), 0, 1)).toISOString()); + expect(filters).toEqual(expect.objectContaining({ provider: 'aws', account_ids: ['uuid-acct-1'] })); + }); + + test('draws the YTD series into the tile', async () => { + const svg = addYtdTile(); + (api.getSavingsAnalytics as jest.Mock).mockResolvedValue({ + data_points: [ + { timestamp: '2026-01-05T00:00:00Z', cumulative_savings: 10 }, + { timestamp: '2026-05-05T00:00:00Z', cumulative_savings: 40 }, + ], + }); + + await loadYtdSparkline(); + + expect(svg.classList.contains('hidden')).toBe(false); + expect(svg.querySelector('polyline')).not.toBeNull(); + }); + + test('the trend chart (7-day range) no longer redraws the YTD tile', async () => { + const svg = addYtdTile(); + (api.getSavingsAnalytics as jest.Mock).mockResolvedValue({ + data_points: [ + { timestamp: new Date(Date.now() - 2 * 86400_000).toISOString(), cumulative_savings: 1 }, + { timestamp: new Date().toISOString(), cumulative_savings: 2 }, + ], + }); + setupSavingsTrendHandlers(); + (document.querySelector('.trend-range[data-range="7"]') as HTMLButtonElement).click(); + await new Promise((r) => setTimeout(r, 0)); + + expect(api.getSavingsAnalytics).toHaveBeenCalledTimes(1); + expect(svg.querySelector('polyline')).toBeNull(); + }); + + test('a failed fetch hides the sparkline and a missing tile skips the request', async () => { + const svg = addYtdTile(); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + (api.getSavingsAnalytics as jest.Mock).mockRejectedValue(new Error('503')); + await loadYtdSparkline(); + expect(svg.classList.contains('hidden')).toBe(true); + + document.body.replaceChildren(); + (api.getSavingsAnalytics as jest.Mock).mockClear(); + await loadYtdSparkline(); + expect(api.getSavingsAnalytics).not.toHaveBeenCalled(); + warn.mockRestore(); + }); +}); diff --git a/frontend/src/__tests__/inventory.test.ts b/frontend/src/__tests__/inventory.test.ts index d8feb5bba..a0efa396a 100644 --- a/frontend/src/__tests__/inventory.test.ts +++ b/frontend/src/__tests__/inventory.test.ts @@ -434,6 +434,18 @@ describe('loadCoverageBreakdown — fetch + render flow', () => { clearDOM(); }); + test('a provider named like an Object.prototype member renders as text, not a function (#317)', async () => { + (api.getCoverageBreakdown as jest.Mock).mockResolvedValue({ + providers: [makeProviderSection('constructor', null, null), makeProviderSection('toString', null, null)], + }); + + await loadCoverageBreakdown(); + + const titles = Array.from(document.querySelectorAll('#coverage-providers .coverage-provider-card h3')) + .map((h) => h.textContent); + expect(titles).toEqual(['CONSTRUCTOR', 'TOSTRING']); + }); + test('renders per-provider sections with service rows', async () => { (api.getCoverageBreakdown as jest.Mock).mockResolvedValue({ providers: [ diff --git a/frontend/src/__tests__/own-property.test.ts b/frontend/src/__tests__/own-property.test.ts new file mode 100644 index 000000000..93bedcddc --- /dev/null +++ b/frontend/src/__tests__/own-property.test.ts @@ -0,0 +1,17 @@ +import { ownValue } from '../lib/own-property'; + +describe('ownValue (#317)', () => { + const table: Record = { aws: 'AWS', 'no-upfront': 'No Upfront' }; + + test('returns own entries', () => { + expect(ownValue(table, 'aws')).toBe('AWS'); + expect(ownValue(table, 'no-upfront')).toBe('No Upfront'); + }); + + test.each(['constructor', 'toString', '__proto__', 'hasOwnProperty', 'valueOf', ''])( + 'returns undefined for %p instead of an Object.prototype member', + (key) => { + expect(ownValue(table, key)).toBeUndefined(); + }, + ); +}); diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 2b1d0617f..6ab77dbd7 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -93,11 +93,18 @@ function installStorageListener(): void { if (storageListenerInstalled) return; if (typeof window === 'undefined' || !window.addEventListener) return; window.addEventListener('storage', (e: StorageEvent) => { - if (!e.key || !(STORAGE_KEYS as readonly string[]).includes(e.key)) return; - // Only react to a *cleared* key (newValue === null); partial - // updates (e.g. token refresh) propagate via the in-memory state - // and don't need a reload. - if (e.newValue !== null) return; + if (e.key === null) { + // localStorage.clear() in another tab (a browser privacy control, + // devtools, a future clear() call) fires with key === null: every + // key, auth included, is gone. Signed-out tabs have nothing to drop. + if (!authToken && !apiKey && !csrfToken) return; + } else { + if (!(STORAGE_KEYS as readonly string[]).includes(e.key)) return; + // Only react to a *cleared* key (newValue === null); partial + // updates (e.g. token refresh) propagate via the in-memory state + // and don't need a reload. + if (e.newValue !== null) return; + } authToken = ''; apiKey = ''; csrfToken = ''; diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index 2a8e61716..751750ac5 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -260,6 +260,7 @@ export async function loadDashboard(): Promise { // Load the savings-over-time widget independently -- failure shouldn't // block the rest of the dashboard (e.g. analytics not configured). void loadSavingsTrendChart(); + void loadYtdSparkline(); // Auto-refresh the recommendations cache if the last collection // is older than 24h (or never ran). The dashboard's KPI tiles + @@ -1153,11 +1154,6 @@ export async function loadSavingsTrendChart(): Promise { const points = data.data_points ?? []; - // YTD Savings KPI tile sparkline (issue #340 T6) — uses the same - // cumulative_savings series the main chart renders. Skips silently - // when the tile isn't in the DOM (e.g. a different layout is mounted). - attachSparkline('ytd', points.map((p: SavingsDataPoint) => p.cumulative_savings || 0)); - // Determine the left edge of the x-axis. // For 'all': anchor to the earliest purchase so the line fills the // chart rather than clustering at the right end. Fall back to now-365d @@ -1198,7 +1194,6 @@ export async function loadSavingsTrendChart(): Promise { empty.classList.remove('hidden'); } if (savingsTrendChart) { savingsTrendChart.destroy(); savingsTrendChart = null; } - attachSparkline('ytd', []); return; } canvas.classList.remove('hidden'); @@ -1260,7 +1255,6 @@ export async function loadSavingsTrendChart(): Promise { // the dashboard — hide the widget and fall back to a neutral message. console.warn('Savings trend chart unavailable:', err); if (savingsTrendChart) { savingsTrendChart.destroy(); savingsTrendChart = null; } - attachSparkline('ytd', []); canvas.classList.add('hidden'); if (empty) { empty.textContent = 'Savings history is not available yet.'; @@ -1269,6 +1263,41 @@ export async function loadSavingsTrendChart(): Promise { } } +let ytdSparklineGeneration = 0; + +/** + * Sparkline for the YTD Savings tile. It used to reuse the trend chart's + * points, whose window follows the 7d/30d/90d/All toggle, so the line under + * "YTD" changed shape with a control that does not change the number. It now + * plots its own series from 1 January (UTC, the same year start the backend + * uses for ytd_savings) to now, scoped by the same provider/account chips. + */ +export async function loadYtdSparkline(): Promise { + if (!document.querySelector('.kpi-tile-spark[data-spark-key="ytd"]')) return; + const generation = ++ytdSparklineGeneration; + const now = new Date(); + const yearStartMs = Date.UTC(now.getUTCFullYear(), 0, 1); + const days = (now.getTime() - yearStartMs) / 86400_000; + const interval: 'hourly' | 'daily' | 'weekly' = days <= 7 ? 'hourly' : days <= 90 ? 'daily' : 'weekly'; + const accountIDs = state.getCurrentAccountIDs(); + const provider = state.getCurrentProvider(); + try { + const data = await api.getSavingsAnalytics({ + start: new Date(yearStartMs).toISOString(), + end: now.toISOString(), + interval, + ...(provider ? { provider } : {}), + ...(accountIDs.length > 0 ? { account_ids: accountIDs } : {}), + }); + if (generation !== ytdSparklineGeneration) return; + attachSparkline('ytd', (data.data_points ?? []).map((p: SavingsDataPoint) => p.cumulative_savings || 0)); + } catch (err) { + if (generation !== ytdSparklineGeneration) return; + console.warn('YTD sparkline unavailable:', err); + attachSparkline('ytd', []); + } +} + // Wire up the range-toggle buttons. Called once during dashboard setup. export function setupSavingsTrendHandlers(): void { document.querySelectorAll('.trend-range').forEach((btn) => { diff --git a/frontend/src/inventory.ts b/frontend/src/inventory.ts index 43926d1c2..048144e7d 100644 --- a/frontend/src/inventory.ts +++ b/frontend/src/inventory.ts @@ -17,6 +17,7 @@ import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { formatCurrency, formatDate, amortizedMonthly } from './utils'; import * as state from './state'; import { switchInventorySubTab } from './navigation'; +import { ownValue } from './lib/own-property'; type InventorySubSection = 'active-commitments' | 'coverage' | 'ri-exchange'; @@ -429,7 +430,7 @@ function buildProviderSection( const header = document.createElement('div'); header.className = 'section-header'; - const providerLabel = PROVIDER_DISPLAY_NAMES[section.provider] ?? section.provider.toUpperCase(); + const providerLabel = ownValue(PROVIDER_DISPLAY_NAMES, section.provider) ?? section.provider.toUpperCase(); const title = document.createElement('h3'); title.textContent = providerLabel; diff --git a/frontend/src/lib/own-property.ts b/frontend/src/lib/own-property.ts new file mode 100644 index 000000000..62ce41016 --- /dev/null +++ b/frontend/src/lib/own-property.ts @@ -0,0 +1,11 @@ +/** + * Own-property lookup for display/ordering tables keyed by API strings. + * + * `TABLE[key]` walks the prototype chain, so a value such as "constructor" + * or "toString" resolves to an Object.prototype member instead of falling + * through to the caller's `??` default. The same shape reached production via + * tab routing (cloud-commitments-cli#1855) before #1854 fixed it there. + */ +export function ownValue(table: Readonly>, key: string): T | undefined { + return Object.prototype.hasOwnProperty.call(table, key) ? table[key] : undefined; +} diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 307ff2d7a..e866a1750 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -34,6 +34,7 @@ function canActOnRecommendations(): boolean { } import { parseNumericFilter, applyColumnFilters as applyColumnFiltersLib } from './lib/column-filters'; import { fetchOverridesForAccounts } from './lib/overrides'; +import { ownValue } from './lib/own-property'; // Re-export the shared primitives so existing consumers that import from // recommendations.ts keep working without import-path churn (issue #166). export { parseNumericFilter } from './lib/column-filters'; @@ -1322,8 +1323,8 @@ function sortVariantsInCell(variants: LocalRecommendation[]): LocalRecommendatio return variants.slice().sort((a, b) => { const termDiff = a.term - b.term; if (termDiff !== 0) return termDiff; - const pa = PAYMENT_ORDER[a.payment ?? ''] ?? 99; - const pb = PAYMENT_ORDER[b.payment ?? ''] ?? 99; + const pa = ownValue(PAYMENT_ORDER, a.payment ?? '') ?? 99; + const pb = ownValue(PAYMENT_ORDER, b.payment ?? '') ?? 99; return pa - pb; }); } @@ -1441,7 +1442,7 @@ function cellScoreFor( // canonical order. Stringified so the existing string-column comparator // path handles it. const first = sortVariantsInCell(variants)[0]; - return String(PAYMENT_ORDER[first?.payment ?? ''] ?? 99); + return String(ownValue(PAYMENT_ORDER, first?.payment ?? '') ?? 99); } // All other numeric columns (savings, count, on_demand_monthly): preserve @@ -3062,7 +3063,7 @@ const PAYMENT_DISPLAY_LABELS: Record = { function formatPayment(payment: string | undefined): string { if (!payment) return '\u2014'; - return PAYMENT_DISPLAY_LABELS[payment] ?? payment; + return ownValue(PAYMENT_DISPLAY_LABELS, payment) ?? payment; } // renderUsageSparkline returns an inline SVG polyline representing the @@ -3203,7 +3204,7 @@ function buildVariantRowMarkup( rec.region, accountName, formatTerm(rec.term), - PAYMENT_DISPLAY_LABELS[rec.payment ?? ''] ?? '', + ownValue(PAYMENT_DISPLAY_LABELS, rec.payment ?? '') ?? '', ].filter(part => part).join(' '), ); const checkboxCell = showCheckboxes diff --git a/frontend/tests-e2e/crosstab-storage-clear.spec.ts b/frontend/tests-e2e/crosstab-storage-clear.spec.ts new file mode 100644 index 000000000..3b5585d07 --- /dev/null +++ b/frontend/tests-e2e/crosstab-storage-clear.spec.ts @@ -0,0 +1,25 @@ +import { test, expect } from '@playwright/test'; +import { mockApi } from './fixtures/recs'; + +// #317: localStorage.clear() in another tab fires a storage event with +// key === null. The cross-tab sign-out listener used to ignore that shape, so +// this tab kept rendering a signed-in view after every key was wiped. +test('localStorage.clear() in another tab signs this tab out', async ({ page, context }) => { + await mockApi(page); + await page.route('**/api/info', route => route.fulfill({ json: { version: 'test', admin_exists: true } })); + // Sign in with a one-off write rather than an init script, which would + // re-seed the token on the reload this test expects. + await page.goto('/'); + await page.evaluate(() => { localStorage.setItem('authToken', 'smoke-token'); localStorage.setItem('csrfToken', 'smoke-csrf'); }); + await page.reload(); + await expect(page.getByRole('button', { name: 'Logout' })).toBeVisible(); + await expect(page.getByRole('heading', { name: 'CUDly Login' })).toHaveCount(0); + + const other = await context.newPage(); + await mockApi(other); + await other.goto('/'); + await other.evaluate(() => localStorage.clear()); + await other.close(); + + await expect(page.getByRole('heading', { name: 'CUDly Login' })).toBeVisible(); +});