From dcb1590391c4360ed93b1c2c57d183b2310931cc Mon Sep 17 00:00:00 2001 From: adlee-was-taken Date: Sat, 25 Apr 2026 16:23:42 -0400 Subject: [PATCH] fix(ext/popup): guard against sendMessage returning undefined; doc re-wire contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from code review of c5f0449: 1. In MV3 the SW can be killed mid-message; sendMessage then resolves to undefined. Add `(!resp || !resp.ok)` guards at 4 call sites (fetchThumbUrl, settings fetch, upload, download) plus optional chaining on error accessors. 2. JSDoc on wireAttachmentsDisclosure documents the "call once per DOM instance" contract — Task 8's re-wire pattern works because it replaces outerHTML before re-attaching, destroying old listeners via GC. Module-level objectUrlRegistry concern (concurrent disclosure instances) deferred — current popup architecture renders one item at a time, so the issue doesn't manifest today. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../components/attachments-disclosure.ts | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/extension/src/popup/components/attachments-disclosure.ts b/extension/src/popup/components/attachments-disclosure.ts index 670fb2d..5a69a00 100644 --- a/extension/src/popup/components/attachments-disclosure.ts +++ b/extension/src/popup/components/attachments-disclosure.ts @@ -37,7 +37,7 @@ function teardownObjectUrls(): void { async function fetchThumbUrl(itemId: string, attachmentId: string, mime: string): Promise { if (objectUrlRegistry.has(attachmentId)) return objectUrlRegistry.get(attachmentId)!; const resp = await sendMessage({ type: 'download_attachment', itemId, attachmentId }); - if (!resp.ok) return null; + if (!resp || !resp.ok) return null; const data = resp.data as { bytes: ArrayBuffer }; const blob = new Blob([data.bytes], { type: mime }); const url = URL.createObjectURL(blob); @@ -82,6 +82,15 @@ export function renderAttachmentsDisclosure(opts: AttachmentsDisclosureOpts): st `; } +/// Attach event listeners to a disclosure already in the DOM (rendered +/// by `renderAttachmentsDisclosure`). +/// +/// **Contract: call once per DOM instance.** Each invocation adds new +/// listeners; calling this twice on the same `
` element will +/// fire onChange twice per click. The standard re-render pattern is: +/// 1. Replace `disc.outerHTML` with a fresh `renderAttachmentsDisclosure(...)` call +/// 2. Then call `wireAttachmentsDisclosure(...)` to attach handlers to the NEW DOM +/// (Old listeners are GC'd when the previous `
` element is detached.) export function wireAttachmentsDisclosure( root: HTMLElement, opts: AttachmentsDisclosureOpts, @@ -120,7 +129,7 @@ export function wireAttachmentsDisclosure( // Cap enforcement (popup-side, before sending to SW). const settingsResp = await sendMessage({ type: 'get_vault_settings' }); - if (settingsResp.ok) { + if (settingsResp && settingsResp.ok) { const settings = (settingsResp.data as { settings: VaultSettings }).settings; const caps = settings.attachment_caps; if (caps?.per_attachment_max_bytes && file.size > caps.per_attachment_max_bytes) { @@ -143,11 +152,11 @@ export function wireAttachmentsDisclosure( mimeType: file.type || 'application/octet-stream', bytes, }); - if (resp.ok) { + if (resp && resp.ok) { const data = resp.data as { attachment: AttachmentRef }; opts.onChange?.([...opts.attachments, data.attachment]); } else { - alert(`upload failed: ${resp.error}`); + alert(`upload failed: ${resp?.error ?? 'service worker unavailable'}`); } fileInput.value = ''; // allow re-pick of same file later }); @@ -173,8 +182,8 @@ export function wireAttachmentsDisclosure( const att = opts.attachments.find((a) => a.id === attId); if (!att) return; const resp = await sendMessage({ type: 'download_attachment', itemId: opts.itemId, attachmentId: attId }); - if (!resp.ok) { - alert(`download failed: ${resp.error}`); + if (!resp || !resp.ok) { + alert(`download failed: ${resp?.error ?? 'service worker unavailable'}`); return; } const data = resp.data as { bytes: ArrayBuffer; filename: string; mimeType: string };