From 6047b407b6d761a15d9bf2e0fb6446b0e1b40c61 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 4 Aug 2026 00:46:40 +0000 Subject: [PATCH] feat(attachments): shared action descriptors and MenuItem anchor support (TASK-2422) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PLAN-2392 DR-5: the panel and the viewer share one action list only if the list IS the source of truth, so open / download / copy link / delete become descriptors in web/src/lib/attachments/actions.ts. Adding an action means adding one descriptor; both renderers consume the same set. The element is part of the contract: Download stays a real `` because the server sends an inline disposition for most accepted types (a plain navigation would view rather than save — DR-16), and Open needs new-tab / middle-click semantics. So the descriptor type is a union discriminated on `element`: anchors carry href/download/target/rel and no run() (the browser performs the action; a renderer calling both would fire it twice), buttons carry run(). Open is omitted entirely — not disabled — for types a browser cannot preview, via a `canPreview` predicate taken from the context rather than imported, keeping MIME capability with the display helpers. Copy link copies location.origin + downloadUrl(...) because downloadUrl is relative, and names itself "Copy workspace link" so the semantics are honest: it is not a share link (DR-5a). Delete behaves exactly like today's tile delete — api.attachments.delete plus announceAttachmentDeleted, with a 404 treated as authoritative — and deliberately carries no state_generation and no undo; that wiring lands in PLAN-2411 across all three entry points at once (DR-19). MenuItem gains the two capabilities the panel needs, both additive: an icon SNIPPET alongside the string icon (the string is interpolated as text, so SVG markup would render as literal angle brackets — DR-3b), and an anchor branch. A disabled anchor falls back to a disabled button: `` ignores `disabled`, stays focusable and still navigates, and Menu's keyboard navigation skips rows via `[role^="menuitem"]:not(:disabled)`, which no anchor can match. Tests cover the descriptor contract (open absent for a .zip, present for a PDF; download's filename attribute; the absolute same-origin copy URL and its clipboard-failure path; delete disabled without mutations, its 404-as-success path and its error propagation) and MenuItem's unchanged button rendering alongside the new snippet and anchor branches. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC --- web/src/lib/attachments/actions.test.ts | 238 ++++++++++++++++ web/src/lib/attachments/actions.ts | 253 ++++++++++++++++++ web/src/lib/components/common/MenuItem.svelte | 87 +++++- .../components/common/MenuItem.svelte.test.ts | 143 ++++++++++ 4 files changed, 708 insertions(+), 13 deletions(-) create mode 100644 web/src/lib/attachments/actions.test.ts create mode 100644 web/src/lib/attachments/actions.ts create mode 100644 web/src/lib/components/common/MenuItem.svelte.test.ts diff --git a/web/src/lib/attachments/actions.test.ts b/web/src/lib/attachments/actions.test.ts new file mode 100644 index 00000000..51355768 --- /dev/null +++ b/web/src/lib/attachments/actions.test.ts @@ -0,0 +1,238 @@ +// Attachment action descriptors (PLAN-2392 DR-5 / DR-5a / DR-19, TASK-2422). +// +// The point of the descriptor list is that the panel and the viewer cannot +// drift, so the assertions here are about the CONTRACT each descriptor +// publishes — which element it renders as, whether it exists at all for a +// given MIME, and what its href / download / run actually do — rather than +// about either renderer. +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const deleteMock = vi.fn(); +const downloadUrlMock = vi.fn( + (ws: string, id: string) => `/api/v1/workspaces/${ws}/attachments/${id}` +); + +vi.mock('$lib/api/client', () => ({ + api: { + attachments: { + delete: (...args: unknown[]) => deleteMock(...args), + downloadUrl: (ws: string, id: string) => downloadUrlMock(ws, id), + }, + }, +})); + +const announceMock = vi.fn(); +vi.mock('$lib/attachments/events', () => ({ + announceAttachmentDeleted: (...args: unknown[]) => announceMock(...args), +})); + +const copyToClipboardMock = vi.fn(async (_text: string) => true); +vi.mock('$lib/utils/clipboard', () => ({ + copyToClipboard: (text: string) => copyToClipboardMock(text), +})); + +import type { AttachmentAction, AttachmentActionContext } from './actions'; + +const { ATTACHMENT_ACTIONS, attachmentActionsFor, attachmentLinkUrl } = await import('./actions'); + +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); + +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, + }; +} + +function action(id: string): AttachmentAction { + const found = ATTACHMENT_ACTIONS.find((a) => a.id === id); + if (!found) throw new Error(`no descriptor for ${id}`); + return found; +} + +beforeEach(() => { + deleteMock.mockReset(); + deleteMock.mockResolvedValue(undefined); + announceMock.mockReset(); + copyToClipboardMock.mockReset(); + copyToClipboardMock.mockResolvedValue(true); + downloadUrlMock.mockClear(); +}); + +describe('attachment action descriptors', () => { + it('exposes exactly the four in-scope actions, in render order', () => { + expect(ATTACHMENT_ACTIONS.map((a) => a.id)).toEqual([ + 'open', + 'download', + 'copy-link', + 'delete', + ]); + }); + + it('omits Open entirely for a type the browser cannot preview', () => { + const zip = ctx({ + attachment: { id: 'att-2', filename: 'bundle.zip', mime_type: 'application/zip' }, + }); + expect(attachmentActionsFor(zip).map((a) => a.id)).toEqual([ + 'download', + 'copy-link', + 'delete', + ]); + // Not "present but disabled" — a greyed Open would imply a preview + // Pad could give and won't. + expect(action('open').applies(zip)).toBe(false); + }); + + it('offers Open for a PDF, as a new-tab anchor', () => { + const pdf = ctx(); + expect(attachmentActionsFor(pdf).map((a) => a.id)).toEqual([ + 'open', + 'download', + 'copy-link', + 'delete', + ]); + const open = action('open'); + expect(open.element).toBe('anchor'); + if (open.element !== 'anchor') throw new Error('unreachable'); + expect(open.href(pdf)).toBe('/api/v1/workspaces/ws/attachments/att-1'); + expect(open.target).toBe('_blank'); + expect(open.rel).toBe('noopener noreferrer'); + // An anchor performs its own navigation; a `run` here would double-fire. + expect('run' in open).toBe(false); + }); + + it('renders Download as an anchor carrying a real download filename (DR-16)', () => { + const c = ctx(); + const download = action('download'); + expect(download.element).toBe('anchor'); + if (download.element !== 'anchor') throw new Error('unreachable'); + expect(download.applies(c)).toBe(true); + expect(download.href(c)).toBe('/api/v1/workspaces/ws/attachments/att-1'); + expect(download.download?.(c)).toBe('report.pdf'); + }); + + it('copies an absolute same-origin URL and labels itself as workspace-scoped (DR-5a)', async () => { + const c = ctx(); + const copy = action('copy-link'); + expect(copy.element).toBe('button'); + if (copy.element !== 'button') throw new Error('unreachable'); + + const url = attachmentLinkUrl(c); + expect(url).toBe('https://pad.example/api/v1/workspaces/ws/attachments/att-1'); + // Absolute, so it survives being pasted somewhere else. + expect(new URL(url).origin).toBe('https://pad.example'); + + await copy.run(c); + expect(copyToClipboardMock).toHaveBeenCalledWith(url); + + // The user-visible text says what the link actually is — not a share link. + expect(`${copy.label} ${copy.description ?? ''}`.toLowerCase()).toContain('workspace'); + }); + + it('falls back to location.origin when the context does not supply one', () => { + const original = (globalThis as { location?: unknown }).location; + Object.defineProperty(globalThis, 'location', { + value: { origin: 'https://runtime.example' }, + configurable: true, + writable: true, + }); + try { + const c = ctx(); + delete (c as { origin?: string }).origin; + expect(attachmentLinkUrl(c)).toBe( + 'https://runtime.example/api/v1/workspaces/ws/attachments/att-1' + ); + } finally { + if (original === undefined) delete (globalThis as { location?: unknown }).location; + else + Object.defineProperty(globalThis, 'location', { + value: original, + configurable: true, + writable: true, + }); + } + }); + + it('surfaces a clipboard failure rather than reporting a copy that did not happen', async () => { + copyToClipboardMock.mockResolvedValue(false); + const onCopied = vi.fn(); + const copy = action('copy-link'); + if (copy.element !== 'button') throw new Error('unreachable'); + await expect(copy.run(ctx({ onCopied }))).rejects.toThrow(/clipboard/i); + expect(onCopied).not.toHaveBeenCalled(); + }); + + it('disables Delete when mutations are off, and refuses to run anyway', async () => { + const readOnly = ctx({ mutationsEnabled: false }); + const del = action('delete'); + if (del.element !== 'button') throw new Error('unreachable'); + + expect(del.applies(readOnly)).toBe(true); + expect(del.enabled(readOnly)).toBe(false); + expect(del.enabled(ctx())).toBe(true); + expect(del.danger).toBe(true); + + await del.run(readOnly); + expect(deleteMock).not.toHaveBeenCalled(); + expect(announceMock).not.toHaveBeenCalled(); + }); + + it('deletes exactly like the tile does: api.delete then announceAttachmentDeleted', async () => { + const onDeleted = vi.fn(); + const del = action('delete'); + if (del.element !== 'button') throw new Error('unreachable'); + + await del.run(ctx({ onDeleted })); + + expect(deleteMock).toHaveBeenCalledWith('ws', 'att-1'); + expect(announceMock).toHaveBeenCalledWith('ws', 'att-1'); + expect(onDeleted).toHaveBeenCalledWith('att-1'); + // No state_generation / undo token in this plan (DR-19). + expect(announceMock.mock.calls[0]).toHaveLength(2); + }); + + it('aborts the delete before any request when the host confirmation says no', async () => { + const del = action('delete'); + if (del.element !== 'button') throw new Error('unreachable'); + await del.run(ctx({ confirmDelete: () => false })); + expect(deleteMock).not.toHaveBeenCalled(); + expect(announceMock).not.toHaveBeenCalled(); + }); + + it('treats a 404 as authoritative: broadcasts and does not throw', async () => { + deleteMock.mockRejectedValue(Object.assign(new Error('gone'), { code: 'not_found' })); + const onDeleted = vi.fn(); + const del = action('delete'); + if (del.element !== 'button') throw new Error('unreachable'); + + await expect(del.run(ctx({ onDeleted }))).resolves.toBeUndefined(); + expect(announceMock).toHaveBeenCalledWith('ws', 'att-1'); + expect(onDeleted).toHaveBeenCalledWith('att-1'); + }); + + it('propagates a real delete failure without announcing a deletion', async () => { + deleteMock.mockRejectedValue(Object.assign(new Error('boom'), { code: 'internal' })); + const del = action('delete'); + if (del.element !== 'button') throw new Error('unreachable'); + + await expect(del.run(ctx())).rejects.toThrow('boom'); + expect(announceMock).not.toHaveBeenCalled(); + }); + + it('disables the addressable actions when the workspace or id is missing', () => { + const unaddressable = ctx({ workspaceSlug: '' }); + for (const id of ['open', 'download', 'copy-link', 'delete']) { + expect(action(id).enabled(unaddressable)).toBe(false); + } + }); +}); diff --git a/web/src/lib/attachments/actions.ts b/web/src/lib/attachments/actions.ts new file mode 100644 index 00000000..3e170e24 --- /dev/null +++ b/web/src/lib/attachments/actions.ts @@ -0,0 +1,253 @@ +/** + * Attachment actions — defined once, rendered twice (PLAN-2392 DR-5). + * + * "The panel and the viewer share one action list" is a promise with no source + * of truth unless the list IS the source of truth. So the actions live here as + * descriptors: the options panel draws them as a menu/sheet, the image viewer + * draws them as an inline toolbar, and neither owns the set. Adding an action + * means adding one descriptor here. + * + * TWO DESCRIPTOR SHAPES, not one, because the ELEMENT is part of the contract + * (DR-5, round 35/36): + * + * - `element: 'anchor'` — Download must remain a real ``: the + * server sends `Content-Disposition: inline` for most accepted types + * (`handlers_attachments.go:742`), so a plain navigation would *view* the + * file rather than save it (DR-16). Open needs anchor semantics too — new + * tab, middle-click, "copy link address". An anchor descriptor supplies + * `href(ctx)` and optionally `download(ctx)` / `target` / `rel`; the + * browser performs the action, so there is deliberately no `run()` to call. + * (Renderers that called both would fire the action twice.) + * - `element: 'button'` — Copy link and Delete do work in JS, so they carry + * `run(ctx)` and no `href`. + * + * 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. + * + * Not here, deliberately: `state_generation` and Undo. This plan's delete + * behaves exactly like today's tile delete; the generation token, the event + * payload change and the Undo toast arrive together in PLAN-2411 across all + * three entry points at once (DR-19). + */ + +import { api } from '$lib/api/client'; +import { announceAttachmentDeleted } from '$lib/attachments/events'; +import { copyToClipboard } from '$lib/utils/clipboard'; + +export type AttachmentActionId = 'open' | 'download' | 'copy-link' | 'delete'; + +/** + * The attachment an action acts on. Deliberately the three fields every + * surface already has (strip tile, panel, viewer) rather than a full + * `Attachment` row — the viewer's image shape is not the list row's. + */ +export interface AttachmentActionSubject { + id: string; + filename: string; + mime_type: string; +} + +export interface AttachmentActionContext { + workspaceSlug: string; + attachment: AttachmentActionSubject; + /** + * Whether the caller may mutate — a read-only share view, a viewer role, + * 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. + */ + origin?: string; + /** + * Confirmation gate for Delete. The surface owns the wording and the + * modality (the strip uses `window.confirm`), so a descriptor never + * invents one — but when supplied, returning false aborts before any + * request is sent. + */ + confirmDelete?: (attachment: AttachmentActionSubject) => boolean | Promise; + /** Called after the server confirms the row is gone (204 or 404). */ + onDeleted?: (attachmentId: string) => void; + /** Called with the copied URL after a successful clipboard write. */ + onCopied?: (url: string) => void; + /** Clipboard override — the LAN/HTTP-safe `copyToClipboard` by default. */ + copyText?: (text: string) => Promise; +} + +interface BaseAttachmentAction { + id: AttachmentActionId; + /** Row/button label. Visible text, so it carries the honest semantics. */ + label: string; + /** Leading glyph, in `MenuItem`'s string-icon vocabulary. */ + icon: string; + /** Longer explanation for a tooltip / sublabel. */ + description?: string; + /** + * Whether the action EXISTS for this attachment. Open is omitted entirely + * for types a browser cannot preview — never shown disabled, because a + * greyed "Open" implies a preview Pad could give and won't (DR-5). + */ + applies(ctx: AttachmentActionContext): boolean; + /** Whether the action is currently actionable. */ + enabled(ctx: AttachmentActionContext): boolean; +} + +export interface AnchorAttachmentAction extends BaseAttachmentAction { + element: 'anchor'; + href(ctx: AttachmentActionContext): string; + /** `download` attribute value — the filename, or undefined for none. */ + download?(ctx: AttachmentActionContext): string | undefined; + target?: string; + rel?: string; +} + +export interface ButtonAttachmentAction extends BaseAttachmentAction { + element: 'button'; + /** Destructive styling (red row). */ + danger?: boolean; + run(ctx: AttachmentActionContext): Promise; +} + +export type AttachmentAction = AnchorAttachmentAction | ButtonAttachmentAction; + +/** + * The absolute, same-origin URL for an attachment (DR-5a). + * + * `api.attachments.downloadUrl` returns a RELATIVE `/api/v1/...` path + * (`api/client.ts:2128`), so copying it verbatim yields something that does + * not work anywhere it gets pasted. It is still not a share link: the endpoint + * requires the recipient's own authenticated workspace access, and a recipient + * without it gets the normal auth redirect. + */ +export function attachmentLinkUrl(ctx: AttachmentActionContext): string { + const path = api.attachments.downloadUrl(ctx.workspaceSlug, ctx.attachment.id); + const origin = ctx.origin ?? (typeof location !== 'undefined' ? location.origin : ''); + return `${origin}${path}`; +} + +/** Both anchors need a workspace and an id before they can point anywhere. */ +function addressable(ctx: AttachmentActionContext): boolean { + return Boolean(ctx.workspaceSlug && ctx.attachment?.id); +} + +function errorCode(err: unknown): string | null { + if (err && typeof err === 'object' && 'code' in err) { + const code = (err as { code?: unknown }).code; + return typeof code === 'string' ? code : null; + } + return null; +} + +export const ATTACHMENT_ACTIONS: readonly AttachmentAction[] = [ + { + id: 'open', + label: 'Open in new tab', + icon: '⇗', + description: 'Hands the file to the browser to preview.', + element: 'anchor', + // 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), + enabled: addressable, + href: (ctx) => api.attachments.downloadUrl(ctx.workspaceSlug, ctx.attachment.id), + target: '_blank', + rel: 'noopener noreferrer', + } satisfies AnchorAttachmentAction, + { + id: 'download', + label: 'Download', + icon: '⇩', + element: 'anchor', + applies: () => true, + enabled: addressable, + href: (ctx) => api.attachments.downloadUrl(ctx.workspaceSlug, ctx.attachment.id), + // A REAL download attribute, not decoration: without it the inline + // disposition the server sends for most types would open the file + // instead of saving it (DR-16). + download: (ctx) => ctx.attachment.filename || undefined, + } satisfies AnchorAttachmentAction, + { + id: 'copy-link', + // Honest by name (DR-5a): this is a link for people who already have + // access to this workspace, not a public share link. Pad has no + // attachment share token. + label: 'Copy workspace link', + icon: '🔗', + description: 'Opens only for people with access to this workspace.', + element: 'button', + applies: () => true, + enabled: addressable, + async run(ctx) { + const url = attachmentLinkUrl(ctx); + const copy = ctx.copyText ?? copyToClipboard; + const ok = await copy(url); + if (!ok) throw new Error('Could not copy the link to the clipboard'); + ctx.onCopied?.(url); + }, + } satisfies ButtonAttachmentAction, + { + id: 'delete', + label: 'Delete', + icon: '🗑', + element: 'button', + danger: true, + applies: () => true, + enabled: (ctx) => ctx.mutationsEnabled && addressable(ctx), + async run(ctx) { + // Belt and braces: a renderer that draws a disabled row can still + // be asked to run it by a stray keyboard activation. + if (!ctx.mutationsEnabled || !addressable(ctx)) return; + if (ctx.confirmDelete && !(await ctx.confirmDelete(ctx.attachment))) return; + + // Capture identity before the await: the surface may switch views + // mid-flight, and the broadcast + metadata-cache key must name the + // workspace the DELETE actually targeted. + const ws = ctx.workspaceSlug; + const id = ctx.attachment.id; + try { + await api.attachments.delete(ws, id); + } catch (err) { + // A 404 is just as authoritative as a 204 about the row being + // gone (another tab, another user), so it gets the same + // reconciliation rather than being surfaced as a failure — + // exactly what the strip's tile delete does today. + if (errorCode(err) === 'not_found') { + announceAttachmentDeleted(ws, id); + ctx.onDeleted?.(id); + return; + } + throw err; + } + // Tell the live views and drop the cached HEAD metadata. An + // that already painted never re-requests, so without this the body + // keeps showing an image the server no longer has. + announceAttachmentDeleted(ws, id); + ctx.onDeleted?.(id); + }, + } satisfies ButtonAttachmentAction, +]; + +/** + * The actions that exist for this attachment, in render order. Actions that + * don't apply are absent, not disabled; actions that apply but aren't + * currently actionable come back with `enabled(ctx) === false` so the renderer + * can grey them. + */ +export function attachmentActionsFor(ctx: AttachmentActionContext): AttachmentAction[] { + return ATTACHMENT_ACTIONS.filter((action) => action.applies(ctx)); +} diff --git a/web/src/lib/components/common/MenuItem.svelte b/web/src/lib/components/common/MenuItem.svelte index eae41daa..a794db6b 100644 --- a/web/src/lib/components/common/MenuItem.svelte +++ b/web/src/lib/components/common/MenuItem.svelte @@ -2,8 +2,13 @@ import type { Snippet } from 'svelte'; interface Props { - /** Leading icon/emoji. */ + /** Leading icon/emoji. Interpolated as TEXT — markup would render + * literally; pass `iconSnippet` for an SVG icon instead. */ icon?: string; + /** Leading icon as markup (an SVG icon component). Wins over `icon` + * when both are set; the slot is decorative either way (PLAN-2392 + * DR-3b). */ + iconSnippet?: Snippet; /** Right-aligned hint (shortcut, count). */ hint?: string; /** Red row for destructive actions. */ @@ -15,37 +20,88 @@ * destructive confirmation, which is presentational and otherwise * never announced when the row takes focus (PLAN-2326). */ describedBy?: string; + /** Renders the row as an anchor instead of a button (PLAN-2392 DR-5): + * Download must be a real `` and Open needs new-tab / + * middle-click semantics. Ignored while `disabled` — see below. */ + href?: string; + /** `download` attribute for the anchor branch — the filename to save + * as. Only meaningful with `href`. */ + download?: string; + /** Anchor `target` (Open sets `_blank`). Only meaningful with `href`. */ + target?: string; + /** Anchor `rel` (pair `noopener noreferrer` with `target="_blank"`). + * Only meaningful with `href`. */ + rel?: string; onclick?: (e: MouseEvent) => void; children: Snippet; } let { icon, + iconSnippet, hint, danger = false, checked, disabled = false, describedBy, + href, + download, + target, + rel, onclick, children }: Props = $props(); + + const role = $derived(checked !== undefined ? 'menuitemradio' : 'menuitem'); + + // A disabled anchor is not a thing: `` ignores `disabled`, is still + // focusable and still navigates. Falling back to a disabled +{/snippet} + +{#if asAnchor} + + {@render body()} + +{:else} + +{/if}