From 2dc2b36e653709496546e5e8effda11c460eef30 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 09:34:29 +0300 Subject: [PATCH 1/2] fix(frontend): add inline column filters to Active Convertible RIs table (closes #1414) Add per-column categorical and numeric filter controls to the Active Convertible RIs table in the RI Exchange view, matching the existing behavior of the Reshape Recommendations table (introduced in PRs #789 and #791). The regression left `renderRIsTable` with no filter buttons. - Add `ActiveRiColumnId`, `ActiveRiColumnFilter`, `ActiveRiColumnFilters` types and state slice to `state.ts` - Add `applyActiveRiColumnFilters` (exported), popover state machine, and all helper functions to `riexchange.ts`; wire filter buttons into `renderRIsTable` header cells for Instance Type, AZ, Count, Offering, and Utilization columns - Add DOM-level regression test file with 4 tests: button presence, popover detachment to body, categorical narrowing, numeric narrowing - Restore `getActiveRiColumnFilters` mock in existing describe blocks that call `jest.resetAllMocks()`, preventing cross-describe contamination --- .../riexchange-active-ri-filters.test.ts | 220 ++++++++++ .../__tests__/riexchange-permissions.test.ts | 4 + frontend/src/__tests__/riexchange.test.ts | 22 +- frontend/src/riexchange.ts | 384 +++++++++++++++++- frontend/src/state.ts | 41 ++ 5 files changed, 661 insertions(+), 10 deletions(-) create mode 100644 frontend/src/__tests__/riexchange-active-ri-filters.test.ts diff --git a/frontend/src/__tests__/riexchange-active-ri-filters.test.ts b/frontend/src/__tests__/riexchange-active-ri-filters.test.ts new file mode 100644 index 000000000..50852f0b8 --- /dev/null +++ b/frontend/src/__tests__/riexchange-active-ri-filters.test.ts @@ -0,0 +1,220 @@ +/** + * DOM-level regression tests for Active Convertible RIs inline column-filter + * controls (issue #1414). + * + * Before the fix, renderRIsTable rendered no filter buttons. This suite + * verifies the full wiring: button presence in every filterable header, + * popover opening detached from the table, categorical row-narrowing, and + * numeric-expression row-narrowing. + * + * Mirrors the structure of plans-column-filters.test.ts so both tables + * have equivalent DOM-level coverage. + */ + +jest.mock('../api', () => ({ + listConvertibleRIs: jest.fn(), + listExchangeableAzureRIs: jest.fn().mockResolvedValue([]), + getRIUtilization: jest.fn(), + getReshapeRecommendations: jest.fn(), + getExchangeQuote: jest.fn(), + executeExchange: jest.fn(), + getRIExchangeHistory: jest.fn(), + getRIExchangeConfig: jest.fn(), + updateRIExchangeConfig: jest.fn(), + listTargetOfferings: jest.fn().mockResolvedValue([]), +})); + +jest.mock('../navigation', () => ({ + switchTab: jest.fn(), + switchSettingsSubTab: jest.fn(), +})); + +// Module-scoped Active RI filter state; mirrors the real state module's +// slice so the popover commit path round-trips like the real store. +let activeRiFilters: Record = {}; + +jest.mock('../state', () => ({ + subscribeProvider: jest.fn().mockReturnValue(() => {}), + subscribeAccount: jest.fn().mockReturnValue(() => {}), + getCurrentProvider: jest.fn().mockReturnValue('aws'), + getCurrentAccountIDs: jest.fn().mockReturnValue([]), + getCurrentUser: jest.fn().mockReturnValue({ + id: 'u-admin', + email: 'admin@example.com', + groups: ['00000000-0000-5000-8000-000000000001'], + }), + // Reshape-recommendations slice (existing; no-op for these tests) + getRiExchangeColumnFilters: jest.fn().mockReturnValue({}), + setRiExchangeColumnFilter: jest.fn(), + clearAllRiExchangeColumnFilters: jest.fn(), + // Active-RI filter slice (new; backs the popover commit path) + getActiveRiColumnFilters: jest.fn(() => ({ ...activeRiFilters })), + setActiveRiColumnFilter: jest.fn((col: string, filter: unknown) => { + if (filter === null) { + const next = { ...activeRiFilters }; + delete next[col]; + activeRiFilters = next; + return; + } + activeRiFilters = { ...activeRiFilters, [col]: filter }; + }), + clearAllActiveRiColumnFilters: jest.fn(() => { activeRiFilters = {}; }), +})); + +import { loadRIExchange } from '../riexchange'; +import * as api from '../api'; + +// Three convertible RIs with distinct instance types, AZs, and counts +// so the categorical and numeric filter assertions can distinguish rows. +const seedRIs = [ + { + reserved_instance_id: 'ri-1', + instance_type: 'm5.xlarge', + availability_zone: 'us-east-1a', + instance_count: 4, + offering_type: 'Partial Upfront', + start: '2024-01-01', + end: '2025-01-01', + fixed_price: 1000, + usage_price: 0.5, + state: 'active', + normalization_factor: 8, + }, + { + reserved_instance_id: 'ri-2', + instance_type: 'm5.xlarge', + availability_zone: 'us-east-1b', + instance_count: 2, + offering_type: 'No Upfront', + start: '2024-01-01', + end: '2025-01-01', + fixed_price: 0, + usage_price: 0.8, + state: 'active', + normalization_factor: 8, + }, + { + reserved_instance_id: 'ri-3', + instance_type: 'c6i.large', + availability_zone: 'us-east-1a', + instance_count: 6, + offering_type: 'All Upfront', + start: '2024-01-01', + end: '2025-01-01', + fixed_price: 2000, + usage_price: 0, + state: 'active', + normalization_factor: 4, + }, +]; + +const seedUtilization = [ + { reserved_instance_id: 'ri-1', utilization_percent: 50.0, purchased_hours: 100, total_actual_hours: 50, unused_hours: 50 }, + { reserved_instance_id: 'ri-2', utilization_percent: 95.0, purchased_hours: 100, total_actual_hours: 95, unused_hours: 5 }, + { reserved_instance_id: 'ri-3', utilization_percent: 30.0, purchased_hours: 100, total_actual_hours: 30, unused_hours: 70 }, +]; + +describe('Active Convertible RIs column filters (issue #1414)', () => { + beforeEach(() => { + activeRiFilters = {}; + document.body.innerHTML = ` +
+
+
+ + `; + jest.clearAllMocks(); + (api.listConvertibleRIs as jest.Mock).mockResolvedValue(seedRIs); + (api.getRIUtilization as jest.Mock).mockResolvedValue(seedUtilization); + (api.getReshapeRecommendations as jest.Mock).mockResolvedValue({ + recommendations: [], + recs_staleness: '', + recs_collected_at: null, + }); + (api.getRIExchangeHistory as jest.Mock).mockResolvedValue([]); + }); + + afterEach(() => { + // Any detached popovers on body; clean up so they don't bleed. + document.body.querySelectorAll('.column-filter-popover').forEach((n) => n.remove()); + }); + + /** Load and flush all micro/macro-tasks (incl. the fire-and-forget loadUtilization). */ + async function load(): Promise { + await loadRIExchange(); + for (let i = 0; i < 3; i++) { + await new Promise((r) => setTimeout(r, 0)); + } + } + + function countRows(): number { + return document.querySelectorAll( + '#ri-exchange-instances-list tbody tr', + ).length; + } + + test('every filterable column header has a trigger button', async () => { + await load(); + const buttons = document.querySelectorAll( + '#ri-exchange-instances-list th .column-filter-btn[data-column]', + ); + const cols = Array.from(buttons).map((b) => b.dataset['column']); + expect(cols.sort()).toEqual( + ['availability_zone', 'instance_count', 'instance_type', 'offering_type', 'utilization_pct'].sort(), + ); + }); + + test('clicking a trigger button opens a popover detached to document.body', async () => { + await load(); + const btn = document.querySelector( + '#ri-exchange-instances-list th .column-filter-btn[data-column="instance_type"]', + ); + expect(btn).not.toBeNull(); + btn!.click(); + const popover = document.body.querySelector('.column-filter-popover'); + expect(popover).not.toBeNull(); + // Popover must be appended to body, not nested inside the table + expect(popover?.closest('#ri-exchange-instances-list')).toBeNull(); + }); + + test('categorical filter (instance_type) narrows displayed rows', async () => { + await load(); + expect(countRows()).toBe(3); + + const btn = document.querySelector( + '#ri-exchange-instances-list th .column-filter-btn[data-column="instance_type"]', + ); + btn!.click(); + + // Uncheck c6i.large to keep only m5.xlarge rows + const c6iCb = document.querySelector( + '.column-filter-popover .column-filter-item input[data-value="c6i.large"]', + ); + expect(c6iCb).not.toBeNull(); + c6iCb!.checked = false; + c6iCb!.dispatchEvent(new Event('change')); + + // ri-1 and ri-2 remain (m5.xlarge); ri-3 (c6i.large) is filtered out + expect(countRows()).toBe(2); + }); + + test('numeric filter (instance_count >= 4) narrows displayed rows', async () => { + await load(); + expect(countRows()).toBe(3); + + const btn = document.querySelector( + '#ri-exchange-instances-list th .column-filter-btn[data-column="instance_count"]', + ); + btn!.click(); + + const input = document.querySelector( + '.column-filter-popover .column-filter-numeric-input', + ); + expect(input).not.toBeNull(); + input!.value = '>=4'; + input!.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true })); + + // ri-1 (count=4) and ri-3 (count=6) pass; ri-2 (count=2) is filtered out + expect(countRows()).toBe(2); + }); +}); diff --git a/frontend/src/__tests__/riexchange-permissions.test.ts b/frontend/src/__tests__/riexchange-permissions.test.ts index ea8cfe29e..da8784851 100644 --- a/frontend/src/__tests__/riexchange-permissions.test.ts +++ b/frontend/src/__tests__/riexchange-permissions.test.ts @@ -30,6 +30,10 @@ jest.mock('../state', () => ({ getRiExchangeColumnFilters: jest.fn(() => ({})), setRiExchangeColumnFilter: jest.fn(), clearAllRiExchangeColumnFilters: jest.fn(), + // Active Convertible RIs column-filter slice (issue #1414). + getActiveRiColumnFilters: jest.fn(() => ({})), + setActiveRiColumnFilter: jest.fn(), + clearAllActiveRiColumnFilters: jest.fn(), })); import * as api from '../api'; diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index ccd6555d5..a63a3cffc 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -52,6 +52,10 @@ jest.mock('../state', () => ({ getRiExchangeColumnFilters: jest.fn(() => ({})), setRiExchangeColumnFilter: jest.fn(), clearAllRiExchangeColumnFilters: jest.fn(), + // Active Convertible RIs column-filter slice (issue #1414). + getActiveRiColumnFilters: jest.fn(() => ({})), + setActiveRiColumnFilter: jest.fn(), + clearAllActiveRiColumnFilters: jest.fn(), })); import { @@ -447,13 +451,15 @@ describe('reshape recommendations table', () => { tableContainer = document.createElement('div'); tableContainer.id = 'ri-exchange-recommendations-list'; document.body.appendChild(tableContainer); - // Re-apply the column-filter mock impl after a prior test's - // jest.resetAllMocks(); without this the renderer blows up on + // Re-apply the column-filter mock impls after a prior test's + // jest.resetAllMocks(); without this the renderers blow up on // Object.entries(undefined) when reading filter state. const stateMod = jest.requireMock('../state') as { getRiExchangeColumnFilters: jest.Mock; + getActiveRiColumnFilters: jest.Mock; }; stateMod.getRiExchangeColumnFilters.mockReturnValue({}); + stateMod.getActiveRiColumnFilters.mockReturnValue({}); }); afterEach(() => { @@ -559,13 +565,19 @@ describe('reshape recommendations empty state', () => { (api.getReshapeRecommendations as jest.Mock).mockResolvedValue({ recommendations: [], recs_staleness: '', recs_collected_at: null }); // resetAllMocks() in afterEach wipes the state mock implementations; // loadRIExchange reads the chips to scope the request (issue #871), so - // restore the AWS/all-accounts default here. + // restore the AWS/all-accounts default here. Also restore the column- + // filter slices so renderRIsTable / renderRecommendations don't crash + // on Object.entries(undefined) for non-empty tables. const stateMod = jest.requireMock('../state') as { getCurrentProvider: jest.Mock; getCurrentAccountIDs: jest.Mock; + getRiExchangeColumnFilters: jest.Mock; + getActiveRiColumnFilters: jest.Mock; }; stateMod.getCurrentProvider.mockReturnValue('aws'); stateMod.getCurrentAccountIDs.mockReturnValue([]); + stateMod.getRiExchangeColumnFilters.mockReturnValue({}); + stateMod.getActiveRiColumnFilters.mockReturnValue({}); }); afterEach(() => { @@ -807,6 +819,10 @@ describe('RI Exchange global filter scoping (issue #871)', () => { s.subscribeAccount.mockImplementation((cb: () => void) => { _accountListeners.push(cb); return () => undefined; }); s.getCurrentProvider.mockReturnValue('aws'); s.getCurrentAccountIDs.mockReturnValue([]); + // Restore column-filter slices wiped by a prior resetAllMocks() so + // renderRIsTable / renderRecommendations don't throw on non-empty data. + (s as unknown as { getActiveRiColumnFilters: jest.Mock }).getActiveRiColumnFilters?.mockReturnValue({}); + (s as unknown as { getRiExchangeColumnFilters: jest.Mock }).getRiExchangeColumnFilters?.mockReturnValue({}); }); afterEach(() => { diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 1aa5219fa..a4f0772b4 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -341,22 +341,35 @@ function renderRIsTable(container: HTMLElement, accountID?: string): void { // on click. Defense in depth, backend still enforces. const canExchange = canAccess('admin', '*'); + // Apply per-column filters (issue #1414). Mirrors the pattern in + // renderRecommendations; broken numeric expressions are skipped so + // the popover can surface the inline error without forcing a clear. + const filters = state.getActiveRiColumnFilters(); + const visibleRIs = applyActiveRiColumnFilters(currentRIs, currentUtilization, filters); + + const filterBtn = (column: state.ActiveRiColumnId): string => { + const active = filters[column] ? ' active' : ''; + const lbl = activeRiLabelFor(column); + const label = filters[column] ? `Filter ${lbl} — currently active` : `Filter ${lbl}`; + return ``; + }; + container.innerHTML = ` - - - - + + + + - + ${canExchange ? '' : ''} - ${currentRIs.map(ri => { + ${visibleRIs.map(ri => { const util = currentUtilization.get(ri.reserved_instance_id); const utilPct = util ? util.utilization_percent : null; const utilClass = utilPct === null ? '' : utilPct >= 95 ? 'util-green' : utilPct >= 70 ? 'util-yellow' : 'util-red'; @@ -377,6 +390,18 @@ function renderRIsTable(container: HTMLElement, accountID?: string): void {
RI IDInstance TypeAZCountOfferingInstance Type${filterBtn('instance_type')}AZ${filterBtn('availability_zone')}Count${filterBtn('instance_count')}Offering${filterBtn('offering_type')} ExpiryUtilizationUtilization${filterBtn('utilization_pct')}Actions
`; + // Wire per-column filter buttons (same pattern as renderRecommendations). + // e.stopPropagation prevents the surrounding from also handling + // the click so future sort handlers won't conflict. + container.querySelectorAll('.column-filter-btn').forEach((btn) => { + const column = btn.dataset['column'] as state.ActiveRiColumnId | undefined; + if (!column) return; + btn.addEventListener('click', (e) => { + e.stopPropagation(); + activeRiOpenPopoverFor(column, btn); + }); + }); + // Attach "Exchange" handlers for individual RIs container.querySelectorAll('[data-action="quote-ri"]').forEach(btn => { btn.addEventListener('click', () => { @@ -636,7 +661,352 @@ export function applyRiExchangeColumnFilters( }); } -// ── Popover ─────────────────────────────────── +// ── Active Convertible RIs column filters (issue #1414) ────────────────── + +interface ActiveRiColumnDef { + key: state.ActiveRiColumnId; + label: string; + kind: 'numeric' | 'categorical'; +} + +const ACTIVE_RI_COLUMN_DEFS: readonly ActiveRiColumnDef[] = [ + { key: 'instance_type', label: 'Instance Type', kind: 'categorical' }, + { key: 'availability_zone', label: 'AZ', kind: 'categorical' }, + { key: 'offering_type', label: 'Offering', kind: 'categorical' }, + { key: 'instance_count', label: 'Count', kind: 'numeric' }, + { key: 'utilization_pct', label: 'Utilization %', kind: 'numeric' }, +]; + +const ACTIVE_RI_NUMERIC_COLUMNS: ReadonlySet = new Set( + ACTIVE_RI_COLUMN_DEFS.filter((c) => c.kind === 'numeric').map((c) => c.key), +); + +function activeRiLabelFor(col: state.ActiveRiColumnId): string { + return ACTIVE_RI_COLUMN_DEFS.find((c) => c.key === col)?.label ?? col; +} + +function activeRiCategoricalCellValue(ri: ConvertibleRI, col: state.ActiveRiColumnId): string { + switch (col) { + case 'instance_type': return ri.instance_type ?? ''; + case 'availability_zone': return ri.availability_zone ?? ''; + case 'offering_type': return ri.offering_type ?? ''; + case 'instance_count': + case 'utilization_pct': return ''; + } +} + +function activeRiNumericCellValue( + ri: ConvertibleRI, + util: Map, + col: state.ActiveRiColumnId, +): number { + switch (col) { + case 'instance_count': return ri.instance_count ?? 0; + case 'utilization_pct': { + const u = util.get(ri.reserved_instance_id); + return u !== undefined ? u.utilization_percent : Number.NaN; + } + case 'instance_type': + case 'availability_zone': + case 'offering_type': return Number.NaN; + } +} + +function activeRiDisplayPrecision(col: state.ActiveRiColumnId): number { + switch (col) { + case 'instance_count': return 0; + case 'utilization_pct': return 1; + case 'instance_type': + case 'availability_zone': + case 'offering_type': return 0; + } +} + +/** + * Pure filter function for the Active Convertible RIs table. Exported so + * unit tests can exercise the filter pipeline without DOM setup. Mirrors + * applyRiExchangeColumnFilters. + */ +export function applyActiveRiColumnFilters( + ris: readonly ConvertibleRI[], + util: Map, + filters: state.ActiveRiColumnFilters, +): ConvertibleRI[] { + return applyColumnFiltersLib(ris, filters, { + categorical: activeRiCategoricalCellValue, + numeric: (ri, col) => { + const raw = activeRiNumericCellValue(ri, util, col); + if (!Number.isFinite(raw)) return raw; + return Number(raw.toFixed(activeRiDisplayPrecision(col))); + }, + }); +} + +// ── Active RI popover ───────────────────────────────────────────────────── + +interface ActiveRiPopoverState { + column: state.ActiveRiColumnId; + el: HTMLDivElement; + checkboxes: Map; + input: HTMLInputElement | null; + errorEl: HTMLElement | null; +} + +let activeRiOpenPopover: ActiveRiPopoverState | null = null; +let activeRiOutsideHandler: ((e: MouseEvent) => void) | null = null; +let activeRiEscHandler: ((e: KeyboardEvent) => void) | null = null; + +function activeRiDistinctValues(col: state.ActiveRiColumnId): string[] { + const seen = new Set(); + for (const ri of currentRIs) seen.add(activeRiCategoricalCellValue(ri, col)); + return Array.from(seen).sort((a, b) => { + if (a === '' && b !== '') return -1; + if (a !== '' && b === '') return 1; + return a.localeCompare(b); + }); +} + +function activeRiPositionPopover(popover: HTMLElement, anchor: HTMLElement): void { + const rect = anchor.getBoundingClientRect(); + popover.style.display = 'block'; + const popRect = popover.getBoundingClientRect(); + const margin = 8; + let top = rect.bottom + 4; + if (top + popRect.height > window.innerHeight - margin) { + top = Math.max(margin, rect.top - popRect.height - 4); + } + let left = rect.left; + if (left + popRect.width > window.innerWidth - margin) { + left = Math.max(margin, window.innerWidth - margin - popRect.width); + } + popover.style.position = 'absolute'; + popover.style.top = `${top + window.scrollY}px`; + popover.style.left = `${left + window.scrollX}px`; +} + +function rerenderRIs(): void { + const container = document.getElementById('ri-exchange-instances-list'); + if (container) renderRIsTable(container, currentRIAccountID); +} + +function activeRiCloseOpenPopover(): void { + if (!activeRiOpenPopover) return; + const { column, el } = activeRiOpenPopover; + el.remove(); + activeRiOpenPopover = null; + if (activeRiOutsideHandler) { + document.removeEventListener('mousedown', activeRiOutsideHandler); + activeRiOutsideHandler = null; + } + if (activeRiEscHandler) { + document.removeEventListener('keydown', activeRiEscHandler); + activeRiEscHandler = null; + } + const trigger = document.querySelector( + `#ri-exchange-instances-list .column-filter-btn[data-column="${column}"]`, + ); + if (trigger) trigger.setAttribute('aria-expanded', 'false'); +} + +function activeRiBuildPopover(column: state.ActiveRiColumnId): ActiveRiPopoverState { + const popover = document.createElement('div'); + popover.className = 'column-filter-popover'; + popover.setAttribute('role', 'dialog'); + popover.setAttribute('aria-modal', 'false'); + + const headingId = `active-ri-filter-heading-${column}`; + popover.setAttribute('aria-labelledby', headingId); + + const heading = document.createElement('h3'); + heading.id = headingId; + heading.className = 'column-filter-heading'; + heading.textContent = `Filter ${activeRiLabelFor(column)}`; + popover.appendChild(heading); + + const checkboxes = new Map(); + let input: HTMLInputElement | null = null; + let errorEl: HTMLElement | null = null; + let commitAllRef: ((target: boolean) => void) | null = null; + + if (ACTIVE_RI_NUMERIC_COLUMNS.has(column)) { + // Numeric column: free-text expression input + const label = document.createElement('label'); + label.className = 'column-filter-numeric-label'; + label.textContent = 'Expression'; + input = document.createElement('input'); + input.type = 'text'; + input.className = 'column-filter-numeric-input'; + input.placeholder = 'e.g. >50, 10..20, 4'; + input.setAttribute('aria-describedby', `active-ri-filter-error-${column}`); + const current = state.getActiveRiColumnFilters()[column]; + if (current && current.kind === 'expr') input.value = current.expr; + label.appendChild(input); + popover.appendChild(label); + + errorEl = document.createElement('div'); + errorEl.id = `active-ri-filter-error-${column}`; + errorEl.className = 'column-filter-error'; + errorEl.setAttribute('role', 'status'); + popover.appendChild(errorEl); + + const inputEl = input; + const errorElRef = errorEl; + const commit = (): void => { + const expr = inputEl.value.trim(); + if (expr === '') { + state.setActiveRiColumnFilter(column, null); + errorElRef.textContent = ''; + rerenderRIs(); + return; + } + const parsed = parseNumericFilter(expr); + if (!parsed.ok) { + errorElRef.textContent = parsed.error; + return; + } + errorElRef.textContent = ''; + state.setActiveRiColumnFilter(column, { kind: 'expr', expr }); + rerenderRIs(); + }; + inputEl.addEventListener('blur', commit); + inputEl.addEventListener('keydown', (e) => { + if (e.key === 'Enter') { e.preventDefault(); commit(); } + }); + } else { + // Categorical column: checkbox list of distinct values + const distinct = activeRiDistinctValues(column); + const current = state.getActiveRiColumnFilters()[column]; + const activeSet: ReadonlySet | null = + current && current.kind === 'set' ? new Set(current.values) : null; + + const allLabel = document.createElement('label'); + allLabel.className = 'column-filter-all'; + const allBox = document.createElement('input'); + allBox.type = 'checkbox'; + allBox.dataset['role'] = 'all'; + allLabel.appendChild(allBox); + const allText = document.createElement('span'); + allText.textContent = '(All)'; + allLabel.appendChild(allText); + popover.appendChild(allLabel); + + const list = document.createElement('div'); + list.className = 'column-filter-list'; + for (const value of distinct) { + const itemLabel = document.createElement('label'); + itemLabel.className = 'column-filter-item'; + const cb = document.createElement('input'); + cb.type = 'checkbox'; + cb.dataset['value'] = value; + cb.checked = activeSet === null ? true : activeSet.has(value); + itemLabel.appendChild(cb); + const text = document.createElement('span'); + text.textContent = value === '' ? '(empty)' : value; + itemLabel.appendChild(text); + list.appendChild(itemLabel); + checkboxes.set(value, cb); + } + popover.appendChild(list); + + const updateAllTriState = (): void => { + let checked = 0; + checkboxes.forEach((cb) => { if (cb.checked) checked++; }); + const total = checkboxes.size; + allBox.indeterminate = checked > 0 && checked < total; + allBox.checked = total > 0 && checked === total; + }; + updateAllTriState(); + + const commit = (): void => { + const selected: string[] = []; + checkboxes.forEach((cb, value) => { if (cb.checked) selected.push(value); }); + if (selected.length === checkboxes.size) { + state.setActiveRiColumnFilter(column, null); + } else { + state.setActiveRiColumnFilter(column, { kind: 'set', values: selected }); + } + updateAllTriState(); + rerenderRIs(); + }; + + const commitAll = (target: boolean): void => { + checkboxes.forEach((cb) => { cb.checked = target; }); + if (target) { + state.setActiveRiColumnFilter(column, null); + } else { + state.setActiveRiColumnFilter(column, { kind: 'set', values: [] }); + } + updateAllTriState(); + rerenderRIs(); + }; + commitAllRef = commitAll; + + checkboxes.forEach((cb) => { cb.addEventListener('change', commit); }); + allBox.addEventListener('change', () => { commitAll(allBox.checked); }); + } + + const footer = document.createElement('div'); + footer.className = 'column-filter-footer'; + const clearBtn = document.createElement('button'); + clearBtn.type = 'button'; + clearBtn.className = 'column-filter-clear'; + clearBtn.textContent = 'Clear'; + clearBtn.addEventListener('click', () => { + if (input) { + state.setActiveRiColumnFilter(column, null); + input.value = ''; + if (errorEl) errorEl.textContent = ''; + rerenderRIs(); + } else { + commitAllRef?.(false); + } + }); + footer.appendChild(clearBtn); + popover.appendChild(footer); + + return { column, el: popover, checkboxes, input, errorEl }; +} + +function activeRiOpenPopoverFor(column: state.ActiveRiColumnId, anchor: HTMLElement): void { + if (activeRiOpenPopover && !activeRiOpenPopover.el.isConnected) { + activeRiCloseOpenPopover(); + } + if (activeRiOpenPopover && activeRiOpenPopover.column === column) { + activeRiCloseOpenPopover(); + return; + } + if (activeRiOpenPopover) activeRiCloseOpenPopover(); + + const built = activeRiBuildPopover(column); + document.body.appendChild(built.el); + activeRiOpenPopover = built; + activeRiPositionPopover(built.el, anchor); + anchor.setAttribute('aria-expanded', 'true'); + + if (!activeRiOutsideHandler) { + activeRiOutsideHandler = (e: MouseEvent): void => { + if (!activeRiOpenPopover) return; + const target = e.target as Node | null; + if (!target) return; + if (activeRiOpenPopover.el.contains(target)) return; + if (target instanceof Element && target.closest('.column-filter-btn')) return; + activeRiCloseOpenPopover(); + }; + document.addEventListener('mousedown', activeRiOutsideHandler); + } + if (!activeRiEscHandler) { + activeRiEscHandler = (e: KeyboardEvent): void => { + if (e.key === 'Escape') activeRiCloseOpenPopover(); + }; + document.addEventListener('keydown', activeRiEscHandler); + } + + const firstFocusable = built.input + ?? built.el.querySelector('input[type="checkbox"]'); + firstFocusable?.focus(); +} + +// ── Reshape recommendations popover ────────────────────────────────────── interface RiexPopoverState { column: state.RiExchangeColumnId; diff --git a/frontend/src/state.ts b/frontend/src/state.ts index 6e91feb95..9a9e55505 100644 --- a/frontend/src/state.ts +++ b/frontend/src/state.ts @@ -293,6 +293,47 @@ export function clearAllRiExchangeColumnFilters(): void { riExchangeColumnFilters = {}; } +// --------------------------------------------------------------------------- +// Active Convertible RIs per-column filters (issue #1414). +// +// Mirrors the reshape-recommendations filter slice above but scoped to the +// Active Convertible RIs table (ri-exchange-instances-list). Column IDs map +// directly to ConvertibleRI / RIUtilization API fields. +// --------------------------------------------------------------------------- + +export type ActiveRiColumnId = + | 'instance_type' | 'availability_zone' | 'offering_type' + | 'instance_count' | 'utilization_pct'; + +export type ActiveRiColumnFilter = + | { kind: 'set'; values: string[] } + | { kind: 'expr'; expr: string }; + +export type ActiveRiColumnFilters = Partial>; + +let activeRiColumnFilters: ActiveRiColumnFilters = {}; + +export function getActiveRiColumnFilters(): ActiveRiColumnFilters { + return { ...activeRiColumnFilters }; +} + +export function setActiveRiColumnFilter( + column: ActiveRiColumnId, + filter: ActiveRiColumnFilter | null, +): void { + if (filter === null) { + const next = { ...activeRiColumnFilters }; + delete next[column]; + activeRiColumnFilters = next; + return; + } + activeRiColumnFilters = { ...activeRiColumnFilters, [column]: filter }; +} + +export function clearAllActiveRiColumnFilters(): void { + activeRiColumnFilters = {}; +} + // --------------------------------------------------------------------------- // Per-column visibility state (issue #318). // A column id in this set is HIDDEN; an absent id is visible (default visible). From aebaf6cf4fc8bfa6c32f171fb593823bca4de998 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 09:57:19 +0300 Subject: [PATCH 2/2] fix(settings): show override-delete Undo toast immediately (closes #1415) The Undo toast for deleting a Service Defaults override was awaited behind loadOverridesPanel + refreshRecommendationsAfterOverrideChange, the latter issuing a full page-worth of recommendation API calls. A slow refresh delayed the toast long enough that users reported seeing no Undo affordance at all. Show the toast immediately after the server confirms the delete, before the panel reload and recommendations refresh. Split the delete into its own try/catch that returns early on failure, so a refresh error can no longer be misreported as "Failed to delete override" when the delete actually succeeded. Add two regression tests: one asserting the Undo toast is visible while a recommendations refresh is still in flight (fails pre-fix, passes post-fix), and one asserting a failed delete shows an error toast with no Undo and does not trigger the refresh. --- .../src/__tests__/settings-accounts.test.ts | 74 +++++++++++++++++++ frontend/src/settings.ts | 61 ++++++++------- 2 files changed, 110 insertions(+), 25 deletions(-) diff --git a/frontend/src/__tests__/settings-accounts.test.ts b/frontend/src/__tests__/settings-accounts.test.ts index 04e4018cb..70da5d637 100644 --- a/frontend/src/__tests__/settings-accounts.test.ts +++ b/frontend/src/__tests__/settings-accounts.test.ts @@ -1432,6 +1432,80 @@ describe('Account overrides modal', () => { ); }); + test('Undo toast appears immediately, not gated behind a slow recommendations refresh (issue #1415)', async () => { + (api.listAccountServiceOverrides as jest.Mock) + .mockResolvedValueOnce([overrideFixture]) // initial render + .mockResolvedValue([]); // reload after delete + (api.deleteAccountServiceOverride as jest.Mock).mockResolvedValue(undefined); + mockConfirmDialog.mockResolvedValue(true); + + // Simulate a slow recommendations refresh that has not resolved by the + // time we assert. Pre-fix, showToast was awaited behind this call, so a + // slow refresh meant the Undo toast never appeared for the user; post-fix + // the toast fires right after the delete resolves, independent of it. + let resolveRefresh: () => void = () => {}; + const refreshGate = new Promise(res => { resolveRefresh = res; }); + mockLoadRecommendations.mockImplementationOnce(() => refreshGate); + + const panel = document.createElement('div'); + document.body.appendChild(panel); + await loadOverridesPanel('acc-1', panel, 'aws'); + + const deleteBtn = Array.from(panel.querySelectorAll('button')) + .find(b => b.textContent === 'Delete') as HTMLButtonElement; + deleteBtn.click(); + + // Flush the confirm + delete + panel-reload chain. The recommendations + // refresh is still pending (resolveRefresh has NOT been called). + for (let i = 0; i < 5; i++) { await new Promise(r => setTimeout(r, 0)); } + + // The Undo toast must already be visible even though the refresh is + // still in flight. + const undoToast = (mockShowToast.mock.calls.map(c => c[0]) as Array<{ + kind?: string; + message?: string; + actions?: Array<{ label: string }>; + timeout?: number | null; + }>).find(t => t.kind === 'info' && t.message?.includes('aws/rds')); + expect(undoToast).toBeDefined(); + expect(undoToast!.actions?.[0]?.label).toBe('Undo'); + expect(undoToast!.timeout).toBe(5_000); + + // Let the pending refresh resolve so the async chain completes cleanly. + resolveRefresh(); + for (let i = 0; i < 3; i++) { await new Promise(r => setTimeout(r, 0)); } + }); + + test('a delete API failure shows an error toast and no Undo (issue #1415)', async () => { + (api.listAccountServiceOverrides as jest.Mock) + .mockResolvedValueOnce([overrideFixture]) + .mockResolvedValue([]); + (api.deleteAccountServiceOverride as jest.Mock) + .mockRejectedValue(new Error('boom')); + mockConfirmDialog.mockResolvedValue(true); + + const panel = document.createElement('div'); + document.body.appendChild(panel); + await loadOverridesPanel('acc-1', panel, 'aws'); + + const deleteBtn = Array.from(panel.querySelectorAll('button')) + .find(b => b.textContent === 'Delete') as HTMLButtonElement; + deleteBtn.click(); + for (let i = 0; i < 5; i++) { await new Promise(r => setTimeout(r, 0)); } + + const toastCalls = mockShowToast.mock.calls.map(c => c[0]) as Array<{ + kind?: string; + message?: string; + actions?: unknown[]; + }>; + // The failure surfaces as an error toast with the delete-failure copy. + expect(toastCalls.some(t => t.kind === 'error' && t.message?.includes('Failed to delete override'))).toBe(true); + // No info/Undo toast — nothing was deleted, so there is nothing to undo. + expect(toastCalls.some(t => t.kind === 'info' && !!t.actions)).toBe(false); + // A failed delete must not trigger the recommendations refresh. + expect(mockLoadRecommendations).not.toHaveBeenCalled(); + }); + test('let-it-expire path: override is permanently gone after toast timeout', async () => { (api.listAccountServiceOverrides as jest.Mock) .mockResolvedValueOnce([overrideFixture]) diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts index 8c1376f2b..9dede2f3e 100644 --- a/frontend/src/settings.ts +++ b/frontend/src/settings.ts @@ -1247,34 +1247,45 @@ export async function loadOverridesPanel(accountId: string, panel: HTMLElement, }; try { await api.deleteAccountServiceOverride(accountId, o.provider, o.service); - await loadOverridesPanel(accountId, panel, provider); - await refreshRecommendationsAfterOverrideChange(); - // Show a 5-second undo toast. The action closure captures `snapshot` - // and re-PUT it if the user clicks Undo before the toast expires. - // Each Reset click replaces any prior toast handle so a rapid - // double-click cannot stack multiple Undo buttons. - showToast({ - message: `Override for ${o.provider}/${o.service} deleted.`, - kind: 'info', - timeout: 5_000, - actions: [{ - label: 'Undo', - onClick: () => { - void (async () => { - try { - await api.saveAccountServiceOverride(accountId, o.provider, o.service, snapshot); - await loadOverridesPanel(accountId, panel, provider); - await refreshRecommendationsAfterOverrideChange(); - } catch (undoErr) { - showToast({ message: `Failed to restore override: ${(undoErr as Error).message}`, kind: 'error' }); - } - })(); - }, - }], - }); } catch (err) { showToast({ message: `Failed to delete override: ${(err as Error).message}`, kind: 'error' }); + return; } + // The server has confirmed the delete. Show the 5-second undo toast + // IMMEDIATELY, before the panel reload + recommendations refresh below. + // Previously the toast was awaited behind both of those calls, so a + // slow recommendations refresh (a full page-worth of API calls) delayed + // the Undo affordance long enough that users reported seeing no toast + // at all (issue #1415). The snapshot-restore Undo is valid regardless + // of the UI refresh, so it must not be gated behind it. Keeping the + // failed-delete branch above also stops a refresh error from being + // misreported as "Failed to delete override" when the delete succeeded. + // + // The action closure captures `snapshot` and re-PUTs it if the user + // clicks Undo before the toast expires. Each Delete click replaces any + // prior toast handle so a rapid double-click cannot stack multiple + // Undo buttons. + showToast({ + message: `Override for ${o.provider}/${o.service} deleted.`, + kind: 'info', + timeout: 5_000, + actions: [{ + label: 'Undo', + onClick: () => { + void (async () => { + try { + await api.saveAccountServiceOverride(accountId, o.provider, o.service, snapshot); + await loadOverridesPanel(accountId, panel, provider); + await refreshRecommendationsAfterOverrideChange(); + } catch (undoErr) { + showToast({ message: `Failed to restore override: ${(undoErr as Error).message}`, kind: 'error' }); + } + })(); + }, + }], + }); + await loadOverridesPanel(accountId, panel, provider); + await refreshRecommendationsAfterOverrideChange(); }); actionTd.appendChild(resetBtn); });