From afb462ffa372ec81403fae014299072da2662310 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 14:59:11 +0100 Subject: [PATCH 1/6] fix(purchases): keep a failed execute from re-arming the review modal When the server records a failed execution (the error carries its execution_id), close the modal so another click cannot start a second attempt. Co-Authored-By: Claude Sonnet 5.5 --- .../purchase-execution-toast.test.ts | 26 +++++++++++++++++++ frontend/src/app.ts | 7 +++++ 2 files changed, 33 insertions(+) diff --git a/frontend/src/__tests__/purchase-execution-toast.test.ts b/frontend/src/__tests__/purchase-execution-toast.test.ts index d60e94e96..4a43cddee 100644 --- a/frontend/src/__tests__/purchase-execution-toast.test.ts +++ b/frontend/src/__tests__/purchase-execution-toast.test.ts @@ -285,6 +285,32 @@ describe('handleExecutePurchase — single-record path', () => { expect(archera.openArcheraOfferModal).toHaveBeenCalledWith('purchase'); }); + // Issue #1694: a recorded failed execution must not leave Execute armed. + test('definite failure with an execution_id closes the modal', async () => { + const err = Object.assign(new Error('AWS refused the purchase'), { + status: 403, details: { execution_id: 'exec-failed' }, + }); + (api.executePurchase as jest.Mock).mockRejectedValue(err); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + expect(plans.closePurchaseModal).toHaveBeenCalledTimes(1); + expect(recs.clearPurchaseModalRecommendations).toHaveBeenCalled(); + expect(lastToastKind()).toBe('error'); + }); + + test('failure without an execution_id keeps the modal open for retry', async () => { + (api.executePurchase as jest.Mock).mockRejectedValue(new Error('network down')); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + expect(plans.closePurchaseModal).not.toHaveBeenCalled(); + }); + test('does NOT open Archera offer modal when the approval submission throws', async () => { (api.executePurchase as jest.Mock).mockRejectedValue(new Error('boom')); diff --git a/frontend/src/app.ts b/frontend/src/app.ts index 9d68b7409..33455d028 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -610,6 +610,13 @@ export async function handleExecutePurchase(): Promise { const err = error as Error; const verb = isDirect ? 'execute' : 'send for approval'; showToast({ message: `Failed to ${verb} purchase: ${err.message}`, kind: 'error' }); + // The server recorded a failed execution (it returns its id), so the + // attempt is final: close the modal so one more click can't start another. + if ((err as { details?: { execution_id?: unknown } }).details?.execution_id) { + closePurchaseModal(); + clearPurchaseModalRecommendations(); + clearExecuteMode(); + } } finally { setExecutePurchaseSubmitting(executeBtn, false, executeButtonLabel(getExecuteMode() === 'direct')); } From 3bda83418ace5bc6f4954aff8c2da63695286901 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 14:59:21 +0100 Subject: [PATCH 2/6] fix(history): cap failure text and count only completed purchases Cap the toast message and the history status description height, mute Monthly Savings on failed/cancelled/expired rows, and make the Total Purchases card show completed purchases only. Refs #1694 Co-Authored-By: Claude Sonnet 5.5 --- .../history-refused-purchase.test.ts | 98 +++++++++++++++++++ .../__tests__/refused-purchase-css.test.ts | 21 ++++ frontend/src/history.ts | 22 ++++- frontend/src/styles/components.css | 14 +++ 4 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 frontend/src/__tests__/history-refused-purchase.test.ts create mode 100644 frontend/src/__tests__/refused-purchase-css.test.ts diff --git a/frontend/src/__tests__/history-refused-purchase.test.ts b/frontend/src/__tests__/history-refused-purchase.test.ts new file mode 100644 index 000000000..5ebb6a337 --- /dev/null +++ b/frontend/src/__tests__/history-refused-purchase.test.ts @@ -0,0 +1,98 @@ +/** + * Refused-purchase presentation in Purchase History (issue #1694): + * clamped status text, muted savings on rows that bought nothing, and the + * Total Purchases card counting completed purchases only. + */ +import { loadHistory } from '../history'; + +jest.mock('../api', () => ({ + getHistory: jest.fn(), + getConfig: jest.fn().mockResolvedValue({ global: {} }), +})); +jest.mock('../navigation', () => ({ switchTab: jest.fn() })); +jest.mock('../permissions', () => ({ canAccess: jest.fn().mockReturnValue(true) })); +jest.mock('../utils', () => ({ + NO_VALUE: '\u2014', + formatCurrency: jest.fn((v) => (v === null || v === undefined || isNaN(v)) ? '—' : `$${v}`), + formatDate: jest.fn((v) => (v ? new Date(v).toLocaleDateString() : '')), + formatTerm: jest.fn((y) => (y == null ? '' : `${y} Year${y === 1 ? '' : 's'}`)), + escapeHtml: jest.fn((s) => s || ''), + escapeHtmlAttr: jest.fn((s: string | null | undefined) => (s || '').replace(/"/g, '"')), + amortizedMonthly: jest.requireActual('../utils').amortizedMonthly, + populateAccountFilter: jest.fn(() => Promise.resolve()), +})); +jest.mock('../state', () => ({ + getCurrentUser: jest.fn(), + getCurrentProvider: jest.fn().mockReturnValue('all'), + setCurrentProvider: jest.fn(), + getCurrentAccountIDs: jest.fn().mockReturnValue([]), + setCurrentAccountIDs: jest.fn(), + subscribeProvider: jest.fn().mockReturnValue(() => {}), + subscribeAccount: jest.fn().mockReturnValue(() => {}), + getAmortizeUpfront: jest.fn().mockReturnValue(false), + setAmortizeUpfront: jest.fn(), + subscribeAmortizeUpfront: jest.fn().mockReturnValue(() => {}), + getPurchaseHistoryColumnFilters: jest.fn().mockReturnValue({}), + setPurchaseHistoryColumnFilter: jest.fn(), + clearAllPurchaseHistoryColumnFilters: jest.fn(), + getApprovalQueueColumnFilters: jest.fn().mockReturnValue({}), + setApprovalQueueColumnFilter: jest.fn(), + clearAllApprovalQueueColumnFilters: jest.fn(), +})); + +import * as api from '../api'; + +const row = (o: Record) => ({ + purchase_id: 'p1', timestamp: '2026-11-04T00:00:00Z', provider: 'aws', service: 'ec2', + resource_type: 'm5.large', region: 'us-east-1', count: 1, term: 1, + upfront_cost: 0, monthly_cost: 50, estimated_savings: 10, status: 'completed', ...o, +}); + +beforeEach(() => { + document.body.innerHTML = ` + +
+
`; + jest.clearAllMocks(); +}); + +describe('status description (issue #1694)', () => { + const long = 'AuthFailure: '.repeat(80); + test('is clamped and the full text stays reachable via title', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [ + row({ status: 'failed', status_description: long }), + ] }); + await loadHistory(); + const el = document.querySelector('#history-list td[data-label="Status"] .history-status-desc'); + expect(el).not.toBeNull(); + expect(el?.getAttribute('title')).toBe(long); + }); +}); + +describe('Monthly Savings on rows that bought nothing (issue #1694)', () => { + test.each(['failed', 'cancelled', 'expired'])('%s row is not styled as a saving', async (status) => { + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [ + row({ purchase_id: 'x', status }), + row({ purchase_id: 'ok', status: 'completed' }), + ] }); + await loadHistory(); + const cell = (id: string) => document.querySelector(`#history-list tr[data-execution-id="${id}"] td[data-label="Monthly Savings"]`); + expect(cell('x')?.classList.contains('savings')).toBe(false); + expect(cell('x')?.textContent).toBe('—'); + expect(cell('ok')?.classList.contains('savings')).toBe(true); + expect(cell('ok')?.textContent).toBe('$10'); + }); +}); + +describe('Total Purchases card (issue #1694)', () => { + test('counts completed purchases, not failed or cancelled rows', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ + summary: { total_purchases: 4, total_completed: 0, total_failed: 1, total_pending: 0 }, + purchases: [], + }); + await loadHistory(); + const card = Array.from(document.querySelectorAll('#history-summary .card')) + .find((c) => c.querySelector('h3')?.textContent === 'Total Purchases'); + expect(card?.querySelector('.value')?.textContent).toBe('0'); + }); +}); diff --git a/frontend/src/__tests__/refused-purchase-css.test.ts b/frontend/src/__tests__/refused-purchase-css.test.ts new file mode 100644 index 000000000..de069ee5a --- /dev/null +++ b/frontend/src/__tests__/refused-purchase-css.test.ts @@ -0,0 +1,21 @@ +/** Height caps for long failure text (issue #1694); jsdom has no layout, so assert the CSS rules. */ +import * as fs from 'fs'; +import * as path from 'path'; + +const css = fs.readFileSync(path.join(__dirname, '../styles/components.css'), 'utf8'); +const rule = (sel: string): string => { + const m = css.match(new RegExp(`(?:^|\\n)${sel.replace('.', '\\.')}\\s*\\{([^}]*)\\}`)); + return m?.[1] ?? ''; +}; + +test('toast message is height-capped and scrolls', () => { + const r = rule('.toast-message'); + expect(r).toMatch(/max-height:\s*\d/); + expect(r).toMatch(/overflow-y:\s*auto/); +}); + +test('history status description is line-clamped', () => { + const r = rule('.history-status-desc'); + expect(r).toMatch(/-webkit-line-clamp:\s*\d/); + expect(r).toMatch(/overflow:\s*hidden/); +}); diff --git a/frontend/src/history.ts b/frontend/src/history.ts index 99b00219a..c17f90a59 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -98,6 +98,16 @@ async function refreshFourEyesMode(): Promise { if (banner) banner.classList.toggle('hidden', !_fourEyesMode); } +// Rows that bought nothing must not show their estimate as a realized saving. +const NO_SAVINGS_STATUSES = new Set(['failed', 'cancelled', 'canceled', 'expired']); + +function noSavingsCell(p: HistoryPurchase, empty: boolean): string { + if (empty || NO_SAVINGS_STATUSES.has(normalizeStatus(p).toLowerCase())) { + return `${MUTED_DASH}`; + } + return `${formatCurrency(p.estimated_savings)}`; +} + function normalizeStatus(p: HistoryPurchase): string { // Absent status → legacy DB row → counts as completed for filtering. return p.status || 'completed'; @@ -537,17 +547,19 @@ function renderHistorySummary(summary: HistorySummary | null): void { // summary renders sensibly against an older API deploy that hasn't shipped // the new counters yet. const total = summary.total_purchases ?? null; - const totalDisplay = total !== null ? String(total) : NO_VALUE; + // Failed, cancelled and expired rows bought nothing, so the card counts + // completed purchases only (matches Total Upfront Spent). const completed = summary.total_completed ?? total; + const completedDisplay = completed !== null ? String(completed) : NO_VALUE; const pending = summary.total_pending ?? 0; const detail = (total !== null && pending > 0) - ? `

${completed} completed · ${pending} pending

` + ? `

${pending} pending

` : ''; container.innerHTML = `

Total Purchases

-

${totalDisplay}

+

${completedDisplay}

${detail}
@@ -1406,7 +1418,7 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { return `${badge}
awaiting approval from ${escapeHtml(p.approver)}
`; } if (p.status_description) { - return `${badge}
${escapeHtml(p.status_description)}
${opsHintHtml(p)}`; + return `${badge}
${escapeHtml(p.status_description)}
${opsHintHtml(p)}`; } return `${badge}${opsHintHtml(p)}`; })(); @@ -1433,7 +1445,7 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { ${termCell(p)} ${empty ? `${MUTED_DASH}` : upfrontCell(p, amortize)} ${dashOr(monthlyCostCell)} - ${dashOr(formatCurrency(p.estimated_savings))} + ${noSavingsCell(p, empty)} ${planCellContent} `; diff --git a/frontend/src/styles/components.css b/frontend/src/styles/components.css index b38adc2cd..1f410d35f 100644 --- a/frontend/src/styles/components.css +++ b/frontend/src/styles/components.css @@ -410,6 +410,9 @@ button.secondary:hover { .toast-message { line-height: 1.35; word-break: break-word; + /* A provider error body can be huge; scroll instead of covering the page. */ + max-height: 8rem; + overflow-y: auto; } .toast-actions { @@ -1513,6 +1516,17 @@ tr.rec-variant-row:hover td:not(.checkbox-col) { overflow-wrap: anywhere; } +/* Clamp long failure text; the full text is in the title attribute. */ +.history-status-desc { + max-width: 18rem; + overflow: hidden; + overflow-wrap: anywhere; + display: -webkit-box; + -webkit-box-orient: vertical; + -webkit-line-clamp: 4; + line-clamp: 4; +} + .history-approver strong { color: var(--cudly-text); font-weight: var(--cudly-fw-semibold); From 7e40f07dacf4296adbfe3317b48c37d29b3b1403 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 15:57:18 +0100 Subject: [PATCH 3/6] fix(purchases): close the review modal on any response a purchase may follow The modal now also closes on a 5xx (for example a run whose final status could not be saved) and on a 409 after the row was claimed, not only when the error carries an execution ID. Transport failures and other 4xx refusals keep the modal open so the input can be fixed. A late failure no longer closes a modal opened for a newer purchase. Refs #1694 Co-Authored-By: Claude Opus 5.5 --- .../purchase-execution-toast.test.ts | 47 +++++++++++++++++++ frontend/src/app.ts | 15 ++++-- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/frontend/src/__tests__/purchase-execution-toast.test.ts b/frontend/src/__tests__/purchase-execution-toast.test.ts index 4a43cddee..3dc59cce7 100644 --- a/frontend/src/__tests__/purchase-execution-toast.test.ts +++ b/frontend/src/__tests__/purchase-execution-toast.test.ts @@ -301,6 +301,53 @@ describe('handleExecutePurchase — single-record path', () => { expect(lastToastKind()).toBe('error'); }); + // A 5xx (e.g. a run whose final status could not be saved) or a 409 after the + // row was claimed may follow a real purchase: never leave Execute armed. + test.each([ + ['audit-loss 500', 500], + ['stale-pricing 409', 409], + ])('%s without an execution_id closes the modal', async (_name, status) => { + (api.executePurchase as jest.Mock).mockRejectedValue( + Object.assign(new Error('execution ran but its final status could not be saved'), { status }), + ); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + expect(plans.closePurchaseModal).toHaveBeenCalledTimes(1); + expect(recs.clearPurchaseModalRecommendations).toHaveBeenCalled(); + }); + + test('a 400 validation refusal keeps the modal open so the user can fix the input', async () => { + (api.executePurchase as jest.Mock).mockRejectedValue( + Object.assign(new Error('capacity must be between 1 and 100'), { status: 400 }), + ); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + + expect(plans.closePurchaseModal).not.toHaveBeenCalled(); + }); + + test('a late failure does not close a purchase modal opened after the request was sent', async () => { + let reject: (e: unknown) => void = () => {}; + (api.executePurchase as jest.Mock).mockReturnValue(new Promise((_res, rej) => { reject = rej; })); + + const btn = setup(); + btn.click(); + await new Promise((r) => setTimeout(r, 0)); + // The user closed the modal and opened a new purchase while the first was in flight. + (recs.getPurchaseModalRecommendations as jest.Mock).mockReturnValue([{ id: 'newer' }]); + reject(Object.assign(new Error('refused'), { status: 403, details: { execution_id: 'exec-old' } })); + await new Promise((r) => setTimeout(r, 0)); + + expect(plans.closePurchaseModal).not.toHaveBeenCalled(); + expect(recs.clearPurchaseModalRecommendations).not.toHaveBeenCalled(); + expect(lastToastKind()).toBe('error'); + }); + test('failure without an execution_id keeps the modal open for retry', async () => { (api.executePurchase as jest.Mock).mockRejectedValue(new Error('network down')); diff --git a/frontend/src/app.ts b/frontend/src/app.ts index 33455d028..4f2d1657a 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -610,9 +610,10 @@ export async function handleExecutePurchase(): Promise { const err = error as Error; const verb = isDirect ? 'execute' : 'send for approval'; showToast({ message: `Failed to ${verb} purchase: ${err.message}`, kind: 'error' }); - // The server recorded a failed execution (it returns its id), so the - // attempt is final: close the modal so one more click can't start another. - if ((err as { details?: { execution_id?: unknown } }).details?.execution_id) { + // A recorded execution, a 409 after the claim or any 5xx may follow a real + // purchase: close the modal so one more click can't start another. Only + // act on the modal session that sent this request. + if (attemptMayHaveRun(err) && getPurchaseModalRecommendations() === localRecs) { closePurchaseModal(); clearPurchaseModalRecommendations(); clearExecuteMode(); @@ -622,6 +623,14 @@ export async function handleExecutePurchase(): Promise { } } +// attemptMayHaveRun is false only for transport failures and 4xx refusals +// other than 409, which the server answers before any execution. +function attemptMayHaveRun(err: unknown): boolean { + const e = err as { status?: number; details?: { execution_id?: unknown } }; + const status = e.status ?? 0; + return Boolean(e.details?.execution_id) || status >= 500 || status === 409; +} + /** * fanOutBucketLabel renders a short human identifier for a fan-out bucket so a * partial-failure toast can name which buckets created pending executions From 7d21af5b408c0e51c760d1aab841c732c70c6c7b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 15:57:29 +0100 Subject: [PATCH 4/6] fix(history): reach long status text by keyboard and treat revoked rows as no saving A status description longer than the clamp also gets a "Full message" disclosure, so keyboard and touch users can read it. Revoked rows show no savings, and the Monthly Savings sort and filter count rows that bought nothing as zero. Refs #1694 Co-Authored-By: Claude Opus 5.5 --- .../history-refused-purchase.test.ts | 39 ++++++++++++++++++- frontend/src/history.ts | 22 +++++++++-- frontend/src/styles/components.css | 14 ++++++- 3 files changed, 69 insertions(+), 6 deletions(-) diff --git a/frontend/src/__tests__/history-refused-purchase.test.ts b/frontend/src/__tests__/history-refused-purchase.test.ts index 5ebb6a337..a94365e33 100644 --- a/frontend/src/__tests__/history-refused-purchase.test.ts +++ b/frontend/src/__tests__/history-refused-purchase.test.ts @@ -3,7 +3,7 @@ * clamped status text, muted savings on rows that bought nothing, and the * Total Purchases card counting completed purchases only. */ -import { loadHistory } from '../history'; +import { loadHistory, purchaseHistoryNumericCellValue } from '../history'; jest.mock('../api', () => ({ getHistory: jest.fn(), @@ -67,6 +67,34 @@ describe('status description (issue #1694)', () => { expect(el).not.toBeNull(); expect(el?.getAttribute('title')).toBe(long); }); + + test('a long description is also reachable by keyboard and touch via a disclosure', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [ + row({ status: 'failed', status_description: long }), + ] }); + await loadHistory(); + const details = document.querySelector('#history-list td[data-label="Status"] details.history-status-full'); + expect(details?.querySelector('summary')).not.toBeNull(); + expect(details?.textContent).toContain(long); + }); + + test('a short description has no disclosure', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [ + row({ status: 'failed', status_description: 'the provider returned an error' }), + ] }); + await loadHistory(); + expect(document.querySelector('#history-list details.history-status-full')).toBeNull(); + }); +}); + +describe('savings value used by sort and filter (issue #1694)', () => { + test.each([ + ['failed', { status: 'failed' }], + ['revoked', { status: 'completed', revoked_at: '2026-11-04T01:00:00Z' }], + ])('%s row counts as no saving', (_name, o) => { + expect(purchaseHistoryNumericCellValue(row(o) as never, 'savings')).toBe(0); + expect(purchaseHistoryNumericCellValue(row({}) as never, 'savings')).toBe(10); + }); }); describe('Monthly Savings on rows that bought nothing (issue #1694)', () => { @@ -82,6 +110,15 @@ describe('Monthly Savings on rows that bought nothing (issue #1694)', () => { expect(cell('ok')?.classList.contains('savings')).toBe(true); expect(cell('ok')?.textContent).toBe('$10'); }); + + test('a revoked completed row is not styled as a saving', async () => { + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [ + row({ purchase_id: 'rv', status: 'completed', revoked_at: '2026-11-04T01:00:00Z' }), + ] }); + await loadHistory(); + const cell = document.querySelector('#history-list tr[data-execution-id="rv"] td[data-label="Monthly Savings"]'); + expect(cell?.classList.contains('savings')).toBe(false); + }); }); describe('Total Purchases card (issue #1694)', () => { diff --git a/frontend/src/history.ts b/frontend/src/history.ts index c17f90a59..e85ca3a07 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -101,13 +101,27 @@ async function refreshFourEyesMode(): Promise { // Rows that bought nothing must not show their estimate as a realized saving. const NO_SAVINGS_STATUSES = new Set(['failed', 'cancelled', 'canceled', 'expired']); +// A revoked purchase keeps its "completed" status but no longer saves anything. +function boughtNothing(p: HistoryPurchase): boolean { + return Boolean(p.revoked_at) || NO_SAVINGS_STATUSES.has(normalizeStatus(p).toLowerCase()); +} + function noSavingsCell(p: HistoryPurchase, empty: boolean): string { - if (empty || NO_SAVINGS_STATUSES.has(normalizeStatus(p).toLowerCase())) { + if (empty || boughtNothing(p)) { return `${MUTED_DASH}`; } return `${formatCurrency(p.estimated_savings)}`; } +// About what the 4-line clamp shows; longer text also gets a disclosure, since +// a title tooltip is unreachable by keyboard and touch. +const STATUS_CLAMP_CHARS = 160; + +function statusFullTextHtml(text: string): string { + if (text.length <= STATUS_CLAMP_CHARS) return ''; + return `
Full message
${escapeHtml(text)}
`; +} + function normalizeStatus(p: HistoryPurchase): string { // Absent status → legacy DB row → counts as completed for filtering. return p.status || 'completed'; @@ -1221,14 +1235,14 @@ function purchaseHistoryCategoricalCellValue( } } -function purchaseHistoryNumericCellValue( +export function purchaseHistoryNumericCellValue( p: HistoryPurchase, col: PurchaseHistoryColumnId, ): number { switch (col) { case 'count': return p.count ?? 0; case 'upfront_cost': return p.upfront_cost ?? Number.NaN; - case 'savings': return p.estimated_savings ?? 0; + case 'savings': return boughtNothing(p) ? 0 : p.estimated_savings ?? 0; case 'provider': case 'service': case 'resource_type': @@ -1418,7 +1432,7 @@ function renderHistoryList(purchases: HistoryPurchase[]): void { return `${badge}
awaiting approval from ${escapeHtml(p.approver)}
`; } if (p.status_description) { - return `${badge}
${escapeHtml(p.status_description)}
${opsHintHtml(p)}`; + return `${badge}
${escapeHtml(p.status_description)}
${statusFullTextHtml(p.status_description)}${opsHintHtml(p)}`; } return `${badge}${opsHintHtml(p)}`; })(); diff --git a/frontend/src/styles/components.css b/frontend/src/styles/components.css index 1f410d35f..6588d6191 100644 --- a/frontend/src/styles/components.css +++ b/frontend/src/styles/components.css @@ -1516,7 +1516,8 @@ tr.rec-variant-row:hover td:not(.checkbox-col) { overflow-wrap: anywhere; } -/* Clamp long failure text; the full text is in the title attribute. */ +/* Clamp long failure text; the full text is in the title and, when clamped, + in a keyboard- and touch-reachable disclosure below it. */ .history-status-desc { max-width: 18rem; overflow: hidden; @@ -1527,6 +1528,17 @@ tr.rec-variant-row:hover td:not(.checkbox-col) { line-clamp: 4; } +.history-status-full { + max-width: 18rem; + font-size: 0.85em; + overflow-wrap: anywhere; +} + +.history-status-full > div { + max-height: 12rem; + overflow-y: auto; +} + .history-approver strong { color: var(--cudly-text); font-weight: var(--cudly-fw-semibold); From e5d4caf37135987fc49ca6f5482800bce8e609df Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 17:00:07 +0100 Subject: [PATCH 5/6] fix(purchases): tie the failed-execute close to the modal's open token The previous check compared the selection array with the one captured when the request was sent, but the getter returns a new array on every call, so the modal never closed after a failure. The close now compares the purchase modal's open token, which changes on every open. A Playwright spec drives the real recommendations module: a recorded failure, an audit-loss 500 and a 409 close the modal, and a 400 keeps it open. Refs #1694 Co-Authored-By: Claude Opus 5.5 --- .../src/__tests__/app-fanout-retry.test.ts | 1 + .../src/__tests__/app-submit-toast.test.ts | 1 + .../purchase-execution-toast.test.ts | 3 +- .../src/__tests__/sidebar-anchors.test.ts | 1 + frontend/src/app.ts | 5 +- frontend/src/recommendations.ts | 5 ++ .../purchase-failure-closes-modal.spec.ts | 48 +++++++++++++++++++ 7 files changed, 61 insertions(+), 3 deletions(-) create mode 100644 frontend/tests-e2e/purchase-failure-closes-modal.spec.ts diff --git a/frontend/src/__tests__/app-fanout-retry.test.ts b/frontend/src/__tests__/app-fanout-retry.test.ts index 3300e23c8..de1b094f8 100644 --- a/frontend/src/__tests__/app-fanout-retry.test.ts +++ b/frontend/src/__tests__/app-fanout-retry.test.ts @@ -27,6 +27,7 @@ jest.mock('../archera', () => ({ handleArcheraDeeplink: jest.fn(), openArcheraOf jest.mock('../modal', () => ({ closeModal: jest.fn() })); jest.mock('../recommendations', () => ({ setupRecommendationsHandlers: jest.fn(), + getPurchaseModalSession: () => 1, getPurchaseModalRecommendations: () => [], clearPurchaseModalRecommendations: jest.fn(), getFanOutBuckets: () => (current ? current.map((b) => ({ ...b })) : null), diff --git a/frontend/src/__tests__/app-submit-toast.test.ts b/frontend/src/__tests__/app-submit-toast.test.ts index 2d4aec884..58054972a 100644 --- a/frontend/src/__tests__/app-submit-toast.test.ts +++ b/frontend/src/__tests__/app-submit-toast.test.ts @@ -26,6 +26,7 @@ jest.mock('../archera', () => ({ handleArcheraDeeplink: jest.fn(), openArcheraOf jest.mock('../modal', () => ({ closeModal: jest.fn() })); jest.mock('../recommendations', () => ({ setupRecommendationsHandlers: jest.fn(), + getPurchaseModalSession: () => 1, getPurchaseModalRecommendations: () => [ { service: 'ec2', provider: 'aws', region: 'us-east-1', resource_type: 'm5.large', count: 1, term: 1, payment: 'no-upfront', upfront_cost: 0 }, ], diff --git a/frontend/src/__tests__/purchase-execution-toast.test.ts b/frontend/src/__tests__/purchase-execution-toast.test.ts index 3dc59cce7..acfa21f44 100644 --- a/frontend/src/__tests__/purchase-execution-toast.test.ts +++ b/frontend/src/__tests__/purchase-execution-toast.test.ts @@ -43,6 +43,7 @@ jest.mock('../navigation', () => ({ jest.mock('../recommendations', () => ({ setupRecommendationsHandlers: jest.fn(), getPurchaseModalRecommendations: jest.fn(), + getPurchaseModalSession: jest.fn().mockReturnValue(1), clearPurchaseModalRecommendations: jest.fn(), getFanOutBuckets: jest.fn(), clearFanOutBuckets: jest.fn(), @@ -339,7 +340,7 @@ describe('handleExecutePurchase — single-record path', () => { btn.click(); await new Promise((r) => setTimeout(r, 0)); // The user closed the modal and opened a new purchase while the first was in flight. - (recs.getPurchaseModalRecommendations as jest.Mock).mockReturnValue([{ id: 'newer' }]); + (recs.getPurchaseModalSession as jest.Mock).mockReturnValue(2); reject(Object.assign(new Error('refused'), { status: 403, details: { execution_id: 'exec-old' } })); await new Promise((r) => setTimeout(r, 0)); diff --git a/frontend/src/__tests__/sidebar-anchors.test.ts b/frontend/src/__tests__/sidebar-anchors.test.ts index d1b2cd720..1257a60d4 100644 --- a/frontend/src/__tests__/sidebar-anchors.test.ts +++ b/frontend/src/__tests__/sidebar-anchors.test.ts @@ -30,6 +30,7 @@ jest.mock('../dashboard', () => ({ })); jest.mock('../recommendations', () => ({ setupRecommendationsHandlers: jest.fn(), + getPurchaseModalSession: () => 1, getPurchaseModalRecommendations: jest.fn(), clearPurchaseModalRecommendations: jest.fn(), getFanOutBuckets: jest.fn(), diff --git a/frontend/src/app.ts b/frontend/src/app.ts index 4f2d1657a..8419b4a73 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, getAccountName, type FanOutBucket } from './recommendations'; +import { setupRecommendationsHandlers, getPurchaseModalRecommendations, getPurchaseModalSession, clearPurchaseModalRecommendations, getFanOutBuckets, clearFanOutBuckets, getExecuteMode, clearExecuteMode, fanOutBucketAccountLabel, getAccountName, type FanOutBucket } from './recommendations'; import { switchTab, setActiveNav, applyTabFromPath, initRouter, switchSettingsSubTab, canonicalTabPath } from './navigation'; import { savePlan, setupPlanHandlers, closePlanModal, openNewPlanModal, closePurchaseModal } from './plans'; import { saveGlobalSettings, setupSettingsHandlers, resetSettings } from './settings'; @@ -452,6 +452,7 @@ export async function handleExecutePurchase(): Promise { } const localRecs = getPurchaseModalRecommendations(); + const modalSession = getPurchaseModalSession(); if (localRecs.length === 0) { showToast({ message: 'No recommendations selected for purchase.', kind: 'warning' }); return; @@ -613,7 +614,7 @@ export async function handleExecutePurchase(): Promise { // A recorded execution, a 409 after the claim or any 5xx may follow a real // purchase: close the modal so one more click can't start another. Only // act on the modal session that sent this request. - if (attemptMayHaveRun(err) && getPurchaseModalRecommendations() === localRecs) { + if (attemptMayHaveRun(err) && getPurchaseModalSession() === modalSession) { closePurchaseModal(); clearPurchaseModalRecommendations(); clearExecuteMode(); diff --git a/frontend/src/recommendations.ts b/frontend/src/recommendations.ts index 6452527f3..83f1f981e 100644 --- a/frontend/src/recommendations.ts +++ b/frontend/src/recommendations.ts @@ -69,6 +69,11 @@ let checkedPurchaseModalInitialised = false; // it after its override fetch so only the newest open renders and sets // submit state (issue #430). let purchaseModalOpenToken = 0; + +/** Identifies the current purchase-modal session; changes on every open. */ +export function getPurchaseModalSession(): number { + return purchaseModalOpenToken; +} // Cache of account ID → name for column display let accountNamesCache: Map = new Map(); // Whether the last account-list fetch returned any accounts; null when it diff --git a/frontend/tests-e2e/purchase-failure-closes-modal.spec.ts b/frontend/tests-e2e/purchase-failure-closes-modal.spec.ts new file mode 100644 index 000000000..d6286022e --- /dev/null +++ b/frontend/tests-e2e/purchase-failure-closes-modal.spec.ts @@ -0,0 +1,48 @@ +import { test, expect, type Page } from '@playwright/test'; +import { mockApi, seedAuth } from './fixtures/recs'; + +// Issue #1694: after a failed execute that a purchase may have followed, the +// review modal must close so one more click cannot start a second attempt. +// Runs the real recommendations module, whose selection getter returns a new +// array on every call. +const row = { + id: 'refused-ec2', provider: 'aws', service: 'ec2', cloud_account_id: 'acct-001', + resource_type: 'm5.large', region: 'us-east-1', payment: 'no-upfront', term: 1, count: 1, + upfront_cost: 0, monthly_cost: 40, savings: 20, on_demand_monthly: 60, effective_savings_pct: 33, +}; + +async function executeWith(page: Page, status: number, body: Record) { + await mockApi(page); + await seedAuth(page); + await page.route(/\/api\/recommendations(?:\?.*)?$/, route => route.fulfill({ json: { + recommendations: [row], summary: { total_recommendations: 1, total_upfront_cost: 0, potential_monthly_savings: 20, avg_payback_months: 0 }, regions: [row.region], + } })); + await page.route('**/api/accounts/*/service-overrides', route => route.fulfill({ json: [] })); + await page.route('**/api/purchases/execute', route => route.fulfill({ status, json: body })); + await page.goto('/opportunities'); + const selected = page.locator(`tr.recommendation-row[data-rec-id="${row.id}"]`); + await selected.getByRole('checkbox', { name: /^Select / }).check(); + await page.locator('#bulk-purchase-btn').click(); + await page.locator('#execute-mode-direct').check(); + await page.locator('#execute-purchase-btn').click(); + const confirm = page.locator('.confirm-dialog, [role="alertdialog"], [role="dialog"]').filter({ hasText: 'bypasses the approval step' }).last(); + await confirm.getByRole('button').filter({ hasNotText: /cancel/i }).last().click(); +} + +for (const [name, status, body] of [ + ['a recorded failure (502 with execution_id)', 502, { error: 'execution x failed', execution_id: 'exec-1', status: 'failed' }], + ['an audit-loss 500 without execution_id', 500, { error: 'execution x ran but its final status could not be saved' }], + ['a 409 after the claim', 409, { error: 'pricing changed' }], +] as const) { + test(`${name} closes the review modal`, async ({ page }) => { + await executeWith(page, status, body); + await expect(page.locator('.toast').filter({ hasText: 'Failed to execute purchase' })).toBeVisible(); + await expect(page.locator('#purchase-modal')).toBeHidden(); + }); +} + +test('a 400 validation refusal keeps the review modal open', async ({ page }) => { + await executeWith(page, 400, { error: 'capacity must be between 1 and 100' }); + await expect(page.locator('.toast').filter({ hasText: 'Failed to execute purchase' })).toBeVisible(); + await expect(page.locator('#purchase-modal')).toBeVisible(); +}); From 406cfbe58aa66b8cf5f990e7f40da20719c9023b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 17:10:00 +0100 Subject: [PATCH 6/6] fix(history): offer the full status text whenever the phone layout clamps it The disclosure now appears above 80 characters, which the four-line clamp cuts in the phone card layout. The modal spec also covers a refusal recorded with an execution ID (403). Refs #1694 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/history-refused-purchase.test.ts | 9 +++++++++ frontend/src/history.ts | 7 ++++--- frontend/tests-e2e/purchase-failure-closes-modal.spec.ts | 1 + 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/frontend/src/__tests__/history-refused-purchase.test.ts b/frontend/src/__tests__/history-refused-purchase.test.ts index a94365e33..a975de809 100644 --- a/frontend/src/__tests__/history-refused-purchase.test.ts +++ b/frontend/src/__tests__/history-refused-purchase.test.ts @@ -78,6 +78,15 @@ describe('status description (issue #1694)', () => { expect(details?.textContent).toContain(long); }); + test('a description clamped only in the phone layout (~100 chars) also has a disclosure', async () => { + const medium = 'purchase failed: '.repeat(6); + (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [ + row({ status: 'failed', status_description: medium }), + ] }); + await loadHistory(); + expect(document.querySelector('#history-list details.history-status-full')?.textContent).toContain(medium); + }); + test('a short description has no disclosure', async () => { (api.getHistory as jest.Mock).mockResolvedValue({ summary: {}, purchases: [ row({ status: 'failed', status_description: 'the provider returned an error' }), diff --git a/frontend/src/history.ts b/frontend/src/history.ts index e85ca3a07..c6e82a354 100644 --- a/frontend/src/history.ts +++ b/frontend/src/history.ts @@ -113,9 +113,10 @@ function noSavingsCell(p: HistoryPurchase, empty: boolean): string { return `${formatCurrency(p.estimated_savings)}`; } -// About what the 4-line clamp shows; longer text also gets a disclosure, since -// a title tooltip is unreachable by keyboard and touch. -const STATUS_CLAMP_CHARS = 160; +// About what the 4-line clamp shows in the narrowest (phone card) layout; +// longer text also gets a disclosure, since a title tooltip is unreachable by +// keyboard and touch. +const STATUS_CLAMP_CHARS = 80; function statusFullTextHtml(text: string): string { if (text.length <= STATUS_CLAMP_CHARS) return ''; diff --git a/frontend/tests-e2e/purchase-failure-closes-modal.spec.ts b/frontend/tests-e2e/purchase-failure-closes-modal.spec.ts index d6286022e..298565ea6 100644 --- a/frontend/tests-e2e/purchase-failure-closes-modal.spec.ts +++ b/frontend/tests-e2e/purchase-failure-closes-modal.spec.ts @@ -31,6 +31,7 @@ async function executeWith(page: Page, status: number, body: Record