From 39fac68fc132815f557355b41486929f2aebb5bc Mon Sep 17 00:00:00 2001 From: adlee-was-taken Date: Sat, 30 May 2026 21:47:15 -0400 Subject: [PATCH] fix(ext/popup): defensive Promise.allSettled in devices (Plan C Phase 5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DEV-C P2: Promise.all meant one rejected RPC failed the whole render. allSettled + per-slot fallback keeps the active-devices surface usable when the revoked-list feed (or one bad ssh fingerprint) is down. Two call sites converted in devices.ts: 1. list_devices + list_revoked pair — revoked failures now render an inline "couldn't load" slot instead of failing the page. 2. sshFingerprint map — one bad public key falls back to '(unknown)' instead of killing the whole device list. trash.ts only has a single sendMessage in its load path on this branch, so it has no Promise.all to migrate. Plan was written against a slightly different snapshot; documented divergence in report. Co-Authored-By: Claude Opus 4.7 --- .../components/__tests__/devices.test.ts | 26 +++++++++ extension/src/popup/components/devices.ts | 57 ++++++++++++++----- 2 files changed, 70 insertions(+), 13 deletions(-) diff --git a/extension/src/popup/components/__tests__/devices.test.ts b/extension/src/popup/components/__tests__/devices.test.ts index 60c31b7..2b0233c 100644 --- a/extension/src/popup/components/__tests__/devices.test.ts +++ b/extension/src/popup/components/__tests__/devices.test.ts @@ -95,6 +95,32 @@ describe('devices view', () => { expect(app.querySelector('#register-confirm-btn')).not.toBeNull(); }); + // Plan C Phase 5 — defensive Promise.allSettled: + // a rejected secondary feed (list_revoked) should not kill the whole render. + it('renders devices when revoked list fails (load-error slot shown)', async () => { + (sendMessage as ReturnType) + .mockResolvedValueOnce({ ok: true, data: { devices: [{ name: 'CLI', public_key: 'k', added_at: 1 }] } }) + .mockRejectedValueOnce(new Error('boom')); + + await renderDevices(app); + + // Primary list still rendered. + expect(app.innerHTML).toContain('CLI'); + // Inline fallback slot present. + expect(app.innerHTML).toContain("Couldn't load revoked devices"); + }); + + it('renders devices when revoked list returns {ok:false}', async () => { + (sendMessage as ReturnType) + .mockResolvedValueOnce({ ok: true, data: { devices: [{ name: 'CLI', public_key: 'k', added_at: 1 }] } }) + .mockResolvedValueOnce({ ok: false, error: 'list_revoked_failed' }); + + await renderDevices(app); + + expect(app.innerHTML).toContain('CLI'); + expect(app.innerHTML).toContain("Couldn't load revoked devices"); + }); + it('confirming register sends register_this_device with the entered name', async () => { (chrome.storage.local.get as ReturnType).mockResolvedValueOnce({ device_name: 'Unknown' }); // Initial render: list_devices + list_revoked. diff --git a/extension/src/popup/components/devices.ts b/extension/src/popup/components/devices.ts index 1c83394..8b85373 100644 --- a/extension/src/popup/components/devices.ts +++ b/extension/src/popup/components/devices.ts @@ -31,35 +31,64 @@ export function teardown(): void { // No cleanup needed } +/** + * DEV-C P2: defensive per-slot rendering. The active list is the primary + * feed — if it fails entirely, we still surface an error page. The + * revoked list is secondary — its failure renders an inline "couldn't + * load" slot but doesn't kill the page. + */ +function revokedLoadErrorHtml(): string { + return ` +
+ ▸ revoked devices +
+

Couldn't load revoked devices.

+
+
+ `; +} + export async function renderDevices(app: HTMLElement): Promise { // Get current device name from local storage const stored = await chrome.storage.local.get(['device_name']); const currentDeviceName: string | undefined = stored.device_name as string | undefined; - // Fetch active device list and revoked list in parallel - const [devicesResp, revokedResp] = await Promise.all([ + // Fetch active device list and revoked list in parallel. allSettled so a + // rejected secondary feed doesn't kill the whole render. + const [devicesSettled, revokedSettled] = await Promise.allSettled([ sendMessage({ type: 'list_devices' }), sendMessage({ type: 'list_revoked' }), ]); - if (!devicesResp.ok) { + if (devicesSettled.status === 'rejected' || !devicesSettled.value.ok) { app.innerHTML = `

Failed to load devices

`; return; } - const devices = (devicesResp.data as { devices: Device[] }).devices; - const revokedDevices: RevokedEntry[] = revokedResp.ok - ? (revokedResp.data as { revoked: RevokedEntry[] }).revoked + // devicesSettled.value.ok is true here (guarded above), so .data is present. + const devicesData = (devicesSettled.value as { ok: true; data: unknown }).data; + const devices = (devicesData as { devices: Device[] }).devices; + const revokedOk = revokedSettled.status === 'fulfilled' && revokedSettled.value.ok; + const revokedDevices: RevokedEntry[] = revokedOk + ? ((revokedSettled.value as { ok: true; data: unknown }).data as { revoked: RevokedEntry[] }).revoked : []; const isRegistered = currentDeviceName && devices.some((d) => d.name === currentDeviceName); - // Precompute fingerprints for all active devices + // Precompute fingerprints for all active devices. allSettled so one bad + // public key doesn't kill the whole list — fall back to '(unknown)'. const fingerprints = new Map(); - await Promise.all(devices.map(async (d) => { - const fp = await sshFingerprint(d.public_key); - fingerprints.set(d.name, fp ?? '(unknown)'); - })); + const fpResults = await Promise.allSettled( + devices.map((d) => sshFingerprint(d.public_key).then((fp) => [d.name, fp] as const)), + ); + for (let i = 0; i < devices.length; i += 1) { + const r = fpResults[i]; + if (r.status === 'fulfilled' && r.value[1]) { + fingerprints.set(r.value[0], r.value[1]); + } else { + fingerprints.set(devices[i].name, '(unknown)'); + } + } const activeDevicesHtml = devices.length === 0 ? `

No devices registered

` @@ -82,7 +111,9 @@ export async function renderDevices(app: HTMLElement): Promise { `; }).join(''); - const revokedSectionHtml = revokedDevices.length === 0 ? '' : ` + const revokedSectionHtml = !revokedOk + ? revokedLoadErrorHtml() + : revokedDevices.length === 0 ? '' : `
▸ show ${revokedDevices.length} revoked device${revokedDevices.length !== 1 ? 's' : ''}
@@ -117,7 +148,7 @@ export async function renderDevices(app: HTMLElement): Promise { ` : ''} ${devices.length > 0 ? `
ACTIVE · ${devices.length}
` : ''} ${activeDevicesHtml} - ${revokedDevices.length > 0 ? `
REVOKED · ${revokedDevices.length}
` : ''} + ${!revokedOk ? `
REVOKED · ?
` : (revokedDevices.length > 0 ? `
REVOKED · ${revokedDevices.length}
` : '')} ${revokedSectionHtml}
`;