From 7bb9c4acb144eda96eef4a4e9a8d83abb91e7c09 Mon Sep 17 00:00:00 2001
From: xarmian <xarmian@gmail.com>
Date: Tue, 4 Aug 2026 14:52:48 +0000
Subject: [PATCH] fix(attachments): close the final-review findings across the
 feature
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

From the orchestrator's full-diff pass over main...HEAD — the altitude
per-task reviews structurally cannot reach.

- Opening the panel on an ALREADY-archived parent left Open, Download
  and Copy link enabled against endpoints that 404: the host only
  handled the archive TRANSITION, and the strip's event carries complete
  metadata, which is exactly what lets the panel skip its probe. The
  panel is now told the parent is archived and probes anyway, landing in
  the authoritative missing state it already knows how to render. It
  probes through the INVALIDATING path, because reachability is an
  existence question and the cache can hold an `ok` observed before the
  archive — the same lesson as the image placeholder earlier on this
  branch.
- A deleted chip stayed inert after its node was repointed at a
  different attachment: the uuid-swap path cleared `deleted` and the CSS
  but not `disabled`, giving a chip that announces itself as live and
  does nothing. Reachable through a collaborative peer's edit.
- actions.ts claimed to be "rendered twice". It has one consumer today;
  the viewer is phase 3a. Says so now, including that the image
  NodeView's threaded address is held open for the same phase — a list
  with a single consumer is worth re-justifying if 3a stops coming.
- The shared confirmation claimed to own the prompt wording while
  StorageTab built its own inline. Both builders now live in that module
  side by side: an item surface can check the body it has and must hedge
  about the ones it cannot, while a workspace-wide list has nothing to
  check and should say what happens to the blob instead.
- Descriptor `description` was never rendered; it is the row's tooltip
  now rather than a dead field.

Both behavioural fixes are mutation-tested.
---
 web/src/lib/attachments/actions.ts            | 16 ++++++--
 .../AttachmentDeleteConfirm.svelte            | 25 +++++++++--
 .../attachments/AttachmentDetailsPanel.svelte | 25 ++++++++++-
 .../attachments/AttachmentPanelHost.svelte    | 18 ++++++++
 .../AttachmentPanelHost.svelte.test.ts        | 37 +++++++++++++----
 web/src/lib/components/common/MenuItem.svelte |  5 +++
 .../lib/components/editor/attachment-chip.ts  |  9 +++-
 .../editor/attachmentChipPanel.svelte.test.ts | 41 +++++++++++++++++++
 .../lib/components/settings/StorageTab.svelte |  6 ++-
 9 files changed, 162 insertions(+), 20 deletions(-)

diff --git a/web/src/lib/attachments/actions.ts b/web/src/lib/attachments/actions.ts
index 3b7d7601..3ea8e4ca 100644
--- a/web/src/lib/attachments/actions.ts
+++ b/web/src/lib/attachments/actions.ts
@@ -1,11 +1,19 @@
 /**
- * Attachment actions — defined once, rendered twice (PLAN-2392 DR-5).
+ * Attachment actions — defined once, rendered by whoever needs them
+ * (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.
+ * descriptors: a surface renders them, none of them owns the set, and adding an
+ * action means adding one descriptor here.
+ *
+ * TODAY THERE IS ONE CONSUMER: the options panel, which draws them as a
+ * menu/sheet. The unified image viewer — the second renderer, an inline
+ * toolbar over the same list — arrives in phase 3a, which is also what will
+ * consume the `address` option now threaded onto the image NodeView. Stated
+ * plainly because "rendered twice" read as a description of the present and
+ * was not one; a list with a single consumer is a shape held open on purpose,
+ * and worth re-justifying if 3a ever stops coming.
  *
  * TWO DESCRIPTOR SHAPES, not one, because the ELEMENT is part of the contract
  * (DR-5, round 35/36):
diff --git a/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte b/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte
index 9dcae214..21034090 100644
--- a/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte
+++ b/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte
@@ -28,13 +28,18 @@
 	the app's existing behaviours on both surfaces rather than a second
 	implementation.
 
-	The PROMPT TEXT is `attachmentDeletePrompt` below, shared for the same
-	reason the markup is: the hedged arm's honesty is the substance of DR-5, and
-	two copies of it drift.
+	The PROMPT TEXT lives in this module too, but note there are TWO builders,
+	not one: `attachmentDeletePrompt` for an item surface, which can check the
+	body it has and must hedge about the ones it cannot, and
+	`workspaceAttachmentDeletePrompt` for the workspace-wide storage list, where
+	a reference check would be meaningless and the honest thing to say is what
+	happens to the blob. Different questions, so different copy — deliberately.
+	What must not drift is that BOTH are written here, next to each other, where
+	a change to one is read alongside the other.
 -->
 <script lang="ts" module>
 	/**
-	 * The two arms of the delete warning — the only place either is written.
+	 * The two arms of the ITEM-surface delete warning.
 	 *
 	 * `referencedHere` can only ever speak for the body the caller has. So the
 	 * "not referenced" arm deliberately does NOT claim the attachment is
@@ -51,6 +56,18 @@
 			? `Delete ${displayName}? It's still used in this item's content — deleting it will leave a "missing attachment" placeholder where it appears.`
 			: `Delete ${displayName}? It isn't referenced in this item's content, but it may still be referenced by another item or a comment. This cannot be undone.`;
 	}
+
+	/**
+	 * The storage-list prompt (Settings → Storage).
+	 *
+	 * No reference arm, and that is not an omission: this list is workspace-
+	 * wide and includes attachments with no parent item at all, so "still used
+	 * in this item's content" has nothing to be true or false about. What IS
+	 * worth saying here is what actually happens to the bytes.
+	 */
+	export function workspaceAttachmentDeletePrompt(filename: string): string {
+		return `Delete ${filename}? The blob is reclaimed by garbage collection after a grace period.`;
+	}
 </script>
 
 <script lang="ts">
diff --git a/web/src/lib/components/attachments/AttachmentDetailsPanel.svelte b/web/src/lib/components/attachments/AttachmentDetailsPanel.svelte
index a92f79bd..53d895b8 100644
--- a/web/src/lib/components/attachments/AttachmentDetailsPanel.svelte
+++ b/web/src/lib/components/attachments/AttachmentDetailsPanel.svelte
@@ -121,6 +121,15 @@
 		 * (DR-14).
 		 */
 		revalidateToken?: number;
+		/**
+		 * The parent item is archived, so attachment reads will 404 — the server
+		 * refuses them for an archived parent. The seed metadata the event
+		 * carried is therefore not trustworthy as evidence the file is
+		 * REACHABLE, and the panel must probe even when it looks complete, so it
+		 * lands in the authoritative missing state rather than offering a
+		 * Download that fails on click (DR-14).
+		 */
+		parentArchived?: boolean;
 		onclose: () => void;
 		onDeleted?: (attachmentId: string) => void;
 	}
@@ -137,6 +146,7 @@
 		itemContent = null,
 		liveContent = null,
 		revalidateToken = 0,
+		parentArchived = false,
 		onclose,
 		onDeleted,
 	}: Props = $props();
@@ -282,6 +292,7 @@
 		const seedMime = mimeType;
 		const seedSize = sizeBytes;
 		const reloadStamp = `${revalidateToken}:${forceReload}`;
+		const archivedParent = parentArchived;
 
 		let forced = false;
 		untrack(() => {
@@ -304,6 +315,12 @@
 			// An un-addressable token records nothing, which correctly stops the
 			// panel's controls claiming the previous subject.
 			paint.record(req);
+			// An archived parent makes this a REACHABILITY question, and the
+			// metadata cache can hold an `ok` observed before the archive — the
+			// same reason existence probes elsewhere revalidate rather than
+			// read. So force it, which also routes through the invalidate-then-
+			// fetch path instead of replaying a stale success.
+			if (archivedParent) forced = true;
 			if (reloadStamp !== seenReload) {
 				seenReload = reloadStamp;
 				forced = true;
@@ -320,7 +337,11 @@
 
 		if (!isOpen || req.key === null) return;
 		// Nothing to complete: the strip's entry point always has all three.
-		if (!forced && seedMime && seedSize !== null && seedSize !== undefined) return;
+		// Unless the parent is archived — then "complete" and "reachable" are
+		// different claims, and only a probe settles the second one.
+		if (!forced && !archivedParent && seedMime && seedSize !== null && seedSize !== undefined) {
+			return;
+		}
 
 		loading = true;
 		loadFailed = false;
@@ -545,6 +566,7 @@
 					download={action.download?.(ctx)}
 					target={action.target}
 					rel={action.rel}
+					title={action.description}
 					disabled={!action.enabled(ctx) || missing}
 					onclick={closeAfterNavigation}
 				>
@@ -554,6 +576,7 @@
 				<MenuItem
 					icon={action.icon}
 					danger={action.danger}
+					title={action.description}
 					disabled={!action.enabled(ctx) || missing || busy}
 					onclick={() => runAction(action)}
 				>
diff --git a/web/src/lib/components/attachments/AttachmentPanelHost.svelte b/web/src/lib/components/attachments/AttachmentPanelHost.svelte
index 4f47228e..02e699f3 100644
--- a/web/src/lib/components/attachments/AttachmentPanelHost.svelte
+++ b/web/src/lib/components/attachments/AttachmentPanelHost.svelte
@@ -110,6 +110,23 @@
 	$effect(() => {
 		return registerAttachmentPanelListener((event) => {
 			if (!isAttachmentPanelEventForHost(event, { itemId, hostToken })) return;
+			// Opening on an ALREADY-archived parent, not just archiving with the
+			// panel open (orchestrator's full-diff review). The transition
+			// handler below cannot see this case, and the strip's event carries
+			// complete metadata — which is exactly what lets the panel skip its
+			// probe — so Open, Download and Copy link would render enabled and
+			// point at endpoints that 404, because attachment reads refuse an
+			// archived parent (handlers_attachments.go).
+			//
+			// So the panel is TOLD the seeds can't be trusted (see its
+			// `parentArchived` prop) and probes anyway: the 404 comes back as the
+			// authoritative `missing` state it already knows how to show, and the
+			// actions go inert through the path that exists for it. One state
+			// machine, not a second archived-specific one.
+			//
+			// Deliberately a prop rather than bumping `revalidateToken` here: the
+			// panel is created by this same assignment, so it would seed its
+			// reload stamp from the ALREADY-incremented value and see no change.
 			request = event;
 		});
 	});
@@ -159,6 +176,7 @@
 		{itemContent}
 		{liveContent}
 		{revalidateToken}
+		parentArchived={parentArchived === true}
 		onclose={closeRequest(request)}
 	/>
 {/if}
diff --git a/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts b/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts
index 574e0784..ae1dc0e5 100644
--- a/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts
+++ b/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts
@@ -448,21 +448,22 @@ describe('AttachmentPanelHost', () => {
 	});
 
 	it('revalidates an open panel when the parent item is restored (DR-14)', async () => {
-		// Opened while the parent was already archived: the fetch 404s, so the
-		// panel latches the authoritative missing state.
+		// Opened while the parent was already archived: reads 404, so the panel
+		// latches the authoritative missing state. It goes through the
+		// INVALIDATING path — reachability is an existence question and the
+		// cache can hold an `ok` from before the archive.
 		propsA.parentArchived = true;
-		fetchMetaMock.mockResolvedValue({ status: 'missing' });
+		revalidateMetaMock.mockResolvedValue({ status: 'missing' });
 		mountHost(propsA);
 		notifyAttachmentPanelOpen(openEvent({ mime_type: null, size_bytes: null }));
 		await settle();
 		expect(panel()?.textContent).toContain('This file is no longer available.');
 
 		// Restore does NOT assume the previous state still holds — it re-reads
-		// through the invalidating path, and the panel comes back to life.
+		// through the same path, and the panel comes back to life.
 		revalidateMetaMock.mockResolvedValue({ status: 'ok', mime: 'application/pdf', size: 1536 });
 		propsA.parentArchived = false;
 		await settle();
-		expect(revalidateMetaMock).toHaveBeenCalledTimes(1);
 		expect(panel()?.textContent).not.toContain('This file is no longer available.');
 		expect((row('Download') as HTMLElement).tagName).toBe('A');
 	});
@@ -558,6 +559,28 @@ describe('AttachmentPanelHost', () => {
 		expect(announceMock).not.toHaveBeenCalled();
 	});
 
+	it('probes even when the event carries full metadata, if the parent is archived', async () => {
+		// The strip's event carries all three fields, which normally lets the
+		// panel skip the fetch. On an archived parent that would leave Open,
+		// Download and Copy link enabled against endpoints that 404 — attachment
+		// reads refuse an archived parent — so opening here must probe anyway
+		// and land in the authoritative missing state.
+		propsA.parentArchived = true;
+		mountHost(propsA);
+		revalidateMetaMock.mockResolvedValue({ status: 'missing' });
+
+		notifyAttachmentPanelOpen(openEvent());
+		await settle();
+
+		expect(revalidateMetaMock).toHaveBeenCalled();
+		expect(panel()?.textContent).toContain('This file is no longer available.');
+		// Inert, per the established missing-state contract: a disabled anchor
+		// renders as a disabled button, so it cannot be activated or navigated.
+		const download = row('Download') as HTMLButtonElement;
+		expect(download.tagName).toBe('BUTTON');
+		expect(download.disabled).toBe(true);
+	});
+
 	it('shows a retryable error when the restore revalidation fails, instead of a dead end', async () => {
 		// The reachable shape of DR-14's restore path: archiving CLOSES an open
 		// panel, so a forced revalidation only ever lands on a panel opened
@@ -565,9 +588,7 @@ describe('AttachmentPanelHost', () => {
 		// legitimately 404s.
 		propsA.parentArchived = true;
 		mountHost(propsA);
-		fetchMetaMock.mockResolvedValue({ status: 'missing' });
-		// Metadata gaps, so the panel actually probes — a chip's event, not the
-		// strip's (which carries all three and skips the fetch).
+		revalidateMetaMock.mockResolvedValue({ status: 'missing' });
 		const partial = { mime_type: null, size_bytes: null };
 		notifyAttachmentPanelOpen(openEvent(partial));
 		await settle();
diff --git a/web/src/lib/components/common/MenuItem.svelte b/web/src/lib/components/common/MenuItem.svelte
index f3979223..080704fb 100644
--- a/web/src/lib/components/common/MenuItem.svelte
+++ b/web/src/lib/components/common/MenuItem.svelte
@@ -20,6 +20,8 @@
 		 *  destructive confirmation, which is presentational and otherwise
 		 *  never announced when the row takes focus (PLAN-2326). */
 		describedBy?: string;
+		/** Native tooltip. Explanation, never the only place a fact is stated. */
+		title?: 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. */
@@ -44,6 +46,7 @@
 		checked,
 		disabled = false,
 		describedBy,
+		title,
 		href,
 		download,
 		target,
@@ -96,6 +99,7 @@
 		{target}
 		{rel}
 		{role}
+		{title}
 		aria-checked={checked}
 		aria-describedby={describedBy}
 		{onclick}
@@ -109,6 +113,7 @@
 		class="mi"
 		class:danger
 		{role}
+		{title}
 		aria-checked={checked}
 		aria-describedby={describedBy}
 		{disabled}
diff --git a/web/src/lib/components/editor/attachment-chip.ts b/web/src/lib/components/editor/attachment-chip.ts
index ba0aa503..8d1ea458 100644
--- a/web/src/lib/components/editor/attachment-chip.ts
+++ b/web/src/lib/components/editor/attachment-chip.ts
@@ -508,8 +508,15 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
 
 					if (newUuid !== currentUuid) {
 						currentUuid = newUuid;
-						// New target ⇒ the old deletion no longer applies.
+						// New target ⇒ the old deletion no longer applies. Undoing
+						// EVERY part of markDeleted() matters: `disabled` is what
+						// makes a dead chip inert, so leaving it set here would
+						// give a live chip that announces itself as live and does
+						// nothing — worse than the dead one, which at least says
+						// so. Reachable via a peer's uuid swap or a ProseMirror
+						// node replacement (orchestrator's full-diff review).
 						deleted = false;
+						wrapper.disabled = false;
 						wrapper.classList.remove('attachment-missing');
 						wrapper.removeAttribute('title');
 						// New uuid ⇒ stale MIME / size; reset until HEAD probe
diff --git a/web/src/lib/components/editor/attachmentChipPanel.svelte.test.ts b/web/src/lib/components/editor/attachmentChipPanel.svelte.test.ts
index 2e6f28ba..61a77cdb 100644
--- a/web/src/lib/components/editor/attachmentChipPanel.svelte.test.ts
+++ b/web/src/lib/components/editor/attachmentChipPanel.svelte.test.ts
@@ -68,6 +68,27 @@ describe('editor chip → options panel', () => {
 	// panel rather than navigating, and an anchor left the URL reachable by
 	// middle-click straight past the panel (orchestrator review of TASK-2424).
 	// `renderHTML` — the clipboard / read-only shape — is still an <a download>.
+	/**
+	 * Repoint the existing chip node at a different attachment via a
+	 * transaction, which drives the NodeView's `update()` hook — the path a
+	 * collaborative peer's edit takes. Replacing the content instead would
+	 * destroy and rebuild the NodeView and prove nothing about `update()`.
+	 */
+	function repointChip(uuid: string, filename: string) {
+		const ed = (editor ??= makeEditor(target));
+		let pos = -1;
+		ed.state.doc.descendants((node, at) => {
+			if (node.type.name === 'attachmentChip') {
+				pos = at;
+				return false;
+			}
+			return true;
+		});
+		if (pos < 0) throw new Error('no chip node in the document');
+		const tr = ed.state.tr.setNodeMarkup(pos, undefined, { uuid, filename });
+		ed.view.dispatch(tr);
+	}
+
 	function chip(): HTMLButtonElement {
 		editor ??= makeEditor(target);
 		const el = target.querySelector<HTMLButtonElement>('button.file-chip');
@@ -152,6 +173,26 @@ describe('editor chip → options panel', () => {
 		expect(chip().getAttribute('aria-label')).toBe('Options for spec.pdf, PDF');
 	});
 
+	it('comes back to life when the node is repointed at a different attachment', () => {
+		// `disabled` is what makes a dead chip inert, so the uuid-swap path has
+		// to undo it along with everything else markDeleted() set. Leaving it
+		// would produce a chip that ANNOUNCES itself as live and does nothing —
+		// worse than the dead one, which at least says so. Reachable through a
+		// collaborative peer's edit or a ProseMirror node replacement.
+		const el = chip();
+		for (const fn of deletionListeners) fn('uuid-1');
+		expect(el.disabled).toBe(true);
+
+		repointChip('uuid-2', 'other.pdf');
+
+		const live = chip();
+		expect(live.disabled).toBe(false);
+		expect(live.classList.contains('attachment-missing')).toBe(false);
+		live.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 }));
+		expect(panelOpenMock).toHaveBeenCalledTimes(1);
+		expect(panelOpenMock.mock.calls[0][0]).toMatchObject({ attachmentId: 'uuid-2' });
+	});
+
 	it('offers no URL for a middle-click to bypass the panel with', () => {
 		// This is why the chip is a button. As an <a href download>, only the
 		// primary click ran the handler: middle-click and aux-click still opened
diff --git a/web/src/lib/components/settings/StorageTab.svelte b/web/src/lib/components/settings/StorageTab.svelte
index 93664447..21d8093d 100644
--- a/web/src/lib/components/settings/StorageTab.svelte
+++ b/web/src/lib/components/settings/StorageTab.svelte
@@ -20,7 +20,9 @@
 	} from '$lib/attachments/storageFilters';
 	import AttachmentIcon from '$lib/attachments/icons/AttachmentIcon.svelte';
 	import Menu from '$lib/components/common/Menu.svelte';
-	import AttachmentDeleteConfirm from '$lib/components/attachments/AttachmentDeleteConfirm.svelte';
+	import AttachmentDeleteConfirm, {
+		workspaceAttachmentDeletePrompt,
+	} from '$lib/components/attachments/AttachmentDeleteConfirm.svelte';
 	import { viewIdentity, createFence, createPaintFence } from '$lib/attachments/viewFence';
 
 	// ── Props ────────────────────────────────────────────────────────────────
@@ -435,7 +437,7 @@
 		pendingDelete = {
 			att,
 			anchor,
-			prompt: `Delete ${att.filename}? The blob is reclaimed by garbage collection after a grace period.`,
+			prompt: workspaceAttachmentDeletePrompt(att.filename),
 		};
 	}