From 9dc59dfc41f6bc43144e1986492323f784b3d8e0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 06:15:07 +0200 Subject: [PATCH 1/5] fix(frontend): show error and Retry when upcoming purchases fail to load A failed /api/dashboard/upcoming fetch fell through to renderUpcomingPurchases([]) and displayed "No upcoming scheduled purchases", hiding upcoming spend. Render an inline error with a Retry button that refetches only the upcoming widget. The endpoint takes no scope parameters (scoping is server-side by session), so no request change is made. Closes #713 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/dashboard.test.ts | 33 ++++++++++++++++++++ frontend/src/dashboard.ts | 38 +++++++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index f72cd84a7..bcb65d016 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -285,6 +285,39 @@ describe('Dashboard Module', () => { expect(upcomingList?.innerHTML).toContain('Step 1 of 4'); }); + test('shows an inline error with Retry, not the empty state, when the upcoming fetch fails (#713)', async () => { + (api.getDashboardSummary as jest.Mock).mockResolvedValue({ + potential_monthly_savings: 1000, + by_service: {} + }); + (api.getUpcomingPurchases as jest.Mock).mockRejectedValueOnce(new Error('API error: 503 Service Unavailable')); + + await loadDashboard(); + + const upcomingList = document.getElementById('upcoming-list'); + expect(upcomingList?.textContent).not.toContain('No upcoming scheduled purchases'); + expect(upcomingList?.querySelector('.error')?.textContent).toContain('Failed to load upcoming purchases'); + // The summary is unaffected by the upcoming failure. + expect(document.getElementById('summary')?.querySelector('.error')).toBeNull(); + + (api.getUpcomingPurchases as jest.Mock).mockResolvedValueOnce({ + purchases: [{ + execution_id: 'exec-1', plan_id: 'plan-1', plan_name: 'Retry Plan', + scheduled_date: '2025-01-15', provider: 'aws', service: 'ec2', + step_number: 1, total_steps: 2, estimated_savings: 120, + }] + }); + const retry = upcomingList?.querySelector('button[data-action="retry-upcoming"]') as HTMLButtonElement; + expect(retry).not.toBeNull(); + retry.click(); + await new Promise(r => setTimeout(r, 0)); + + expect(api.getUpcomingPurchases).toHaveBeenCalledTimes(2); + expect(api.getDashboardSummary).toHaveBeenCalledTimes(1); + expect(upcomingList?.innerHTML).toContain('Retry Plan'); + expect(upcomingList?.querySelector('.error')).toBeNull(); + }); + test('shows empty message when no upcoming purchases', async () => { (api.getDashboardSummary as jest.Mock).mockResolvedValue({ potential_monthly_savings: 1000, diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index f719c8634..5e7173e1b 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -215,7 +215,12 @@ export async function loadDashboard(): Promise { // Single merged per-service chart (#908): potential range from recs + // current-savings underlay from the summary's by_service map. renderSavingsByService(recs, summaryData.by_service || {}, filterDesc); - renderUpcomingPurchases(upcomingData?.purchases || []); + if (upcomingResult.status === 'rejected') { + console.error('Failed to load upcoming purchases:', upcomingResult.reason); + renderUpcomingError(); + } else { + renderUpcomingPurchases(upcomingData?.purchases || []); + } // Load the savings-over-time widget independently -- failure shouldn't // block the rest of the dashboard (e.g. analytics not configured). void loadSavingsTrendChart(); @@ -428,6 +433,37 @@ export function canCancelUpcomingPurchase(purchase: UpcomingPurchase): boolean { return canAccess('delete', 'purchases') || canAccess('cancel-own', 'purchases'); } +// A failed fetch must not read as "nothing scheduled" (#713): show an inline +// error with a Retry that refetches only this widget. +function renderUpcomingError(): void { + const container = document.getElementById('upcoming-list'); + if (!container) return; + teardownSkeleton(container); + container.textContent = ''; + const msg = document.createElement('p'); + msg.className = 'error'; + msg.textContent = 'Failed to load upcoming purchases.'; + const retry = document.createElement('button'); + retry.type = 'button'; + retry.className = 'btn btn-secondary'; + retry.dataset['action'] = 'retry-upcoming'; + retry.textContent = 'Retry'; + retry.addEventListener('click', () => { void retryUpcomingPurchases(); }); + container.append(msg, retry); +} + +async function retryUpcomingPurchases(): Promise { + const container = document.getElementById('upcoming-list'); + if (container) showSkeletonBlock(container, '100%', '6rem'); + try { + const data = (await api.getUpcomingPurchases()) as { purchases?: UpcomingPurchase[] } | null; + renderUpcomingPurchases(data?.purchases || []); + } catch (error) { + console.error('Failed to load upcoming purchases:', error); + renderUpcomingError(); + } +} + function renderUpcomingPurchases(purchases: UpcomingPurchase[]): void { const container = document.getElementById('upcoming-list'); if (!container) return; From d8e42c1909861babb674597decf527c793f36c64 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 06:16:46 +0200 Subject: [PATCH 2/5] fix(frontend): keep savings chart colours stable per service Bars were coloured by rank index, so a service changed colour whenever the ranking moved after a refresh, and the legend swatches (taken from the first bar) matched only one service. Colour each bar from a stable per-service map (fixed slots for known services, name hash otherwise) and give the legend neutral swatches that mirror the three stacked layers. Stop Chart.js from dropping x-axis service labels on narrow screens by disabling autoSkip and allowing rotation. Closes #700 Co-Authored-By: Claude Opus 5.5 --- frontend/src/__tests__/dashboard.test.ts | 47 +++++++++++++++++++++ frontend/src/dashboard.ts | 52 +++++++++++++++++++++--- 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/frontend/src/__tests__/dashboard.test.ts b/frontend/src/__tests__/dashboard.test.ts index bcb65d016..e2d6afd91 100644 --- a/frontend/src/__tests__/dashboard.test.ts +++ b/frontend/src/__tests__/dashboard.test.ts @@ -1684,6 +1684,53 @@ describe('Dashboard Module', () => { expect(totalLine).toMatch(/\$400/); }); + describe('stable per-service colours and matching legend (#700)', () => { + type ChartCfg = { + data: { labels: string[]; datasets: { label: string; backgroundColor: string[] }[] }; + options: { plugins: { legend: { labels: { generateLabels?: (c: unknown) => { text: string; fillStyle: string }[] } } } }; + }; + const lastCfg = (): ChartCfg => { + const calls = (Chart as unknown as jest.Mock).mock.calls; + return calls[calls.length - 1]?.[1] as ChartCfg; + }; + const colourOf = (cfg: ChartCfg, svc: string): string => { + const ds = cfg.data.datasets.find((d) => d.label === 'Lowest option'); + return ds?.backgroundColor[cfg.data.labels.indexOf(svc)] ?? ''; + }; + + test('a service keeps its colour when its rank changes between renders', () => { + buildDOM(); + renderSavingsByService([rec('ec2', 500), rec('rds', 300), rec('elasticache', 100)]); + const before = lastCfg(); + expect(before.data.labels[0]).toBe('ec2'); + const ec2Before = colourOf(before, 'ec2'); + const rdsBefore = colourOf(before, 'rds'); + const cacheBefore = colourOf(before, 'elasticache'); + + // Data refresh: rds overtakes ec2, elasticache jumps to rank 2. + renderSavingsByService([rec('ec2', 120), rec('rds', 900), rec('elasticache', 400)]); + const after = lastCfg(); + expect(after.data.labels[0]).toBe('rds'); + expect(colourOf(after, 'ec2')).toBe(ec2Before); + expect(colourOf(after, 'rds')).toBe(rdsBefore); + expect(colourOf(after, 'elasticache')).toBe(cacheBefore); + expect(new Set([ec2Before, rdsBefore, cacheBefore]).size).toBe(3); + }); + + test('legend swatches use neutral tones matching the three bar layers, not the first bar colour', () => { + buildDOM(); + renderSavingsByService([rec('ec2', 500), rec('rds', 300)]); + const cfg = lastCfg(); + const gen = cfg.options.plugins.legend.labels.generateLabels; + expect(typeof gen).toBe('function'); + const items = gen?.({ data: { datasets: cfg.data.datasets }, isDatasetVisible: () => true }) ?? []; + expect(items.map((i) => i.text)).toEqual(['Current / Committed', 'Lowest option', 'Upside']); + const barColours = cfg.data.datasets.flatMap((d) => d.backgroundColor); + for (const item of items) expect(barColours).not.toContain(item.fillStyle); + expect(new Set(items.map((i) => i.fillStyle)).size).toBe(3); + }); + }); + test('tooltip shows the committed value for a service with a commitment but no recommendation (#752)', () => { buildDOM(); renderSavingsByService( diff --git a/frontend/src/dashboard.ts b/frontend/src/dashboard.ts index 5e7173e1b..3ef259485 100644 --- a/frontend/src/dashboard.ts +++ b/frontend/src/dashboard.ts @@ -31,8 +31,8 @@ let savingsByServiceChart: Chart | null = null; // Maximum number of services to show in the bar chart before truncating. const SAVINGS_BY_SERVICE_MAX = 10; -// Default palette for per-service bars. Cycles if there are more services -// than colours; alpha variant is computed inline so the array stays short. +// Palette for per-service bars. A service's colour is fixed by its name (see +// serviceBarColor), never by its rank, so it survives data refreshes (#700). const SERVICE_BAR_COLORS = [ '#1a73e8', // blue '#34a853', // green @@ -46,6 +46,36 @@ const SERVICE_BAR_COLORS = [ '#4caf50', // light-green ]; +// Known services get a fixed palette slot so common services never collide; +// anything else falls back to a name hash. +const KNOWN_SERVICE_COLOR_INDEX: Record = { + ec2: 0, compute: 0, computeengine: 0, + rds: 1, sqldb: 1, cloudsql: 1, + elasticache: 2, redis: 2, + savingsplans: 3, + opensearch: 4, + redshift: 5, + memorydb: 6, + lambda: 7, +}; + +/** Stable colour for a service name: identical on every render, regardless of rank. */ +export function serviceBarColor(service: string): string { + const key = service.toLowerCase(); + let idx = KNOWN_SERVICE_COLOR_INDEX[key]; + if (idx === undefined) { + let h = 0; + for (let i = 0; i < key.length; i++) h = (h * 31 + key.charCodeAt(i)) >>> 0; + idx = h; + } + return SERVICE_BAR_COLORS[idx % SERVICE_BAR_COLORS.length] ?? '#1a73e8'; +} + +// The legend names the three stacked layers, which every service shares, so +// its swatches are neutral tones that mirror the bar shading (dark / solid / +// 35% alpha) instead of the first bar's service colour. +const LEGEND_SWATCH_COLORS = ['#37474f', '#78909c', 'rgba(120,144,156,0.35)']; + /** * Parse a #rrggbb hex string into its r/g/b components. Falls back to the * default service bar blue (#1a73e8) for malformed input so the chart never @@ -943,8 +973,8 @@ export function renderSavingsByService( const totalSavings = positive.reduce((acc, e) => acc + e.visibleTotal, 0); - // Colour palette: one base colour per service, then derive lighter/darker shades. - const bgColors = visible.map((_, i) => SERVICE_BAR_COLORS[i % SERVICE_BAR_COLORS.length] ?? '#1a73e8'); + // Colour: one stable base colour per service name, then derive lighter/darker shades. + const bgColors = visible.map(e => serviceBarColor(e.svc)); // Current / Committed: darkest shade (30% darker than base). const currentColors = bgColors.map(c => darkenHexColor(c)); @@ -996,7 +1026,17 @@ export function renderSavingsByService( legend: { display: true, position: 'top', - labels: { boxWidth: 12 }, + labels: { + boxWidth: 12, + generateLabels: (chart) => chart.data.datasets.map((ds, i) => ({ + text: ds.label ?? '', + fillStyle: LEGEND_SWATCH_COLORS[i] ?? LEGEND_SWATCH_COLORS[1] ?? '#78909c', + strokeStyle: LEGEND_SWATCH_COLORS[i] ?? LEGEND_SWATCH_COLORS[1] ?? '#78909c', + lineWidth: 0, + hidden: !chart.isDatasetVisible(i), + datasetIndex: i, + })), + }, }, tooltip: { callbacks: { @@ -1037,6 +1077,8 @@ export function renderSavingsByService( x: { stacked: true, title: { display: true, text: 'Service' }, + // Keep every service label on narrow screens (rotate instead of skipping). + ticks: { autoSkip: false, maxRotation: 60 }, }, y: { stacked: true, From 2b4d2c627af3a03c744f65ca2600569de5e5ff05 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 06:19:35 +0200 Subject: [PATCH 3/5] fix(frontend): name the purchase action and amount in the confirm step The purchase modal was titled "Configure Purchase" and its confirm dialog button read "Send for approval" / "Execute Purchase Now" with no amount, while the modal button used different casing, so the verb and the money involved were easy to miss. Retitle the modal "Review Purchase", and make the confirm button use the same verb as the modal button plus the total upfront amount ("Send for Approval ($X upfront on approval)" / "Execute Purchase Now ($X charged today)"). The mode label literals duplicated across app.ts and recommendations.ts now come from one helper. Closes #701 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/purchase-modal-submit.test.ts | 32 +++++++++++++++++++ frontend/src/app.ts | 22 +++++++------ frontend/src/commitmentSummary.ts | 14 ++++++++ frontend/src/index.html | 2 +- frontend/src/recommendations.ts | 6 ++-- frontend/tests-e2e/recommendations.spec.ts | 2 +- 6 files changed, 63 insertions(+), 15 deletions(-) diff --git a/frontend/src/__tests__/purchase-modal-submit.test.ts b/frontend/src/__tests__/purchase-modal-submit.test.ts index f8b91b880..d9d6ccaee 100644 --- a/frontend/src/__tests__/purchase-modal-submit.test.ts +++ b/frontend/src/__tests__/purchase-modal-submit.test.ts @@ -314,6 +314,38 @@ describe('Issue #251: Execute Now makes no cancellation promise', () => { expect(text).not.toContain('Charged today'); }); + test('confirm button names the action and the upfront amount in both modes (#701)', async () => { + const rec: LocalRecommendation = { ...buildRows()[0]!, provider: 'aws', service: 'ec2', term: 1 }; + (state.getRecommendations as jest.Mock).mockReturnValue([rec]); + const amount = formatCurrency(rec.upfront_cost, '$', 2); + const confirmButtonLabel = (): string => + ((confirmDialog as jest.Mock).mock.calls[0]![0] as { confirmLabel: string }).confirmLabel; + + (confirmDialog as jest.Mock).mockClear(); + await openPurchaseModal([rec]); + const modalBtn = document.getElementById('execute-purchase-btn') as HTMLButtonElement; + const approvalVerb = modalBtn.textContent; + modalBtn.click(); + await flush(); + expect(confirmButtonLabel()).toBe(`${approvalVerb} (${amount} upfront on approval)`); + + (confirmDialog as jest.Mock).mockClear(); + (document.getElementById('close-purchase-modal-btn') as HTMLButtonElement).click(); + await openPurchaseModal([rec]); + (document.getElementById('execute-mode-direct') as HTMLInputElement).click(); + const directVerb = (document.getElementById('execute-purchase-btn') as HTMLButtonElement).textContent; + (document.getElementById('execute-purchase-btn') as HTMLButtonElement).click(); + await flush(); + expect(confirmButtonLabel()).toBe(`${directVerb} (${amount} charged today)`); + }); + + test('the purchase modal title is not the generic "Configure Purchase" (#701)', () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const html = (require('fs') as typeof import('fs')).readFileSync(require('path').join(__dirname, '..', 'index.html'), 'utf8'); + const title = /

([^<]*)<\/h2>/.exec(html)?.[1]; + expect(title).toBe('Review Purchase'); + }); + 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/app.ts b/frontend/src/app.ts index fa0509443..c73b9658e 100644 --- a/frontend/src/app.ts +++ b/frontend/src/app.ts @@ -18,7 +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 { buildCommitmentSummary, commitmentTotals, executeButtonLabel, executeConfirmLabel, withIntro } from './commitmentSummary'; import { handlePurchaseDeeplink } from './purchases-deeplink'; import { handleArcheraDeeplink, openArcheraOfferModal } from './archera'; import { closeModal } from './modal'; @@ -432,8 +432,7 @@ 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) => ({ + const summaryLines = recsWithPayment.map((r) => ({ service: r.service, provider: r.provider, region: r.region, @@ -444,25 +443,28 @@ export async function handleExecutePurchase(): Promise { upfront: r.upfront_cost, monthly: r.monthly_cost ?? null, ...(r.cloud_account_id ? { account: getAccountName(r.cloud_account_id) } : {}), - })), + })); + const totals = commitmentTotals(summaryLines); + const summary = buildCommitmentSummary( + summaryLines, isDirect ? {} : { upfrontLabel: 'Upfront on approval' }, ); const ok = isDirect ? await confirmDialog({ title: `Execute ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'} now?`, 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', + confirmLabel: executeConfirmLabel(true, totals.upfront), destructive: true, }) : await confirmDialog({ title: `Send ${localRecs.length} purchase${localRecs.length === 1 ? '' : 's'} for approval?`, 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', + confirmLabel: executeConfirmLabel(false, totals.upfront), destructive: false, }); if (!ok) { - setExecutePurchaseSubmitting(executeBtn, false, getExecuteMode() === 'direct' ? 'Execute Purchase Now' : 'Send for Approval'); + setExecutePurchaseSubmitting(executeBtn, false, executeButtonLabel(getExecuteMode() === 'direct')); return; } @@ -560,7 +562,7 @@ export async function handleExecutePurchase(): Promise { const verb = isDirect ? 'execute' : 'send for approval'; showToast({ message: `Failed to ${verb} purchase: ${err.message}`, kind: 'error' }); } finally { - setExecutePurchaseSubmitting(executeBtn, false, getExecuteMode() === 'direct' ? 'Execute Purchase Now' : 'Send for Approval'); + setExecutePurchaseSubmitting(executeBtn, false, executeButtonLabel(getExecuteMode() === 'direct')); } } @@ -604,14 +606,14 @@ async function handleFanOutExecute(buckets: FanOutBucket[]): Promise { destructive: false, }); if (!ok) { - setExecutePurchaseSubmitting(executeBtn, false, 'Send for Approval'); + setExecutePurchaseSubmitting(executeBtn, false, executeButtonLabel(false)); return; } try { await submitFanOutBuckets(buckets); } finally { - setExecutePurchaseSubmitting(executeBtn, false, 'Send for Approval'); + setExecutePurchaseSubmitting(executeBtn, false, executeButtonLabel(false)); } } diff --git a/frontend/src/commitmentSummary.ts b/frontend/src/commitmentSummary.ts index 84b0762d6..b01059c8a 100644 --- a/frontend/src/commitmentSummary.ts +++ b/frontend/src/commitmentSummary.ts @@ -50,6 +50,20 @@ export function commitmentTotals(lines: readonly CommitmentLine[]): CommitmentTo return { upfront, monthly, termTotal }; } +/** Label of the purchase modal's primary button; one source for every place that sets it. */ +export function executeButtonLabel(direct: boolean): string { + return direct ? 'Execute Purchase Now' : 'Send for Approval'; +} + +/** + * Confirm-dialog button: the same verb as the modal button plus the upfront + * amount, so the highest-stakes click states the money involved. + */ +export function executeConfirmLabel(direct: boolean, upfront: number): string { + const amount = formatCurrency(upfront, '$', 2); + return `${executeButtonLabel(direct)} (${amount} ${direct ? 'charged today' : 'upfront on approval'})`; +} + export function paymentLabel(payment: string): string { switch (payment.replace(/_/g, '-')) { case 'upfront': // Azure spelling of all-upfront diff --git a/frontend/src/index.html b/frontend/src/index.html index 444da187b..ebbbdd24c 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -995,7 +995,7 @@

Automation Settings