From 2ec6c954e68a89e1a1d30809c53520b35d8190db Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 4 Aug 2026 00:44:16 +0000 Subject: [PATCH 01/22] feat(attachments): typed metadata result and MIME capability helpers (TASK-2420) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fetchAttachmentMetadata` collapsed 404, every other non-2xx and network throws into a single `null` — and cached it. Two consequences: nothing could treat "gone" as authoritative (so editor undo resurrected deleted attachments as live-looking nodes), and a one-off blip was sticky for the page's lifetime. It now returns a discriminated result: `ok` carries mime/size, `missing` is the authoritative 404, `transient` is everything else. `ok` and `missing` stay cached; `transient` is evicted the moment it settles, so a retry re-issues the HEAD while concurrent callers still share one in-flight request (PLAN-2392 DR-17). Both NodeView consumers act on the split. The chip latches the missing/deleted treatment on `missing` and leaves the filename-guess icon alone on `transient`. The image NodeView probes on load failure — an error event carries no status code, so a deleted row and a network blip are indistinguishable there — and only a 404 latches the permanent placeholder; the toolbar's MIME probe latches too, since it may beat the image to the answer. ItemTimeline drops its probed-mark on `transient` so a blip doesn't permanently strand an entry's metadata. Adds `canOpenInViewer` (DR-16: exact five-type raster allowlist, not an `image/` prefix — SVG carries active content and TIFF/HEIC may not decode) and `canBrowserPreview` (DR-5: that set plus PDF and text/plain) next to `isImage`, which survives unchanged as the general picture predicate. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC --- web/src/lib/attachments/display.test.ts | 96 +++++++- web/src/lib/attachments/display.ts | 49 ++++ .../lib/components/editor/attachment-chip.ts | 19 +- .../lib/components/editor/attachment-image.ts | 64 ++++- .../editor/attachment-metadata.test.ts | 226 ++++++++++++++++++ .../components/editor/attachment-metadata.ts | 63 ++++- .../components/timeline/ItemTimeline.svelte | 13 +- 7 files changed, 508 insertions(+), 22 deletions(-) create mode 100644 web/src/lib/components/editor/attachment-metadata.test.ts diff --git a/web/src/lib/attachments/display.test.ts b/web/src/lib/attachments/display.test.ts index 995fdc3d..4fb427d4 100644 --- a/web/src/lib/attachments/display.test.ts +++ b/web/src/lib/attachments/display.test.ts @@ -1,5 +1,11 @@ import { describe, expect, it } from 'vitest'; -import { formatBytes, iconForAttachment, isImage } from './display'; +import { + canBrowserPreview, + canOpenInViewer, + formatBytes, + iconForAttachment, + isImage +} from './display'; import { ATTACHMENT_ICON_IDS, ATTACHMENT_ICON_PATHS, iconSvg } from './icons/index'; import familyFixture from './mime-families.json'; @@ -182,4 +188,92 @@ describe('isImage', () => { expect(isImage('image/png')).toBe(true); expect(isImage('application/pdf')).toBe(false); }); + + // The DR-16 point in one assertion: isImage is deliberately looser than + // the viewer gate, which is why the gate has to be its own helper. + it('is looser than the viewer gate — it accepts what canOpenInViewer refuses', () => { + expect(isImage('image/svg+xml')).toBe(true); + expect(canOpenInViewer('image/svg+xml')).toBe(false); + }); +}); + +const VIEWER_TYPES = ['image/png', 'image/jpeg', 'image/gif', 'image/webp', 'image/avif']; + +describe('canOpenInViewer — PLAN-2392 DR-16', () => { + it('accepts exactly the five safe raster types', () => { + for (const mime of VIEWER_TYPES) expect(canOpenInViewer(mime)).toBe(true); + }); + + // The whole reason this isn't `startsWith('image/')`: SVG carries active + // content, and TIFF/HEIC are types a browser may simply not decode. + it('refuses image/* types outside the allowlist', () => { + expect(canOpenInViewer('image/svg+xml')).toBe(false); + expect(canOpenInViewer('image/tiff')).toBe(false); + expect(canOpenInViewer('image/heic')).toBe(false); + expect(canOpenInViewer('image/bmp')).toBe(false); + expect(canOpenInViewer('image/jxl')).toBe(false); + }); + + it('refuses non-image types and a missing MIME', () => { + expect(canOpenInViewer('application/pdf')).toBe(false); + expect(canOpenInViewer('text/xml')).toBe(false); + expect(canOpenInViewer('')).toBe(false); + expect(canOpenInViewer(null)).toBe(false); + expect(canOpenInViewer(undefined)).toBe(false); + }); + + it('normalizes case and parameters before matching', () => { + expect(canOpenInViewer('IMAGE/PNG')).toBe(true); + expect(canOpenInViewer('image/jpeg; charset=binary')).toBe(true); + expect(canOpenInViewer(' image/webp ')).toBe(true); + }); + + // A prefix test would let `image/svg+xml; charset=utf-8` through some + // naive normalizations; pin that it doesn't. + it('does not admit a disallowed type by dressing it in parameters', () => { + expect(canOpenInViewer('image/svg+xml; charset=utf-8')).toBe(false); + }); +}); + +describe('canBrowserPreview — PLAN-2392 DR-5', () => { + it('accepts PDF, plain text and the whole viewer raster set', () => { + expect(canBrowserPreview('application/pdf')).toBe(true); + expect(canBrowserPreview('text/plain')).toBe(true); + for (const mime of VIEWER_TYPES) expect(canBrowserPreview(mime)).toBe(true); + }); + + it('refuses the other text/* subtypes browsers handle inconsistently', () => { + expect(canBrowserPreview('text/markdown')).toBe(false); + expect(canBrowserPreview('text/csv')).toBe(false); + expect(canBrowserPreview('text/xml')).toBe(false); + expect(canBrowserPreview('application/xml')).toBe(false); + }); + + it('refuses office documents, archives and force-downloaded types', () => { + expect(canBrowserPreview('application/msword')).toBe(false); + expect( + canBrowserPreview( + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' + ) + ).toBe(false); + expect(canBrowserPreview('application/zip')).toBe(false); + expect(canBrowserPreview('text/html')).toBe(false); + expect(canBrowserPreview('application/javascript')).toBe(false); + expect(canBrowserPreview('image/svg+xml')).toBe(false); + }); + + it('refuses a missing MIME and normalizes like the viewer gate', () => { + expect(canBrowserPreview(null)).toBe(false); + expect(canBrowserPreview(undefined)).toBe(false); + expect(canBrowserPreview('')).toBe(false); + expect(canBrowserPreview('TEXT/PLAIN; charset=utf-8')).toBe(true); + }); + + // Superset relationship, stated once so a future edit to either set + // can't silently break it. + it('is a superset of the viewer gate', () => { + for (const mime of VIEWER_TYPES) { + expect(canOpenInViewer(mime) && canBrowserPreview(mime)).toBe(true); + } + }); }); diff --git a/web/src/lib/attachments/display.ts b/web/src/lib/attachments/display.ts index 86e769e2..3bfa9572 100644 --- a/web/src/lib/attachments/display.ts +++ b/web/src/lib/attachments/display.ts @@ -221,3 +221,52 @@ export function iconForAttachment( export function isImage(mime: string): boolean { return mime.startsWith('image/'); } + +/** + * The exact raster types the in-app image viewer may open (PLAN-2392 + * DR-16). Deliberately an allowlist and NOT an `image/` prefix test: + * `image/svg+xml` carries active content, and a legacy row, a + * mislabelled upload or an extensionless SVG sniffed as XML can all + * arrive wearing an `image/*` label. Formats a browser may not decode + * at all (`image/tiff`, `image/heic`) are excluded for the separate + * reason that a viewer that silently shows nothing is worse than the + * file panel. + * + * `isImage` survives unchanged as the general "is this a picture" + * predicate (icon choice, grouping); this is the narrower question of + * what may be handed to the viewer. + */ +const VIEWER_MIMES: ReadonlySet = new Set([ + 'image/png', + 'image/jpeg', + 'image/gif', + 'image/webp', + 'image/avif' +]); + +/** + * Additional types a browser renders honestly in a new tab (PLAN-2392 + * DR-5). PDF and plain text only — every other `text/*` subtype + * (markdown, CSV, XML) is downloaded or rendered inconsistently across + * browsers, and office documents, archives and the types the server + * force-downloads (HTML, JS) never preview. Those surfaces offer + * Download alone. + */ +const BROWSER_PREVIEW_MIMES: ReadonlySet = new Set([ + 'application/pdf', + 'text/plain' +]); + +/** May this MIME be opened in the in-app image viewer? (DR-16) */ +export function canOpenInViewer(mime: string | null | undefined): boolean { + return VIEWER_MIMES.has(normalizeMime(mime)); +} + +/** + * May this MIME be handed to the browser to display — the viewer's + * raster set plus PDF and plain text? (DR-5) + */ +export function canBrowserPreview(mime: string | null | undefined): boolean { + const m = normalizeMime(mime); + return VIEWER_MIMES.has(m) || BROWSER_PREVIEW_MIMES.has(m); +} diff --git a/web/src/lib/components/editor/attachment-chip.ts b/web/src/lib/components/editor/attachment-chip.ts index 89e40caf..cab0e2d7 100644 --- a/web/src/lib/components/editor/attachment-chip.ts +++ b/web/src/lib/components/editor/attachment-chip.ts @@ -306,19 +306,30 @@ export const AttachmentChip = Node.create({ this.options.workspaceSlug, forUuid, this.options.getDownloadUrl, - ).then((meta) => { - if (!meta) return; + ).then((result) => { if (destroyed) return; // NodeView torn down while HEAD was in flight if (deleted) return; // the target is gone; don't un-mark the chip if (currentUuid !== forUuid) return; // superseded - currentMime = meta.mime; + // A transient failure says nothing about whether the row + // exists — keep the filename-guess icon and stay + // retryable (PLAN-2392 DR-17). + if (result.status === 'transient') return; + // A 404 IS authoritative. This is the path editor undo + // takes: undo restores the chip node, but the delete was a + // REST row mutation Tiptap's history can't roll back, so + // the chip must render dead rather than link to a 404. + if (result.status === 'missing') { + markDeleted(); + return; + } + currentMime = result.mime; refreshIcon(); // The shared formatter renders "0 B" and doesn't guard // non-finite input; a chip with no known size should show // nothing at all, so the conditional lives here rather // than in the helper (PLAN-2392 DR-3b). const size = - Number.isFinite(meta.size) && meta.size > 0 ? formatBytes(meta.size) : ''; + Number.isFinite(result.size) && result.size > 0 ? formatBytes(result.size) : ''; sizeEl.textContent = size ? `· ${size}` : ''; }); }; diff --git a/web/src/lib/components/editor/attachment-image.ts b/web/src/lib/components/editor/attachment-image.ts index 175bc509..171ee3c7 100644 --- a/web/src/lib/components/editor/attachment-image.ts +++ b/web/src/lib/components/editor/attachment-image.ts @@ -310,9 +310,50 @@ export const AttachmentImage = Node.create({ // queued, so a superseded load simply has no callback left to run. let detachLoadListeners = () => {}; + /** + * Latch the permanent placeholder for a row the server says is + * gone. Same end state as the deletion broadcast, reached by a + * different route: the broadcast only fires for a delete that + * happened in THIS tab's session, while this covers a node whose + * row was already gone when it rendered — which is exactly what + * editor undo produces (PLAN-2392 DR-17). Tiptap/Yjs history + * owns the document; the delete was a REST row mutation it can't + * roll back, so undo restores a node pointing at nothing. + */ + function latchMissing(forUuid: string) { + if (deleted) return; + if (!forUuid || currentUuid !== forUuid) return; + deleted = true; + // Same reason the deletion listener does this: an in-flight + // load's `load` event would otherwise paint the image back. + detachLoadListeners(); + showMissing(); + } + + /** + * An `error` event carries no status code, so a deleted row + * and a network blip are indistinguishable at that layer — which + * is why every load failure has to stay retryable by default. A + * HEAD probe is what tells them apart: only a 404 latches, and a + * `transient` result leaves the retryable placeholder exactly as + * it was (and is not cached, so Retry re-issues the HEAD). + */ + function probeForMissing(forUuid: string) { + if (!forUuid || !opts.workspaceSlug || deleted) return; + void fetchAttachmentMetadata(opts.workspaceSlug, forUuid, opts.getDownloadUrl).then( + (result) => { + if (result.status === 'missing') latchMissing(forUuid); + } + ); + } + function loadImage(url: string) { detachLoadListeners(); - const onError = () => showMissing(); + const forUuid = currentUuid; + const onError = () => { + showMissing(); + probeForMissing(forUuid); + }; const onLoad = () => resetMissing(); img.addEventListener('error', onError, { once: true }); img.addEventListener('load', onLoad, { once: true }); @@ -404,12 +445,19 @@ export const AttachmentImage = Node.create({ if (currentUuid && opts.workspaceSlug) { const probeUuid = currentUuid; fetchAttachmentMetadata(opts.workspaceSlug, probeUuid, opts.getDownloadUrl).then( - (meta) => { + (result) => { // Bail if the NodeView's uuid changed (rotate/peer // op) while the probe was in flight — otherwise // we'd cache stale MIME state for the new image. - if (!toolbar || currentUuid !== probeUuid) return; - toolbarMime = meta?.mime ?? null; + if (currentUuid !== probeUuid) return; + // A 404 here is the same authoritative signal the + // load path acts on (DR-17), and this probe may + // well beat the to it. + if (result.status === 'missing') latchMissing(probeUuid); + if (!toolbar) return; + // `transient` leaves the MIME unknown rather than + // wrong: gating falls back to supportedFormats. + toolbarMime = result.status === 'ok' ? result.mime : null; refresh(); } ); @@ -591,9 +639,11 @@ export const AttachmentImage = Node.create({ opts.workspaceSlug, probeUuid, opts.getDownloadUrl - ).then((meta) => { - if (!toolbar || currentUuid !== probeUuid) return; - toolbarMime = meta?.mime ?? null; + ).then((result) => { + if (currentUuid !== probeUuid) return; + if (result.status === 'missing') latchMissing(probeUuid); + if (!toolbar) return; + toolbarMime = result.status === 'ok' ? result.mime : null; refresh(); }); } diff --git a/web/src/lib/components/editor/attachment-metadata.test.ts b/web/src/lib/components/editor/attachment-metadata.test.ts new file mode 100644 index 00000000..1e8e279a --- /dev/null +++ b/web/src/lib/components/editor/attachment-metadata.test.ts @@ -0,0 +1,226 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { + fetchAttachmentMetadata, + invalidateAttachmentMetadata, + mimeToFormat +} from './attachment-metadata'; + +// PLAN-2392 DR-17. The three arms exist so a caller can tell "the row is +// gone" (latch the placeholder — editor undo must not resurrect a deleted +// attachment) apart from "the request didn't make it" (stay retryable). +// The old helper collapsed both into `null` AND cached it, which made a +// one-off blip permanently sticky for the page's lifetime. + +const url = (id: string) => `/api/v1/workspaces/ws/attachments/${id}`; + +/** A HEAD response with the headers the helper reads. */ +function head(status: number, headers: Record = {}): Response { + return new Response(null, { status, headers }); +} + +let fetchMock: ReturnType; + +// Each test uses a fresh uuid AND invalidates it, because the module-level +// promise cache is process-wide and deliberately outlives a single probe. +let counter = 0; +function freshUuid(): string { + counter += 1; + return `uuid-${counter}`; +} + +beforeEach(() => { + fetchMock = vi.fn(); + vi.stubGlobal('fetch', fetchMock); +}); + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('fetchAttachmentMetadata — result arms', () => { + it('returns ok with the parsed MIME and size on 200', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue( + head(200, { 'content-type': 'image/png; charset=binary', 'content-length': '4096' }) + ); + + const result = await fetchAttachmentMetadata('ws', uuid, url); + + expect(result).toEqual({ status: 'ok', mime: 'image/png', size: 4096 }); + // HEAD, not GET — a GET would pull the whole blob across the wire. + expect(fetchMock).toHaveBeenCalledWith(url(uuid), { + method: 'HEAD', + credentials: 'same-origin' + }); + }); + + it('falls back to a zero size when content-length is absent or junk', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue(head(200, { 'content-type': 'application/pdf' })); + + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ + status: 'ok', + mime: 'application/pdf', + size: 0 + }); + }); + + it('reports 404 as missing — the authoritative "row is gone" answer', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue(head(404)); + + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' }); + }); + + it('reports a 500 as transient, not missing', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue(head(500)); + + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'transient' }); + }); + + it('reports a mid-session 403 as transient, not missing', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue(head(403)); + + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'transient' }); + }); + + it('reports a network throw as transient rather than rejecting', async () => { + const uuid = freshUuid(); + fetchMock.mockRejectedValue(new TypeError('Failed to fetch')); + + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'transient' }); + }); +}); + +describe('fetchAttachmentMetadata — caching is per-arm', () => { + it('caches an ok result for the page lifetime', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue(head(200, { 'content-type': 'image/webp', 'content-length': '10' })); + + await fetchAttachmentMetadata('ws', uuid, url); + await fetchAttachmentMetadata('ws', uuid, url); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('caches a missing result — deletion is durable, so stop asking', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue(head(404)); + + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' }); + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' }); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('does NOT cache a transient failure — a retry re-issues the HEAD', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValueOnce(head(503)); + + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'transient' }); + + // ...and the row was fine all along: the second probe must reach the + // network and see that, rather than replaying the cached failure. + fetchMock.mockResolvedValueOnce( + head(200, { 'content-type': 'image/jpeg', 'content-length': '7' }) + ); + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ + status: 'ok', + mime: 'image/jpeg', + size: 7 + }); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('does NOT cache a network throw either', async () => { + const uuid = freshUuid(); + fetchMock.mockRejectedValueOnce(new TypeError('offline')); + + await fetchAttachmentMetadata('ws', uuid, url); + + fetchMock.mockResolvedValueOnce(head(404)); + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' }); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('still shares one in-flight HEAD between concurrent callers that fail', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue(head(500)); + + const [a, b] = await Promise.all([ + fetchAttachmentMetadata('ws', uuid, url), + fetchAttachmentMetadata('ws', uuid, url) + ]); + + expect(a).toEqual({ status: 'transient' }); + expect(b).toEqual({ status: 'transient' }); + // Eviction happens when the promise SETTLES, so dedupe survives. + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('keys the cache by workspace as well as uuid', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue(head(200, { 'content-type': 'image/gif', 'content-length': '1' })); + + await fetchAttachmentMetadata('ws-a', uuid, url); + await fetchAttachmentMetadata('ws-b', uuid, url); + + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('invalidate drops a cached entry so the next call refetches', async () => { + const uuid = freshUuid(); + fetchMock.mockResolvedValue(head(200, { 'content-type': 'image/png', 'content-length': '2' })); + + await fetchAttachmentMetadata('ws', uuid, url); + invalidateAttachmentMetadata('ws', uuid); + await fetchAttachmentMetadata('ws', uuid, url); + + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('transient eviction cannot delete a newer entry installed by an invalidate race', async () => { + const uuid = freshUuid(); + let releaseFirst!: (r: Response) => void; + fetchMock.mockImplementationOnce( + () => new Promise((resolve) => (releaseFirst = resolve)) + ); + + const slow = fetchAttachmentMetadata('ws', uuid, url); + + // A delete/transform lands, the entry is invalidated, and a fresh + // probe caches a good result — all before the first HEAD settles. + invalidateAttachmentMetadata('ws', uuid); + fetchMock.mockResolvedValueOnce( + head(200, { 'content-type': 'image/avif', 'content-length': '3' }) + ); + await fetchAttachmentMetadata('ws', uuid, url); + + releaseFirst(head(500)); + expect(await slow).toEqual({ status: 'transient' }); + + // The newer, good entry survived the older promise's eviction. + expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({ + status: 'ok', + mime: 'image/avif', + size: 3 + }); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); +}); + +describe('mimeToFormat', () => { + it('maps the recognized image MIMEs to the server-side format names', () => { + expect(mimeToFormat('image/jpeg')).toBe('jpeg'); + expect(mimeToFormat('image/jpg')).toBe('jpeg'); + expect(mimeToFormat('image/heif')).toBe('heic'); + expect(mimeToFormat('IMAGE/PNG')).toBe('png'); + }); + + it('returns null for non-images and unknown image subtypes', () => { + expect(mimeToFormat('application/pdf')).toBeNull(); + expect(mimeToFormat('image/jxl')).toBeNull(); + }); +}); diff --git a/web/src/lib/components/editor/attachment-metadata.ts b/web/src/lib/components/editor/attachment-metadata.ts index 0507db0d..643e2024 100644 --- a/web/src/lib/components/editor/attachment-metadata.ts +++ b/web/src/lib/components/editor/attachment-metadata.ts @@ -14,9 +14,9 @@ * immutable (the row is content-addressed; transforms produce * NEW rows), so there's no staleness concern. * - * Skipped silently when no workspace context is available (e.g. - * headless rendering / SSR) — callers see `null` and degrade UI - * accordingly without raising errors. + * Callers never see an exception: every failure is reported through the + * discriminated result below, so a surface with no workspace context + * (headless rendering / SSR) simply doesn't call this at all. */ /** Variants the download URL builder must support. Mirrors AttachmentImage. */ @@ -30,38 +30,83 @@ export interface AttachmentMetadata { size: number; } -const cache = new Map>(); +/** + * The outcome of a metadata probe (PLAN-2392 DR-17). + * + * The three arms exist because callers need to tell "the row is gone" + * apart from "the request didn't make it", and the old `null` return + * collapsed both: + * + * - `ok` — the HEAD succeeded; `mime` / `size` are usable. + * - `missing` — the server answered 404. AUTHORITATIVE: the row is + * gone, and a caller may latch a permanent + * missing-attachment placeholder on it. This is what + * keeps editor undo from resurrecting a deleted + * attachment as a live-looking node. + * - `transient` — any other non-2xx (5xx, 401/403 mid-session, a + * proxy hiccup) or a network throw. Says NOTHING + * about whether the row exists; callers keep whatever + * they were showing and stay retryable. + */ +export type AttachmentMetadataResult = + | ({ status: 'ok' } & AttachmentMetadata) + | { status: 'missing' } + | { status: 'transient' }; + +const cache = new Map>(); /** * Fetch (or read from cache) the MIME + size for an attachment. The * server registers HEAD alongside GET (TASK-877); chi doesn't auto- * route HEAD on GET handlers, so this must use HEAD — a GET would * pull the entire blob across the wire. + * + * Caching is per-arm (PLAN-2392 DR-17). `ok` and `missing` are both + * durable facts about a content-addressed row, so they're kept for the + * page lifetime. A `transient` result is NOT — it's evicted the moment + * it settles, so a blip can't make a live attachment look permanently + * unreadable for the rest of the session. The entry is still installed + * BEFORE the request settles, so concurrent callers for the same key + * share one in-flight HEAD either way; only the settled failure is + * dropped. */ export function fetchAttachmentMetadata( workspaceSlug: string, uuid: string, getDownloadUrl: AttachmentUrlBuilder -): Promise { +): Promise { const key = `${workspaceSlug}:${uuid}`; const existing = cache.get(key); if (existing) return existing; - const promise: Promise = (async () => { + const promise: Promise = (async () => { try { const resp = await fetch(getDownloadUrl(uuid), { method: 'HEAD', credentials: 'same-origin' }); - if (!resp.ok) return null; + if (resp.status === 404) return { status: 'missing' as const }; + if (!resp.ok) return { status: 'transient' as const }; const ctype = resp.headers.get('content-type') ?? ''; const mime = ctype.split(';')[0].trim(); const len = parseInt(resp.headers.get('content-length') ?? '0', 10); - return { mime, size: Number.isFinite(len) && len >= 0 ? len : 0 }; + return { + status: 'ok' as const, + mime, + size: Number.isFinite(len) && len >= 0 ? len : 0 + }; } catch { - return null; + return { status: 'transient' as const }; } })(); cache.set(key, promise); + // Evict a transient failure once it settles. The identity check keeps + // this from deleting a NEWER entry installed by an invalidate-then- + // refetch that raced this promise's resolution. + void promise.then((result) => { + if (result.status === 'transient' && cache.get(key) === promise) { + cache.delete(key); + } + }); return promise; } diff --git a/web/src/lib/components/timeline/ItemTimeline.svelte b/web/src/lib/components/timeline/ItemTimeline.svelte index 23a9a357..1ea411c5 100644 --- a/web/src/lib/components/timeline/ItemTimeline.svelte +++ b/web/src/lib/components/timeline/ItemTimeline.svelte @@ -120,7 +120,18 @@ fetchAttachmentMetadata(wsSlug, uuid, (id, variant) => attachmentDownloadUrl(wsSlug, id, variant) ).then((m) => { - if (!m) return; + // 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 + // authoritative: leave the mark set and leave `attMeta` without an + // entry, which is what the renderer already degrades to a missing + // placeholder on. + if (m.status === 'transient') { + probed.delete(uuid); + return; + } + if (m.status !== 'ok') return; if (reqWs !== wsSlug) return; const next = new Map(attMeta); // filename is left empty — the markdown alt text is the chip/img From 51b90003a904d24f7f120b5feb68f39b34b9a77b Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 4 Aug 2026 00:46:11 +0000 Subject: [PATCH 02/22] feat(attachments): thread host identity into the attachment surfaces (TASK-2421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The addressing layer the attachment options panel needs (PLAN-2392 DR-2 / DR-8). No visible behaviour change — nothing consumes the channel yet. `$lib/attachments/events.ts` gains the panel channel: `AttachmentPanelOpenEvent` ({attachmentId, itemId, hostToken, anchor, filename, mime_type, size_bytes} — the three metadata fields nullable because a chip fills them from an async HEAD probe that may be incomplete or failed, while the strip always has all three from its list row), `notifyAttachmentPanelOpen`, `registerAttachmentPanelListener`, `createAttachmentHostToken` and `isAttachmentPanelEventForHost`. The two identity fields are the point. The bus is module-global, but `ItemDetail` is mounted more than once at a time — the pane host runs a master plus a peeked pane, which can be showing the same item. `itemId` alone would let both hosts consume one NodeView's event (two panels for one tap, one of them permissioned by the wrong host's mutationsEnabled). So a host consumes an event only when BOTH fields are its own, and a null/empty token on either side matches nothing — an unconfigured NodeView must not be able to address every host at once. `ItemDetail` mints ONE token per mount (a plain const, stable across the no-{#key} item switch) and passes it to every attachment surface it owns: the strip (which had no token path at all), both `Editor` branches, and — through `ItemTimeline` and `TimelineCommentCard` — every `CommentEditor`. `itemId` + `hostToken` are threaded into `AttachmentChipOptions` and `AttachmentImageOptions` and wired at both configure sites. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC --- web/src/lib/attachments/events.test.ts | 168 ++++++++++++++++++ web/src/lib/attachments/events.ts | 103 +++++++++++ web/src/lib/components/CommentEditor.svelte | 22 ++- web/src/lib/components/editor/Editor.svelte | 24 ++- .../lib/components/editor/attachment-chip.ts | 16 ++ .../lib/components/editor/attachment-image.ts | 15 ++ .../items/ItemAttachmentStrip.svelte | 10 ++ .../lib/components/items/ItemDetail.svelte | 21 +++ .../components/timeline/ItemTimeline.svelte | 15 +- .../timeline/TimelineCommentCard.svelte | 13 +- 10 files changed, 403 insertions(+), 4 deletions(-) create mode 100644 web/src/lib/attachments/events.test.ts diff --git a/web/src/lib/attachments/events.test.ts b/web/src/lib/attachments/events.test.ts new file mode 100644 index 00000000..fe98a63b --- /dev/null +++ b/web/src/lib/attachments/events.test.ts @@ -0,0 +1,168 @@ +import { describe, expect, it } from 'vitest'; +import { + createAttachmentHostToken, + isAttachmentPanelEventForHost, + notifyAttachmentPanelOpen, + registerAttachmentPanelListener, + type AttachmentPanelOpenEvent, +} from './events'; + +/** + * The addressing layer for the attachment options panel (PLAN-2392 DR-8 / + * TASK-2421). + * + * The thing under test is NOT "does a bus deliver" — it's that two + * simultaneously-mounted ItemDetail hosts (a master and a peeked pane, which + * can be showing the SAME item) each consume only their own surfaces' events. + * Every failure mode below has a concrete two-panel bug behind it. + */ + +function event(over: Partial = {}): AttachmentPanelOpenEvent { + return { + attachmentId: 'att-1', + itemId: 'item-1', + hostToken: 'host-a', + anchor: null, + filename: 'notes.pdf', + mime_type: 'application/pdf', + size_bytes: 1234, + ...over, + }; +} + +describe('createAttachmentHostToken', () => { + it('mints a distinct, non-empty token per call', () => { + const seen = new Set(); + for (let i = 0; i < 100; i++) { + const token = createAttachmentHostToken(); + expect(token).toBeTruthy(); + expect(seen.has(token)).toBe(false); + seen.add(token); + } + }); +}); + +describe('isAttachmentPanelEventForHost', () => { + const host = { itemId: 'item-1', hostToken: 'host-a' }; + + it('matches when BOTH the item and the token are the host’s', () => { + expect(isAttachmentPanelEventForHost(event(), host)).toBe(true); + }); + + it('ignores an event that matches only the item (the two-panes-one-item case)', () => { + // Master and peeked pane showing the same item: itemId alone is not an + // address, or one tap opens two panels. + expect(isAttachmentPanelEventForHost(event({ hostToken: 'host-b' }), host)).toBe(false); + }); + + it('ignores an event that matches only the token', () => { + // One host, but the emitting surface belongs to a different item — + // e.g. a stale NodeView configured before an item switch. + expect(isAttachmentPanelEventForHost(event({ itemId: 'item-2' }), host)).toBe(false); + }); + + it('never matches when the EVENT carries no token', () => { + // An unconfigured NodeView (options default to '') must not be able to + // address every host at once. + expect(isAttachmentPanelEventForHost(event({ hostToken: '' }), host)).toBe(false); + }); + + it('never matches when the HOST has no token', () => { + expect(isAttachmentPanelEventForHost(event(), { itemId: 'item-1', hostToken: '' })).toBe(false); + expect(isAttachmentPanelEventForHost(event(), { itemId: 'item-1', hostToken: null })).toBe( + false + ); + expect( + isAttachmentPanelEventForHost(event(), { itemId: 'item-1', hostToken: undefined }) + ).toBe(false); + }); + + it('never matches when either side has no item', () => { + expect(isAttachmentPanelEventForHost(event({ itemId: '' }), host)).toBe(false); + expect(isAttachmentPanelEventForHost(event(), { itemId: null, hostToken: 'host-a' })).toBe( + false + ); + }); +}); + +describe('the panel channel with two live hosts', () => { + it('delivers one surface’s event to exactly one of two hosts on the same item', () => { + const master = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + const peeked = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + const masterSeen: AttachmentPanelOpenEvent[] = []; + const peekedSeen: AttachmentPanelOpenEvent[] = []; + + const offMaster = registerAttachmentPanelListener((e) => { + if (isAttachmentPanelEventForHost(e, master)) masterSeen.push(e); + }); + const offPeeked = registerAttachmentPanelListener((e) => { + if (isAttachmentPanelEventForHost(e, peeked)) peekedSeen.push(e); + }); + + try { + notifyAttachmentPanelOpen(event({ hostToken: peeked.hostToken })); + expect(masterSeen).toHaveLength(0); + expect(peekedSeen).toHaveLength(1); + expect(peekedSeen[0].attachmentId).toBe('att-1'); + + notifyAttachmentPanelOpen( + event({ attachmentId: 'att-2', hostToken: master.hostToken }) + ); + expect(masterSeen).toHaveLength(1); + expect(masterSeen[0].attachmentId).toBe('att-2'); + expect(peekedSeen).toHaveLength(1); + } finally { + offMaster(); + offPeeked(); + } + }); + + it('carries nullable metadata through unchanged (a chip’s HEAD probe may be incomplete)', () => { + const host = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + let received: AttachmentPanelOpenEvent | null = null; + const off = registerAttachmentPanelListener((e) => { + if (isAttachmentPanelEventForHost(e, host)) received = e; + }); + try { + notifyAttachmentPanelOpen( + event({ + hostToken: host.hostToken, + filename: null, + mime_type: null, + size_bytes: null, + }) + ); + } finally { + off(); + } + expect(received).not.toBeNull(); + expect(received!.filename).toBeNull(); + expect(received!.mime_type).toBeNull(); + expect(received!.size_bytes).toBeNull(); + }); + + it('drops an unaddressable emission rather than broadcasting it', () => { + const seen: AttachmentPanelOpenEvent[] = []; + const off = registerAttachmentPanelListener((e) => seen.push(e)); + try { + notifyAttachmentPanelOpen(event({ hostToken: '' })); + notifyAttachmentPanelOpen(event({ itemId: '' })); + notifyAttachmentPanelOpen(event({ attachmentId: '' })); + } finally { + off(); + } + expect(seen).toHaveLength(0); + }); + + it('stops delivering after dispose', () => { + const host = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + const seen: AttachmentPanelOpenEvent[] = []; + const off = registerAttachmentPanelListener((e) => { + if (isAttachmentPanelEventForHost(e, host)) seen.push(e); + }); + notifyAttachmentPanelOpen(event({ hostToken: host.hostToken })); + off(); + notifyAttachmentPanelOpen(event({ hostToken: host.hostToken })); + expect(seen).toHaveLength(1); + }); +}); diff --git a/web/src/lib/attachments/events.ts b/web/src/lib/attachments/events.ts index ebf3fcb8..3275351f 100644 --- a/web/src/lib/attachments/events.ts +++ b/web/src/lib/attachments/events.ts @@ -115,3 +115,106 @@ export function notifyAttachmentUploaded( if (!itemId || !attachment?.id) return; for (const fn of uploadListeners) fn(itemId, attachment); } + +/** + * Attachment options panel (PLAN-2392 DR-2 / DR-8, TASK-2421). + * + * Tapping a file — a strip tile or an inline editor chip — opens a metadata + + * options panel instead of downloading it. The panel is a Svelte component + * owned by an `ItemDetail` host; the emitters include Tiptap NodeViews, which + * are imperative DOM and cannot mount Svelte themselves. So they signal + * through this bus, exactly as the deletion / upload channels above. + * + * ADDRESSING (DR-8) is the whole reason this channel carries two identity + * fields rather than one. The bus is module-global, but `ItemDetail` is + * mounted MORE THAN ONCE at a time — the pane host runs a master pane plus a + * peeked pane, both showing attachment surfaces. Matching on `itemId` alone + * is not enough (both panes can show the same item), and matching on the + * token alone is not enough either (a host must not open a panel for an + * attachment belonging to a different item). A host consumes an event only + * when BOTH are its own — see `isAttachmentPanelEventForHost`. + * + * Permission never travels on the event: the host supplies `mutationsEnabled` + * from its own `computeMutationsEnabled(canEdit, peeking)`. A NodeView has no + * mutation context and must not be trusted to assert one. + * + * The three metadata fields are NULLABLE. A chip knows only what its options + * give it and fills these from an asynchronous HEAD probe that may not have + * completed, or may have failed. The panel opens immediately with whatever is + * known and fetches the rest itself (DR-2 round 36). The strip, by contrast, + * always populates all three from its list row. + */ +export interface AttachmentPanelOpenEvent { + /** UUID of the attachment whose options are being opened. */ + attachmentId: string; + /** UUID of the item the emitting surface belongs to. */ + itemId: string; + /** Identity of the `ItemDetail` mount that owns the emitting surface. */ + hostToken: string; + /** + * The element the panel positions against and returns focus to on close. + * Null when the emitter has no stable element to offer (the panel then + * falls back to its own placement / focus handling). + */ + anchor: HTMLElement | null; + filename: string | null; + mime_type: string | null; + size_bytes: number | null; +} + +/** + * Mint the identity for ONE `ItemDetail` mount. Call it once per host and + * pass the result to every attachment surface that host owns — the strip, the + * body `Editor`, every `CommentEditor`. One token per host, NOT one per + * component: surfaces of the same host must be indistinguishable to the + * panel, while the master and peeked panes must never be. + */ +export function createAttachmentHostToken(): string { + const c = typeof globalThis !== 'undefined' ? globalThis.crypto : undefined; + if (c && typeof c.randomUUID === 'function') return `apanel-${c.randomUUID()}`; + return `apanel-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 10)}`; +} + +/** + * "Is this event mine?" — the single predicate every panel host must use. + * + * Both fields must match. An empty / null token on EITHER side never matches + * anything: a surface that was never given a token (an older call site, an + * editor mounted outside a host) must not be able to address every host at + * once, and a host without a token must not consume unaddressed events. + */ +export function isAttachmentPanelEventForHost( + event: AttachmentPanelOpenEvent, + host: { itemId: string | null | undefined; hostToken: string | null | undefined } +): boolean { + if (!event) return false; + if (!host?.itemId || !host?.hostToken) return false; + if (!event.itemId || !event.hostToken) return false; + return event.itemId === host.itemId && event.hostToken === host.hostToken; +} + +const panelListeners = new Set<(event: AttachmentPanelOpenEvent) => void>(); + +/** + * Subscribe to open-panel requests. Returns a dispose function — call it from + * the host's teardown, or the listener leaks and fires into a dead component. + * Listeners receive EVERY emission; filter with + * `isAttachmentPanelEventForHost`. + */ +export function registerAttachmentPanelListener( + fn: (event: AttachmentPanelOpenEvent) => void +): () => void { + panelListeners.add(fn); + return () => panelListeners.delete(fn); +} + +/** + * Request that the owning host open the options panel for an attachment. + * No-op when the event can't address a host — an emission missing any of the + * three identity fields would either reach nobody or, worse, invite a + * "matches anything" reading of the predicate. + */ +export function notifyAttachmentPanelOpen(event: AttachmentPanelOpenEvent): void { + if (!event?.attachmentId || !event.itemId || !event.hostToken) return; + for (const fn of panelListeners) fn(event); +} diff --git a/web/src/lib/components/CommentEditor.svelte b/web/src/lib/components/CommentEditor.svelte index 41eb51e1..411c9037 100644 --- a/web/src/lib/components/CommentEditor.svelte +++ b/web/src/lib/components/CommentEditor.svelte @@ -42,6 +42,17 @@ * it uploads fall back to the workspace editor-role gate. */ itemId?: string; + /** + * Identity of the `ItemDetail` mount that owns this composer + * (PLAN-2392 DR-8 / TASK-2421). Threaded down from ItemDetail through + * ItemTimeline (and TimelineCommentCard for edits/replies) so an + * attachment chip in a comment body can address the ONE host that + * owns it — a master and a peeked pane are both mounted, and `itemId` + * alone would let both consume the same event. Empty (the default, + * for composers mounted outside an ItemDetail) disables addressing + * rather than broadcasting. + */ + hostToken?: string; /** Label for the submit button (e.g. "Comment", "Reply", "Save"). */ submitLabel?: string; /** External busy flag (network in flight in the host). */ @@ -62,6 +73,7 @@ placeholder = 'Write a comment…', wsSlug, itemId, + hostToken = '', submitLabel = 'Comment', submitting = false, autofocus = false, @@ -127,13 +139,21 @@ AttachmentImage.configure({ getDownloadUrl: attachmentUrl, workspaceSlug: wsSlug, + // Panel / viewer addressing (PLAN-2392 DR-8). + itemId: itemId ?? '', + hostToken, // Rotate/crop stays disabled in comments — keep it lean. supportedFormats: [] as string[], transform: async () => { throw new Error('Image transforms are not available in comments.'); } }), - AttachmentChip.configure({ getDownloadUrl: attachmentUrl, workspaceSlug: wsSlug }), + AttachmentChip.configure({ + getDownloadUrl: attachmentUrl, + workspaceSlug: wsSlug, + itemId: itemId ?? '', + hostToken + }), AttachmentUpload.configure({ // Wrap upload so the host can track in-flight uploads and gate // submit — the plugin doesn't expose its placeholder count. diff --git a/web/src/lib/components/editor/Editor.svelte b/web/src/lib/components/editor/Editor.svelte index ac1d4597..3bdcd8a4 100644 --- a/web/src/lib/components/editor/Editor.svelte +++ b/web/src/lib/components/editor/Editor.svelte @@ -604,6 +604,7 @@ content = '', editable = true, itemId, + hostToken = '', ydoc, awareness, collabUser, @@ -621,6 +622,16 @@ * fall back to the workspace editor-role gate. */ itemId?: string; + /** + * Identity of the `ItemDetail` mount that owns this editor + * (PLAN-2392 DR-8 / TASK-2421). Passed straight through to the + * attachment NodeViews so a chip / image can address the ONE host + * that owns it — `ItemDetail` runs as a master plus a peeked pane, + * and `itemId` alone would let both consume the same event. Empty + * (the default, for editors mounted outside an ItemDetail) disables + * panel addressing rather than broadcasting. + */ + hostToken?: string; /** * Optional Yjs document to bind this editor to via the Tiptap * Collaboration extension (PLAN-1248). When set, the y-tiptap @@ -914,6 +925,12 @@ AttachmentImage.configure({ getDownloadUrl: getAttachmentUrl, workspaceSlug: wsSlug, + // Panel / viewer addressing (PLAN-2392 DR-8). Read once at + // editor construction, which is correct: both are fixed for + // the life of a mount — ItemDetail remounts this editor per + // item ({#key item.id}) and mints one token per ItemDetail. + itemId: itemId ?? '', + hostToken, // Initial supportedFormats is empty — server capabilities // are fetched async below. The toolbar starts disabled // for all formats until capabilities resolve, then @@ -934,7 +951,12 @@ } }, }), - AttachmentChip.configure({ getDownloadUrl: getAttachmentUrl, workspaceSlug: wsSlug }), + AttachmentChip.configure({ + getDownloadUrl: getAttachmentUrl, + workspaceSlug: wsSlug, + itemId: itemId ?? '', + hostToken, + }), // When a Y.Doc is provided, register the Collaboration // extension so the y-tiptap binding takes over document // state. Without ydoc this slot is empty and the editor diff --git a/web/src/lib/components/editor/attachment-chip.ts b/web/src/lib/components/editor/attachment-chip.ts index 89e40caf..5758537d 100644 --- a/web/src/lib/components/editor/attachment-chip.ts +++ b/web/src/lib/components/editor/attachment-chip.ts @@ -56,6 +56,20 @@ export interface AttachmentChipOptions { getDownloadUrl: AttachmentUrlBuilder; /** Workspace slug used by the metadata HEAD fetcher. Empty disables the fetch. */ workspaceSlug: string; + /** + * UUID of the item this editor is editing. Half of the open-panel event's + * address (PLAN-2392 DR-8). Empty when the editor has no item context — + * the chip then can't address a panel host and stays on its plain + * behaviour. + */ + itemId: string; + /** + * Identity of the `ItemDetail` mount that owns this editor — the other + * half of the address. `ItemDetail` is mounted more than once at a time + * (master + peeked pane), so `itemId` alone would let both hosts consume + * one chip's event. Empty disables panel addressing (DR-8). + */ + hostToken: string; } declare module '@tiptap/core' { @@ -84,6 +98,8 @@ export const AttachmentChip = Node.create({ HTMLAttributes: {}, getDownloadUrl: (uuid: string) => `${PAD_ATTACHMENT_PREFIX}${uuid}`, workspaceSlug: '', + itemId: '', + hostToken: '', }; }, diff --git a/web/src/lib/components/editor/attachment-image.ts b/web/src/lib/components/editor/attachment-image.ts index 175bc509..ffca970b 100644 --- a/web/src/lib/components/editor/attachment-image.ts +++ b/web/src/lib/components/editor/attachment-image.ts @@ -102,6 +102,19 @@ export interface AttachmentImageOptions { * shows but skips per-format gating. */ workspaceSlug: string; + /** + * UUID of the item this editor is editing. Half of the panel / viewer + * event address (PLAN-2392 DR-8). Empty when the editor has no item + * context. + */ + itemId: string; + /** + * Identity of the `ItemDetail` mount that owns this editor — the other + * half of the address. `ItemDetail` is mounted more than once at a time + * (master + peeked pane), so `itemId` alone would let both hosts consume + * one NodeView's event. Empty disables addressing (DR-8). + */ + hostToken: string; /** * Image formats the server-side processor supports. Drives the * rotate toolbar's enabled state per attachment: a button is @@ -161,6 +174,8 @@ export const AttachmentImage = Node.create({ HTMLAttributes: {}, getDownloadUrl: (uuid: string) => `${PAD_ATTACHMENT_PREFIX}${uuid}`, workspaceSlug: '', + itemId: '', + hostToken: '', supportedFormats: [] as string[], transform: async () => { throw new Error('AttachmentImage: configure({ transform }) is required to use rotate/crop'); diff --git a/web/src/lib/components/items/ItemAttachmentStrip.svelte b/web/src/lib/components/items/ItemAttachmentStrip.svelte index c89d52a4..2c00b42b 100644 --- a/web/src/lib/components/items/ItemAttachmentStrip.svelte +++ b/web/src/lib/components/items/ItemAttachmentStrip.svelte @@ -79,6 +79,15 @@ * (Codex round 2). Consulted at confirm time only. */ liveContent?: (() => string | null) | null; + /** + * Identity of the `ItemDetail` mount that owns this strip + * (PLAN-2392 DR-8 / TASK-2421). The strip is an EMITTER on the + * open-panel channel, and the channel is module-global while + * ItemDetail is mounted more than once (master + peeked pane) — so a + * tile's event has to name its host, not just its item. Empty + * disables addressing rather than broadcasting to every host. + */ + hostToken?: string; } let { wsSlug, @@ -87,6 +96,7 @@ canDelete = false, itemContent = null, liveContent = null, + hostToken = '', }: Props = $props(); // Hard bound on what the strip will ever hold (DR-9 / DR-11). Past this the diff --git a/web/src/lib/components/items/ItemDetail.svelte b/web/src/lib/components/items/ItemDetail.svelte index c3263416..26dc11a9 100644 --- a/web/src/lib/components/items/ItemDetail.svelte +++ b/web/src/lib/components/items/ItemDetail.svelte @@ -42,6 +42,7 @@ import ShareDialog from '$lib/components/ShareDialog.svelte'; import CopyItemDialog from '$lib/components/items/CopyItemDialog.svelte'; import ItemAttachmentStrip from '$lib/components/items/ItemAttachmentStrip.svelte'; + import { createAttachmentHostToken } from '$lib/attachments/events'; import { copyToClipboard } from '$lib/utils/clipboard'; import { repairDeadItemLastRoute } from '$lib/collections/paneUrlParams'; import { isSamePaneTarget, breadcrumbParentTarget } from '$lib/collections/paneTarget'; @@ -772,6 +773,22 @@ // (unit-tested). `peeking` defaults false → `mutationsEnabled === canEdit` for // every non-host caller (byte-identical). let mutationsEnabled = $derived(computeMutationsEnabled(canEdit, peeking)); + + // PLAN-2392 DR-8 (TASK-2421): this mount's identity on the module-global + // attachment event bus. The pane host mounts ItemDetail MORE THAN ONCE at + // a time (a master plus a peeked pane), so an open-panel event addressed + // only by `itemId` would be consumed by BOTH — two panels for one tap, and + // one of them permissioned by the wrong host's `mutationsEnabled`. + // + // ONE token per host, not one per component: it is passed to every + // attachment surface this host owns — the strip, the body Editor, and + // every CommentEditor under ItemTimeline — so all of them address THIS + // mount and nothing else does. Deliberately a plain `const`, not `$state` + // or `$derived`: it must be stable for the whole mount, including across + // the no-{#key} A→B item switch this pane is built around. (The `itemId` + // half of the address changes with the item; the token does not, and does + // not need to — the pair is what disambiguates.) + const attachmentHostToken = createAttachmentHostToken(); $effect(() => { if (wsSlug && collSlug && itemSlug) { loadData(); @@ -5012,6 +5029,7 @@ {wsSlug} {username} itemId={itemMatchesRef ? item?.id : null} + hostToken={attachmentHostToken} canDelete={mutationsEnabled} itemContent={itemMatchesRef ? item?.content : null} liveContent={() => { @@ -5276,6 +5294,7 @@ onUpdate={handleContentUpdate} editable={false} itemId={item.id} + hostToken={attachmentHostToken} onEditor={(e) => editorInstance = e} onImportInserted={handleImportInserted} /> @@ -5333,6 +5352,7 @@ onUpdate={handleContentUpdate} editable={!peeking} itemId={item.id} + hostToken={attachmentHostToken} ydoc={ydoc} awareness={collabProvider?.awareness} collabUser={collabUserState} @@ -5535,6 +5555,7 @@ onRestore={handleVersionRestore} flushBeforeRestore={flushCollabBeforeRestore} itemId={item.id} + hostToken={attachmentHostToken} collectionId={item.collection_id} frozen={false} restoreFrozen={peeking} diff --git a/web/src/lib/components/timeline/ItemTimeline.svelte b/web/src/lib/components/timeline/ItemTimeline.svelte index 23a9a357..b6b47717 100644 --- a/web/src/lib/components/timeline/ItemTimeline.svelte +++ b/web/src/lib/components/timeline/ItemTimeline.svelte @@ -68,9 +68,20 @@ * callers, so existing usage is unaffected. */ flushBeforeRestore?: () => Promise; + /** + * Identity of the `ItemDetail` mount that owns this timeline + * (PLAN-2392 DR-8 / TASK-2421). Forwarded verbatim to every + * CommentEditor this timeline mounts — the composer here, and the + * edit/reply composers inside TimelineCommentCard — so an attachment + * chip in a comment body can address the ONE host that owns it. A + * master and a peeked pane are both mounted on the same module-global + * bus, so `itemId` alone is not an address. Empty (the default, for + * callers outside an ItemDetail) disables addressing. + */ + hostToken?: string; } - let { wsSlug, username = '', itemSlug, currentContent, items = [], onRestore, itemId, collectionId, frozen = false, restoreFrozen = false, flushBeforeRestore, visibleKinds }: Props = $props(); + let { wsSlug, username = '', itemSlug, currentContent, items = [], onRestore, itemId, collectionId, frozen = false, restoreFrozen = false, flushBeforeRestore, visibleKinds, hostToken = '' }: Props = $props(); // Resolve canEditItem reactively; falls to false if itemId/collectionId // aren't supplied (e.g. an older caller). Folds in the master-freeze gate @@ -471,6 +482,7 @@ void; onReply: (commentId: string, body: string) => void | Promise; /** Edits a comment/reply body. Should throw on failure so the editor keeps the draft. */ @@ -46,7 +54,7 @@ onRemoveReaction: (commentId: string, emoji: string) => void; } - let { comment, wsSlug, username = '', items, currentUserId = '', canEdit = true, frozen = false, attachmentResolver, isAdmin = false, onDelete, onReply, onEdit, onReaction, onRemoveReaction }: Props = $props(); + let { comment, wsSlug, username = '', items, currentUserId = '', canEdit = true, frozen = false, attachmentResolver, isAdmin = false, hostToken = '', onDelete, onReply, onEdit, onReaction, onRemoveReaction }: Props = $props(); let showReplyForm = $state(false); let submittingReply = $state(false); @@ -222,6 +230,7 @@ Date: Tue, 4 Aug 2026 00:46:40 +0000 Subject: [PATCH 03/22] 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} diff --git a/web/src/lib/components/attachments/AttachmentPanelHost.svelte b/web/src/lib/components/attachments/AttachmentPanelHost.svelte new file mode 100644 index 00000000..9259cefb --- /dev/null +++ b/web/src/lib/components/attachments/AttachmentPanelHost.svelte @@ -0,0 +1,143 @@ + + + + +{#if request} + (request = null)} + /> +{/if} diff --git a/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts b/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts new file mode 100644 index 00000000..144b8c8d --- /dev/null +++ b/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts @@ -0,0 +1,511 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { flushSync, mount, unmount } from 'svelte'; +import type { AttachmentMetadataResult } from '$lib/components/editor/attachment-metadata'; + +// TASK-2423. The options panel is exercised THROUGH its host, because the host +// is where the two rules that matter live: an event is consumed only when both +// `itemId` and `hostToken` are this host's own (DR-8), and the permission the +// panel's Delete uses comes from the host rather than from the emitting +// surface. Nothing routes INTO the panel yet — the strip's tiles and the +// editor's chips start emitting in the next task — so these tests emit on the +// bus directly, which is also the only way to drive a NodeView-originated open. +// +// What jsdom CANNOT prove here, and is therefore phase 3d's browser suite: +// focus entry/return, background inertness, the desktop popover's real +// placement, the mobile sheet swap, and Enter/Space activation of the rows. + +const deleteMock = vi.fn<(ws: string, id: string) => Promise>(); +const toastMock = vi.fn<(message: string, kind?: string) => void>(); + +class FakeApiError extends Error { + code: string; + constructor(code: string) { + super(code); + this.code = code; + } +} + +vi.mock('$lib/api/client', () => ({ + PadApiError: FakeApiError, + api: { + attachments: { + downloadUrl: (ws: string, id: string, variant?: string) => + `/api/v1/workspaces/${ws}/attachments/${id}${variant ? `?variant=${variant}` : ''}`, + delete: (ws: string, id: string) => deleteMock(ws, id), + }, + }, +})); + +// The metadata cache is mocked so a test can hand back each arm of the typed +// result (DR-10) and, crucially, assert WHICH entry point was used: Retry must +// go through `revalidate*` (invalidate-then-fetch), because a plain refetch +// replays the cached failure and looks broken. +const fetchMetaMock = vi.fn<() => Promise>(); +const revalidateMetaMock = vi.fn<() => Promise>(); +const invalidateMetaMock = vi.fn<(ws: string, id: string) => void>(); +vi.mock('$lib/components/editor/attachment-metadata', () => ({ + fetchAttachmentMetadata: () => fetchMetaMock(), + revalidateAttachmentMetadata: () => revalidateMetaMock(), + invalidateAttachmentMetadata: (ws: string, id: string) => invalidateMetaMock(ws, id), +})); + +vi.mock('$lib/stores/toast.svelte', () => ({ + toastStore: { show: (message: string, kind?: string) => toastMock(message, kind) }, +})); + +// The events bus stays REAL — addressing is the thing under test — with only +// the deletion broadcast wrapped so a test can assert the panel announces +// exactly as the strip's tile does. +const announceMock = vi.fn<(ws: string, id: string) => void>(); +vi.mock('$lib/attachments/events', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + announceAttachmentDeleted: (ws: string, id: string) => announceMock(ws, id), + }; +}); + +const { notifyAttachmentPanelOpen } = await import('$lib/attachments/events'); +const { default: AttachmentPanelHost } = await import('./AttachmentPanelHost.svelte'); + +interface HostProps { + wsSlug: string; + itemId: string | null; + hostToken: string; + mutationsEnabled: boolean; + itemContent: string | null; + liveContent: (() => string | null) | null; + parentArchived: boolean; +} + +// A canonical UUID: `attachmentRefsIn` only recognizes the 36-char form, so +// the "still used in this item's content" warning is only exercised with a +// real id. +const ATT_ID = '11111111-2222-4333-8444-555555555555'; +const ATT_ID_2 = '99999999-8888-4777-8666-555555555555'; + +function openEvent(overrides: Partial[0]> = {}) { + return { + attachmentId: ATT_ID, + itemId: 'item-a', + hostToken: 'host-1', + anchor: null, + filename: 'spec.pdf', + mime_type: 'application/pdf', + size_bytes: 1536, + ...overrides, + }; +} + +/** Rows are portaled to , so queries are document-wide by necessity. */ +function panel(): HTMLElement | null { + return document.querySelector('[role="menu"]'); +} + +function rows(): HTMLElement[] { + return Array.from(document.querySelectorAll('[role="menu"] [role="menuitem"]')); +} + +/** By VISIBLE label — MenuItem's icon span is part of `textContent`. */ +function row(label: string): HTMLElement | undefined { + return rows().find((el) => el.querySelector('.mi-label')?.textContent?.trim() === label); +} + +async function settle() { + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + flushSync(); +} + +// Reactive props objects, declared at the top level because `$state(...)` may +// only initialize a declaration. Two of them: the pane host runs a master and a +// peeked ItemDetail at once, and that concurrency is exactly what DR-8's +// addressing exists for. +const propsA = $state({ + wsSlug: 'ws', + itemId: 'item-a', + hostToken: 'host-1', + mutationsEnabled: true, + itemContent: null, + liveContent: null, + parentArchived: false, +}); +const propsB = $state({ + wsSlug: 'ws', + itemId: 'item-a', + hostToken: 'host-2', + mutationsEnabled: false, + itemContent: null, + liveContent: null, + parentArchived: false, +}); + +describe('AttachmentPanelHost', () => { + let target: HTMLElement; + const mounted: ReturnType[] = []; + + beforeEach(() => { + deleteMock.mockReset(); + deleteMock.mockResolvedValue(undefined); + toastMock.mockReset(); + announceMock.mockReset(); + fetchMetaMock.mockReset(); + fetchMetaMock.mockResolvedValue({ status: 'ok', mime: 'application/pdf', size: 2048 }); + revalidateMetaMock.mockReset(); + revalidateMetaMock.mockResolvedValue({ status: 'ok', mime: 'application/pdf', size: 2048 }); + invalidateMetaMock.mockReset(); + Object.assign(propsA, { + wsSlug: 'ws', + itemId: 'item-a', + hostToken: 'host-1', + mutationsEnabled: true, + itemContent: null, + liveContent: null, + parentArchived: false, + }); + Object.assign(propsB, { + wsSlug: 'ws', + itemId: 'item-a', + hostToken: 'host-2', + mutationsEnabled: false, + itemContent: null, + liveContent: null, + parentArchived: false, + }); + target = document.body.appendChild(document.createElement('div')); + }); + + afterEach(() => { + while (mounted.length) unmount(mounted.pop()!); + target.remove(); + }); + + function mountHost(props: HostProps) { + mounted.push(mount(AttachmentPanelHost, { target, props })); + flushSync(); + } + + it('opens for an event addressed to it', () => { + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + flushSync(); + + expect(panel()).not.toBeNull(); + expect(panel()?.textContent).toContain('spec.pdf'); + }); + + it('ignores an event addressed to the OTHER host, with both mounted', () => { + mountHost(propsA); + mountHost(propsB); + + // Same item, other host token: only one panel may open, and it must be + // the addressed one. Matching on itemId alone would open two. + notifyAttachmentPanelOpen(openEvent({ hostToken: 'host-2' })); + flushSync(); + + const panels = document.querySelectorAll('[role="menu"]'); + expect(panels).toHaveLength(1); + // host-2 is the peeked pane in this fixture (mutationsEnabled false), so + // the panel that opened must be the one WITHOUT a live Delete. + expect((row('Delete') as HTMLButtonElement | undefined)?.disabled).toBe(true); + }); + + it('ignores an event for a different item on its own token', () => { + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent({ itemId: 'item-b' })); + flushSync(); + + expect(panel()).toBeNull(); + }); + + it('renders the actions from the shared descriptor list, honouring element', async () => { + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + + // PDF previews natively, so Open applies; both anchors are real + // / elements, not buttons that navigate. + const open = row('Open in new tab'); + expect(open?.tagName).toBe('A'); + expect(open?.getAttribute('target')).toBe('_blank'); + expect(open?.getAttribute('rel')).toBe('noopener noreferrer'); + const download = row('Download'); + expect(download?.tagName).toBe('A'); + expect(download?.getAttribute('href')).toBe(`/api/v1/workspaces/ws/attachments/${ATT_ID}`); + expect(download?.getAttribute('download')).toBe('spec.pdf'); + expect(row('Copy workspace link')?.tagName).toBe('BUTTON'); + expect(row('Delete')?.tagName).toBe('BUTTON'); + }); + + it('omits Open for a type the browser cannot preview', async () => { + mountHost(propsA); + notifyAttachmentPanelOpen( + openEvent({ mime_type: 'application/zip', filename: 'logs.zip' }) + ); + await settle(); + + // Absent, never disabled: a greyed Open implies a preview Pad could + // give and won't. + expect(row('Open in new tab')).toBeUndefined(); + expect(row('Download')).toBeDefined(); + expect(panel()?.textContent).toContain('ZIP archive'); + }); + + it('opens IMMEDIATELY with partial metadata, then completes it', async () => { + let resolveMeta!: (r: AttachmentMetadataResult) => void; + fetchMetaMock.mockReturnValue( + new Promise((r) => (resolveMeta = r)) + ); + mountHost(propsA); + // A chip's HEAD probe may not have completed: all three fields null. + notifyAttachmentPanelOpen( + openEvent({ filename: null, mime_type: null, size_bytes: null }) + ); + flushSync(); + + // Painted before the fetch settles — never a blank sheet, never a wait. + expect(panel()).not.toBeNull(); + expect(panel()?.textContent).toContain('Attachment'); + expect(panel()?.textContent).toContain('Reading details…'); + + resolveMeta({ status: 'ok', mime: 'application/pdf', size: 1024 }); + await settle(); + expect(panel()?.textContent).toContain('PDF'); + expect(panel()?.textContent).toContain('1.0 KB'); + }); + + it('does not fetch when the event carried all three fields', async () => { + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + + // The strip's entry point always has them from its list row. + expect(fetchMetaMock).not.toHaveBeenCalled(); + expect(panel()?.textContent).toContain('1.5 KB'); + }); + + it('shows an inline retryable error on a transient failure, keeping what it knows', async () => { + fetchMetaMock.mockResolvedValue({ status: 'transient' }); + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent({ mime_type: null, size_bytes: null })); + await settle(); + + expect(panel()?.textContent).toContain("Couldn't load the file details."); + // Beside the row it already knows, not instead of it. + expect(panel()?.textContent).toContain('spec.pdf'); + // Actions stay live: transient says NOTHING about whether the row exists. + expect((row('Download') as HTMLElement).tagName).toBe('A'); + + revalidateMetaMock.mockResolvedValue({ status: 'ok', mime: 'application/pdf', size: 4096 }); + row('Retry')!.click(); + await settle(); + + // Retry INVALIDATES before refetching — a plain refetch would replay the + // cached failure (DR-10). + expect(revalidateMetaMock).toHaveBeenCalledTimes(1); + expect(panel()?.textContent).not.toContain("Couldn't load the file details."); + expect(panel()?.textContent).toContain('4.0 KB'); + }); + + it('latches an authoritative missing state and makes every action inert', async () => { + fetchMetaMock.mockResolvedValue({ status: 'missing' }); + mountHost(propsA); + // Size unknown, so the panel fetches; the MIME is known, so Open is in + // the rendered set and its inertness is observable too. + notifyAttachmentPanelOpen(openEvent({ size_bytes: null })); + await settle(); + + expect(panel()?.textContent).toContain('This file is no longer available.'); + expect(panel()?.textContent).toContain('No longer available'); + // A disabled anchor is not a thing, so MenuItem falls back to a disabled + // button — the row is inert AND skipped by the menu's keyboard walk. + for (const label of ['Open in new tab', 'Download', 'Copy workspace link', 'Delete']) { + const el = row(label) as HTMLButtonElement | undefined; + expect(el?.tagName).toBe('BUTTON'); + expect(el?.disabled).toBe(true); + } + }); + + it('takes Delete permission from the HOST: peeked pane cannot, master can', async () => { + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + expect((row('Delete') as HTMLButtonElement).disabled).toBe(false); + + // Peek freezes this side; the event said nothing about permission and + // must not be able to. + propsA.mutationsEnabled = false; + flushSync(); + expect((row('Delete') as HTMLButtonElement).disabled).toBe(true); + }); + + it('deletes through an in-app drill-down confirmation, Cancel first', async () => { + propsA.itemContent = `body with ![x](pad-attachment:${ATT_ID}) inline`; + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + + row('Delete')!.click(); + await settle(); + + // The confirmation is a sub-view of the panel, not a window.confirm. + const prompt = document.querySelector('.ap-note-warn'); + expect(prompt?.getAttribute('role')).toBe('presentation'); + expect(prompt?.textContent).toContain("still used in this item's content"); + const confirmRows = rows(); + const labelOf = (el: HTMLElement) => el.querySelector('.mi-label')?.textContent?.trim(); + expect(labelOf(confirmRows[0])).toBe('Cancel'); + expect(labelOf(confirmRows[confirmRows.length - 1])).toBe('Delete file'); + // The destructive row points back at the prompt, which is otherwise + // never announced. + expect(confirmRows[confirmRows.length - 1].getAttribute('aria-describedby')).toBe( + prompt?.id + ); + expect(deleteMock).not.toHaveBeenCalled(); + + row('Delete file')!.click(); + await settle(); + + expect(deleteMock).toHaveBeenCalledWith('ws', ATT_ID); + // Exactly what the tile does, so the strip and the editor reconcile. + expect(announceMock).toHaveBeenCalledWith('ws', ATT_ID); + expect(panel()).toBeNull(); + }); + + it('warns honestly when the attachment is not referenced in this body', async () => { + propsA.itemContent = 'nothing here'; + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + + row('Delete')!.click(); + await settle(); + expect(document.querySelector('.ap-note-warn')?.textContent).toContain( + "isn't referenced in this item's content" + ); + }); + + it('reads the LIVE editor markdown for the in-use warning, not just the saved body', async () => { + // The persisted body lags the editor, so an image inserted seconds ago + // would otherwise slip past the warning. + propsA.itemContent = 'nothing here'; + propsA.liveContent = () => `just pasted ![x](pad-attachment:${ATT_ID})`; + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + + row('Delete')!.click(); + await settle(); + expect(document.querySelector('.ap-note-warn')?.textContent).toContain( + "still used in this item's content" + ); + }); + + it('cancelling the confirmation sends no request and returns to the actions', async () => { + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + + row('Delete')!.click(); + await settle(); + row('Cancel')!.click(); + await settle(); + + expect(deleteMock).not.toHaveBeenCalled(); + expect(row('Download')).toBeDefined(); + expect(panel()).not.toBeNull(); + }); + + it('surfaces a failed delete inline and leaves the panel open', async () => { + deleteMock.mockRejectedValue(new Error('Network unreachable')); + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + + row('Delete')!.click(); + await settle(); + row('Delete file')!.click(); + await settle(); + + expect(panel()?.textContent).toContain('Network unreachable'); + expect(announceMock).not.toHaveBeenCalled(); + }); + + it('closes an open panel when the parent item is archived (DR-14)', async () => { + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + expect(panel()).not.toBeNull(); + + // An archived parent's attachment fetch returns a generic 404, so an + // open panel would keep offering an Open and a Download that both fail. + // The strip sits outside ItemDetail's keyed lifecycle block, so this is + // added, not inherited. + propsA.parentArchived = true; + await settle(); + expect(panel()).toBeNull(); + }); + + 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. + propsA.parentArchived = true; + fetchMetaMock.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. + 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'); + }); + + it('closes when the host switches item', async () => { + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + expect(panel()).not.toBeNull(); + + propsA.itemId = 'item-b'; + await settle(); + expect(panel()).toBeNull(); + }); + + it('re-targets in place when a second attachment is opened, dropping the first state', async () => { + fetchMetaMock.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.'); + + // The panel is NOT re-keyed per attachment, so the previous subject's + // latched state has to be cleared explicitly. + notifyAttachmentPanelOpen( + openEvent({ attachmentId: ATT_ID_2, filename: 'notes.txt', mime_type: 'text/plain', size_bytes: 12 }) + ); + await settle(); + expect(panel()?.textContent).not.toContain('This file is no longer available.'); + expect(panel()?.textContent).toContain('notes.txt'); + expect((row('Download') as HTMLElement).getAttribute('href')).toBe( + `/api/v1/workspaces/ws/attachments/${ATT_ID_2}` + ); + }); + + it('keeps the full filename in the accessible name while the visible row truncates', async () => { + const long = `${'a'.repeat(200)}.pdf`; + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent({ filename: long })); + await settle(); + + // Truncation is a visual affordance, never an information loss (DR-13). + expect(panel()?.getAttribute('aria-label')).toContain(long); + expect(document.querySelector('.ap-name')?.getAttribute('title')).toBe(long); + }); +}); diff --git a/web/src/lib/components/items/ItemDetail.svelte b/web/src/lib/components/items/ItemDetail.svelte index 26dc11a9..48bce3bd 100644 --- a/web/src/lib/components/items/ItemDetail.svelte +++ b/web/src/lib/components/items/ItemDetail.svelte @@ -42,6 +42,7 @@ import ShareDialog from '$lib/components/ShareDialog.svelte'; import CopyItemDialog from '$lib/components/items/CopyItemDialog.svelte'; import ItemAttachmentStrip from '$lib/components/items/ItemAttachmentStrip.svelte'; + import AttachmentPanelHost from '$lib/components/attachments/AttachmentPanelHost.svelte'; import { createAttachmentHostToken } from '$lib/attachments/events'; import { copyToClipboard } from '$lib/utils/clipboard'; import { repairDeadItemLastRoute } from '$lib/collections/paneUrlParams'; @@ -789,6 +790,27 @@ // half of the address changes with the item; the token does not, and does // not need to — the pair is what disambiguates.) const attachmentHostToken = createAttachmentHostToken(); + + /** + * The editor's LIVE markdown, or null when there is no live editor to + * read. Consumed by every attachment surface that warns "this file is + * still used in this item's content" — the strip's tile delete and the + * options panel's (TASK-2423). + * + * The persisted `item.content` lags the editor by design (written on + * flush, not per keystroke), so an image inserted moments ago wouldn't + * trip that warning for exactly the attachment a user is most likely to + * delete by mistake. Callers fall back to `item.content` when this + * returns null. + */ + function liveEditorMarkdown(): string | null { + if (!editorInstance || editorInstance.isDestroyed) return null; + try { + return (editorInstance.storage as any).markdown?.getMarkdown?.() ?? null; + } catch { + return null; + } + } $effect(() => { if (wsSlug && collSlug && itemSlug) { loadData(); @@ -5032,19 +5054,31 @@ hostToken={attachmentHostToken} canDelete={mutationsEnabled} itemContent={itemMatchesRef ? item?.content : null} - liveContent={() => { - // The persisted item.content lags the editor by design (it's - // written on flush, not per keystroke), so an image inserted - // moments ago wouldn't trip the "still used" warning. Read the - // live editor when it's genuinely alive; the strip falls back - // to item.content otherwise. - if (!editorInstance || editorInstance.isDestroyed) return null; - try { - return (editorInstance.storage as any).markdown?.getMarkdown?.() ?? null; - } catch { - return null; - } - }} + liveContent={liveEditorMarkdown} + /> + + + +
- {#if isImage(att.mime_type)} + {#if canOpenInViewer(att.mime_type)} {:else} - , not an (DR-1 / DR-12). + Tapping a file opens its options panel; nothing is + downloaded until the user picks Download there. + + Deliberately a native button rather than an anchor with + an overridden activation: the UA gives us Enter AND + Space, Space's page-scroll already suppressed, and + EXACTLY ONE `click` per activation from either key — + which a hand-rolled keydown handler alongside a click + handler is precisely how you get twice (DR-12). + --> + {/if} {#if canDelete} @@ -994,6 +1060,12 @@ text-decoration: none; overflow: hidden; cursor: pointer; + /* Both tiles are @@ -922,6 +987,43 @@ {/if} + +{#if pendingDelete} + + + + +{/if} + {#if lightbox} { }); - // ── Delete (TASK-2384) ──────────────────────────────────────────────── + // ── Delete (TASK-2384, confirmation reworked in TASK-2425) ──────────── // // The affordance is gated on ItemDetail's `mutationsEnabled` // (canEdit && !peeking) per PLAN-2382 DR-6, and the confirm text has to // stay honest about what was actually checked (DR-5): "referenced in this // item's content" is knowable client-side; "unused anywhere" is not. + // + // TASK-2425 (PLAN-2392 DR-18) replaced the browser-native `window.confirm` + // these tests used to spy on with the SAME in-app drill-down the options + // panel shows — so they now drive the real rows. That is not a cosmetic + // change to the tests: the native confirm blocked the thread, so nothing + // could move between the entry fence and the request, while the in-app one + // leaves a window in which the user can switch item or workspace. Every + // fence and rollback assertion below is preserved, and the confirmation is + // driven through the rows a user would actually click. function deleteButtons(): HTMLButtonElement[] { return Array.from(target.querySelectorAll('.att-delete')); } + /** Portaled to like every other Menu, so queried document-wide. */ + function confirmPanel(): HTMLElement | null { + return document.querySelector('[role="menu"]'); + } + + function confirmRows(): HTMLElement[] { + return Array.from(document.querySelectorAll('[role="menu"] [role="menuitem"]')); + } + + /** By VISIBLE label — MenuItem's icon span is part of `textContent`. */ + function confirmRow(label: string): HTMLElement | undefined { + return confirmRows().find( + (el) => el.querySelector('.mi-label')?.textContent?.trim() === label + ); + } + + function promptText(): string { + return document.querySelector('.attachment-delete-prompt')?.textContent ?? ''; + } + + /** Click a tile's `×`. Opens the confirmation; sends nothing. */ + function openConfirm(index = 0) { + deleteButtons()[index].click(); + flushSync(); + } + + /** The destructive row — the only thing that issues a DELETE. */ + function clickConfirm() { + confirmRow('Delete file')!.click(); + flushSync(); + } + + function clickCancel() { + confirmRow('Cancel')!.click(); + flushSync(); + } + it('offers no delete control when canDelete is false', async () => { listMock.mockResolvedValue(response([att({ id: 'a1' })])); props.canDelete = false; @@ -863,6 +909,54 @@ describe('ItemAttachmentStrip', () => { expect(buttons[0].disabled).toBe(false); }); + it('confirms in-app, never with a browser dialog, Cancel first (DR-18)', async () => { + // The shape the item menu establishes and the options panel already + // used: prompt as `role="presentation"` (a role="menu" owns only + // menuitem / separator / group children), an aria-describedby + // back-reference from the destructive row so the otherwise-unannounced + // prompt is read out, Cancel FIRST so the menu's focus handoff can + // never land Enter on Delete. + const nativeConfirm = vi.spyOn(window, 'confirm').mockReturnValue(true); + listMock.mockResolvedValue(response([att({ id: 'a1' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + openConfirm(); + + expect(nativeConfirm).not.toHaveBeenCalled(); + const prompt = document.querySelector('.attachment-delete-prompt'); + expect(prompt?.getAttribute('role')).toBe('presentation'); + const rows = confirmRows(); + const labelOf = (el: HTMLElement) => el.querySelector('.mi-label')?.textContent?.trim(); + expect(labelOf(rows[0])).toBe('Cancel'); + expect(labelOf(rows[rows.length - 1])).toBe('Delete file'); + expect(rows[rows.length - 1].getAttribute('aria-describedby')).toBe(prompt?.id); + // Opening the confirmation is not a delete. + expect(deleteMock).not.toHaveBeenCalled(); + nativeConfirm.mockRestore(); + }); + + it('cancelling sends nothing, keeps the tile, and refocuses the × control', async () => { + listMock.mockResolvedValue(response([att({ id: 'a1' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + const closeBtn = deleteButtons()[0]; + openConfirm(); + clickCancel(); + await settle(); + + expect(deleteMock).not.toHaveBeenCalled(); + expect(tiles()).toHaveLength(1); + expect(confirmPanel()).toBeNull(); + // `window.confirm` restored focus for free. The control is also + // opacity-hidden unless its cell has focus-within, so dropping focus to + // would make the affordance vanish under a keyboard user. + expect(document.activeElement).toBe(closeBtn); + }); + it('warns that the attachment is still used in this item content', async () => { // A canonical UUID: attachmentRefsIn() is anchored to that shape (the // ids the upload endpoint returns), so the reference scan only matches @@ -874,16 +968,14 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); - deleteButtons()[0].click(); - await settle(); + openConfirm(); - expect(confirmSpy).toHaveBeenCalledOnce(); - expect(confirmSpy.mock.calls[0][0]).toContain("still used in this item's content"); + expect(promptText()).toContain("still used in this item's content"); // Declined → nothing deleted, tile stays. + clickCancel(); + await settle(); expect(deleteMock).not.toHaveBeenCalled(); expect(tiles()).toHaveLength(1); - confirmSpy.mockRestore(); }); it('never claims an unreferenced attachment is unused', async () => { @@ -893,16 +985,13 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); - deleteButtons()[0].click(); - await settle(); + openConfirm(); - const message = String(confirmSpy.mock.calls[0][0]); + const message = promptText(); // Comment bodies and other items are NOT scanned client-side (DR-5), // so the copy must hedge rather than assert non-use. expect(message).toContain('may still be referenced'); expect(message).not.toContain('not used'); - confirmSpy.mockRestore(); }); it('removes the tile optimistically and calls the API on confirm', async () => { @@ -911,19 +1000,62 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(deleteMock).toHaveBeenCalledWith('ws', 'a1'); expect(tiles()).toHaveLength(1); expect(toastMock).not.toHaveBeenCalled(); + // The confirmation goes with the row it was asking about. + expect(confirmPanel()).toBeNull(); // An already painted in the editor never re-requests, so the // NodeView has to be told or the body keeps showing a deleted image // until reload (Codex round 12). expect(notifyDeletedMock).toHaveBeenCalledWith('a1'); expect(invalidateMock).toHaveBeenCalledWith('ws', 'a1'); - confirmSpy.mockRestore(); + }); + + it('abandons an open confirmation when the item switches under it', async () => { + // The in-app confirmation does NOT block the thread the way + // `window.confirm` did, so this window exists at all only as of + // TASK-2425: the prompt can still be up when the strip repaints for a + // different item. Leaving it there would delete the PREVIOUS item's + // attachment from behind the new one. + listMock.mockResolvedValueOnce(response([att({ id: 'a1' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + openConfirm(); + expect(confirmPanel()).not.toBeNull(); + + listMock.mockResolvedValueOnce(response([att({ id: 'b1' })])); + props.itemId = 'item-b'; + flushSync(); + await settle(); + + expect(confirmPanel()).toBeNull(); + expect(deleteMock).not.toHaveBeenCalled(); + }); + + it('drops an open confirmation when another surface deletes that row', async () => { + // The tile the confirmation is anchored to has just been unmounted, so + // the menu would be left pointing at a detached element — and the + // question it is asking has already been answered. + listMock.mockResolvedValue(response([att({ id: 'a1' }), att({ id: 'a2' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + openConfirm(); + expect(confirmPanel()).not.toBeNull(); + + broadcastDeletion('a1'); + flushSync(); + + expect(confirmPanel()).toBeNull(); + expect(deleteMock).not.toHaveBeenCalled(); }); it('refuses a delete click that lands after the ITEM already switched', async () => { @@ -940,18 +1072,16 @@ describe('ItemAttachmentStrip', () => { expect(deleteButtons()).toHaveLength(1); listMock.mockResolvedValueOnce(response([att({ id: 'b1' })])); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); props.itemId = 'item-b'; // No flushSync between the switch and the click: that IS the window. deleteButtons()[0].click(); await settle(); // Not even prompted — the tile the user aimed at no longer exists. - expect(confirmSpy).not.toHaveBeenCalled(); + expect(confirmPanel()).toBeNull(); expect(deleteMock).not.toHaveBeenCalled(); expect(notifyDeletedMock).not.toHaveBeenCalled(); expect(tiles()[0].getAttribute('aria-label')).toContain('b1.png'); - confirmSpy.mockRestore(); }); it('refuses a delete click that lands after the WORKSPACE already switched', async () => { @@ -964,15 +1094,37 @@ describe('ItemAttachmentStrip', () => { await settle(); listMock.mockResolvedValueOnce(response([att({ id: 'ws2-row' })])); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); props.wsSlug = 'ws2'; deleteButtons()[0].click(); await settle(); - expect(confirmSpy).not.toHaveBeenCalled(); + expect(confirmPanel()).toBeNull(); expect(deleteMock).not.toHaveBeenCalled(); expect(tiles()[0].getAttribute('aria-label')).toContain('ws2-row.png'); - confirmSpy.mockRestore(); + }); + + it('refuses a CONFIRMATION that lands after the item already switched', async () => { + // The window `window.confirm` did not have: it blocked the thread, so + // the entry fence taken when the `×` was clicked was still true by + // definition when it returned. An in-app confirmation can sit on screen + // across a switch, so the fence is re-checked at the point that + // actually sends the request (TASK-2425). + listMock.mockResolvedValueOnce(response([att({ id: 'a1' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + openConfirm(); + + listMock.mockResolvedValueOnce(response([att({ id: 'b1' })])); + props.itemId = 'item-b'; + // No flushSync: the prompt is still up and the props already read B. + clickConfirm(); + await settle(); + + expect(deleteMock).not.toHaveBeenCalled(); + expect(notifyDeletedMock).not.toHaveBeenCalled(); + expect(tiles()[0].getAttribute('aria-label')).toContain('b1.png'); }); it('rolls the tile back and toasts when the delete fails', async () => { @@ -982,8 +1134,8 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(tiles()).toHaveLength(2); @@ -991,7 +1143,6 @@ describe('ItemAttachmentStrip', () => { expect(notifyDeletedMock).not.toHaveBeenCalled(); expect(toastMock).toHaveBeenCalledOnce(); expect(String(toastMock.mock.calls[0][0])).toContain('a1.png'); - confirmSpy.mockRestore(); }); it('does not roll a failed delete back into a DIFFERENT item strip', async () => { @@ -1010,9 +1161,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); expect(tiles()).toHaveLength(0); // optimistic removal happened // Switch to B before the delete settles. @@ -1028,7 +1178,6 @@ describe('ItemAttachmentStrip', () => { expect(names.some((n) => n?.includes('a1.png'))).toBe(false); expect(names.some((n) => n?.includes('b1.png'))).toBe(true); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('rolls back only the failed row, never resurrecting a concurrent success', async () => { @@ -1048,11 +1197,10 @@ describe('ItemAttachmentStrip', () => { failFirst = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - - deleteButtons()[0].click(); // a1 — in flight, will fail - flushSync(); - deleteButtons()[0].click(); // now b1 — resolves immediately + openConfirm(); // a1 — in flight, will fail + clickConfirm(); + openConfirm(); // now b1 — resolves immediately + clickConfirm(); await settle(); failFirst(new Error('boom')); @@ -1063,7 +1211,6 @@ describe('ItemAttachmentStrip', () => { expect(names.some((n) => n.includes('b1.png'))).toBe(false); // stays deleted // ...and restored at its original position, not appended. expect(names[0]).toContain('a1.png'); - confirmSpy.mockRestore(); }); it('still announces the deletion when the delete 404s', async () => { @@ -1075,13 +1222,12 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(notifyDeletedMock).toHaveBeenCalledWith('a1'); expect(invalidateMock).toHaveBeenCalledWith('ws', 'a1'); - confirmSpy.mockRestore(); }); it('still announces a 404 delete when the view switched under it', async () => { @@ -1102,9 +1248,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); listMock.mockResolvedValue(response([att({ id: 'b1' })])); props.itemId = 'item-b'; @@ -1121,7 +1266,6 @@ describe('ItemAttachmentStrip', () => { expect(names.some((n) => n.includes('a1.png'))).toBe(false); expect(names.some((n) => n.includes('b1.png'))).toBe(true); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('does not roll back a failed delete that another surface already announced', async () => { @@ -1139,9 +1283,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); broadcastDeletion('a1'); flushSync(); @@ -1151,7 +1294,6 @@ describe('ItemAttachmentStrip', () => { const names = tiles().map((el) => el.getAttribute('aria-label') ?? ''); expect(names.some((n) => n.includes('a1.png'))).toBe(false); - confirmSpy.mockRestore(); }); it('keeps the tile removed when the delete 404s (already gone)', async () => { @@ -1164,13 +1306,12 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(tiles()).toHaveLength(1); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('names permission as the reason on a 403, and restores the tile', async () => { @@ -1180,13 +1321,12 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(tiles()).toHaveLength(1); expect(String(toastMock.mock.calls[0][0])).toContain("don't have permission"); - confirmSpy.mockRestore(); }); it('still rolls back and toasts when a Retry re-ran the load mid-delete', async () => { @@ -1210,9 +1350,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); expect(tiles()).toHaveLength(0); // optimistic removal // Retry while the delete is still in flight. @@ -1228,7 +1367,6 @@ describe('ItemAttachmentStrip', () => { expect(toastMock).toHaveBeenCalledOnce(); expect(String(toastMock.mock.calls[0][0])).toContain('survivor.png'); expect(notifyDeletedMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('suppresses a failed delete when the WORKSPACE changed under it', async () => { @@ -1249,9 +1387,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); expect(tiles()).toHaveLength(0); // Retry clicked, THEN the workspace swapped before the effect flushed — @@ -1270,7 +1407,6 @@ describe('ItemAttachmentStrip', () => { expect(names.some((n) => n.includes('survivor.png'))).toBe(false); expect(names.some((n) => n.includes('other-ws.png'))).toBe(true); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('still suppresses a failed delete after an A→B→A round trip', async () => { @@ -1290,9 +1426,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); listMock.mockResolvedValue(response([att({ id: 'b1' })])); props.itemId = 'item-b'; @@ -1309,7 +1444,6 @@ describe('ItemAttachmentStrip', () => { expect(tiles()).toHaveLength(0); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); // ── Upload refresh (TASK-2385) ──────────────────────────────────────── @@ -1655,12 +1789,9 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); - deleteButtons()[0].click(); - await settle(); + openConfirm(); - expect(String(confirmSpy.mock.calls[0][0])).toContain("still used in this item's content"); - confirmSpy.mockRestore(); + expect(promptText()).toContain("still used in this item's content"); }); it('falls back to persisted content when the live read throws', async () => { @@ -1674,11 +1805,8 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); - deleteButtons()[0].click(); - await settle(); + openConfirm(); - expect(String(confirmSpy.mock.calls[0][0])).toContain("still used in this item's content"); - confirmSpy.mockRestore(); + expect(promptText()).toContain("still used in this item's content"); }); }); diff --git a/web/src/lib/components/settings/StorageTab.svelte b/web/src/lib/components/settings/StorageTab.svelte index dbc3835e..93664447 100644 --- a/web/src/lib/components/settings/StorageTab.svelte +++ b/web/src/lib/components/settings/StorageTab.svelte @@ -19,6 +19,8 @@ type StorageFilterSelections } 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 { viewIdentity, createFence, createPaintFence } from '$lib/attachments/viewFence'; // ── Props ──────────────────────────────────────────────────────────────── @@ -49,6 +51,22 @@ let loading = $state(true); let usage = $state(null); let attachments = $state([]); + /** + * The delete confirmation currently on screen (PLAN-2392 DR-18 / + * TASK-2425). This row used to raise a browser-native `confirm()`; every + * attachment delete now goes through the same in-app drill-down — Cancel + * first, destructive row last, prompt back-referenced by + * `aria-describedby`. The WORDING stays this surface's own: the strip's + * "referenced in this item's content" check has no meaning in a + * workspace-wide list, and what matters here is the GC grace period. + */ + let pendingDelete = $state<{ + att: AttachmentListItem; + anchor: HTMLElement | null; + prompt: string; + } | null>(null); + const uid = $props.id(); + const promptId = `storage-delete-note-${uid}`; let total = $state(0); let limit = $state(50); let offset = $state(0); @@ -248,6 +266,10 @@ attachments = []; total = 0; usage = null; + // A confirmation left up for a row from the previous workspace + // goes with it: the button it is anchored to has just been + // unmounted, and `confirmDelete`'s fence would refuse it anyway. + pendingDelete = null; void loadWorkspaceView(); return; } @@ -400,6 +422,49 @@ // ── Actions ────────────────────────────────────────────────────────────── + /** + * Open the delete confirmation (PLAN-2392 DR-18 / TASK-2425). + * + * ENTRY fence, for the same reason `handleDelete` re-takes it below: the + * clicked row was painted for the workspace this tab has LOADED, which + * during the prop-update → effect-flush window is not necessarily the one + * `wsSlug` already names. + */ + function requestDelete(att: AttachmentListItem, anchor: HTMLElement | null) { + if (!paint.isCurrent()) return; + pendingDelete = { + att, + anchor, + prompt: `Delete ${att.filename}? The blob is reclaimed by garbage collection after a grace period.`, + }; + } + + /** Escape / outside-click. `Menu` handles the Escape refocus itself. */ + function dismissDelete() { + pendingDelete = null; + } + + /** The Cancel row — returns focus to the button it was anchored to. */ + function cancelDelete() { + const anchor = pendingDelete?.anchor; + pendingDelete = null; + anchor?.focus(); + } + + /** + * The user confirmed. `confirm()` blocked the thread, so the entry fence + * was still true by definition when it returned; an in-app confirmation + * does not, and the workspace can change while it is up — so the fence is + * re-checked at the point that actually sends the request. + */ + function confirmDelete() { + const pending = pendingDelete; + pendingDelete = null; + if (!pending) return; + if (!paint.isCurrent()) return; + void handleDelete(pending.att); + } + async function handleDelete(att: AttachmentListItem) { // ENTRY fence (fence 3). The clicked row was painted for the workspace // this tab has LOADED, which during the prop-update → effect-flush window @@ -417,10 +482,6 @@ // continuation toast and refetch (Codex round 3). const req = viewFence.begin(); - const ok = confirm( - `Delete ${att.filename}? The blob is reclaimed by garbage collection after a grace period.` - ); - if (!ok) return; try { await api.attachments.delete(reqWsSlug, att.id); // Same broadcast the item attachment strip does (PLAN-2382 / @@ -716,7 +777,7 @@ @@ -751,6 +812,33 @@ {/if}
+ +{#if pendingDelete} + + + +{/if} +