diff --git a/web/src/lib/attachments/actions.test.ts b/web/src/lib/attachments/actions.test.ts index 40988193..ae45ef2f 100644 --- a/web/src/lib/attachments/actions.test.ts +++ b/web/src/lib/attachments/actions.test.ts @@ -220,6 +220,29 @@ describe('attachment action descriptors', () => { expect(announceMock).not.toHaveBeenCalled(); }); + it('still deletes when an async confirmation says yes and nothing moved', async () => { + // The guard rails above only prove the descriptor ABANDONS a delete in + // the bad cases. Without this, a regression that dropped every + // async-confirmed delete — which is what the in-app confirm will + // use — would pass the whole suite. + const del = action('delete'); + if (del.element !== 'button') throw new Error('unreachable'); + const order: string[] = []; + deleteMock.mockImplementation(async () => { + order.push('delete'); + }); + announceMock.mockImplementation(() => { + order.push('announce'); + }); + + await del.run(ctx({ confirmDelete: async () => true })); + + expect(deleteMock).toHaveBeenCalledWith('ws', 'att-1'); + // The broadcast must follow the server's confirmation, never precede + // it — subscribers latch it as authoritative. + expect(order).toEqual(['delete', 'announce']); + }); + it('deletes the attachment the user confirmed, not whatever the context holds later', async () => { // An in-app confirmation is a whole UI interaction, so the surface can // switch items underneath it — the pane this renders in is built around diff --git a/web/src/lib/attachments/events.ts b/web/src/lib/attachments/events.ts index 3275351f..5e5219ae 100644 --- a/web/src/lib/attachments/events.ts +++ b/web/src/lib/attachments/events.ts @@ -21,6 +21,7 @@ import { invalidateAttachmentMetadata } from '$lib/components/editor/attachment-metadata'; import type { AttachmentUploadResult } from '$lib/types'; +import { isAddressable } from '$lib/attachments/hostAddress'; const listeners = new Set<(uuid: string) => void>(); @@ -188,9 +189,13 @@ export function isAttachmentPanelEventForHost( 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; + // Both sides must be fully addressable before a comparison means anything: + // two empty tokens are not a match, they are two absences. `isAddressable` + // is the single statement of that rule (see hostAddress.ts). + const from = { itemId: event.itemId, hostToken: event.hostToken }; + const to = { itemId: host?.itemId ?? '', hostToken: host?.hostToken ?? '' }; + if (!isAddressable(from) || !isAddressable(to)) return false; + return from.itemId === to.itemId && from.hostToken === to.hostToken; } const panelListeners = new Set<(event: AttachmentPanelOpenEvent) => void>(); diff --git a/web/src/lib/attachments/hostAddress.test.ts b/web/src/lib/attachments/hostAddress.test.ts new file mode 100644 index 00000000..61fd232c --- /dev/null +++ b/web/src/lib/attachments/hostAddress.test.ts @@ -0,0 +1,66 @@ +// Host addressing for attachment NodeViews (PLAN-2392 DR-8, TASK-2421). +// +// The interesting assertion in this file is the LAST one. The address is a +// reader instead of two string options because Tiptap's `options` is a getter +// returning a fresh spread per access — writing to it after configure() is a +// no-op that looks exactly like working code. That is a property of a +// dependency, so it is pinned here: if a future @tiptap/core bump makes +// options writable, this test fails and someone re-reads the reasoning +// instead of discovering it through a chip that silently does nothing. +import { describe, it, expect } from 'vitest'; +import { AttachmentChip } from '$lib/components/editor/attachment-chip'; +import { + isAddressable, + readUnaddressed, + type AttachmentHostAddress, + type AttachmentHostAddressReader, +} from './hostAddress'; + +describe('attachment host address', () => { + it('reads through to the host live, so a reused editor re-addresses on an item switch', () => { + // Exactly the composer's situation: the component instance survives an + // A→B item switch and its `itemId` prop just changes underneath. + let itemId = 'item-A'; + const hostToken = 'apanel-1'; + const read: AttachmentHostAddressReader = () => ({ itemId, hostToken }); + + expect(read()).toEqual({ itemId: 'item-A', hostToken: 'apanel-1' }); + itemId = 'item-B'; + expect(read()).toEqual({ itemId: 'item-B', hostToken: 'apanel-1' }); + }); + + it('treats a half-address as unaddressable, in both directions', () => { + // A token without an item, or an item without a token, cannot pick out + // ONE of two concurrently-mounted hosts — which is the whole job. + expect(isAddressable({ itemId: 'item-A', hostToken: 'apanel-1' })).toBe(true); + expect(isAddressable({ itemId: 'item-A', hostToken: '' })).toBe(false); + expect(isAddressable({ itemId: '', hostToken: 'apanel-1' })).toBe(false); + expect(isAddressable(readUnaddressed())).toBe(false); + expect(isAddressable(null)).toBe(false); + }); + + it('defaults to unaddressed, so an editor with no host broadcasts to nobody', () => { + const ext = AttachmentChip.configure({}); + expect(isAddressable(ext.options.address())).toBe(false); + }); + + it('carries the configured reader through to the extension options', () => { + const address: AttachmentHostAddress = { itemId: 'item-A', hostToken: 'apanel-1' }; + const ext = AttachmentChip.configure({ address: () => address }); + expect(ext.options.address()).toEqual(address); + + // And it stays live: the extension holds the reader, not a snapshot. + address.itemId = 'item-B'; + expect(ext.options.address().itemId).toBe('item-B'); + }); + + it('pins the reason this is a reader: Tiptap options are a per-access snapshot', () => { + const ext = AttachmentChip.configure({ workspaceSlug: 'ws' }); + + // Each read builds a new object... + expect(ext.options).not.toBe(ext.options); + // ...so assigning to one is discarded, silently. + ext.options.workspaceSlug = 'clobbered'; + expect(ext.options.workspaceSlug).toBe('ws'); + }); +}); diff --git a/web/src/lib/attachments/hostAddress.ts b/web/src/lib/attachments/hostAddress.ts new file mode 100644 index 00000000..bb6d732c --- /dev/null +++ b/web/src/lib/attachments/hostAddress.ts @@ -0,0 +1,57 @@ +/** + * The address a Tiptap attachment NodeView stamps on the events it emits + * (PLAN-2392 DR-8), and why it is a FUNCTION rather than two strings. + * + * DR-8 needs two facts at emit time: which item the editor is editing, and + * which `ItemDetail` mount owns it (a master pane and a peeked pane are both + * mounted, so `itemId` alone would let both hosts consume one NodeView's + * event). The obvious shape is two string options set at `configure()` time. + * + * That shape is a trap here, for two independent reasons: + * + * 1. **The comment composer outlives the item.** `CommentEditor` is + * deliberately reused across a no-`{#key}` item switch — its `itemId` prop + * just changes — so a value captured when its extensions were configured + * goes stale, and its chips would emit events addressed to the PREVIOUS + * item. The host matches on both fields and would correctly ignore them: + * a tap that silently does nothing. + * + * 2. **You cannot fix that by writing to the options.** Tiptap's `options` is + * a GETTER that returns a fresh spread on every access + * (`@tiptap/core@3.22.5`, `dist/index.cjs:3452`), so `ext.options.itemId = + * next` mutates a temporary that is discarded on the next line. The + * assignment looks like it works and does nothing. (`optionsAreASnapshot` + * in the sibling test pins this, so a future Tiptap bump that changes it + * is a visible test failure rather than a silent invitation to go back to + * mutating.) + * + * So the option is a reader the host supplies once and keeps honest: a closure + * over its own live props. Called at emit time, it is always current, for a + * remounted host (the body editor, re-keyed per item) and a reused one (the + * composer) alike — one shape, no per-host special case. + */ + +export interface AttachmentHostAddress { + /** UUID of the item being edited. Empty when there is no item context. */ + itemId: string; + /** Identity of the `ItemDetail` mount that owns this editor. */ + hostToken: string; +} + +/** Reads the CURRENT address. Called at emit time, never cached by callers. */ +export type AttachmentHostAddressReader = () => AttachmentHostAddress; + +/** The no-context address: an editor with no host cannot address a panel. */ +export const UNADDRESSED: AttachmentHostAddress = { itemId: '', hostToken: '' }; + +/** Default option value — an editor mounted without a host addresses nothing. */ +export const readUnaddressed: AttachmentHostAddressReader = () => UNADDRESSED; + +/** + * Whether an address can reach a host at all. Both halves are required: a + * missing token would make the event ambiguous between concurrently-mounted + * hosts, which is the exact failure DR-8 exists to prevent. + */ +export function isAddressable(address: AttachmentHostAddress | null | undefined): boolean { + return Boolean(address?.itemId && address?.hostToken); +} diff --git a/web/src/lib/components/CommentEditor.svelte b/web/src/lib/components/CommentEditor.svelte index 360d0116..6c89dce6 100644 --- a/web/src/lib/components/CommentEditor.svelte +++ b/web/src/lib/components/CommentEditor.svelte @@ -27,6 +27,7 @@ import { AttachmentImage } from './editor/attachment-image'; import { AttachmentChip } from './editor/attachment-chip'; import { AttachmentUpload } from './editor/attachment-upload'; + import type { AttachmentHostAddress } from '$lib/attachments/hostAddress'; interface Props { /** Initial markdown body. Parsed as markdown on mount. */ @@ -121,6 +122,21 @@ } } + /** + * Reads the CURRENT host address at emit time (PLAN-2392 DR-8). + * + * This composer is reused across a no-{#key} item switch — the same reason + * `doSubmit` above captures its item before awaiting — so a value baked + * into the extension config at mount would address the PREVIOUS item after + * a switch, and the host would correctly ignore the event. Tiptap's + * `options` getter returns a fresh spread per access, so there is no + * writing the new value in afterwards either (see hostAddress.ts). + */ + const readHostAddress = (): AttachmentHostAddress => ({ + itemId: itemId ?? '', + hostToken + }); + const attachmentUrl = (uuid: string, variant?: 'thumb-sm' | 'thumb-md' | 'original') => wsSlug ? api.attachments.downloadUrl(wsSlug, uuid, variant) : `pad-attachment:${uuid}`; @@ -140,8 +156,7 @@ getDownloadUrl: attachmentUrl, workspaceSlug: wsSlug, // Panel / viewer addressing (PLAN-2392 DR-8). - itemId: itemId ?? '', - hostToken, + address: readHostAddress, // Rotate/crop stays disabled in comments — keep it lean. supportedFormats: [] as string[], transform: async () => { @@ -151,8 +166,7 @@ AttachmentChip.configure({ getDownloadUrl: attachmentUrl, workspaceSlug: wsSlug, - itemId: itemId ?? '', - hostToken + address: readHostAddress }), AttachmentUpload.configure({ // Wrap upload so the host can track in-flight uploads and gate @@ -206,37 +220,6 @@ empty = editor.isEmpty; }); - /** - * Keep the attachment NodeViews' addressing current (PLAN-2392 DR-8). - * - * The extensions are configured once, inside `onMount`, which captures - * whatever `itemId` / `hostToken` were at that moment. That is fine for the - * body editor — it is remounted per item behind a `{#key}` — but this - * composer is deliberately REUSED across a no-{#key} item switch (see - * `doSubmit` above, which exists for the same reason). Left alone, a chip - * in the composer would keep emitting events addressed to the PREVIOUS - * item, and the host, which matches on both fields, would correctly ignore - * them — a tap that silently does nothing. - * - * Mutating the live extension options is the established shape here; - * `Editor.svelte` pushes `supportedFormats` the same way once server - * capabilities resolve. The NodeViews read these at emit time, so a write - * is enough — nothing needs to re-render. - */ - $effect(() => { - const nextItemId = itemId ?? ''; - const nextToken = hostToken; - if (!editor || editor.isDestroyed) return; - for (const name of ['attachmentChip', 'attachmentImage']) { - const ext = editor.extensionManager.extensions.find( - (e: { name: string }) => e.name === name - ); - if (!ext) continue; - ext.options.itemId = nextItemId; - ext.options.hostToken = nextToken; - } - }); - onDestroy(() => { editor?.destroy(); }); diff --git a/web/src/lib/components/editor/Editor.svelte b/web/src/lib/components/editor/Editor.svelte index 3bdcd8a4..79015f8e 100644 --- a/web/src/lib/components/editor/Editor.svelte +++ b/web/src/lib/components/editor/Editor.svelte @@ -598,6 +598,7 @@ notifyAttachmentImageCapabilitiesChanged, } from './attachment-image'; import { AttachmentChip } from './attachment-chip'; + import type { AttachmentHostAddress } from '$lib/attachments/hostAddress'; import { AttachmentUpload } from './attachment-upload'; let { @@ -872,6 +873,16 @@ const getAttachmentUrl = (uuid: string, variant?: AttachmentVariant) => wsSlug ? api.attachments.downloadUrl(wsSlug, uuid, variant) : `pad-attachment:${uuid}`; + // Reads the CURRENT host address at emit time (PLAN-2392 DR-8). This + // editor is remounted per item behind a {#key}, so a captured value + // would in fact be correct here — it is a reader anyway so both editor + // hosts publish one shape, and so nobody has to know which of them is + // remounted and which is reused (see hostAddress.ts). + const readHostAddress = (): AttachmentHostAddress => ({ + itemId: itemId ?? '', + hostToken + }); + // When a Y.Doc is supplied, the Collaboration extension owns // undo/redo (Yjs maintains its own history that survives peer // edits correctly) and StarterKit's undoRedo would fight it. @@ -929,8 +940,7 @@ // 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, + address: readHostAddress, // Initial supportedFormats is empty — server capabilities // are fetched async below. The toolbar starts disabled // for all formats until capabilities resolve, then @@ -954,8 +964,7 @@ AttachmentChip.configure({ getDownloadUrl: getAttachmentUrl, workspaceSlug: wsSlug, - itemId: itemId ?? '', - hostToken, + address: readHostAddress, }), // When a Y.Doc is provided, register the Collaboration // extension so the y-tiptap binding takes over document diff --git a/web/src/lib/components/editor/attachment-chip.ts b/web/src/lib/components/editor/attachment-chip.ts index 42d6d06b..7a912e16 100644 --- a/web/src/lib/components/editor/attachment-chip.ts +++ b/web/src/lib/components/editor/attachment-chip.ts @@ -41,6 +41,10 @@ import { fetchAttachmentMetadata } from './attachment-metadata'; import { registerAttachmentDeletionListener } from '$lib/attachments/events'; +import { + type AttachmentHostAddressReader, + readUnaddressed +} from '$lib/attachments/hostAddress'; import { formatBytes, iconForAttachment } from '$lib/attachments/display'; import { iconSvg } from '$lib/attachments/icons/index'; @@ -57,19 +61,12 @@ export interface AttachmentChipOptions { /** 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. + * Reads the host address (item + owning `ItemDetail` mount) to stamp on + * open-panel events (PLAN-2392 DR-8). A reader rather than two strings + * because one host is reused across an item switch and Tiptap options + * cannot be written after configure — see `$lib/attachments/hostAddress`. */ - 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; + address: AttachmentHostAddressReader; } declare module '@tiptap/core' { @@ -98,8 +95,7 @@ export const AttachmentChip = Node.create({ HTMLAttributes: {}, getDownloadUrl: (uuid: string) => `${PAD_ATTACHMENT_PREFIX}${uuid}`, workspaceSlug: '', - itemId: '', - hostToken: '', + address: readUnaddressed, }; }, diff --git a/web/src/lib/components/editor/attachment-image.ts b/web/src/lib/components/editor/attachment-image.ts index 310219ca..6eab2e12 100644 --- a/web/src/lib/components/editor/attachment-image.ts +++ b/web/src/lib/components/editor/attachment-image.ts @@ -38,6 +38,10 @@ import { } from './attachment-metadata'; import { openCropModal, type CropResult } from './attachment-crop-modal'; import { registerAttachmentDeletionListener } from '$lib/attachments/events'; +import { + type AttachmentHostAddressReader, + readUnaddressed +} from '$lib/attachments/hostAddress'; import type { AttachmentTransformRequest, AttachmentTransformResult } from '$lib/types'; // Re-export the shared types so existing call sites keep working. @@ -104,18 +108,12 @@ export interface AttachmentImageOptions { */ 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. + * Reads the host address (item + owning `ItemDetail` mount) to stamp on + * panel / viewer events (PLAN-2392 DR-8). A reader rather than two + * strings — see `$lib/attachments/hostAddress` for why writing options + * after configure cannot work. */ - 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; + address: AttachmentHostAddressReader; /** * Image formats the server-side processor supports. Drives the * rotate toolbar's enabled state per attachment: a button is @@ -175,8 +173,7 @@ export const AttachmentImage = Node.create({ HTMLAttributes: {}, getDownloadUrl: (uuid: string) => `${PAD_ATTACHMENT_PREFIX}${uuid}`, workspaceSlug: '', - itemId: '', - hostToken: '', + address: readUnaddressed, supportedFormats: [] as string[], transform: async () => { throw new Error('AttachmentImage: configure({ transform }) is required to use rotate/crop'); @@ -470,9 +467,11 @@ export const AttachmentImage = Node.create({ const probeUuid = currentUuid; fetchAttachmentMetadata(opts.workspaceSlug, probeUuid, opts.getDownloadUrl).then( (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. + // Bail if the NodeView was torn down, or if its uuid + // changed (rotate/peer op) while the probe was in + // flight — otherwise we'd touch detached DOM, or + // cache stale MIME state for the new image. + if (destroyed) return; if (currentUuid !== probeUuid) return; // A 404 here is the same authoritative signal the // load path acts on (DR-17), and this probe may @@ -491,6 +490,11 @@ export const AttachmentImage = Node.create({ }; const swapNodeUuid = (newId: string): void => { + // A transform that resolves after teardown has no position left + // to dispatch against — `editor.isDestroyed` is false whenever + // the editor outlives this one NodeView, which is the common + // case (the node was replaced, the doc was re-rendered). + if (destroyed) return; // Master-freeze / R12 (TASK-2172): runRotate/runCrop gate editability // at CLICK time, but the transform awaits a network round-trip during // which the master can begin peeking — flipping the editor read-only @@ -664,6 +668,7 @@ export const AttachmentImage = Node.create({ probeUuid, opts.getDownloadUrl ).then((result) => { + if (destroyed) return; if (currentUuid !== probeUuid) return; if (result.status === 'missing') latchMissing(probeUuid); if (!toolbar) return;