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 cab0e2d7..42d6d06b 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 a3bc8af6..71f3f774 100644 --- a/web/src/lib/components/editor/attachment-image.ts +++ b/web/src/lib/components/editor/attachment-image.ts @@ -103,6 +103,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 @@ -162,6 +175,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 1ea411c5..d995841c 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 @@ -482,6 +493,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 @@