From 21cf13f7fbd284e6ad85ba28cab7c28bc1101a1b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 1 Jun 2026 16:07:15 +0200 Subject: [PATCH] feat(ux): URL-addressable sub-tabs (default-first) for Inventory + Admin (closes #902) Resolve QA A.4: Inventory & Coverage remembered its last sub-tab via hidden in-memory session state while Admin always read its sub-tab from the /admin/ URL. The inconsistency was the defect. Make Inventory sub-tabs URL-addressable as /inventory/, matching the existing Admin convention: - loadInventory() now derives the sub-section from the URL path (getInventorySubTabFromPath), not the module-level currentSubSection, so a fresh /inventory lands on the default (active-commitments) and a /inventory/ deep link lands on that sub-tab. - A sub-nav click routes through navigation.switchInventorySubTab, which pushes /inventory/ via history.pushState so the view is shareable/bookmarkable and browser back/forward works. - The history push lives in navigation.ts alongside switchSettingsSubTab so the single historyId counter stays authoritative for the back/forward dirty-guard; inventory.ts keeps the pure DOM view switch. Supersedes the partial PR #757 session-memory behavior with explicit URL state. Both pages now behave identically; existing /admin/* deep links are unaffected. Tests assert, for each page: no param -> default sub-tab, explicit param -> that sub-tab, switching updates the URL (query/hash preserved, no duplicate entry), and unknown param -> default. --- frontend/src/__tests__/inventory.test.ts | 70 +++++++++++++- frontend/src/__tests__/navigation.test.ts | 111 +++++++++++++++++++++- frontend/src/inventory.ts | 52 ++++++++-- frontend/src/navigation.ts | 69 +++++++++++++- 4 files changed, 285 insertions(+), 17 deletions(-) diff --git a/frontend/src/__tests__/inventory.test.ts b/frontend/src/__tests__/inventory.test.ts index 23851792c..4cf38e2f0 100644 --- a/frontend/src/__tests__/inventory.test.ts +++ b/frontend/src/__tests__/inventory.test.ts @@ -34,7 +34,21 @@ jest.mock('../state', () => ({ getCurrentAccountIDs: jest.fn(() => []), })); +// inventory.ts routes sub-nav clicks through navigation.switchInventorySubTab +// so the click both switches the view AND pushes /inventory/ +// (QA A.4). Mock it to delegate to the real (pure) view switcher: that +// keeps the click->DOM behaviour these tests assert, while letting us spy +// on the router call. The URL-push half of switchInventorySubTab is tested +// in navigation.test.ts where the router owns history. +jest.mock('../navigation', () => { + const actual = jest.requireActual('../inventory'); + return { + switchInventorySubTab: jest.fn((name: string) => actual.switchInventorySubSection(name)), + }; +}); + import { loadInventory, switchInventorySubSection, loadActiveCommitments, loadCoverageBreakdown } from '../inventory'; +import { switchInventorySubTab } from '../navigation'; import { loadRIExchange } from '../riexchange'; import * as api from '../api'; import * as state from '../state'; @@ -202,13 +216,67 @@ describe('Inventory & Coverage sub-section switching', () => { expect(document.getElementById('inventory-active-commitments')?.classList.contains('hidden')).toBe(false); expect(document.getElementById('inventory-ri-exchange')?.classList.contains('hidden')).toBe(true); - // Clicking a sub-tab button switches the section. + // Clicking a sub-tab button routes through the router (QA A.4) so the + // click both switches the view AND pushes /inventory/. The + // mocked router delegates to the real view switcher, so the DOM flips. const coverageBtn = document.querySelector('[data-inv-subtab="coverage"]')!; coverageBtn.click(); + expect(switchInventorySubTab).toHaveBeenCalledWith('coverage'); + expect(document.getElementById('inventory-coverage')?.classList.contains('hidden')).toBe(false); + expect(document.getElementById('inventory-active-commitments')?.classList.contains('hidden')).toBe(true); + }); + +}); + +// QA A.4: Inventory sub-tabs are URL-addressable (/inventory/), +// default-first, and shareable. inventory.ts owns the view switch + the +// click->router wiring; the URL push itself lives in navigation.ts and is +// covered in navigation.test.ts. Here we assert: +// - the pure switcher returns the resolved (validated) sub-section, +// - loadInventory honours an explicit sub-section, defaults when absent, +// and falls back when unknown (default-first), +// - a sub-nav click routes through navigation.switchInventorySubTab so +// the URL gets updated (no hidden session state). +describe('Inventory & Coverage sub-tab addressing (QA A.4)', () => { + beforeEach(() => { + buildInventoryDOM(); + (loadRIExchange as jest.Mock).mockClear(); + (switchInventorySubTab as jest.Mock).mockClear(); + (api.listActiveCommitments as jest.Mock).mockReset().mockResolvedValue([]); + (api.getCoverageBreakdown as jest.Mock).mockReset().mockResolvedValue({ providers: [] }); + (state.subscribeProvider as jest.Mock).mockReset().mockReturnValue(jest.fn()); + (state.subscribeAccount as jest.Mock).mockReset().mockReturnValue(jest.fn()); + (state.getCurrentProvider as jest.Mock).mockReturnValue(''); + (state.getCurrentAccountIDs as jest.Mock).mockReturnValue([]); + }); + + afterEach(() => { + clearDOM(); + }); + + test('switchInventorySubSection returns the resolved sub-section', () => { + expect(switchInventorySubSection('coverage')).toBe('coverage'); + // Unknown input resolves to the default (default-first). + expect(switchInventorySubSection('bogus')).toBe('active-commitments'); + }); + + test('(a) loadInventory(undefined) -> default sub-section (active-commitments)', () => { + loadInventory(undefined); + expect(document.getElementById('inventory-active-commitments')?.classList.contains('hidden')).toBe(false); + expect(api.listActiveCommitments).toHaveBeenCalled(); + }); + + test('(b) loadInventory() -> that sub-section', () => { + loadInventory('coverage'); expect(document.getElementById('inventory-coverage')?.classList.contains('hidden')).toBe(false); expect(document.getElementById('inventory-active-commitments')?.classList.contains('hidden')).toBe(true); + expect(api.getCoverageBreakdown).toHaveBeenCalled(); }); + test('(d) loadInventory() -> falls back to default', () => { + loadInventory('bogus-subtab'); + expect(document.getElementById('inventory-active-commitments')?.classList.contains('hidden')).toBe(false); + }); }); describe('loadActiveCommitments — fetch + render flow', () => { diff --git a/frontend/src/__tests__/navigation.test.ts b/frontend/src/__tests__/navigation.test.ts index 14cad345e..72eadf170 100644 --- a/frontend/src/__tests__/navigation.test.ts +++ b/frontend/src/__tests__/navigation.test.ts @@ -1,7 +1,7 @@ /** * Navigation module tests */ -import { switchTab, switchSettingsSubTab, getSettingsSubTabFromPath } from '../navigation'; +import { switchTab, switchSettingsSubTab, switchInventorySubTab, getSettingsSubTabFromPath, getInventorySubTabFromPath } from '../navigation'; // Mock the dependent modules jest.mock('../dashboard', () => ({ @@ -33,6 +33,21 @@ jest.mock('../riexchange', () => ({ jest.mock('../auth', () => ({ isAdmin: jest.fn().mockReturnValue(true), })); +// Mock inventory so navigation tests stay focused on routing/history and +// don't pull in the real fetch/render machinery. switchInventorySubSection +// must still resolve+return the sub-section (default-first) because +// navigation.switchInventorySubTab uses the return value to build the URL. +jest.mock('../inventory', () => { + const VALID = ['active-commitments', 'coverage', 'ri-exchange']; + const DEFAULT = 'active-commitments'; + const isValid = (n: string): boolean => VALID.includes(n); + return { + DEFAULT_INVENTORY_SUB_SECTION: DEFAULT, + isValidInventorySubSection: isValid, + switchInventorySubSection: jest.fn((n: string) => (isValid(n) ? n : DEFAULT)), + loadInventory: jest.fn(), + }; +}); import { loadDashboard } from '../dashboard'; import { loadRecommendations } from '../recommendations'; @@ -164,6 +179,28 @@ describe('Navigation Module', () => { expect(homeBtn?.classList.contains('active')).toBe(false); }); + // QA A.4: a bare inventory switch lands on the default sub-tab and the + // canonical URL carries the sub-tab segment (/inventory/active-commitments), + // mirroring how the admin switch pushes /admin/. + test('switching to inventory pushes /inventory/', () => { + // currentTab is module state that may already be 'inventory' from a + // prior test; switch away first so the inventory switch is genuine + // (a self-switch would correctly skip the push). + switchTab('home'); + window.history.replaceState(null, '', '/'); + switchTab('inventory'); + expect(window.location.pathname).toBe('/inventory/active-commitments'); + }); + + // A deep link to a specific inventory sub-tab is honoured: switchTab + // reads the path and the canonical URL keeps that sub-tab. + test('switching to inventory honours a /inventory/ deep link', () => { + switchTab('home'); + window.history.replaceState(null, '', '/inventory/coverage'); + switchTab('inventory'); + expect(window.location.pathname).toBe('/inventory/coverage'); + }); + test('deactivates previously active tab', () => { // Dashboard is initially active const dashboardBtn = document.querySelector('[data-tab="home"]'); @@ -300,6 +337,50 @@ describe('Navigation Module', () => { }); }); + // QA A.4: switchInventorySubTab owns the /inventory/ history push, + // mirroring switchSettingsSubTab. The DOM switch is delegated to the + // (mocked) inventory module. Placed BEFORE the *FromPath describes, which + // destructively replace window.location with a plain object and would + // otherwise break the real history.pushState these tests rely on. + describe('switchInventorySubTab', () => { + beforeEach(() => { + window.history.replaceState(null, '', '/inventory/active-commitments'); + }); + + test('(c) pushes /inventory/ on a real switch', () => { + switchInventorySubTab('coverage'); + expect(window.location.pathname).toBe('/inventory/coverage'); + }); + + test('(c) preserves existing query params and hash', () => { + window.history.replaceState(null, '', '/inventory/active-commitments?provider=aws#frag'); + switchInventorySubTab('ri-exchange'); + expect(window.location.pathname).toBe('/inventory/ri-exchange'); + expect(window.location.search).toBe('?provider=aws'); + expect(window.location.hash).toBe('#frag'); + }); + + test('(d) an unknown sub-tab resolves to the default in the URL', () => { + window.history.replaceState(null, '', '/inventory/coverage'); + switchInventorySubTab('bogus'); + expect(window.location.pathname).toBe('/inventory/active-commitments'); + }); + + test('does NOT push a duplicate entry when already on the target sub-tab', () => { + window.history.replaceState(null, '', '/inventory/coverage'); + const before = window.history.length; + switchInventorySubTab('coverage'); + expect(window.location.pathname).toBe('/inventory/coverage'); + expect(window.history.length).toBe(before); + }); + + test('push: false switches the view without touching history', () => { + switchInventorySubTab('coverage', { push: false }); + // URL unchanged: the caller (initial load / popstate) owns the URL. + expect(window.location.pathname).toBe('/inventory/active-commitments'); + }); + }); + describe('getSettingsSubTabFromPath', () => { // Canonical /admin/* paths (issue #340 IA rename) test('returns general for root admin path', () => { @@ -351,4 +432,32 @@ describe('Navigation Module', () => { expect(getSettingsSubTabFromPath()).toBe('general'); }); }); + + // QA A.4: Inventory sub-tabs become URL-addressable (/inventory/), + // matching the Admin /admin/ convention. + describe('getInventorySubTabFromPath', () => { + test('returns the default (active-commitments) for a bare /inventory path', () => { + delete (window as unknown as Record).location; + (window as unknown as Record).location = { pathname: '/inventory' } as Location; + expect(getInventorySubTabFromPath()).toBe('active-commitments'); + }); + + test('returns coverage for /inventory/coverage', () => { + delete (window as unknown as Record).location; + (window as unknown as Record).location = { pathname: '/inventory/coverage' } as Location; + expect(getInventorySubTabFromPath()).toBe('coverage'); + }); + + test('returns ri-exchange for /inventory/ri-exchange', () => { + delete (window as unknown as Record).location; + (window as unknown as Record).location = { pathname: '/inventory/ri-exchange' } as Location; + expect(getInventorySubTabFromPath()).toBe('ri-exchange'); + }); + + test('falls back to the default for an unknown sub-tab', () => { + delete (window as unknown as Record).location; + (window as unknown as Record).location = { pathname: '/inventory/bogus' } as Location; + expect(getInventorySubTabFromPath()).toBe('active-commitments'); + }); + }); }); diff --git a/frontend/src/inventory.ts b/frontend/src/inventory.ts index 8d2663610..960be35c6 100644 --- a/frontend/src/inventory.ts +++ b/frontend/src/inventory.ts @@ -16,6 +16,7 @@ import { loadRIExchange } from './riexchange'; import { showSkeletonRows, teardownSkeleton } from './lib/skeleton'; import { formatCurrency, formatDate } from './utils'; import * as state from './state'; +import { switchInventorySubTab } from './navigation'; type InventorySubSection = 'active-commitments' | 'coverage' | 'ri-exchange'; @@ -25,12 +26,17 @@ const SUB_SECTION_IDS: Record = { 'ri-exchange': 'inventory-ri-exchange', }; -const DEFAULT_SUB_SECTION: InventorySubSection = 'active-commitments'; +export const DEFAULT_INVENTORY_SUB_SECTION: InventorySubSection = 'active-commitments'; let currentSubSection: InventorySubSection | undefined; let listenersWired = false; -function isValidSubSection(name: string): name is InventorySubSection { +/** + * Type guard for the Inventory sub-section identifiers. Exported so the + * router (navigation.ts) can validate the `/inventory/` path + * segment without duplicating the closed set. + */ +export function isValidInventorySubSection(name: string): name is InventorySubSection { return name === 'active-commitments' || name === 'coverage' || name === 'ri-exchange'; } @@ -38,9 +44,20 @@ function isValidSubSection(name: string): name is InventorySubSection { * Show one sub-section, hide the others. Activates the matching sub-nav * button and (for ri-exchange) triggers the RI exchange data load so the * existing flow stays identical to its pre-#340 behaviour. + * + * This is the pure view switcher: it does NOT touch the URL. URL history + * (the `/inventory/` addressing from QA A.4) is owned by + * navigation.ts' switchInventorySubTab, mirroring how switchSettingsSubTab + * owns the `/admin/` history so a single counter (historyId) stays + * authoritative for the back/forward dirty-guard. + * + * Returns the resolved (validated, default-substituted) sub-section so the + * caller can reflect the same value in the URL. */ -export function switchInventorySubSection(name: string): void { - const target: InventorySubSection = isValidSubSection(name) ? name : DEFAULT_SUB_SECTION; +export function switchInventorySubSection(name: string): InventorySubSection { + const target: InventorySubSection = isValidInventorySubSection(name) + ? name + : DEFAULT_INVENTORY_SUB_SECTION; document.querySelectorAll('#inventory-tab .sub-tab-btn').forEach((btn) => { const isActive = btn.dataset['invSubtab'] === target; @@ -62,6 +79,7 @@ export function switchInventorySubSection(name: string): void { } currentSubSection = target; + return target; } // ────────────────────────────────────────────── @@ -417,8 +435,11 @@ function wireSubNavListeners(): void { if (buttons.length === 0) return; buttons.forEach((btn) => { btn.addEventListener('click', () => { - const name = btn.dataset['invSubtab'] ?? DEFAULT_SUB_SECTION; - switchInventorySubSection(name); + const name = btn.dataset['invSubtab'] ?? DEFAULT_INVENTORY_SUB_SECTION; + // Route through the router so the click both switches the view AND + // pushes /inventory/ (QA A.4), keeping history consistent + // with the Admin sub-tab flow. + switchInventorySubTab(name); }); }); listenersWired = true; @@ -485,11 +506,22 @@ function wireChipSubscriptions(): void { /** * Initialize the Inventory & Coverage section. Called by navigation.ts' - * switchTab when 'inventory' is selected. Defaults to active-commitments - * if the user hasn't selected a sub-section this session. + * switchTab when 'inventory' is selected, passing the sub-section parsed + * from the `/inventory/` URL path (QA A.4). + * + * The sub-section comes from the URL, not hidden session state: a fresh + * `/inventory` with no sub-segment lands on the default (active-commitments) + * and a `/inventory/` deep link lands on that sub-section. The + * switch is URL-driven (push: false) so re-entering the tab doesn't stack + * a redundant history entry on top of the one switchTab already pushed. */ -export function loadInventory(): void { +export function loadInventory(subSection?: string): void { wireSubNavListeners(); wireChipSubscriptions(); - switchInventorySubSection(currentSubSection ?? DEFAULT_SUB_SECTION); + const target = subSection !== undefined && isValidInventorySubSection(subSection) + ? subSection + : DEFAULT_INVENTORY_SUB_SECTION; + // Pure view switch (no history push): switchTab already pushed the + // canonical /inventory/ URL when this tab was entered. + switchInventorySubSection(target); } diff --git a/frontend/src/navigation.ts b/frontend/src/navigation.ts index 503e6b071..505dfce90 100644 --- a/frontend/src/navigation.ts +++ b/frontend/src/navigation.ts @@ -11,7 +11,12 @@ import { loadUsers } from './users'; import { loadApiKeys } from './apikeys'; import { loadSavingsHistory } from './modules/savings-history'; import { loadAutomationSettings } from './riexchange'; -import { loadInventory } from './inventory'; +import { + loadInventory, + switchInventorySubSection, + isValidInventorySubSection, + DEFAULT_INVENTORY_SUB_SECTION, +} from './inventory'; import { isAdmin } from './auth'; interface TabMeta { @@ -116,7 +121,7 @@ export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { switchSettingsSubTab(getSettingsSubTabFromPath(), { push: false }); break; case 'inventory': - loadInventory(); + loadInventory(getInventorySubTabFromPath()); break; } @@ -129,9 +134,18 @@ export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { if (opts.push !== false) { historyId += 1; - const url = tabName === 'admin' - ? '/admin/' + (currentSettingsSubTab ?? 'general') - : '/' + tabName; + let url: string; + if (tabName === 'admin') { + url = '/admin/' + (currentSettingsSubTab ?? 'general'); + } else if (tabName === 'inventory') { + // Inventory carries a sub-tab segment in the URL (QA A.4), mirroring + // Admin. loadInventory() above already applied the sub-section from + // the path (or the default); reflect that same segment here so the + // canonical URL is /inventory/, never a bare /inventory. + url = '/inventory/' + getInventorySubTabFromPath(); + } else { + url = '/' + tabName; + } window.history.pushState( { tab: tabName, id: historyId }, '', @@ -153,6 +167,51 @@ export function getSettingsSubTabFromPath(): string { return sub in SETTINGS_SUBTABS ? sub : 'general'; } +/** + * Switch between Inventory & Coverage sub-tabs (Active commitments / + * Coverage / RI Exchange) and reflect the selection in the URL as + * `/inventory/` (QA A.4). Mirrors switchSettingsSubTab: the DOM + * switch is delegated to inventory.ts; the history push lives here so the + * single historyId counter stays authoritative for back/forward. + * + * A user-initiated switch (sub-nav click) pushes a new history entry so it + * is shareable/bookmarkable and browser back/forward works. A no-op switch + * (already on the target sub-tab) does not push. + */ +export function switchInventorySubTab(subTab: string, opts: SwitchTabOptions = {}): void { + const before = window.location.pathname; + const target = switchInventorySubSection(subTab); + const canonical = '/inventory/' + target; + + if (opts.push === false) return; + // Skip the push when the canonical URL already points at this sub-tab, + // so a redundant click doesn't stack duplicate history entries. + if (before === canonical) return; + + historyId += 1; + window.history.pushState( + { tab: 'inventory', subTab: target, id: historyId }, + '', + canonical + window.location.search + window.location.hash, + ); +} + +/** + * Parse the Inventory sub-tab from segment[1] of the current URL (QA A.4). + * Mirrors getSettingsSubTabFromPath: a `/inventory/` deep link + * resolves to that sub-section; a bare `/inventory` (or any unknown + * segment) falls back to the default (active-commitments) so fresh + * navigation always lands on the first sub-tab. + */ +export function getInventorySubTabFromPath(): string { + const segments = window.location.pathname + .replace(/^\/+/, '') + .replace(/\/+$/, '') + .split('/'); + const sub = (segments[1] ?? '').toLowerCase(); + return isValidInventorySubSection(sub) ? sub : DEFAULT_INVENTORY_SUB_SECTION; +} + /** * Switch between settings sub-tabs (General / Accounts / Users). * Manages section visibility, load lifecycle, and sub-tab URL history.