Merge branch 'task/2422-action-descriptors' into feat/attachment-options-panel

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> = {}): 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);
+		}
+	});
+});

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 `<a download>`: 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<boolean>;
+	/** 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<boolean>;
+}
+
+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<void>;
+}
+
+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 <img>
+			// 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));
+}

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 `<a download>` 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: `<a>` ignores `disabled`, is still
+	// focusable and still navigates. Falling back to a disabled <button> keeps
+	// the semantics honest AND keeps Menu's keyboard navigation correct — it
+	// skips rows with `[role^="menuitem"]:not(:disabled)` (Menu.svelte:130),
+	// which no anchor can ever match.
+	const asAnchor = $derived(href !== undefined && !disabled);
 </script>
 
-<button
-	type="button"
-	class="mi"
-	class:danger
-	role={checked !== undefined ? 'menuitemradio' : 'menuitem'}
-	aria-checked={checked}
-	aria-describedby={describedBy}
-	{disabled}
-	{onclick}
->
-	{#if icon}<span class="mi-icon" aria-hidden="true">{icon}</span>{/if}
+{#snippet body()}
+	{#if iconSnippet}
+		<span class="mi-icon" aria-hidden="true">{@render iconSnippet()}</span>
+	{:else if icon}
+		<span class="mi-icon" aria-hidden="true">{icon}</span>
+	{/if}
 	<span class="mi-label">{@render children()}</span>
 	{#if hint}<span class="mi-hint">{hint}</span>{/if}
 	{#if checked}<span class="mi-check" aria-hidden="true">✓</span>{/if}
-</button>
+{/snippet}
+
+{#if asAnchor}
+	<a
+		class="mi"
+		class:danger
+		{href}
+		{download}
+		{target}
+		{rel}
+		{role}
+		aria-checked={checked}
+		aria-describedby={describedBy}
+		{onclick}
+	>
+		{@render body()}
+	</a>
+{:else}
+	<button
+		type="button"
+		class="mi"
+		class:danger
+		{role}
+		aria-checked={checked}
+		aria-describedby={describedBy}
+		{disabled}
+		{onclick}
+	>
+		{@render body()}
+	</button>
+{/if}
 
 <style>
 	.mi {
@@ -53,6 +109,11 @@
 		align-items: center;
 		gap: 9px;
 		width: 100%;
+		/* The anchor branch is not a button: it needs the border-box sizing
+		   and the link-color/underline reset spelled out, or a row renders
+		   wider than the panel and in the link palette. */
+		box-sizing: border-box;
+		text-decoration: none;
 		padding: 7px 9px;
 		border: none;
 		border-radius: var(--radius-sm);

web/src/lib/components/common/MenuItem.svelte.test.ts

@@ -0,0 +1,143 @@
+// Runs in the jsdom vitest project (filename ends `.svelte.test.ts`).
+//
+// MenuItem grew two capabilities for the attachment options panel
+// (PLAN-2392 DR-5 / DR-3b, TASK-2422): an icon SNIPPET alongside the existing
+// string icon, and an ANCHOR branch so a row can be a real `<a download>` or
+// an Open-in-new-tab link. Both are additive, so the first assertion here is
+// that the plain button row every existing call site uses is unchanged.
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import { render, cleanup } from '@testing-library/svelte';
+import { createRawSnippet, tick } from 'svelte';
+import MenuItem from './MenuItem.svelte';
+
+const label = createRawSnippet(() => ({ render: () => `<span>Download</span>` }));
+const svgIcon = createRawSnippet(() => ({
+	render: () => `<svg data-testid="icon-svg" viewBox="0 0 24 24"><path d="M0 0h1" /></svg>`,
+}));
+
+function row(): HTMLElement {
+	const el = document.querySelector('.mi');
+	if (!el) throw new Error('.mi not found');
+	return el as HTMLElement;
+}
+
+/** What Menu.svelte's keyboard navigation actually queries (Menu.svelte:130). */
+function menuNavigable(): HTMLElement[] {
+	return Array.from(
+		document.querySelectorAll<HTMLElement>('[role^="menuitem"]:not(:disabled)')
+	);
+}
+
+afterEach(() => {
+	cleanup();
+	vi.restoreAllMocks();
+	document.body.innerHTML = '';
+});
+
+describe('MenuItem.svelte', () => {
+	it('still renders the existing string-icon button row unchanged', async () => {
+		const onclick = vi.fn();
+		render(MenuItem, { props: { icon: '🗑', hint: '›', danger: true, onclick, children: label } });
+		await tick();
+
+		const el = row();
+		expect(el.tagName).toBe('BUTTON');
+		expect(el.getAttribute('type')).toBe('button');
+		expect(el.getAttribute('role')).toBe('menuitem');
+		expect(el.classList.contains('danger')).toBe(true);
+		expect(el.querySelector('.mi-icon')?.textContent).toBe('🗑');
+		expect(el.querySelector('.mi-hint')?.textContent).toBe('›');
+		expect(el.hasAttribute('href')).toBe(false);
+
+		(el as HTMLButtonElement).click();
+		expect(onclick).toHaveBeenCalledTimes(1);
+	});
+
+	it('keeps the menuitemradio + check row for the checked variant', async () => {
+		render(MenuItem, { props: { checked: true, describedBy: 'desc-1', children: label } });
+		await tick();
+
+		const el = row();
+		expect(el.getAttribute('role')).toBe('menuitemradio');
+		expect(el.getAttribute('aria-checked')).toBe('true');
+		expect(el.getAttribute('aria-describedby')).toBe('desc-1');
+		expect(el.querySelector('.mi-check')?.textContent).toBe('✓');
+	});
+
+	it('renders an icon snippet as markup, not as literal angle brackets', async () => {
+		render(MenuItem, { props: { iconSnippet: svgIcon, children: label } });
+		await tick();
+
+		const icon = row().querySelector('.mi-icon');
+		expect(icon?.getAttribute('aria-hidden')).toBe('true');
+		// The whole point: an <svg> ELEMENT, not the text "<svg ...>".
+		expect(icon?.querySelector('svg')).not.toBeNull();
+		expect(icon?.textContent).not.toContain('<svg');
+	});
+
+	it('prefers the icon snippet when both icon forms are supplied', async () => {
+		render(MenuItem, { props: { icon: '🗑', iconSnippet: svgIcon, children: label } });
+		await tick();
+
+		const icons = document.querySelectorAll('.mi-icon');
+		expect(icons).toHaveLength(1);
+		expect(icons[0].querySelector('svg')).not.toBeNull();
+		expect(icons[0].textContent).not.toContain('🗑');
+	});
+
+	it('renders an anchor row with href/download and keeps menuitem semantics', async () => {
+		render(MenuItem, {
+			props: {
+				href: '/api/v1/workspaces/ws/attachments/att-1',
+				download: 'report.pdf',
+				icon: '⇩',
+				children: label,
+			},
+		});
+		await tick();
+
+		const el = row();
+		expect(el.tagName).toBe('A');
+		expect(el.getAttribute('href')).toBe('/api/v1/workspaces/ws/attachments/att-1');
+		// A REAL download attribute — the server's inline disposition would
+		// otherwise open the file rather than save it (DR-16).
+		expect(el.getAttribute('download')).toBe('report.pdf');
+		expect(el.getAttribute('role')).toBe('menuitem');
+		expect(el.classList.contains('mi')).toBe(true);
+		// Reachable by Menu's arrow-key navigation exactly like a button row.
+		expect(menuNavigable()).toEqual([el]);
+	});
+
+	it('passes target/rel through for the open-in-new-tab anchor', async () => {
+		render(MenuItem, {
+			props: {
+				href: '/api/v1/workspaces/ws/attachments/att-1',
+				target: '_blank',
+				rel: 'noopener noreferrer',
+				children: label,
+			},
+		});
+		await tick();
+
+		const el = row();
+		expect(el.tagName).toBe('A');
+		expect(el.getAttribute('target')).toBe('_blank');
+		expect(el.getAttribute('rel')).toBe('noopener noreferrer');
+		expect(el.hasAttribute('download')).toBe(false);
+	});
+
+	it('renders a disabled anchor as a disabled button so keyboard nav skips it', async () => {
+		render(MenuItem, {
+			props: { href: '/api/v1/workspaces/ws/attachments/att-1', disabled: true, children: label },
+		});
+		await tick();
+
+		const el = row();
+		// `<a>` ignores `disabled`, stays focusable and still navigates — a
+		// disabled link would be a live link that only LOOKS unavailable.
+		expect(el.tagName).toBe('BUTTON');
+		expect((el as HTMLButtonElement).disabled).toBe(true);
+		expect(el.hasAttribute('href')).toBe(false);
+		expect(menuNavigable()).toEqual([]);
+	});
+});