diff --git a/web/src/lib/attachments/actions.test.ts b/web/src/lib/attachments/actions.test.ts index 51355768..013e807d 100644 --- a/web/src/lib/attachments/actions.test.ts +++ b/web/src/lib/attachments/actions.test.ts @@ -37,18 +37,15 @@ const { ATTACHMENT_ACTIONS, attachmentActionsFor, attachmentLinkUrl } = await im type Ctx = AttachmentActionContext; -// A browser-preview predicate with the DR-19 shape: PDFs and plain text yes, -// archives no. The real one is the shared display helper; the descriptors take -// it as context precisely so this test doesn't need it. -const PREVIEWABLE = new Set(['application/pdf', 'text/plain', 'image/png']); -const canPreview = (mime: string) => PREVIEWABLE.has(mime); +// The descriptors import the real `canBrowserPreview` (DR-16 keeps every +// "what can this MIME do" question in one module), so these cases assert +// against the shipping predicate rather than a stand-in. function ctx(overrides: Partial = {}): Ctx { return { workspaceSlug: 'ws', attachment: { id: 'att-1', filename: 'report.pdf', mime_type: 'application/pdf' }, mutationsEnabled: true, - canPreview, origin: 'https://pad.example', ...overrides, }; @@ -93,6 +90,20 @@ describe('attachment action descriptors', () => { expect(action('open').applies(zip)).toBe(false); }); + it('does not offer Open for an SVG, even though it is labelled image/*', () => { + // DR-16: the predicate is an exact allowlist, not an `image/` prefix, + // and the descriptors reach it directly — a caller cannot hand them a + // looser one. An SVG can carry active content, so it gets Download only. + const svg = ctx({ + attachment: { id: 'att-3', filename: 'diagram.svg', mime_type: 'image/svg+xml' }, + }); + expect(attachmentActionsFor(svg).map((a) => a.id)).toEqual([ + 'download', + 'copy-link', + 'delete', + ]); + }); + it('offers Open for a PDF, as a new-tab anchor', () => { const pdf = ctx(); expect(attachmentActionsFor(pdf).map((a) => a.id)).toEqual([ diff --git a/web/src/lib/attachments/actions.ts b/web/src/lib/attachments/actions.ts index 3e170e24..eb45ccea 100644 --- a/web/src/lib/attachments/actions.ts +++ b/web/src/lib/attachments/actions.ts @@ -24,10 +24,13 @@ * The union is discriminated on `element`, so a renderer that switches on it * gets `href` or `run` narrowed for free and cannot reach for the wrong one. * - * `canPreview` is INJECTED rather than imported (see `AttachmentActionContext`): - * the "what can this MIME do" predicate lives in the display helpers, and - * taking it as context keeps this module free of that dependency and trivially - * testable. + * "Can the browser preview this?" is answered by importing `canBrowserPreview` + * from the display helpers, NOT by taking a predicate from the caller. DR-16 + * puts every "what can this MIME do" question in one module precisely so a + * single call site cannot be given a looser answer — an injected predicate + * that admitted `image/svg+xml` would reopen the hole the exact-allowlist + * decision exists to close. (It was injected while this module and the + * predicate were built in parallel; collapsed at integration.) * * Not here, deliberately: `state_generation` and Undo. This plan's delete * behaves exactly like today's tile delete; the generation token, the event @@ -37,6 +40,7 @@ import { api } from '$lib/api/client'; import { announceAttachmentDeleted } from '$lib/attachments/events'; +import { canBrowserPreview } from '$lib/attachments/display'; import { copyToClipboard } from '$lib/utils/clipboard'; export type AttachmentActionId = 'open' | 'download' | 'copy-link' | 'delete'; @@ -60,14 +64,6 @@ export interface AttachmentActionContext { * or a pane whose mutation gate is closed. Delete is disabled without it. */ mutationsEnabled: boolean; - /** - * "Can the browser preview this MIME natively?" — the predicate that - * decides whether Open exists at all (DR-19's Open note). Injected by the - * host rather than imported here so this module owns actions and the - * display helpers own MIME capability. Hosts pass the shared - * `canBrowserPreview`. - */ - canPreview: (mime: string) => boolean; /** * Origin for the absolute copy-link URL. Defaults to `location.origin`; * present so the URL builder is testable outside a DOM. @@ -162,7 +158,7 @@ export const ATTACHMENT_ACTIONS: readonly AttachmentAction[] = [ // Only for what a browser previews natively. Never a Pad-rendered // preview, and never offered for a .zip or an office document — // those get Download only. - applies: (ctx) => ctx.canPreview(ctx.attachment.mime_type), + applies: (ctx) => canBrowserPreview(ctx.attachment.mime_type), enabled: addressable, href: (ctx) => api.attachments.downloadUrl(ctx.workspaceSlug, ctx.attachment.id), target: '_blank', diff --git a/web/src/lib/components/timeline/ItemTimeline.svelte b/web/src/lib/components/timeline/ItemTimeline.svelte index d995841c..2b7561b7 100644 --- a/web/src/lib/components/timeline/ItemTimeline.svelte +++ b/web/src/lib/components/timeline/ItemTimeline.svelte @@ -133,8 +133,13 @@ ).then((m) => { // A transient failure (5xx / network) is not evidence about the // row, and the helper deliberately doesn't cache it — so drop the - // probed mark too, or this panel would never ask again for the - // rest of the session (PLAN-2392 DR-17). A `missing` result IS + // probed mark too, or this panel could never ask again for the + // rest of the mount (PLAN-2392 DR-17). Note what this does and + // does not buy: clearing the mark makes the attachment eligible + // again on the NEXT run of the probe effect (a new comment, an + // edit, a remount), it does not schedule a retry of its own. A + // proactive retry belongs with the timeline's deletion + // subscription in phase 3c, not here. A `missing` result IS // authoritative: leave the mark set and leave `attMeta` without an // entry, which is what the renderer already degrades to a missing // placeholder on.