fix(ext/popup): guard against sendMessage returning undefined; doc re-wire contract
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) <noreply@anthropic.com>
This commit is contained in:
@@ -37,7 +37,7 @@ function teardownObjectUrls(): void {
|
|||||||
async function fetchThumbUrl(itemId: string, attachmentId: string, mime: string): Promise<string | null> {
|
async function fetchThumbUrl(itemId: string, attachmentId: string, mime: string): Promise<string | null> {
|
||||||
if (objectUrlRegistry.has(attachmentId)) return objectUrlRegistry.get(attachmentId)!;
|
if (objectUrlRegistry.has(attachmentId)) return objectUrlRegistry.get(attachmentId)!;
|
||||||
const resp = await sendMessage({ type: 'download_attachment', itemId, 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 data = resp.data as { bytes: ArrayBuffer };
|
||||||
const blob = new Blob([data.bytes], { type: mime });
|
const blob = new Blob([data.bytes], { type: mime });
|
||||||
const url = URL.createObjectURL(blob);
|
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 `<details>` 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 `<details>` element is detached.)
|
||||||
export function wireAttachmentsDisclosure(
|
export function wireAttachmentsDisclosure(
|
||||||
root: HTMLElement,
|
root: HTMLElement,
|
||||||
opts: AttachmentsDisclosureOpts,
|
opts: AttachmentsDisclosureOpts,
|
||||||
@@ -120,7 +129,7 @@ export function wireAttachmentsDisclosure(
|
|||||||
|
|
||||||
// Cap enforcement (popup-side, before sending to SW).
|
// Cap enforcement (popup-side, before sending to SW).
|
||||||
const settingsResp = await sendMessage({ type: 'get_vault_settings' });
|
const settingsResp = await sendMessage({ type: 'get_vault_settings' });
|
||||||
if (settingsResp.ok) {
|
if (settingsResp && settingsResp.ok) {
|
||||||
const settings = (settingsResp.data as { settings: VaultSettings }).settings;
|
const settings = (settingsResp.data as { settings: VaultSettings }).settings;
|
||||||
const caps = settings.attachment_caps;
|
const caps = settings.attachment_caps;
|
||||||
if (caps?.per_attachment_max_bytes && file.size > caps.per_attachment_max_bytes) {
|
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',
|
mimeType: file.type || 'application/octet-stream',
|
||||||
bytes,
|
bytes,
|
||||||
});
|
});
|
||||||
if (resp.ok) {
|
if (resp && resp.ok) {
|
||||||
const data = resp.data as { attachment: AttachmentRef };
|
const data = resp.data as { attachment: AttachmentRef };
|
||||||
opts.onChange?.([...opts.attachments, data.attachment]);
|
opts.onChange?.([...opts.attachments, data.attachment]);
|
||||||
} else {
|
} else {
|
||||||
alert(`upload failed: ${resp.error}`);
|
alert(`upload failed: ${resp?.error ?? 'service worker unavailable'}`);
|
||||||
}
|
}
|
||||||
fileInput.value = ''; // allow re-pick of same file later
|
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);
|
const att = opts.attachments.find((a) => a.id === attId);
|
||||||
if (!att) return;
|
if (!att) return;
|
||||||
const resp = await sendMessage({ type: 'download_attachment', itemId: opts.itemId, attachmentId: attId });
|
const resp = await sendMessage({ type: 'download_attachment', itemId: opts.itemId, attachmentId: attId });
|
||||||
if (!resp.ok) {
|
if (!resp || !resp.ok) {
|
||||||
alert(`download failed: ${resp.error}`);
|
alert(`download failed: ${resp?.error ?? 'service worker unavailable'}`);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
const data = resp.data as { bytes: ArrayBuffer; filename: string; mimeType: string };
|
const data = resp.data as { bytes: ArrayBuffer; filename: string; mimeType: string };
|
||||||
|
|||||||
Reference in New Issue
Block a user