fix(attachments): make the host address a reader, not written-in options

The addressing fix in the previous commit could not have worked. Tiptap's
`options` is a getter returning a fresh spread on every access
(@tiptap/core 3.22.5, dist/index.cjs:3452), so `ext.options.itemId = next`
mutates a temporary and is discarded — an assignment that looks exactly
like working code.

So the address stops being two strings pushed in after the fact and
becomes a reader the host supplies once and keeps honest: a closure over
its own live props, called at emit time. That is correct for a host that
is remounted per item (the body editor) and one that is reused across an
item switch (the comment composer) without either knowing which it is.

New $lib/attachments/hostAddress.ts states the contract; its test pins
the dependency behaviour that forces it, so a future Tiptap bump that
makes options writable fails a test instead of quietly inviting the
mutation approach back. The event predicate now reuses isAddressable so
'both halves required' is stated once.

Also completes the NodeView teardown fence (both MIME probe
continuations and swapNodeUuid could run after destroy) and adds the
stable-async-confirm and ordering cases the delete tests were missing —
without them, a regression dropping every async-confirmed delete passed.

The same dependency trap makes the editor's existing capabilities push a
no-op; that is pre-existing and independent, filed as BUG-2426.

All three from the orchestrator's second fresh-angle Codex pass.
This commit is contained in:
xarmian
2026-08-04 02:17:48 +00:00
parent 042bd7e477
commit ebe531ebaa
8 changed files with 216 additions and 72 deletions
+23
View File
@@ -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
+8 -3
View File
@@ -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>();
@@ -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');
});
});
+57
View File
@@ -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);
}
+18 -35
View File
@@ -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();
});
+13 -4
View File
@@ -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
@@ -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<AttachmentChipOptions>({
HTMLAttributes: {},
getDownloadUrl: (uuid: string) => `${PAD_ATTACHMENT_PREFIX}${uuid}`,
workspaceSlug: '',
itemId: '',
hostToken: '',
address: readUnaddressed,
};
},
@@ -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<AttachmentImageOptions>({
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<AttachmentImageOptions>({
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<AttachmentImageOptions>({
};
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<AttachmentImageOptions>({
probeUuid,
opts.getDownloadUrl
).then((result) => {
if (destroyed) return;
if (currentUuid !== probeUuid) return;
if (result.status === 'missing') latchMissing(probeUuid);
if (!toolbar) return;