From e56aea5ed88547fac200e0c944f644fee32eaaee Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 12:35:18 +0200 Subject: [PATCH 1/8] fix(frontend): confirm dialog focuses the safe action and is accessible confirmDialog focuses Cancel for destructive dialogs (or when a caller passes initialFocus: 'cancel'), names the dialog via aria-labelledby / aria-describedby, and cycles focus backwards on Shift+Tab. The native confirm() calls in navigation (unsaved settings guard) and bulk add to group, and the native prompt() in registration reject, now use the styled dialog. Refs #705, #731 Co-Authored-By: Claude Sonnet 5.5 --- frontend/src/__tests__/confirmDialog.test.ts | 36 ++++++++++++++++++++ frontend/src/__tests__/navigation.test.ts | 32 ++++++++++++++++- frontend/src/__tests__/users.test.ts | 6 ++-- frontend/src/confirmDialog.ts | 32 ++++++++++++++--- frontend/src/modules/registrations.ts | 19 +++++++++-- frontend/src/navigation.ts | 29 +++++++++++++--- frontend/src/users/userActions.ts | 9 +++-- 7 files changed, 144 insertions(+), 19 deletions(-) diff --git a/frontend/src/__tests__/confirmDialog.test.ts b/frontend/src/__tests__/confirmDialog.test.ts index d57d7ab96..b0c8e9c96 100644 --- a/frontend/src/__tests__/confirmDialog.test.ts +++ b/frontend/src/__tests__/confirmDialog.test.ts @@ -88,4 +88,40 @@ describe('confirmDialog', () => { expect(document.querySelector('.btn-primary')?.textContent).toBe('Reset all'); expect(document.querySelector('.btn-secondary')?.textContent).toBe('Keep them'); }); + + it('names the dialog by its title and describes it by its body', () => { + void confirmDialog({ title: 'Delete?', body: 'Permanent action.' }); + const backdrop = document.querySelector('.modal-confirm-backdrop')!; + const titleId = backdrop.getAttribute('aria-labelledby')!; + const bodyId = backdrop.getAttribute('aria-describedby')!; + expect(document.getElementById(titleId)?.textContent).toBe('Delete?'); + expect(document.getElementById(bodyId)?.textContent).toBe('Permanent action.'); + }); + + it('focuses Cancel for destructive dialogs and Confirm otherwise', () => { + void confirmDialog({ title: 't', body: 'b', destructive: true }); + expect(document.activeElement?.textContent).toBe('Cancel'); + document.body.replaceChildren(); + void confirmDialog({ title: 't', body: 'b' }); + expect(document.activeElement?.textContent).toBe('Confirm'); + }); + + it('focuses Cancel when initialFocus is cancel even if not destructive', () => { + void confirmDialog({ title: 't', body: 'b', initialFocus: 'cancel' }); + expect(document.activeElement?.textContent).toBe('Cancel'); + }); + + it('focuses the close-X when the cancel button is hidden and focus is cancel', () => { + void confirmDialog({ title: 't', body: 'b', destructive: true, hideCancelButton: true }); + expect(document.activeElement?.classList.contains('modal-confirm-close')).toBe(true); + }); + + it('cycles focus backwards on Shift+Tab', () => { + void confirmDialog({ title: 't', body: 'b', destructive: true }); + // cycle order: confirm, cancel, close; focus starts on cancel + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Tab', shiftKey: true })); + expect(document.activeElement?.textContent).toBe('Confirm'); + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Tab', shiftKey: true })); + expect(document.activeElement?.classList.contains('modal-confirm-close')).toBe(true); + }); }); diff --git a/frontend/src/__tests__/navigation.test.ts b/frontend/src/__tests__/navigation.test.ts index a7a46af23..acc672af4 100644 --- a/frontend/src/__tests__/navigation.test.ts +++ b/frontend/src/__tests__/navigation.test.ts @@ -57,7 +57,7 @@ import { loadDashboard } from '../dashboard'; import { loadRecommendations } from '../recommendations'; import { loadPlans } from '../plans'; import { initHistoryDateRange, loadHistory } from '../history'; -import { loadGlobalSettings } from '../settings'; +import { loadGlobalSettings, isUnsavedChanges } from '../settings'; import { loadAutomationSettings } from '../riexchange'; import { canAccess } from '../permissions'; import { loadInventory } from '../inventory'; @@ -663,4 +663,34 @@ describe('Navigation Module', () => { expect(getInventorySubTabFromPath()).toBe('active-commitments'); }); }); + + describe('unsaved settings guard', () => { + afterEach(() => { + (isUnsavedChanges as jest.Mock).mockReturnValue(false); + }); + + const dialogButton = (label: string): HTMLButtonElement => + Array.from(document.querySelectorAll('.modal-confirm button')).find( + b => b.textContent === label + )!; + + test('asks via the styled dialog, stays put on cancel and leaves on confirm', async () => { + switchTab('admin'); + (isUnsavedChanges as jest.Mock).mockReturnValue(true); + + switchTab('home'); + expect(document.querySelector('.modal-confirm-title')?.textContent).toBe('Leave without saving?'); + expect(document.getElementById('admin-tab')?.classList.contains('active')).toBe(true); + dialogButton('Stay on this tab').click(); + await Promise.resolve(); + await Promise.resolve(); + expect(document.getElementById('admin-tab')?.classList.contains('active')).toBe(true); + + switchTab('home'); + dialogButton('Leave without saving').click(); + await Promise.resolve(); + await Promise.resolve(); + expect(document.getElementById('home-tab')?.classList.contains('active')).toBe(true); + }); + }); }); diff --git a/frontend/src/__tests__/users.test.ts b/frontend/src/__tests__/users.test.ts index 503d2c2ba..cf77085cb 100644 --- a/frontend/src/__tests__/users.test.ts +++ b/frontend/src/__tests__/users.test.ts @@ -1305,13 +1305,13 @@ describe('users/userActions', () => { await userActions.bulkAddToGroup('admins'); - expect(global.confirm).toHaveBeenCalledWith( - expect.stringContaining('Admins') + expect(mockConfirmDialog).toHaveBeenCalledWith( + expect.objectContaining({ title: expect.stringContaining('Admins') }) ); }); it('should not add when cancelled', async () => { - (global.confirm as jest.Mock).mockReturnValue(false); + mockConfirmDialog.mockResolvedValueOnce(false); userState.addSelectedUserId('1'); await userActions.bulkAddToGroup('admins'); diff --git a/frontend/src/confirmDialog.ts b/frontend/src/confirmDialog.ts index 98ca74986..bf805d0df 100644 --- a/frontend/src/confirmDialog.ts +++ b/frontend/src/confirmDialog.ts @@ -4,11 +4,19 @@ * on confirm and false on cancel / ESC / backdrop click. * * Rendered as a centred modal with backdrop, ESC cancels, focus-trap - * keeps Tab inside the dialog. The `destructive: true` flag renders the - * confirm button in the destructive variant so users see red before they - * click a deletion. + * keeps Tab and Shift+Tab inside the dialog. The `destructive: true` flag + * renders the confirm button in the destructive variant so users see red + * before they click a deletion. + * + * Initial focus lands on the safe action (Cancel) for destructive dialogs + * and for any dialog that passes `initialFocus: 'cancel'` (money-moving + * confirms), so a reflex Enter never executes the action. The dialog is + * named by its title (aria-labelledby) and described by its body + * (aria-describedby). */ +let dialogSeq = 0; + export interface ConfirmDialogOptions { title: string; body: string | HTMLElement; @@ -21,6 +29,11 @@ export interface ConfirmDialogOptions { // both labelled "Cancel"). Users can still dismiss via the close-X, // ESC, or a backdrop click. hideCancelButton?: boolean; + // Which control receives focus on open. Defaults to 'cancel' for + // destructive dialogs and 'confirm' otherwise. Pass 'cancel' for + // dialogs that move money so Enter cannot execute by reflex. With + // hideCancelButton the safe target is the close-X. + initialFocus?: 'cancel' | 'confirm'; } export function confirmDialog(opts: ConfirmDialogOptions): Promise { @@ -29,6 +42,11 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise { backdrop.className = 'modal-confirm-backdrop'; backdrop.setAttribute('role', 'dialog'); backdrop.setAttribute('aria-modal', 'true'); + const seq = ++dialogSeq; + const titleId = `modal-confirm-title-${seq}`; + const bodyId = `modal-confirm-body-${seq}`; + backdrop.setAttribute('aria-labelledby', titleId); + backdrop.setAttribute('aria-describedby', bodyId); const dialog = document.createElement('div'); dialog.className = 'modal-confirm'; @@ -45,11 +63,13 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise { dialog.appendChild(closeBtn); const titleEl = document.createElement('h3'); + titleEl.id = titleId; titleEl.className = 'modal-confirm-title'; titleEl.textContent = opts.title; dialog.appendChild(titleEl); const bodyEl = document.createElement('div'); + bodyEl.id = bodyId; bodyEl.className = 'modal-confirm-body'; if (typeof opts.body === 'string') { bodyEl.textContent = opts.body; @@ -81,7 +101,8 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise { document.body.appendChild(backdrop); const previousActive = document.activeElement as HTMLElement | null; - confirmBtn.focus(); + const focusSafe = (opts.initialFocus ?? (opts.destructive ? 'cancel' : 'confirm')) === 'cancel'; + (focusSafe ? (hideCancel ? closeBtn : cancelBtn) : confirmBtn).focus(); let settled = false; const settle = (value: boolean): void => { @@ -108,7 +129,8 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise { ? [confirmBtn, closeBtn] : [confirmBtn, cancelBtn, closeBtn]; const idx = cycle.indexOf(document.activeElement as HTMLButtonElement); - const next = cycle[(idx + 1 + cycle.length) % cycle.length] ?? confirmBtn; + const step = e.shiftKey ? -1 : 1; + const next = cycle[(idx + step + cycle.length) % cycle.length] ?? confirmBtn; next.focus(); } }; diff --git a/frontend/src/modules/registrations.ts b/frontend/src/modules/registrations.ts index 6e41989c4..45cd58660 100644 --- a/frontend/src/modules/registrations.ts +++ b/frontend/src/modules/registrations.ts @@ -238,8 +238,23 @@ async function handleDelete(reg: AccountRegistration): Promise { } async function handleReject(reg: AccountRegistration): Promise { - const reason = prompt(`Reject registration for "${reg.account_name}"?\n\nOptional reason:`); - if (reason === null) return; // User cancelled. + const body = document.createElement('div'); + const label = document.createElement('label'); + label.textContent = 'Reason (optional)'; + label.htmlFor = 'registration-reject-reason'; + const reasonInput = document.createElement('textarea'); + reasonInput.id = 'registration-reject-reason'; + reasonInput.rows = 3; + reasonInput.style.width = '100%'; + body.append(label, reasonInput); + const ok = await confirmDialog({ + title: `Reject registration for "${reg.account_name}"?`, + body, + confirmLabel: 'Reject registration', + destructive: true, + }); + if (!ok) return; + const reason = reasonInput.value.trim(); try { await api.rejectRegistration(reg.id, reason || undefined); diff --git a/frontend/src/navigation.ts b/frontend/src/navigation.ts index 014f11e2c..abc0408dc 100644 --- a/frontend/src/navigation.ts +++ b/frontend/src/navigation.ts @@ -18,6 +18,7 @@ import { DEFAULT_INVENTORY_SUB_SECTION, } from './inventory'; import { isAdmin } from './auth'; +import { confirmDialog } from './confirmDialog'; import { canAccess } from './permissions'; interface TabMeta { @@ -123,6 +124,16 @@ function renderNoAccess(tabId: string): void { /** * Switch between tabs */ +function confirmLeaveSettings(): Promise { + return confirmDialog({ + title: 'Leave without saving?', + body: 'You have unsaved settings changes. Leaving this tab discards them.', + confirmLabel: 'Leave without saving', + cancelLabel: 'Stay on this tab', + destructive: true, + }); +} + export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { if (!isKnownKey(TABS, tabName)) tabName = 'home'; @@ -134,7 +145,10 @@ export function switchTab(tabName: string, opts: SwitchTabOptions = {}): void { tabName !== 'admin' && isUnsavedChanges() ) { - if (!confirm('You have unsaved settings changes. Leave without saving?')) return; + void confirmLeaveSettings().then(leave => { + if (leave) switchTab(tabName, { ...opts, skipDirtyGuard: true }); + }); + return; } document.querySelectorAll('.tab-btn').forEach(btn => { @@ -359,10 +373,15 @@ export function initRouter(): void { target !== 'admin' && isUnsavedChanges() ) { - if (!confirm('You have unsaved settings changes. Leave without saving?')) { - if (delta !== 0) window.history.go(-delta); - return; - } + void confirmLeaveSettings().then(leave => { + if (leave) { + historyId = newId; + switchTab(target, { push: false, skipDirtyGuard: true }); + } else if (delta !== 0) { + window.history.go(-delta); + } + }); + return; } historyId = newId; diff --git a/frontend/src/users/userActions.ts b/frontend/src/users/userActions.ts index ab1a3bc45..8827509d0 100644 --- a/frontend/src/users/userActions.ts +++ b/frontend/src/users/userActions.ts @@ -197,9 +197,12 @@ export async function bulkAddToGroup(groupId: string): Promise { const group = availableGroups.find(g => g.id === groupId); if (!group) return; - if (!confirm(`Add ${count} user(s) to group "${group.name}"?`)) { - return; - } + const addOk = await confirmDialog({ + title: `Add ${count} user${count === 1 ? '' : 's'} to "${group.name}"?`, + body: `The ${count === 1 ? 'selected user' : 'selected users'} will gain the permissions of the "${group.name}" group.`, + confirmLabel: 'Add to group', + }); + if (!addOk) return; // Partition the selected users by whether adding this group would produce a // contradictory combination (view-only group + write-capable group, issue From 2524d418a08860d18ced4f97732e5d755edfbaae Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 12:46:49 +0200 Subject: [PATCH 2/8] fix(frontend): confirm dialog focus trap includes body inputs Tab now cycles every focusable in the dialog in DOM order, so the registration reject reason textarea is reachable. The textarea width moves from an inline style to a class. Refs #705 Co-Authored-By: Claude Sonnet 5.5 --- frontend/src/__tests__/confirmDialog.test.ts | 22 +++++++++++++++----- frontend/src/confirmDialog.ts | 14 ++++++------- frontend/src/modules/registrations.ts | 2 +- frontend/src/styles/forms.css | 5 +++++ 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/frontend/src/__tests__/confirmDialog.test.ts b/frontend/src/__tests__/confirmDialog.test.ts index b0c8e9c96..2a949a3f7 100644 --- a/frontend/src/__tests__/confirmDialog.test.ts +++ b/frontend/src/__tests__/confirmDialog.test.ts @@ -116,12 +116,24 @@ describe('confirmDialog', () => { expect(document.activeElement?.classList.contains('modal-confirm-close')).toBe(true); }); - it('cycles focus backwards on Shift+Tab', () => { - void confirmDialog({ title: 't', body: 'b', destructive: true }); - // cycle order: confirm, cancel, close; focus starts on cancel - document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Tab', shiftKey: true })); + it('cycles focus over all focusables in DOM order, including body inputs', () => { + const body = document.createElement('div'); + const ta = document.createElement('textarea'); + body.appendChild(ta); + void confirmDialog({ title: 't', body, destructive: true }); + const tab = (shiftKey = false): void => { + document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Tab', shiftKey })); + }; + // DOM order: close-X, textarea, Cancel, Confirm; focus starts on Cancel. + tab(true); + expect(document.activeElement).toBe(ta); + tab(true); + expect(document.activeElement?.classList.contains('modal-confirm-close')).toBe(true); + tab(); + expect(document.activeElement).toBe(ta); + tab(); tab(); expect(document.activeElement?.textContent).toBe('Confirm'); - document.dispatchEvent(new KeyboardEvent('keydown', { key: 'Tab', shiftKey: true })); + tab(); expect(document.activeElement?.classList.contains('modal-confirm-close')).toBe(true); }); }); diff --git a/frontend/src/confirmDialog.ts b/frontend/src/confirmDialog.ts index bf805d0df..6b5a7f136 100644 --- a/frontend/src/confirmDialog.ts +++ b/frontend/src/confirmDialog.ts @@ -15,6 +15,8 @@ * (aria-describedby). */ +import { FOCUSABLE_SELECTOR } from './modal'; + let dialogSeq = 0; export interface ConfirmDialogOptions { @@ -121,14 +123,12 @@ export function confirmDialog(opts: ConfirmDialogOptions): Promise { e.preventDefault(); settle(false); } else if (e.key === 'Tab') { - // Focus trap: cycle Tab between the focusable elements the dialog - // actually rendered. When hideCancelButton is true, cycle between - // close-X and confirm; otherwise include the dismiss button too. + // Focus trap: cycle over every focusable the dialog rendered (close-X, + // any inputs in a caller-supplied body, then the action buttons), in + // DOM order. e.preventDefault(); - const cycle: HTMLButtonElement[] = hideCancel - ? [confirmBtn, closeBtn] - : [confirmBtn, cancelBtn, closeBtn]; - const idx = cycle.indexOf(document.activeElement as HTMLButtonElement); + const cycle = Array.from(dialog.querySelectorAll(FOCUSABLE_SELECTOR)); + const idx = cycle.indexOf(document.activeElement as HTMLElement); const step = e.shiftKey ? -1 : 1; const next = cycle[(idx + step + cycle.length) % cycle.length] ?? confirmBtn; next.focus(); diff --git a/frontend/src/modules/registrations.ts b/frontend/src/modules/registrations.ts index 45cd58660..81e5272c3 100644 --- a/frontend/src/modules/registrations.ts +++ b/frontend/src/modules/registrations.ts @@ -245,7 +245,7 @@ async function handleReject(reg: AccountRegistration): Promise { const reasonInput = document.createElement('textarea'); reasonInput.id = 'registration-reject-reason'; reasonInput.rows = 3; - reasonInput.style.width = '100%'; + reasonInput.className = 'registration-reject-reason'; body.append(label, reasonInput); const ok = await confirmDialog({ title: `Reject registration for "${reg.account_name}"?`, diff --git a/frontend/src/styles/forms.css b/frontend/src/styles/forms.css index 1ea2163d2..ac372bb47 100644 --- a/frontend/src/styles/forms.css +++ b/frontend/src/styles/forms.css @@ -422,3 +422,8 @@ input:checked + .slider:before { tr.selected { background: var(--cudly-info-bg) !important; } + +.registration-reject-reason { + width: 100%; + box-sizing: border-box; +} From d7fb436f298da28a899a2279e489ad7751318d2a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 13:00:48 +0200 Subject: [PATCH 3/8] test(frontend): e2e confirms click the action instead of relying on Enter Destructive confirm dialogs now open with Cancel focused, so the specs that confirmed by pressing Enter (and asserted Confirm had focus) now click the action button and assert the safe-action focus. Refs #705 Co-Authored-By: Claude Sonnet 5.5 --- .../tests-e2e/history-action-confirmation.spec.ts | 14 +++++++------- .../tests-e2e/ri-exchange-confirmation.spec.ts | 7 ++++--- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/frontend/tests-e2e/history-action-confirmation.spec.ts b/frontend/tests-e2e/history-action-confirmation.spec.ts index 4d27b96d6..dc224748b 100644 --- a/frontend/tests-e2e/history-action-confirmation.spec.ts +++ b/frontend/tests-e2e/history-action-confirmation.spec.ts @@ -79,12 +79,12 @@ for (const action of actions) { fixture.failNextMutation(); await btn.click(); await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(1); await expect(btn).toBeEnabled(); await btn.click(); await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(2); await expect(btn).toHaveCount(0); expect(fixture.posts.every(post => post.url.includes(ID))).toBe(true); @@ -112,7 +112,7 @@ test('actual double-click and second projection cannot duplicate pending approva details.release(); await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); expect(detailGets).toBe(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(1); }); @@ -133,7 +133,7 @@ test('approve is not offered when the purchase details cannot be loaded (issue # await expect(btn).toBeEnabled(); await btn.click(); await expect(page.locator('.modal-confirm-body')).toContainText('$1,200'); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(1); }); @@ -200,7 +200,7 @@ test('success survives failed refresh, cached redraw and old GET; fresh same-ID await page.locator('#history-list .history-approve-btn').click(); failHistory = true; await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect(page.locator('#history-list .error')).toBeVisible(); const oldResponse = page.waitForResponse(response => response.url().includes('/api/history') && response.status() === 200); oldGet.release(); @@ -215,7 +215,7 @@ test('success survives failed refresh, cached redraw and old GET; fresh same-ID await page.getByRole('button', { name: 'Load History', exact: true }).click(); await page.locator('#history-list .history-revoke-btn').click(); await expect(page.locator('.modal-confirm-backdrop')).toHaveCount(1); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(2); expect(fixture.posts[1]!.url).toContain(`/purchases/${ID}/revoke`); }); @@ -241,6 +241,6 @@ test('completed revoke owns its row before refund quote and recovers from quote expect(fixture.posts).toHaveLength(0); await btn.click(); await expect(page.locator('.modal-confirm-body')).toContainText('100.00 USD'); - await page.keyboard.press('Enter'); + await page.locator('.modal-confirm-actions button').last().click(); await expect.poll(() => fixture.posts.length).toBe(1); }); diff --git a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts index 8560d6b11..060679ba9 100644 --- a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts +++ b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts @@ -109,9 +109,10 @@ for (const multiple of [false, true]) { await expect(dialog(page)).toContainText('us-east-1'); if (multiple) await expect(dialog(page)).toContainText(`3 × ${secondOffering}`); expect(state.requests).toHaveLength(0); - await expect(confirm(page)).toBeFocused(); - await page.keyboard.press('Tab'); + // Destructive dialogs open on the safe action, so a reflex Enter cancels. await expect(dialog(page).getByRole('button', { name: 'Cancel', exact: true })).toBeFocused(); + await page.keyboard.press('Tab'); + await expect(confirm(page)).toBeFocused(); await confirm(page).click(); await expect(page.locator('#modal-exchange-result')).toContainText('Exchange completed. ID: synthetic-exchange'); expect(state.requests).toEqual([{ path: '/api/ri-exchange/execute', body: { @@ -143,7 +144,7 @@ for (const action of ['Execute', 'Approve']) { await expect(trigger).toBeEnabled(); await expect(trigger).toBeFocused(); await trigger.click(); - await page.keyboard.press('Enter'); + await confirm(page).click(); await expect.poll(() => state.requests.length).toBe(1); if (action === 'Approve') { expect(state.requests[0]).toEqual({ path: `/api/ri-exchange/approve/${state.history.id}`, body: null }); From db11d3d2c6a9289baa197276fb60ccaa4510aeef Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 12:42:04 +0200 Subject: [PATCH 4/8] fix(frontend): money confirms state what is bought and how much Execute now, Run now and the RI exchange execute confirm now show a shared stat band (charged today, monthly, total over term) plus the commitment lines with account and region, focus Cancel first, and Run now ignores a second click while its dialog or request is open. The RI exchange confirm names source and target instance types instead of offering UUIDs and shows the hourly price change from the quote. Refs #707, #709, #701, #711 Co-Authored-By: Claude Sonnet 5.5 --- .../src/__tests__/commitmentSummary.test.ts | 62 +++++++++ frontend/src/__tests__/plans.test.ts | 28 +++++ .../__tests__/purchase-modal-submit.test.ts | 9 +- frontend/src/__tests__/riexchange.test.ts | 32 +++++ frontend/src/app.ts | 21 +++- frontend/src/commitmentSummary.ts | 119 ++++++++++++++++++ frontend/src/plans.ts | 59 +++++++-- frontend/src/riexchange.ts | 59 ++++++++- frontend/src/styles/components.css | 32 +++++ 9 files changed, 398 insertions(+), 23 deletions(-) create mode 100644 frontend/src/__tests__/commitmentSummary.test.ts create mode 100644 frontend/src/commitmentSummary.ts diff --git a/frontend/src/__tests__/commitmentSummary.test.ts b/frontend/src/__tests__/commitmentSummary.test.ts new file mode 100644 index 000000000..ee2f1c023 --- /dev/null +++ b/frontend/src/__tests__/commitmentSummary.test.ts @@ -0,0 +1,62 @@ +import { buildCommitmentSummary, commitmentTotals, paymentLabel, withIntro, type CommitmentLine } from '../commitmentSummary'; + +const line = (over: Partial = {}): CommitmentLine => ({ + service: 'ec2', + resourceType: 'm5.large', + count: 2, + term: 3, + payment: 'no-upfront', + upfront: 0, + monthly: 100, + ...over, +}); + +describe('commitmentTotals', () => { + it('sums upfront, monthly and the term total (3 years = 36 months)', () => { + const t = commitmentTotals([line({ upfront: 500, monthly: 100 }), line({ upfront: 0, monthly: 50, term: 1 })]); + expect(t.upfront).toBe(500); + expect(t.monthly).toBe(150); + expect(t.termTotal).toBe(500 + 100 * 36 + 50 * 12); + }); + + it('is null for monthly and term total when any line has no monthly cost', () => { + const t = commitmentTotals([line(), line({ monthly: null, upfront: 10 })]); + expect(t.upfront).toBe(10); + expect(t.monthly).toBeNull(); + expect(t.termTotal).toBeNull(); + }); +}); + +describe('paymentLabel', () => { + it.each([ + ['all-upfront', 'All upfront'], + ['partial_upfront', 'Partial upfront'], + ['no-upfront', 'No upfront'], + ['monthly', 'monthly'], + ])('%s -> %s', (raw, label) => expect(paymentLabel(raw)).toBe(label)); +}); + +describe('buildCommitmentSummary', () => { + it('shows charged today, monthly and total over term', () => { + const el = buildCommitmentSummary([line({ upfront: 1200, monthly: 10, term: 1, account: 'prod' })]); + const text = el.textContent ?? ''; + expect(text).toContain('Charged today$1,200.00'); + expect(text).toContain('Monthly$10.00'); + expect(text).toContain('Total over term$1,320.00'); + expect(text).toContain('2 × ec2 m5.large · 1 Year, No upfront · prod'); + }); + + it('says so when the monthly cost is not reported, and truncates long lists', () => { + const el = buildCommitmentSummary(Array.from({ length: 7 }, () => line({ monthly: null }))); + expect(el.textContent).toContain('MonthlyNot reported'); + expect(el.textContent).toContain('Total over termNot available'); + expect(el.querySelectorAll('li')).toHaveLength(6); + expect(el.textContent).toContain('and 2 more'); + }); + + it('withIntro puts the sentence before the summary', () => { + const body = withIntro('Intro.', buildCommitmentSummary([line()])); + expect(body.firstElementChild?.textContent).toBe('Intro.'); + expect(body.querySelector('.commitment-summary')).not.toBeNull(); + }); +}); diff --git a/frontend/src/__tests__/plans.test.ts b/frontend/src/__tests__/plans.test.ts index 92fa41ddb..32eba8acd 100644 --- a/frontend/src/__tests__/plans.test.ts +++ b/frontend/src/__tests__/plans.test.ts @@ -957,6 +957,34 @@ describe('Plans Module', () => { expect(api.runPlannedPurchase).not.toHaveBeenCalled(); }); + test('run confirm names the step and shows the amount charged today', async () => { + mockConfirmDialog.mockClear(); + mockConfirmDialog.mockResolvedValue(false); + + (document.querySelector('[data-action="run"]') as HTMLButtonElement).click(); + await new Promise(resolve => setTimeout(resolve, 50)); + + const opts = mockConfirmDialog.mock.calls[0]![0] as { title: string; body: HTMLElement }; + expect(opts.title).toBe('Run step 1/4 of "Test Plan" now?'); + expect(opts.body.textContent).toMatch(/Charged today\$1,?000/); + expect(opts.body.textContent).toContain('5 \u00d7 ec2 t3.medium'); + }); + + test('a second click while the run confirm is open does not stack a second dialog', async () => { + mockConfirmDialog.mockClear(); + let resolveDialog: (v: boolean) => void = () => undefined; + mockConfirmDialog.mockReturnValue(new Promise(r => { resolveDialog = r; })); + + const runBtn = document.querySelector('[data-action="run"]') as HTMLButtonElement; + runBtn.click(); + runBtn.click(); + await new Promise(resolve => setTimeout(resolve, 20)); + expect(mockConfirmDialog).toHaveBeenCalledTimes(1); + + resolveDialog(false); + await new Promise(resolve => setTimeout(resolve, 20)); + }); + test('pause action pauses purchase', async () => { (api.pausePlannedPurchase as jest.Mock).mockResolvedValue({}); (api.getPlannedPurchases as jest.Mock).mockResolvedValue({ purchases: [] }); diff --git a/frontend/src/__tests__/purchase-modal-submit.test.ts b/frontend/src/__tests__/purchase-modal-submit.test.ts index d10cf9f09..efa5810b4 100644 --- a/frontend/src/__tests__/purchase-modal-submit.test.ts +++ b/frontend/src/__tests__/purchase-modal-submit.test.ts @@ -308,9 +308,12 @@ describe('Issue #251: Execute Now makes no cancellation promise', () => { (document.getElementById('execute-purchase-btn') as HTMLButtonElement).click(); await flush(); expect(confirmDialog).toHaveBeenCalledTimes(1); - const opts = (confirmDialog as jest.Mock).mock.calls[0]![0] as { title: string; body: string }; - expect(opts.body).toMatch(/charge the full upfront amount immediately/); - expect(`${opts.title} ${opts.body}`).not.toMatch(/AWS|cancell|24 hours/i); + const opts = (confirmDialog as jest.Mock).mock.calls[0]![0] as { title: string; body: HTMLElement }; + const bodyText = opts.body.textContent ?? ''; + expect(bodyText).toMatch(/charges the amount shown under "Charged today" immediately/); + expect(bodyText).toContain('Charged today'); + expect(bodyText).toContain(formatCurrency(rec.upfront_cost, '$', 2)); + expect(`${opts.title} ${bodyText}`).not.toMatch(/AWS|cancell|24 hours/i); }); }); diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index 83b17691b..798ccdd0b 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -223,6 +223,38 @@ describe('openExchangeModal', () => { expect(execReq.region).toBe('us-west-2'); }); + it('names instance types and the hourly price change in the execute confirm', async () => { + const mockGetQuote = api.getExchangeQuote as jest.Mock; + mockGetQuote.mockResolvedValueOnce({ + IsValidExchange: true, + ValidationFailureReason: '', + CurrencyCode: 'USD', + PaymentDueRaw: '12.50', + SourceHourlyPriceRaw: '0.1000', + SourceRemainingUpfrontRaw: '', + SourceRemainingTotalRaw: '', + TargetHourlyPriceRaw: '0.1250', + TargetRemainingUpfrontRaw: '', + TargetRemainingTotalRaw: '', + Region: 'us-west-2', + }); + const offeringUUID = '4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91'; + openExchangeModal('ri-abc', 3, 'm5.large', [ + { instance_type: 'm5.large', offering_id: offeringUUID, effective_monthly_cost: 42.5 }, + ]); + Array.from(modal.querySelectorAll('button')).find((b) => b.textContent === 'Get Quote')?.click(); + await new Promise((resolve) => setTimeout(resolve, 0)); + Array.from(modal.querySelectorAll('button')).find((b) => b.textContent === 'Execute Exchange')?.click(); + + const text = document.querySelector('.modal-confirm-body')?.textContent ?? ''; + expect(text).toContain('Pay now: USD 12.50'); + expect(text).toContain('3 \u00d7 m5.large'); + expect(text).not.toContain(offeringUUID); + expect(text).toContain('USD 0.1000 \u2192 0.1250 (+0.0250/hr, about +18.25/month)'); + expect(document.activeElement?.textContent).toBe('Cancel'); + document.querySelector('.modal-confirm-close')?.click(); + }); + it('invalidates a fetched quote when the count changes, hiding Execute (issue #244)', async () => { const mockGetQuote = api.getExchangeQuote as jest.Mock; mockGetQuote.mockResolvedValueOnce({ diff --git a/frontend/src/app.ts b/frontend/src/app.ts index b187ae818..a4b9cee92 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -6,7 +6,7 @@ import * as api from './api'; import * as state from './state'; import { showLoginModal, showAdminSetupModal, showResetPasswordModal, updateUserUI } from './auth'; import { loadDashboard, setupDashboardHandlers } from './dashboard'; -import { setupRecommendationsHandlers, getPurchaseModalRecommendations, clearPurchaseModalRecommendations, getFanOutBuckets, clearFanOutBuckets, getExecuteMode, clearExecuteMode, fanOutBucketAccountLabel, type FanOutBucket } from './recommendations'; +import { setupRecommendationsHandlers, getPurchaseModalRecommendations, clearPurchaseModalRecommendations, getFanOutBuckets, clearFanOutBuckets, getExecuteMode, clearExecuteMode, fanOutBucketAccountLabel, getAccountName, type FanOutBucket } from './recommendations'; import { switchTab, applyTabFromPath, initRouter, switchSettingsSubTab, canonicalTabPath } from './navigation'; import { savePlan, setupPlanHandlers, closePlanModal, openNewPlanModal, closePurchaseModal } from './plans'; import { saveGlobalSettings, setupSettingsHandlers, resetSettings } from './settings'; @@ -18,6 +18,7 @@ import { setupRIExchangeHandlers, saveAutomationSettings } from './riexchange'; import { showToast } from './toast'; import { formatPaymentAdjustmentNotice } from './commitmentOptions'; import { confirmDialog } from './confirmDialog'; +import { buildCommitmentSummary, withIntro } from './commitmentSummary'; import { handlePurchaseDeeplink } from './purchases-deeplink'; import { handleArcheraDeeplink, openArcheraOfferModal } from './archera'; import { closeModal } from './modal'; @@ -398,16 +399,30 @@ export async function handleExecutePurchase(): Promise { // - Approval path: low-friction, non-destructive. // - Direct-execute path: red destructive dialog with cost callout and // immediate-charge callout; no cancellation promise (issue #251). + const summary = buildCommitmentSummary( + recsWithPayment.map((r) => ({ + service: r.service, + provider: r.provider, + region: r.region, + resourceType: r.resource_type, + count: r.count, + term: r.term, + payment: r.payment, + upfront: r.upfront_cost, + monthly: r.monthly_cost ?? null, + ...(r.cloud_account_id ? { account: getAccountName(r.cloud_account_id) } : {}), + })), + ); const ok = isDirect ? await confirmDialog({ title: `Execute ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'} now?`, - body: 'This will charge the full upfront amount immediately. This bypasses the approval step.', + body: withIntro('This charges the amount shown under "Charged today" immediately and bypasses the approval step. The monthly amount, if any, is billed for the whole term.', summary), confirmLabel: 'Execute Purchase Now', destructive: true, }) : await confirmDialog({ title: `Send ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'} for approval?`, - body: 'This will email an approval request to the configured approver. Cloud commitments are charged only after the approver clicks the link in that email.', + body: withIntro('This emails an approval request to the configured approver. Nothing is charged until the approver clicks the link in that email.', summary), confirmLabel: 'Send for approval', destructive: false, }); diff --git a/frontend/src/commitmentSummary.ts b/frontend/src/commitmentSummary.ts new file mode 100644 index 000000000..f3c1e5503 --- /dev/null +++ b/frontend/src/commitmentSummary.ts @@ -0,0 +1,119 @@ +/** + * Shared "what exactly will be bought" block for money-moving confirm + * dialogs (Execute now, Run now, RI exchange). Pure DOM builder plus the + * totals helper, so every confirm states the same three numbers: what is + * charged today, what recurs monthly, and the total over the term. + */ + +import { formatCurrency, formatTerm } from './utils'; + +export interface CommitmentLine { + service: string; + provider?: string; + region?: string; + resourceType: string; + count: number; + /** Term in years. */ + term: number; + payment: string; + upfront: number; + /** null when the provider did not report a recurring breakdown. */ + monthly: number | null; + /** Resolved display name of the account, when known. */ + account?: string; +} + +export interface CommitmentTotals { + upfront: number; + /** null when any line has no reported monthly cost. */ + monthly: number | null; + /** upfront + monthly x 12 x term; null when monthly is null. */ + termTotal: number | null; +} + +const MAX_LISTED_LINES = 5; + +export function commitmentTotals(lines: readonly CommitmentLine[]): CommitmentTotals { + let upfront = 0; + let monthly: number | null = 0; + let termTotal: number | null = 0; + for (const l of lines) { + upfront += l.upfront; + if (l.monthly === null || monthly === null || termTotal === null) { + monthly = null; + termTotal = null; + continue; + } + monthly += l.monthly; + termTotal += l.upfront + l.monthly * 12 * l.term; + } + return { upfront, monthly, termTotal }; +} + +export function paymentLabel(payment: string): string { + switch (payment.replace(/_/g, '-')) { + case 'all-upfront': return 'All upfront'; + case 'partial-upfront': return 'Partial upfront'; + case 'no-upfront': return 'No upfront'; + default: return payment; + } +} + +function stat(label: string, value: string): HTMLElement { + const el = document.createElement('div'); + el.className = 'commitment-summary-stat'; + const l = document.createElement('span'); + l.className = 'commitment-summary-label'; + l.textContent = label; + const v = document.createElement('strong'); + v.className = 'commitment-summary-value'; + v.textContent = value; + el.append(l, v); + return el; +} + +function describe(l: CommitmentLine): string { + const where = [l.account, l.region].filter(Boolean).join(', '); + const what = `${l.count} × ${l.service} ${l.resourceType}`; + const terms = `${formatTerm(l.term)}, ${paymentLabel(l.payment)}`; + return [what, terms, where].filter(Boolean).join(' · '); +} + +export function buildCommitmentSummary(lines: readonly CommitmentLine[]): HTMLElement { + const totals = commitmentTotals(lines); + const root = document.createElement('div'); + root.className = 'commitment-summary'; + + const band = document.createElement('div'); + band.className = 'commitment-summary-band'; + band.append( + stat('Charged today', formatCurrency(totals.upfront, '$', 2)), + stat('Monthly', totals.monthly === null ? 'Not reported' : formatCurrency(totals.monthly, '$', 2)), + stat('Total over term', totals.termTotal === null ? 'Not available' : formatCurrency(totals.termTotal, '$', 2)), + ); + root.appendChild(band); + + const list = document.createElement('ul'); + list.className = 'commitment-summary-lines'; + for (const l of lines.slice(0, MAX_LISTED_LINES)) { + const li = document.createElement('li'); + li.textContent = describe(l); + list.appendChild(li); + } + if (lines.length > MAX_LISTED_LINES) { + const li = document.createElement('li'); + li.textContent = `and ${lines.length - MAX_LISTED_LINES} more`; + list.appendChild(li); + } + root.appendChild(list); + return root; +} + +/** Intro sentence followed by the summary block, as one dialog body. */ +export function withIntro(intro: string, summary: HTMLElement): HTMLElement { + const root = document.createElement('div'); + const p = document.createElement('p'); + p.textContent = intro; + root.append(p, summary); + return root; +} diff --git a/frontend/src/plans.ts b/frontend/src/plans.ts index 25215ab55..a403aca24 100644 --- a/frontend/src/plans.ts +++ b/frontend/src/plans.ts @@ -4,6 +4,7 @@ import * as api from './api'; import * as state from './state'; +import { buildCommitmentSummary, withIntro } from './commitmentSummary'; import { formatDate, formatTerm, getStatusBadge, escapeHtml, escapeHtmlAttr, formatCurrency, CURRENCY_DEFAULT_DIGITS, providerBadgeHtml } from './utils'; import { showToast } from './toast'; import { confirmDialog } from './confirmDialog'; @@ -775,26 +776,58 @@ function getPlannedPurchaseStatusClass(status: string): string { } } +// Purchase ids whose "Run now" confirm or request is open, so a second click +// on the row button cannot stack a second dialog or send a second request. +const runNowInFlight = new Set(); + +function runNowBody(target: PlannedPurchase | undefined): string | HTMLElement { + if (!target) return 'This immediately executes the purchase.'; + if (target.has_recommendations === false) { + return 'This step has no recommendations attached, so running it will not buy anything.'; + } + const summary = buildCommitmentSummary([{ + service: target.service, + provider: target.provider, + region: target.region, + resourceType: target.resource_type, + count: target.count, + term: target.term, + payment: target.payment, + upfront: target.upfront_cost, + // Planned purchases carry no recurring-cost breakdown. + monthly: null, + }]); + return withIntro(`This immediately buys the commitment below for plan "${target.plan_name}", scheduled for ${formatDate(target.scheduled_date)}. It cannot be undone.`, summary); +} + +async function runPlannedPurchaseNow(purchaseId: string): Promise { + if (runNowInFlight.has(purchaseId)) return; + runNowInFlight.add(purchaseId); + try { + const target = lastLoadedPurchases.find(p => p.id === purchaseId); + const ok = await confirmDialog({ + title: target ? `Run step ${target.step_number}/${target.total_steps} of "${target.plan_name}" now?` : 'Run purchase now?', + body: runNowBody(target), + confirmLabel: 'Run now', + destructive: true, + }); + if (!ok) return; + await api.runPlannedPurchase(purchaseId); + showToast({ message: 'Purchase executed successfully', kind: 'success', timeout: 5_000 }); + } finally { + runNowInFlight.delete(purchaseId); + } +} + /** * Handle planned purchase action */ async function handlePlannedPurchaseAction(action: string, purchaseId: string, planId = ''): Promise { try { switch (action) { - case 'run': { - // Use styled async dialog (11-L2) instead of blocking browser confirm(). - const runOk = await confirmDialog({ - title: 'Run purchase now?', - body: 'This will immediately execute the purchase.', - confirmLabel: 'Run now', - destructive: true, - }); - if (runOk) { - await api.runPlannedPurchase(purchaseId); - showToast({ message: 'Purchase executed successfully', kind: 'success', timeout: 5_000 }); - } + case 'run': + await runPlannedPurchaseNow(purchaseId); break; - } case 'pause': // Pause is reversible and scoped to a single execution: the plan stays // enabled (unlike Disable plan) and the row stays listed with a Paused diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 12674fd93..35dfba0f9 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -1847,12 +1847,24 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy try { confirmed = await confirmDialog({ title: 'Execute RI Exchange', - body: `Quoted payment: ${quote.CurrencyCode} ${quote.PaymentDueRaw}. ` - + `Source RIs: ${request.ri_ids.join(', ')}. Region: ${quote.Region}. ` - + `Targets: ${targets.map(target => `${target.count} × ${target.offering_id}`).join(', ')}. ` - + 'This exchange executes immediately and cannot be reversed.', + body: buildExchangeConfirmBody({ + quote, + sources: request.ri_ids.map(id => { + const ri = currentRIs.find(r => r.reserved_instance_id === id); + return ri ? `${ri.instance_count} \u00d7 ${ri.instance_type} (${id})` : id; + }), + targets: targets.map(target => { + const o = awsOfferings.find(x => x.offering_id === target.offering_id); + if (o) return `${target.count} \u00d7 ${o.instance_type} (${o.offering_type})`; + const alt = alternativeTargets?.find(x => x.offering_id === target.offering_id); + return alt + ? `${target.count} \u00d7 ${alt.instance_type}` + : `${target.count} \u00d7 offering ${target.offering_id}`; + }), + }), confirmLabel: 'Execute Exchange', destructive: true, + initialFocus: 'cancel', }); if (!confirmed) return; setResultText(resultContainer, 'Executing exchange...', 'loading'); @@ -1884,6 +1896,45 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy } } +interface ExchangeConfirmParts { + quote: ExchangeQuoteSummary; + sources: string[]; + targets: string[]; +} + +// Hours in an average month; only used for the approximate monthly delta. +const HOURS_PER_MONTH = 730; + +function buildExchangeConfirmBody({ quote, sources, targets }: ExchangeConfirmParts): HTMLElement { + const root = document.createElement('div'); + const line = (label: string, value: string): void => { + const p = document.createElement('p'); + const strong = document.createElement('strong'); + strong.textContent = value; + p.append(`${label}: `, strong); + root.appendChild(p); + }; + line('Pay now', `${quote.CurrencyCode} ${quote.PaymentDueRaw}`); + line('Exchange', sources.join(', ')); + line('For', targets.join(', ')); + line('Region', quote.Region ?? ''); + const src = Number(quote.SourceHourlyPriceRaw); + const tgt = Number(quote.TargetHourlyPriceRaw); + if (quote.SourceHourlyPriceRaw && quote.TargetHourlyPriceRaw && Number.isFinite(src) && Number.isFinite(tgt)) { + const delta = tgt - src; + const sign = delta >= 0 ? '+' : '-'; + const abs = Math.abs(delta); + line( + 'Hourly price change', + `${quote.CurrencyCode} ${src.toFixed(4)} \u2192 ${tgt.toFixed(4)} (${sign}${abs.toFixed(4)}/hr, about ${sign}${(abs * HOURS_PER_MONTH).toFixed(2)}/month)`, + ); + } + const warn = document.createElement('p'); + warn.textContent = 'This exchange executes immediately and cannot be reversed.'; + root.appendChild(warn); + return root; +} + function setResultText(container: HTMLElement, message: string, cls: string): void { container.textContent = ''; const p = document.createElement('p'); diff --git a/frontend/src/styles/components.css b/frontend/src/styles/components.css index d9f1fe6f0..5af4fd1e9 100644 --- a/frontend/src/styles/components.css +++ b/frontend/src/styles/components.css @@ -1696,3 +1696,35 @@ tr.rec-variant-row:hover td:not(.checkbox-col) { text-align: right; font-variant-numeric: tabular-nums; } + +/* Money confirm dialogs: shared stat band (commitmentSummary.ts). */ +.commitment-summary-band { + display: grid; + grid-template-columns: repeat(auto-fit, minmax(120px, 1fr)); + gap: 0.5rem; + margin: 0.75rem 0; +} + +.commitment-summary-stat { + display: flex; + flex-direction: column; + padding: 0.5rem 0.75rem; + background: var(--cudly-surface-muted); + border-radius: 6px; +} + +.commitment-summary-label { + font-size: 0.8rem; + color: var(--cudly-text-muted); +} + +.commitment-summary-value { + font-size: 1.15rem; + font-variant-numeric: tabular-nums; +} + +.commitment-summary-lines { + margin: 0.5rem 0 0; + padding-left: 1.1rem; + font-size: 0.9rem; +} From 0857a56fd2e7a420e2468ef797371d1fce47a8c6 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 13:01:34 +0200 Subject: [PATCH 5/8] fix(frontend): keep the exact offering id in the RI exchange confirm The confirm shows the instance type first and the offering id next to it, so the exact target stays verifiable. Refs #709 Co-Authored-By: Claude Sonnet 5.5 --- frontend/src/__tests__/riexchange.test.ts | 3 ++- frontend/src/riexchange.ts | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index 798ccdd0b..ad2f11cb7 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -249,7 +249,8 @@ describe('openExchangeModal', () => { const text = document.querySelector('.modal-confirm-body')?.textContent ?? ''; expect(text).toContain('Pay now: USD 12.50'); expect(text).toContain('3 \u00d7 m5.large'); - expect(text).not.toContain(offeringUUID); + // The exact offering id stays visible next to the instance type. + expect(text).toContain(`3 \u00d7 m5.large (offering ${offeringUUID})`); expect(text).toContain('USD 0.1000 \u2192 0.1250 (+0.0250/hr, about +18.25/month)'); expect(document.activeElement?.textContent).toBe('Cancel'); document.querySelector('.modal-confirm-close')?.click(); diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index 35dfba0f9..c057be884 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -1855,10 +1855,10 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy }), targets: targets.map(target => { const o = awsOfferings.find(x => x.offering_id === target.offering_id); - if (o) return `${target.count} \u00d7 ${o.instance_type} (${o.offering_type})`; + if (o) return `${target.count} \u00d7 ${o.instance_type} (${o.offering_type}, offering ${target.offering_id})`; const alt = alternativeTargets?.find(x => x.offering_id === target.offering_id); return alt - ? `${target.count} \u00d7 ${alt.instance_type}` + ? `${target.count} \u00d7 ${alt.instance_type} (offering ${target.offering_id})` : `${target.count} \u00d7 offering ${target.offering_id}`; }), }), From fc4c1c2f2f63f9b86c4a61c3de73446bb13689df Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 13:06:44 +0200 Subject: [PATCH 6/8] fix(frontend): label approval-path amounts and flag unreported monthly fees The approval confirm shows "Upfront on approval" instead of "Charged today", since nothing is charged until the approver acts. A line that is not all upfront with no monthly figure says monthly fees apply. The exchange price change names its currency. Refs #707, #709 Co-Authored-By: Claude Sonnet 5.5 --- .../src/__tests__/commitmentSummary.test.ts | 17 +++++++++++++++++ .../__tests__/purchase-modal-submit.test.ts | 14 ++++++++++++++ frontend/src/__tests__/riexchange.test.ts | 2 +- frontend/src/app.ts | 1 + frontend/src/commitmentSummary.ts | 18 +++++++++++++++--- frontend/src/riexchange.ts | 2 +- 6 files changed, 49 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/commitmentSummary.test.ts b/frontend/src/__tests__/commitmentSummary.test.ts index ee2f1c023..c08d55875 100644 --- a/frontend/src/__tests__/commitmentSummary.test.ts +++ b/frontend/src/__tests__/commitmentSummary.test.ts @@ -27,6 +27,23 @@ describe('commitmentTotals', () => { }); }); +describe('summary options and hints', () => { + it('relabels the upfront stat for the approval path', () => { + const text = buildCommitmentSummary([line({ upfront: 8400 })], { upfrontLabel: 'Upfront on approval' }).textContent; + expect(text).toContain('Upfront on approval$8,400.00'); + expect(text).not.toContain('Charged today'); + }); + + it('hints that monthly fees apply for non all-upfront lines with no monthly figure', () => { + expect(buildCommitmentSummary([line({ monthly: null, payment: 'partial-upfront' })]).textContent) + .toContain('MonthlyNot reported (monthly fees apply)'); + expect(buildCommitmentSummary([line({ monthly: null, payment: 'all-upfront' })]).textContent) + .toContain('MonthlyNot reported'); + expect(buildCommitmentSummary([line({ monthly: null, payment: 'all-upfront' })]).textContent) + .not.toContain('fees apply'); + }); +}); + describe('paymentLabel', () => { it.each([ ['all-upfront', 'All upfront'], diff --git a/frontend/src/__tests__/purchase-modal-submit.test.ts b/frontend/src/__tests__/purchase-modal-submit.test.ts index efa5810b4..21b50e881 100644 --- a/frontend/src/__tests__/purchase-modal-submit.test.ts +++ b/frontend/src/__tests__/purchase-modal-submit.test.ts @@ -299,6 +299,20 @@ describe('Issue #251: Execute Now makes no cancellation promise', () => { expect(api.executePurchase).not.toHaveBeenCalled(); }); + test('approval confirm labels the amount as upfront on approval, not charged today', async () => { + const rec: LocalRecommendation = { ...buildRows()[0]!, provider: 'aws', service: 'ec2', term: 1 }; + (state.getRecommendations as jest.Mock).mockReturnValue([rec]); + (confirmDialog as jest.Mock).mockClear(); + await openPurchaseModal([rec]); + (document.getElementById('execute-purchase-btn') as HTMLButtonElement).click(); + await flush(); + const opts = (confirmDialog as jest.Mock).mock.calls[0]![0] as { body: HTMLElement }; + const text = opts.body.textContent ?? ''; + expect(text).toContain('Nothing is charged until the approver'); + expect(text).toContain(`Upfront on approval${formatCurrency(rec.upfront_cost, '$', 2)}`); + expect(text).not.toContain('Charged today'); + }); + test('direct-execute confirm dialog makes no cancellation promise', async () => { const rec: LocalRecommendation = { ...buildRows()[0]!, provider: 'aws', service: 'ec2', term: 1 }; (state.getRecommendations as jest.Mock).mockReturnValue([rec]); diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index ad2f11cb7..8ec4b2ef3 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -251,7 +251,7 @@ describe('openExchangeModal', () => { expect(text).toContain('3 \u00d7 m5.large'); // The exact offering id stays visible next to the instance type. expect(text).toContain(`3 \u00d7 m5.large (offering ${offeringUUID})`); - expect(text).toContain('USD 0.1000 \u2192 0.1250 (+0.0250/hr, about +18.25/month)'); + expect(text).toContain('USD 0.1000 \u2192 0.1250 (+0.0250 USD/hr, about +18.25 USD/month)'); expect(document.activeElement?.textContent).toBe('Cancel'); document.querySelector('.modal-confirm-close')?.click(); }); diff --git a/frontend/src/app.ts b/frontend/src/app.ts index a4b9cee92..b69b82051 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -412,6 +412,7 @@ export async function handleExecutePurchase(): Promise { monthly: r.monthly_cost ?? null, ...(r.cloud_account_id ? { account: getAccountName(r.cloud_account_id) } : {}), })), + isDirect ? {} : { upfrontLabel: 'Upfront on approval' }, ); const ok = isDirect ? await confirmDialog({ diff --git a/frontend/src/commitmentSummary.ts b/frontend/src/commitmentSummary.ts index f3c1e5503..ffbc4b3ed 100644 --- a/frontend/src/commitmentSummary.ts +++ b/frontend/src/commitmentSummary.ts @@ -72,6 +72,13 @@ function stat(label: string, value: string): HTMLElement { return el; } +// A partial or no-upfront commitment bills monthly; saying only "not +// reported" would read as "no monthly fee". +function monthlyUnknownText(lines: readonly CommitmentLine[]): string { + const bills = lines.some(l => paymentLabel(l.payment) !== 'All upfront'); + return bills ? 'Not reported (monthly fees apply)' : 'Not reported'; +} + function describe(l: CommitmentLine): string { const where = [l.account, l.region].filter(Boolean).join(', '); const what = `${l.count} × ${l.service} ${l.resourceType}`; @@ -79,7 +86,12 @@ function describe(l: CommitmentLine): string { return [what, terms, where].filter(Boolean).join(' · '); } -export function buildCommitmentSummary(lines: readonly CommitmentLine[]): HTMLElement { +export interface SummaryOptions { + /** Label for the upfront stat; 'Upfront on approval' when nothing is charged yet. */ + upfrontLabel?: string; +} + +export function buildCommitmentSummary(lines: readonly CommitmentLine[], opts: SummaryOptions = {}): HTMLElement { const totals = commitmentTotals(lines); const root = document.createElement('div'); root.className = 'commitment-summary'; @@ -87,8 +99,8 @@ export function buildCommitmentSummary(lines: readonly CommitmentLine[]): HTMLEl const band = document.createElement('div'); band.className = 'commitment-summary-band'; band.append( - stat('Charged today', formatCurrency(totals.upfront, '$', 2)), - stat('Monthly', totals.monthly === null ? 'Not reported' : formatCurrency(totals.monthly, '$', 2)), + stat(opts.upfrontLabel ?? 'Charged today', formatCurrency(totals.upfront, '$', 2)), + stat('Monthly', totals.monthly === null ? monthlyUnknownText(lines) : formatCurrency(totals.monthly, '$', 2)), stat('Total over term', totals.termTotal === null ? 'Not available' : formatCurrency(totals.termTotal, '$', 2)), ); root.appendChild(band); diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index c057be884..109e48478 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -1926,7 +1926,7 @@ function buildExchangeConfirmBody({ quote, sources, targets }: ExchangeConfirmPa const abs = Math.abs(delta); line( 'Hourly price change', - `${quote.CurrencyCode} ${src.toFixed(4)} \u2192 ${tgt.toFixed(4)} (${sign}${abs.toFixed(4)}/hr, about ${sign}${(abs * HOURS_PER_MONTH).toFixed(2)}/month)`, + `${quote.CurrencyCode} ${src.toFixed(4)} \u2192 ${tgt.toFixed(4)} (${sign}${abs.toFixed(4)} ${quote.CurrencyCode}/hr, about ${sign}${(abs * HOURS_PER_MONTH).toFixed(2)} ${quote.CurrencyCode}/month)`, ); } const warn = document.createElement('p'); From 108fddbd0a6c3ee679edc3f9f2755413e3c56f49 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 13:13:08 +0200 Subject: [PATCH 7/8] test(frontend): e2e asserts instance type and offering id in the exchange confirm Refs #709 Co-Authored-By: Claude Sonnet 5.5 --- frontend/tests-e2e/ri-exchange-confirmation.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts index 060679ba9..e61ebe377 100644 --- a/frontend/tests-e2e/ri-exchange-confirmation.spec.ts +++ b/frontend/tests-e2e/ri-exchange-confirmation.spec.ts @@ -104,10 +104,10 @@ for (const multiple of [false, true]) { } await execute(page).click(); await expect(dialog(page)).toContainText(`USD ${quotedPayment}`); - await expect(dialog(page)).toContainText(`2 × ${offering}`); + await expect(dialog(page)).toContainText(`2 × m5.xlarge (No Upfront, offering ${offering})`); await expect(dialog(page)).toContainText(ri); await expect(dialog(page)).toContainText('us-east-1'); - if (multiple) await expect(dialog(page)).toContainText(`3 × ${secondOffering}`); + if (multiple) await expect(dialog(page)).toContainText(`3 × m5.xlarge (No Upfront, offering ${secondOffering})`); expect(state.requests).toHaveLength(0); // Destructive dialogs open on the safe action, so a reflex Enter cancels. await expect(dialog(page).getByRole('button', { name: 'Cancel', exact: true })).toBeFocused(); From 7d66c1a98d8fed4f53a8cff9932100025558682c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 13:16:13 +0200 Subject: [PATCH 8/8] fix(frontend): recognise the Azure upfront and monthly payment spellings Refs #707 Co-Authored-By: Claude Sonnet 5.5 --- frontend/src/__tests__/commitmentSummary.test.ts | 11 ++++++++++- frontend/src/commitmentSummary.ts | 2 ++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/commitmentSummary.test.ts b/frontend/src/__tests__/commitmentSummary.test.ts index c08d55875..03aa4ded6 100644 --- a/frontend/src/__tests__/commitmentSummary.test.ts +++ b/frontend/src/__tests__/commitmentSummary.test.ts @@ -34,6 +34,13 @@ describe('summary options and hints', () => { expect(text).not.toContain('Charged today'); }); + it('treats the Azure "upfront" spelling as all upfront (no monthly-fee hint)', () => { + const text = buildCommitmentSummary([line({ monthly: null, payment: 'upfront' })]).textContent; + expect(text).toContain('MonthlyNot reported'); + expect(text).not.toContain('fees apply'); + expect(text).toContain('All upfront'); + }); + it('hints that monthly fees apply for non all-upfront lines with no monthly figure', () => { expect(buildCommitmentSummary([line({ monthly: null, payment: 'partial-upfront' })]).textContent) .toContain('MonthlyNot reported (monthly fees apply)'); @@ -49,7 +56,9 @@ describe('paymentLabel', () => { ['all-upfront', 'All upfront'], ['partial_upfront', 'Partial upfront'], ['no-upfront', 'No upfront'], - ['monthly', 'monthly'], + ['upfront', 'All upfront'], + ['monthly', 'Monthly'], + ['custom', 'custom'], ])('%s -> %s', (raw, label) => expect(paymentLabel(raw)).toBe(label)); }); diff --git a/frontend/src/commitmentSummary.ts b/frontend/src/commitmentSummary.ts index ffbc4b3ed..84b0762d6 100644 --- a/frontend/src/commitmentSummary.ts +++ b/frontend/src/commitmentSummary.ts @@ -52,9 +52,11 @@ export function commitmentTotals(lines: readonly CommitmentLine[]): CommitmentTo export function paymentLabel(payment: string): string { switch (payment.replace(/_/g, '-')) { + case 'upfront': // Azure spelling of all-upfront case 'all-upfront': return 'All upfront'; case 'partial-upfront': return 'Partial upfront'; case 'no-upfront': return 'No upfront'; + case 'monthly': return 'Monthly'; default: return payment; } }