Merge pull request #1059 from PerpetualSoftware/feat/attachment-options-panel

feat(attachments): an options panel for files, everywhere you meet them (PLAN-2392 phase 2)
This commit is contained in:
xarmian
2026-08-04 19:00:57 -04:00
committed by GitHub
31 changed files with 5722 additions and 279 deletions
+154 -11
View File
@@ -67,18 +67,48 @@ async function uploadTo(
return ((await resp.json()) as { id: string }).id;
}
/**
* Upload a NON-image bound to `itemId`. `text/plain` is in the server's
* allowlist and is one of the types the panel offers Open for, so the tile it
* produces is a file tile — the one that opens the options panel.
*/
async function uploadTextTo(
fixture: SuiteFixture,
request: APIRequestContext,
itemId: string,
filename: string
): Promise<string> {
const ws = fixture.workspaceSlug;
const resp = await request.post(
`/api/v1/workspaces/${ws}/attachments?item_id=${encodeURIComponent(itemId)}`,
{
headers: { Authorization: `Bearer ${fixture.apiToken}` },
multipart: {
file: { name: filename, mimeType: 'text/plain', buffer: Buffer.from('notes\n') }
}
}
);
if (!resp.ok()) throw new Error(`upload failed (${resp.status()}): ${await resp.text()}`);
return ((await resp.json()) as { id: string }).id;
}
/**
* Drop a file onto the live editor, the way a user does. The upload plugin
* listens for a real `drop` with a DataTransfer, so we build one in the page
* rather than driving the (nonexistent) file input.
*/
async function dropFileIntoEditor(page: Page, filename: string, base64: string): Promise<void> {
async function dropFileIntoEditor(
page: Page,
filename: string,
base64: string,
mimeType = 'image/png'
): Promise<void> {
const target = page.locator('.editor-content .ProseMirror').first();
await target.waitFor({ state: 'visible' });
await target.evaluate(
(el, { filename, base64 }) => {
(el, { filename, base64, mimeType }) => {
const bytes = Uint8Array.from(atob(base64), (c) => c.charCodeAt(0));
const file = new File([bytes], filename, { type: 'image/png' });
const file = new File([bytes], filename, { type: mimeType });
const dt = new DataTransfer();
dt.items.add(file);
const rect = el.getBoundingClientRect();
@@ -92,7 +122,7 @@ async function dropFileIntoEditor(page: Page, filename: string, base64: string):
})
);
},
{ filename, base64 }
{ filename, base64, mimeType }
);
}
@@ -209,14 +239,27 @@ test.describe('item attachment strip', () => {
const del = page.locator(DELETE_BTN).first();
await del.focus();
await expect(del).toBeFocused();
// WCAG 2.2 target size (2.5.8), from PLAN-2382. Only a real browser
// applies the scoped CSS, so this is the only place it can be checked.
const box = await del.boundingBox();
expect(box?.width).toBeGreaterThanOrEqual(24);
expect(box?.height).toBeGreaterThanOrEqual(24);
page.once('dialog', (dialog) => {
// The attachment IS embedded in the body (the drop inserted it), so
// the confirm must say so rather than hedging.
expect(dialog.message()).toContain("still used in this item's content");
void dialog.accept();
});
// TASK-2425 / DR-18: the confirmation is the app's own drill-down, not
// a browser dialog. A native `confirm()` would hang this click until
// Playwright auto-dismissed it, so the absence of a dialog handler is
// itself part of the assertion.
await del.click();
const confirmMenu = page.locator('[role="menu"]');
await expect(confirmMenu).toBeVisible();
// The attachment IS embedded in the body (the drop inserted it), so the
// prompt must say so rather than hedging.
await expect(confirmMenu.locator('.attachment-delete-prompt')).toContainText(
"still used in this item's content"
);
// Cancel is first, so the focus handoff can never land Enter on Delete.
await expect(confirmMenu.getByRole('menuitem').first()).toContainText('Cancel');
await confirmMenu.getByRole('menuitem', { name: 'Delete file' }).click();
await expect(page.locator(TILE)).toHaveCount(0);
// ...and the strip disappears entirely once empty.
@@ -263,4 +306,104 @@ test.describe('item attachment strip', () => {
await expect(masterHost.locator(TILE)).toHaveCount(1);
await expect(masterStrip).toHaveCount(0);
});
});
test('a file tile opens the options panel, from mouse and from the keyboard (PLAN-2392 phase 2)', async ({
page,
fixture,
request
}, testInfo) => {
test.skip(testInfo.project.name !== 'desktop-chromium', 'desktop-only surface');
// The ONE test that exercises the real producer→host wiring end to end.
// The unit suites necessarily stop short of it: the strip's tests mock
// the event bus, and the panel host's tests emit on the bus directly, so
// between them a broken `hostToken` thread through ItemDetail would pass
// everything (final review round 3). Only a real page has a real
// ItemDetail minting a real token.
//
// It is also the only place "activates exactly once per key press"
// (DR-12) can actually be demonstrated: jsdom does not synthesise a
// button's activation click, so the unit test can only prove the
// narrower "no handler races the UA click".
await page.setViewportSize(DESKTOP);
await browserLogin(page);
const doc = await seedDoc(fixture, request, 'Panel wiring');
await uploadTextTo(fixture, request, doc.id, 'notes.txt');
await page.goto(itemUrl(fixture, doc.slug));
const tile = page.locator(TILE).first();
await expect(tile).toBeVisible();
// A file tile is a real button naming its action, not a download link —
// the whole point of the change (DR-1, DR-12).
await expect(tile).toHaveJSProperty('tagName', 'BUTTON');
await expect(tile).toHaveAttribute('aria-label', /^Options for notes\.txt/);
await tile.click();
const panel = page.locator('[role="menu"]').filter({ hasText: 'notes.txt' });
await expect(panel).toBeVisible();
// Download is a REAL anchor carrying the filename: the server sends an
// inline disposition for most types, so a plain navigation would view
// rather than save (DR-16).
const download = panel.getByRole('menuitem', { name: 'Download' });
await expect(download).toHaveJSProperty('tagName', 'A');
await expect(download).toHaveAttribute('download', 'notes.txt');
// text/plain is browser-previewable, so Open is offered.
await expect(panel.getByRole('menuitem', { name: 'Open in new tab' })).toBeVisible();
await page.keyboard.press('Escape');
await expect(panel).toHaveCount(0);
// Keyboard activation, for real this time: focus the tile and press each
// key. Exactly one panel opens per press — two would mean a hand-rolled
// handler firing alongside the UA's activation click.
for (const key of ['Enter', ' ']) {
await tile.focus();
await page.keyboard.press(key);
await expect(page.locator('[role="menu"]')).toHaveCount(1);
await page.keyboard.press('Escape');
await expect(page.locator('[role="menu"]')).toHaveCount(0);
}
});
test('an editor file chip opens the same panel as a strip tile (PLAN-2392 DR-2)', async ({
page,
fixture,
request
}, testInfo) => {
test.skip(testInfo.project.name !== 'desktop-chromium', 'desktop-only surface');
// The chip half of "the same panel wherever you meet an attachment".
// Unit coverage stops at the seam on both sides — the chip's tests mock
// the event bus and the host's inject events directly — so only a real
// page proves a real NodeView reaches a real host through the real bus.
await page.setViewportSize(DESKTOP);
await browserLogin(page);
const doc = await seedDoc(fixture, request, 'Chip panel wiring');
await page.goto(itemUrl(fixture, doc.slug));
// Drop a NON-image so the editor renders a file chip rather than an
// inline image.
await dropFileIntoEditor(
page,
'handbook.txt',
Buffer.from('chapter one\n').toString('base64'),
'text/plain'
);
const chip = page.locator('.editor-content .ProseMirror button.file-chip').first();
await expect(chip).toBeVisible();
// A button, not a link: an anchor left the URL reachable by middle-click,
// straight past the panel (DR-12).
await expect(chip).toHaveJSProperty('tagName', 'BUTTON');
await chip.click();
const panel = page.locator('[role="menu"]').filter({ hasText: 'handbook.txt' });
await expect(panel).toBeVisible();
const download = panel.getByRole('menuitem', { name: 'Download' });
await expect(download).toHaveJSProperty('tagName', 'A');
await expect(download).toHaveAttribute('download', 'handbook.txt');
});
});
+16
View File
@@ -349,6 +349,12 @@ input:focus, textarea:focus {
attachments. Used by both the editor's AttachmentChip node and the
read-only markdown render path (TASK-874). Styles target the same
class names from both surfaces so the visual matches edge to edge. */
/* Two elements share this class: the editor NodeView renders a <button> (a
live chip opens the options panel, it does not navigate — PLAN-2392 DR-12),
while the read-only markdown path and the clipboard shape render an <a
download>. So it carries the button reset explicitly: without `font:
inherit` the UA's 13.33px Arial would replace the inherited font and shrink
the chip, and `text-align` keeps the label left-aligned in the button. */
.file-chip {
display: inline-flex;
align-items: center;
@@ -356,6 +362,8 @@ input:focus, textarea:focus {
max-width: 100%;
padding: 4px 10px;
margin: 0 1px;
font: inherit;
text-align: start;
background: var(--bg-tertiary);
border: 1px solid var(--border-subtle);
border-radius: 6px;
@@ -398,6 +406,14 @@ input:focus, textarea:focus {
cursor: default;
text-decoration: line-through;
}
/* The NodeView marks a dead chip `disabled`, which is what makes it inert and
unfocusable; keep it looking the same as the read-only path's missing chip
rather than inheriting the UA's disabled-button greying. */
.file-chip:disabled {
color: var(--text-muted);
cursor: default;
opacity: 1;
}
.file-chip.attachment-missing:hover {
background: var(--bg-tertiary);
border-color: var(--border-subtle);
+336
View File
@@ -0,0 +1,336 @@
// 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;
// The descriptors import the real `canBrowserPreview` (DR-16 keeps every
// "what can this MIME do" question in one module), so these cases assert
// against the shipping predicate rather than a stand-in.
function ctx(overrides: Partial<Ctx> = {}): Ctx {
return {
workspaceSlug: 'ws',
attachment: { id: 'att-1', filename: 'report.pdf', mime_type: 'application/pdf' },
mutationsEnabled: true,
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('does not offer Open for an SVG, even though it is labelled image/*', () => {
// DR-16: the predicate is an exact allowlist, not an `image/` prefix,
// and the descriptors reach it directly — a caller cannot hand them a
// looser one. An SVG can carry active content, so it gets Download only.
const svg = ctx({
attachment: { id: 'att-3', filename: 'diagram.svg', mime_type: 'image/svg+xml' },
});
expect(attachmentActionsFor(svg).map((a) => a.id)).toEqual([
'download',
'copy-link',
'delete',
]);
});
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('keeps the download attribute even when the row has no filename', () => {
// Returning undefined here DROPS the attribute, which turns Download
// back into a navigation — and the server sends an inline disposition
// for most types, so the file would open instead of saving. Reachable
// whenever a chip's metadata is still partial. `download=""` forces the
// save and lets the browser name it.
const nameless = ctx({
attachment: { id: 'att-4', filename: '', mime_type: 'application/pdf' },
});
const download = action('download');
if (download.element !== 'anchor') throw new Error('unreachable');
expect(download.download?.(nameless)).toBe('');
expect(download.download?.(nameless)).not.toBeUndefined();
});
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('omits 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');
// ABSENT, not disabled: the strip hides its delete control outright in
// the same state, and one object must not offer different affordances
// for one permission depending on which surface you meet it through.
expect(del.applies(readOnly)).toBe(false);
expect(attachmentActionsFor(readOnly).map((a) => a.id)).not.toContain('delete');
// `enabled` remains the second gate, and `run` re-checks a third time.
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('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
// a no-{#key} A→B switch. The descriptor must act on what was confirmed.
const live: Ctx = ctx();
const del = action('delete');
if (del.element !== 'button') throw new Error('unreachable');
await del.run({
...live,
confirmDelete: async () => {
live.attachment = { id: 'att-OTHER', filename: 'other.pdf', mime_type: 'application/pdf' };
return true;
},
get attachment() {
return live.attachment;
},
} as Ctx);
// Identity moved while the confirmation was open, so the delete is
// abandoned rather than aimed at the newly-shown attachment.
expect(deleteMock).not.toHaveBeenCalled();
expect(announceMock).not.toHaveBeenCalled();
});
it('abandons the delete if mutation rights are lost while the confirmation is open', async () => {
const live: Ctx = ctx();
const del = action('delete');
if (del.element !== 'button') throw new Error('unreachable');
await del.run({
...live,
confirmDelete: async () => {
live.mutationsEnabled = false;
return true;
},
get mutationsEnabled() {
return live.mutationsEnabled;
},
} as Ctx);
expect(deleteMock).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);
}
});
});
+284
View File
@@ -0,0 +1,284 @@
/**
* Attachment actions — defined once, rendered by whoever needs them
* (PLAN-2392 DR-5).
*
* "The panel and the viewer share one action list" is a promise with no source
* of truth unless the list IS the source of truth. So the actions live here as
* descriptors: a surface renders them, none of them owns the set, and adding an
* action means adding one descriptor here.
*
* TODAY THERE IS ONE CONSUMER: the options panel, which draws them as a
* menu/sheet. The unified image viewer — the second renderer, an inline
* toolbar over the same list — arrives in phase 3a, which is also what will
* consume the `address` option now threaded onto the image NodeView. Stated
* plainly because "rendered twice" read as a description of the present and
* was not one; a list with a single consumer is a shape held open on purpose,
* and worth re-justifying if 3a ever stops coming.
*
* TWO DESCRIPTOR SHAPES, not one, because the ELEMENT is part of the contract
* (DR-5, round 35/36):
*
* - `element: 'anchor'` — Download must remain a real `<a download>`: 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.
*
* "Can the browser preview this?" is answered by importing `canBrowserPreview`
* from the display helpers, NOT by taking a predicate from the caller. DR-16
* puts every "what can this MIME do" question in one module precisely so a
* single call site cannot be given a looser answer — an injected predicate
* that admitted `image/svg+xml` would reopen the hole the exact-allowlist
* decision exists to close. (It was injected while this module and the
* predicate were built in parallel; collapsed at integration.)
*
* 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 { canBrowserPreview } from '$lib/attachments/display';
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;
/**
* 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 modality, so a
* descriptor never invents one — but when supplied, returning false aborts
* before any request is sent. Every surface now resolves this through the
* shared `AttachmentDeleteConfirm` drill-down (DR-18); the native
* `window.confirm` the strip used to raise is gone.
*/
confirmDelete?: (attachment: AttachmentActionSubject) => boolean | Promise<boolean>;
/** 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<boolean>;
}
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<void>;
}
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) => canBrowserPreview(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).
//
// ALWAYS present, empty string included. A nameless row returned
// undefined here, which drops the attribute entirely and turns Download
// back into a navigation — the exact regression this action exists to
// prevent, reachable whenever a chip's metadata is partial. `download=""`
// still forces the save; the browser just picks the name (round 8).
download: (ctx) => ctx.attachment.filename || '',
} 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,
// ABSENT, not disabled, where the caller cannot mutate — a peeked pane,
// a read-only viewer. The strip hides its delete control outright in
// exactly the same state, and two views of one object showing different
// affordances for one permission is the divergence this plan kept
// tripping over. `enabled` stays as the second gate: a renderer that
// ignores `applies` still cannot activate it, and `run` re-checks again.
applies: (ctx) => ctx.mutationsEnabled,
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;
// Snapshot identity BEFORE the confirmation, not after. `ctx` may be
// a live object owned by a surface that survives an item switch (the
// no-{#key} pane is built around exactly that), and `confirmDelete`
// is allowed to be async — an in-app confirmation is a whole UI
// interaction, so the user has all the time in the world to switch
// items or lose their mutation rights while it is up. Reading
// `ctx.attachment.id` after the await could name a DIFFERENT
// attachment than the one the user was shown.
const ws = ctx.workspaceSlug;
const id = ctx.attachment.id;
const subject = { ...ctx.attachment };
if (ctx.confirmDelete && !(await ctx.confirmDelete(subject))) return;
// Re-check the gate on the way out of the confirmation: permission
// can be revoked while it is open (a pane being peeked closes the
// mutation gate), and identity must still agree with what was
// confirmed.
if (!ctx.mutationsEnabled) return;
if (ctx.workspaceSlug !== ws || ctx.attachment.id !== id) return;
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 <img>
// 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));
}
+134 -1
View File
@@ -1,5 +1,12 @@
import { describe, expect, it } from 'vitest';
import { formatBytes, iconForAttachment, isImage } from './display';
import {
canBrowserPreview,
canOpenInViewer,
describeAttachmentType,
formatBytes,
iconForAttachment,
isImage
} from './display';
import { ATTACHMENT_ICON_IDS, ATTACHMENT_ICON_PATHS, iconSvg } from './icons/index';
import familyFixture from './mime-families.json';
@@ -182,4 +189,130 @@ 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);
}
});
});
/**
* The panel's type line (PLAN-2392 DR-2 / TASK-2423). Built on
* `iconForAttachment` so the words and the icon beside them can never
* disagree about what a file is.
*/
describe('describeAttachmentType', () => {
it('drops an extension that merely repeats the family', () => {
expect(describeAttachmentType('application/pdf', 'spec.pdf')).toBe('PDF');
});
it('keeps an extension that names the specific format', () => {
expect(describeAttachmentType('image/png', 'shot.png')).toBe('PNG image');
expect(
describeAttachmentType(
'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet',
'budget.xlsx'
)
).toBe('XLSX spreadsheet');
expect(describeAttachmentType('application/zip', 'logs.zip')).toBe('ZIP archive');
});
it('falls back to the family alone without a usable filename', () => {
expect(describeAttachmentType('application/pdf', null)).toBe('PDF');
expect(describeAttachmentType('image/webp', 'no-extension')).toBe('Image');
});
it('never returns an empty string — a panel with nothing known still says something', () => {
// The chip entry point can open the panel with no MIME and no filename
// at all; an empty answer would render as a stray separator.
expect(describeAttachmentType(null, null)).toBe('File');
expect(describeAttachmentType('', '')).toBe('File');
});
it('reads the filename when the stored MIME is uselessly generic', () => {
expect(describeAttachmentType('application/octet-stream', 'notes.md')).toBe('MD text');
});
});
+107
View File
@@ -25,6 +25,24 @@ import { GENERIC_ICON_ID, isAttachmentIconId, type AttachmentIconId } from './ic
// non-finite input — deliberately (DR-3b). A surface that would rather show
// nothing than "0 B" (the editor chip) keeps that conditional at its own call
// site; the helper does not grow a mode.
/**
* A filename safe to put in a sentence.
*
* `filename` is nominally always present, but a row can carry an empty one —
* an upload with no name, a legacy row — and every surface then renders the
* gap differently: a blank tile, an accessible name that says nothing, and a
* confirmation reading "Delete ?", which is the one place it actually matters
* because the user is being asked to approve something unnamed (final review
* round 4).
*
* Deliberately generic rather than clever: the id is not a name, and inventing
* one from the MIME would claim knowledge the row does not have.
*/
export function displayFilename(filename: string | null | undefined): string {
const trimmed = (filename ?? '').trim();
return trimmed || 'Untitled file';
}
export function formatBytes(bytes: number): string {
if (bytes < 0) return `${bytes} B`;
const KB = 1024;
@@ -221,3 +239,92 @@ export function iconForAttachment(
export function isImage(mime: string): boolean {
return mime.startsWith('image/');
}
/** Human label per icon family, for the "what IS this file" line. */
const FAMILY_LABELS: Record<AttachmentIconId, string> = {
image: 'Image',
video: 'Video',
audio: 'Audio',
document: 'Document',
spreadsheet: 'Spreadsheet',
presentation: 'Presentation',
pdf: 'PDF',
archive: 'Archive',
text: 'Text',
generic: 'File',
};
/**
* A short, human file-type description — "PDF", "PNG image", "XLSX
* spreadsheet", "File" (PLAN-2392 DR-2 / DR-18).
*
* Built on `iconForAttachment` on purpose: the icon and the words beside it
* must never disagree about what a file is, and reading the family from the
* same mapper is the only way to guarantee that. The raw MIME is deliberately
* NOT what surfaces show — `application/vnd.openxmlformats-officedocument.
* spreadsheetml.sheet` is not a type a human reads — but it stays available
* to call sites for a `title`.
*
* The extension is dropped when it merely repeats the family ("PDF · PDF")
* and kept when it adds the specific format ("PNG image"). With neither a
* usable MIME nor an extension the answer is the family fallback, "File" —
* never an empty string, so the line never renders as a stray separator.
*/
export function describeAttachmentType(
mime: string | null | undefined,
filename?: string | null,
): string {
const family = FAMILY_LABELS[iconForAttachment(mime, filename)];
const ext = extensionOf(filename).toUpperCase();
if (!ext || ext === family.toUpperCase()) return family;
return `${ext} ${family.toLowerCase()}`;
}
/**
* 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<string> = 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<string> = 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);
}
+168
View File
@@ -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> = {}): 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<string>();
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 hosts', () => {
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 surfaces 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 chips 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);
});
});
+132
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>();
@@ -115,3 +116,134 @@ 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 whose `ItemDetail` mount should SHOW the panel.
*
* This is ROUTING, not ownership, and the difference is load-bearing:
* it names the host that displays the panel, and it does NOT assert that
* the attachment belongs to that item. The two can genuinely differ — the
* comment composer is reused across an item switch, so a chip sitting in
* an unsubmitted draft can be tapped while the pane shows a different
* item, and it will (correctly) route to the host in front of the user.
*
* Nothing downstream should read it as a permission or an ownership
* claim. Attachment authorization is the SERVER's, per attachment, against
* that attachment's own parent item — `handlers_storage.go` checks
* visibility and then edit permission on the parent it resolves itself,
* and the delete endpoint (`DELETE /workspaces/{ws}/attachments/{id}`) is
* never told which item the client thought it was acting from. What the
* host supplies locally (`mutationsEnabled`) decides whether to OFFER a
* mutation; the server decides whether to perform it.
*
* The one place the distinction leaks into UX: a panel whose "still used
* in this item's content" check runs against the HOST's content can only
* speak for that item, so it must keep the hedged wording ("may still be
* referenced by another item or a comment") rather than claiming the
* attachment is unreferenced.
*/
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;
// 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>();
/**
* 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);
}
@@ -0,0 +1,76 @@
// 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';
let workspaceSlug = 'ws-a';
const read: AttachmentHostAddressReader = () => ({ workspaceSlug, itemId, hostToken });
expect(read()).toEqual({ workspaceSlug: 'ws-a', itemId: 'item-A', hostToken: 'apanel-1' });
itemId = 'item-B';
expect(read()).toEqual({ workspaceSlug: 'ws-a', itemId: 'item-B', hostToken: 'apanel-1' });
// The workspace rides along for the same reason: it keys the metadata
// cache, and the pane switches workspace without remounting.
workspaceSlug = 'ws-b';
expect(read().workspaceSlug).toBe('ws-b');
});
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 = {
workspaceSlug: 'ws-a',
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');
});
});
+75
View File
@@ -0,0 +1,75 @@
/**
* 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 {
/**
* Workspace the editor is currently in. Read through the reader for the
* same reason as the other two: the pane switches workspace without
* remounting, and this value keys the attachment metadata CACHE — a stale
* one makes a mounted chip probe under the previous workspace's key, which
* is a cross-workspace answer to a question about this one.
*/
workspaceSlug: string;
/** 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 = {
workspaceSlug: '',
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.
*
* Deliberately takes only the two ROUTING fields, not a whole address: the
* workspace is carried alongside them for cache keying and says nothing about
* whether an event can find its host.
*/
export function isAddressable(
address: Pick<AttachmentHostAddress, 'itemId' | 'hostToken'> | null | undefined
): boolean {
return Boolean(address?.itemId && address?.hostToken);
}
+36 -1
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. */
@@ -42,6 +43,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 +74,7 @@
placeholder = 'Write a comment…',
wsSlug,
itemId,
hostToken = '',
submitLabel = 'Comment',
submitting = false,
autofocus = false,
@@ -109,6 +122,22 @@
}
}
/**
* 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 => ({
workspaceSlug: wsSlug,
itemId: itemId ?? '',
hostToken
});
const attachmentUrl = (uuid: string, variant?: 'thumb-sm' | 'thumb-md' | 'original') =>
wsSlug ? api.attachments.downloadUrl(wsSlug, uuid, variant) : `pad-attachment:${uuid}`;
@@ -127,13 +156,19 @@
AttachmentImage.configure({
getDownloadUrl: attachmentUrl,
workspaceSlug: wsSlug,
// Panel / viewer addressing (PLAN-2392 DR-8).
address: readHostAddress,
// 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,
address: readHostAddress
}),
AttachmentUpload.configure({
// Wrap upload so the host can track in-flight uploads and gate
// submit — the plugin doesn't expose its placeholder count.
@@ -0,0 +1,124 @@
<!--
AttachmentDeleteConfirm — the ONE delete confirmation for an attachment
(PLAN-2392 DR-18, TASK-2425).
One object must not have two confirmation styles. The options panel drilled
down to an in-app sub-view while the strip's hover `×` still raised a
browser-native `window.confirm` — different chrome, different focus
behaviour, different copy shape, for the same DELETE. This component is what
both surfaces render instead, so the shape can only ever be changed in one
place.
THE SHAPE, copied from the item menu's confirm (ItemDetail, PLAN-2326 DR-6):
- The prompt is `role="presentation"`. A `role="menu"` owns menuitem /
separator / group children, and this says explicitly that the prompt is
none of them — but presentational text is then never announced on its
own, hence...
- ...the `aria-describedby` back-reference from the destructive row, which
is how a screen-reader user hears WHY they are being asked.
- Cancel comes FIRST. The menu's focus handoff lands on the first row, so
any other order would put Enter-on-arrival on Delete.
- The destructive row comes LAST, `danger`-styled.
It renders rows only — no `role="menu"` container of its own. Each caller
supplies its own `Menu` (the panel drills down inside the one it already
has; the strip opens one anchored to the tile's `×`), so ESC ordering,
outside-click, portal placement, focus return and the mobile sheet swap are
the app's existing behaviours on both surfaces rather than a second
implementation.
The PROMPT TEXT lives in this module too, but note there are TWO builders,
not one: `attachmentDeletePrompt` for an item surface, which can check the
body it has and must hedge about the ones it cannot, and
`workspaceAttachmentDeletePrompt` for the workspace-wide storage list, where
a reference check would be meaningless and the honest thing to say is what
happens to the blob. Different questions, so different copy — deliberately.
What must not drift is that BOTH are written here, next to each other, where
a change to one is read alongside the other.
-->
<script lang="ts" module>
import { displayFilename } from '$lib/attachments/display';
/**
* The two arms of the ITEM-surface delete warning.
*
* `referencedHere` can only ever speak for the body the caller has. So the
* "not referenced" arm deliberately does NOT claim the attachment is
* unused: a reference can live in another item's content, in an item's
* fields JSON, or in any comment. The server's `AttachmentReferenced` scan
* covers all three and none of it is visible client-side, so the wording
* stays hedged (DR-5). Do not "tighten" it.
*/
export function attachmentDeletePrompt(
filename: string | null | undefined,
referencedHere: boolean
): string {
// Normalized HERE, not at each call site: a blank name is rare, but a
// confirmation reading "Delete ?" is the one place it actually matters,
// because the user is being asked to approve destroying something the
// prompt cannot name (final review round 4).
const displayName = displayFilename(filename);
return referencedHere
? `Delete ${displayName}? It's still used in this item's content — deleting it will leave a "missing attachment" placeholder where it appears.`
: `Delete ${displayName}? It isn't referenced in this item's content, but it may still be referenced by another item or a comment. This cannot be undone.`;
}
/**
* The storage-list prompt (Settings → Storage).
*
* No reference arm, and that is not an omission: this list is workspace-
* wide and includes attachments with no parent item at all, so "still used
* in this item's content" has nothing to be true or false about. What IS
* worth saying here is what actually happens to the bytes.
*/
export function workspaceAttachmentDeletePrompt(filename: string | null | undefined): string {
return `Delete ${displayFilename(filename)}? The blob is reclaimed by garbage collection after a grace period.`;
}
</script>
<script lang="ts">
import MenuItem from '$lib/components/common/MenuItem.svelte';
interface Props {
/** The warning line — build it with `attachmentDeletePrompt`. */
prompt: string;
/**
* Unique id for the prompt element. Owned by the caller because it is
* the caller that may have several of these on a page; the destructive
* row's `aria-describedby` points at it.
*/
promptId: string;
oncancel: () => void;
onconfirm: () => void;
}
let { prompt, promptId, oncancel, onconfirm }: Props = $props();
</script>
<div class="attachment-delete-prompt" role="presentation" id={promptId}>{prompt}</div>
<MenuItem icon="" onclick={oncancel}>Cancel</MenuItem>
<div class="attachment-delete-divider" role="separator"></div>
<MenuItem icon="🗑" danger describedBy={promptId} onclick={onconfirm}>Delete file</MenuItem>
<style>
/* Logical properties throughout: the prompt carries a filename, which may
be 200 characters or right-to-left. */
.attachment-delete-prompt {
padding-block: 4px 6px;
padding-inline: 9px;
font-size: 12px;
line-height: 1.35;
font-weight: 500;
color: var(--accent-orange);
/* Wraps rather than ellipsizes: the prompt carries the filename and has
to stay readable in full. */
overflow-wrap: anywhere;
}
.attachment-delete-divider {
border-block-start: 1px solid var(--border-subtle);
margin-block: 5px;
margin-inline: 4px;
}
</style>
@@ -0,0 +1,733 @@
<!--
AttachmentDetailsPanel — what a file IS, and what you can do with it
(PLAN-2392 DR-2 / DR-6 / DR-10 / DR-13 / DR-18, TASK-2423).
Tapping a file used to do the most destructive-adjacent thing available:
a strip tile was a bare `<a download>`, so one tap put the file in your
Downloads folder with no way to see what it was first. This panel is what
a tap opens instead — the metadata, then the actions.
PRESENTATION is the existing `Menu` with `sheetOnMobile`: a popover on
desktop, a BottomSheet at the mobile breakpoint (DR-6). No new overlay
primitive, so ESC ordering, outside-click, portal placement and the sheet's
focus handling are the app's existing ones rather than a second
implementation of each.
THE ACTIONS ARE NOT DEFINED HERE. They come from
`$lib/attachments/actions` and are rendered from the descriptor list
(DR-5) — this component chooses between the anchor and button branches of
`MenuItem` on the descriptor's own `element` discriminant, and never calls
`run()` on an anchor (the browser performs those; calling both would fire
the action twice). Adding an action means adding a descriptor, not editing
this file.
IT OPENS IMMEDIATELY AND COMPLETES THE METADATA AFTER (DR-2, DR-10). The
open event's `filename` / `mime_type` / `size_bytes` are nullable by
contract: a chip NodeView knows only what its options give it and fills
these from an asynchronous HEAD probe that may be incomplete or failed.
Awaiting that before opening would make a tap feel broken on a slow
connection, so the panel paints what it was handed and fetches the rest
itself. The three states are distinguishable, deliberately:
- `ok` — gaps filled in place.
- `missing` — the row is gone (404). AUTHORITATIVE: the panel says so
and every action goes inert, rather than offering a
Download that will fail.
- `transient` — an inline, retryable error BESIDE the row it already
knows. Never a blank sheet. Retry goes through
`revalidateAttachmentMetadata`, which invalidates before
refetching — a plain refetch would replay the cached
failure and look broken (DR-10).
THE DELETE CONFIRMATION IS AN IN-APP DRILL-DOWN (DR-18) — and it is the
SAME one the strip's tile shows, `AttachmentDeleteConfirm`, rows and prompt
text both. This panel supplies the sub-view slot; the confirmation owns the
shape (prompt as `role="presentation"`, `aria-describedby` back-reference,
Cancel first, destructive row last) and the two warning arms. It is wired as
the descriptor's `confirmDelete` promise rather than as a bespoke delete
path, so the descriptor's own identity-snapshot and permission re-check
across the confirmation stay in force.
SWITCH-SAFETY. The host swaps this component's props from one attachment
to another without a `{#key}` remount (a second tap while the panel is
open), so every await-then-write is fenced through
`$lib/attachments/viewFence` against the (workspace, attachment) pair —
the same bug class as the strip's. Read that module's header for why there
are three fences.
NOT HERE: `state_generation` and Undo. Delete behaves exactly like today's
tile delete; the generation token and the Undo toast land across all three
entry points at once in PLAN-2411 (DR-19).
-->
<script lang="ts">
import { onDestroy, untrack } from 'svelte';
import Menu from '$lib/components/common/Menu.svelte';
import MenuItem from '$lib/components/common/MenuItem.svelte';
import AttachmentIcon from '$lib/attachments/icons/AttachmentIcon.svelte';
import AttachmentDeleteConfirm, {
attachmentDeletePrompt,
} from './AttachmentDeleteConfirm.svelte';
import {
attachmentActionsFor,
type AttachmentActionContext,
type ButtonAttachmentAction,
} from '$lib/attachments/actions';
import {
describeAttachmentType,
displayFilename,
formatBytes,
iconForAttachment,
} from '$lib/attachments/display';
import { api } from '$lib/api/client';
import {
fetchAttachmentMetadata,
revalidateAttachmentMetadata,
} from '$lib/components/editor/attachment-metadata';
import { attachmentRefsIn } from '$lib/utils/commentAttachments';
import { toastStore } from '$lib/stores/toast.svelte';
import { createFence, createPaintFence, viewIdentity } from '$lib/attachments/viewFence';
interface Props {
open: boolean;
wsSlug: string;
attachmentId: string;
/**
* Seed metadata from the open event. All three are NULLABLE by
* contract (DR-2) — the strip populates them from its list row, a chip
* may have none of them yet.
*/
filename: string | null;
mimeType: string | null;
sizeBytes: number | null;
/** Element the panel positions against and returns focus to. */
anchor: HTMLElement | null;
/**
* Supplied by the HOST from its own `computeMutationsEnabled(canEdit,
* peeking)` — never by the emitting surface, which has no mutation
* context (DR-8). Delete is absent-as-disabled without it.
*/
mutationsEnabled: boolean;
/** Persisted item body, for the "still used here" delete warning. */
itemContent?: string | null;
/**
* The editor's LIVE markdown. `itemContent` lags by design (saved on
* flush, not per keystroke), so an image inserted seconds ago wouldn't
* trip the warning for exactly the attachment a user is most likely to
* delete by mistake. Consulted at confirm time only.
*/
liveContent?: (() => string | null) | null;
/**
* Bumped by the host to force a fresh metadata read — parent-item
* restore revalidates rather than assuming the prior state holds
* (DR-14).
*/
revalidateToken?: number;
/**
* The parent item is archived, so attachment reads will 404 — the server
* refuses them for an archived parent. The seed metadata the event
* carried is therefore not trustworthy as evidence the file is
* REACHABLE, and the panel must probe even when it looks complete, so it
* lands in the authoritative missing state rather than offering a
* Download that fails on click (DR-14).
*/
parentArchived?: boolean;
onclose: () => void;
onDeleted?: (attachmentId: string) => void;
}
let {
open,
wsSlug,
attachmentId,
filename,
mimeType,
sizeBytes,
anchor,
mutationsEnabled,
itemContent = null,
liveContent = null,
revalidateToken = 0,
parentArchived = false,
onclose,
onDeleted,
}: Props = $props();
/**
* How long a metadata read may hang before the panel calls it a failure.
* Generous: this is the "something is wrong" threshold, not a latency
* budget — a slow answer that arrives still wins.
*/
const METADATA_SLOW_MS = 10_000;
const uid = $props.id();
const promptId = `attachment-delete-note-${uid}`;
// What the server told us, filling the gaps in what the event carried.
let fetchedMime = $state<string | null>(null);
let fetchedSize = $state<number | null>(null);
let loading = $state(false);
/** 404 — authoritative. Actions go inert. */
let missing = $state(false);
/** Non-404 failure — inline, retryable, alongside what we already know. */
let loadFailed = $state(false);
let view = $state<'root' | 'delete'>('root');
let busy = $state(false);
let actionError = $state<string | null>(null);
let deletePrompt = $state('');
/** Bumped by Retry; drives the loader effect's forced-revalidate path. */
let forceReload = $state(0);
// --- fences (see $lib/attachments/viewFence) ------------------------------
// The identity of what this panel is showing. The PAIR, not the id alone:
// the workspace half is what a hand-rolled fence keeps forgetting.
const identity = viewIdentity(() => ({ ws: wsSlug, att: attachmentId }));
// 1. Request fence — restarted per metadata read, so a Retry supersedes
// its own predecessor and only the newest response may write.
const loadFence = createFence(identity);
// 2. View fence — invalidated only when the panel really changes subject,
// so an in-flight delete of the attachment still on screen can still
// reconcile even after a Retry reloaded its metadata.
const viewFence = createFence(identity);
// 3. Paint fence — "does the control the user clicked belong to what is on
// screen?" Checked at ENTRY by every control, because the other two run
// after an await and no fence can unsend a request.
const paint = createPaintFence(identity);
// Plain `let`, never $state: read and written only inside effects, and a
// $state here would make the effect below depend on what it writes
// (CONVE-1688 — the self-write loop that silently aborts the flush).
let paintedKey: string | null = null;
/**
* The reload stamp this component has already acted on — the host's
* revalidate signal and the local Retry counter together. Seeded from the
* incoming prop so a host that has already bumped its counter (an earlier
* restore, before this panel existed) doesn't read as a pending reload on
* the first render.
*/
let seenReload = untrack(() => `${revalidateToken}:0`);
/** Resolver for the in-app confirmation currently on screen, if any. */
let pendingConfirm: ((confirmed: boolean) => void) | null = null;
/**
* Teardown latch and the deferred-close timer. Plain `let`, not `$state`:
* nothing renders from them, and they are read by continuations that must
* see the CURRENT value rather than a reactive snapshot.
*/
let destroyed = false;
let deferredClose: ReturnType<typeof setTimeout> | undefined;
/**
* Counts confirmed deletes. `run()` resolves the same way whether the row
* was deleted or the user CANCELLED the confirmation, so closing on a
* resolved delete would dismiss the panel out from under a cancel — this is
* how the two are told apart.
*/
let deleteSignal = 0;
const displayName = $derived(displayFilename(filename));
/**
* An archived parent's reachability probe is still in flight.
*
* Normally the panel offers its actions immediately — that is DR-2's whole
* point, and waiting on a probe would make a tap feel broken. But when the
* parent is archived we already know reads are refused; the probe is only
* confirming it. Offering Download and Open in that window hands the user
* an action whose sole outcome is a 404 (final review round 6).
*/
const unreachablePending = $derived(parentArchived && loading && !missing);
// The event's value wins when it has one — it came from a list row, which
// is at least as good as a HEAD and is available before any fetch.
const mime = $derived(mimeType || fetchedMime || '');
const size = $derived(sizeBytes ?? fetchedSize);
const iconId = $derived(iconForAttachment(mime || null, filename));
const typeLabel = $derived(describeAttachmentType(mime || null, filename));
// Always says at least the type — a panel that opened on a chip with no
// metadata at all still has to describe SOMETHING while the HEAD is in
// flight, and "Reading details…" replaces only the part that is genuinely
// unknown rather than the whole line (DR-10).
const metaLine = $derived(
missing
? 'No longer available'
: [
typeLabel,
size !== null && size !== undefined
? formatBytes(size)
: loading
? 'Reading details…'
: null,
]
.filter(Boolean)
.join(' · ')
);
/**
* The panel's accessible name carries filename, type and size (DR-12) —
* and the FULL filename, unelided: truncation is a visual affordance, never
* an information loss (DR-13).
*/
const panelLabel = $derived(`Options for ${displayName}, ${metaLine}`);
/**
* The action context. Built with GETTERS rather than as a snapshot object:
* the delete descriptor deliberately re-reads `mutationsEnabled` and the
* attachment identity on the far side of the confirmation, and a frozen
* object would make those re-checks read the values as they were when the
* confirmation opened — exactly the staleness they exist to catch.
*/
const ctx: AttachmentActionContext = {
get workspaceSlug() {
return wsSlug;
},
get attachment() {
return { id: attachmentId, filename: filename ?? '', mime_type: mime };
},
get mutationsEnabled() {
// PERMISSION only — "may this user delete here", which is the host's
// answer and nothing else. Deliberately NOT `&& !missing`: that
// conflates permission with reachability, and once the descriptor
// started using this to decide whether Delete EXISTS, a gone row
// lost the row entirely while Open and Download stayed present and
// disabled beside it. Reachability is enforced where it belongs —
// the render site disables every action while `missing`, and a
// delete that races a deletion elsewhere 404s, which the descriptor
// already treats as authoritative.
return mutationsEnabled;
},
confirmDelete: () => confirmDelete(),
onDeleted: (id) => {
deleteSignal += 1;
onDeleted?.(id);
},
onCopied: () => toastStore.show('Link copied to clipboard', 'success'),
};
const actions = $derived(attachmentActionsFor(ctx));
function downloadUrl(uuid: string, variant?: 'thumb-sm' | 'thumb-md' | 'original'): string {
return api.attachments.downloadUrl(wsSlug, uuid, variant);
}
/**
* Load whatever the event didn't carry, and re-read on demand.
*
* Reads only props + the fence identity in tracked scope; every piece of
* state it writes (`loading`, `missing`, `fetched*`) is read in the markup
* and in `untrack`ed blocks only, so the effect cannot self-invalidate.
*/
$effect(() => {
const req = loadFence.restart();
const isOpen = open;
const seedMime = mimeType;
const seedSize = sizeBytes;
const reloadStamp = `${revalidateToken}:${forceReload}`;
const archivedParent = parentArchived;
let forced = false;
untrack(() => {
// A genuine subject change: drop everything the previous attachment
// left behind, stop any in-flight continuation from reconciling, and
// abandon a confirmation that was up for a file the user is no longer
// looking at.
if (req.key !== paintedKey) {
paintedKey = req.key;
viewFence.invalidate();
settleConfirm(false);
fetchedMime = null;
fetchedSize = null;
missing = false;
loadFailed = false;
busy = false;
actionError = null;
}
// Whatever this run paints belongs to this (workspace, attachment).
// An un-addressable token records nothing, which correctly stops the
// panel's controls claiming the previous subject.
paint.record(req);
// An archived parent makes this a REACHABILITY question, and the
// metadata cache can hold an `ok` observed before the archive — the
// same reason existence probes elsewhere revalidate rather than
// read. So force it, which also routes through the invalidate-then-
// fetch path instead of replaying a stale success.
if (archivedParent) forced = true;
if (reloadStamp !== seenReload) {
seenReload = reloadStamp;
forced = true;
// A forced revalidation exists because the previous answer may no
// longer hold — the parent was just restored (DR-14). So drop the
// latched `missing` NOW rather than only on an `ok`: if this
// revalidation comes back transient, the render must fall through
// to the retryable error. Leaving it latched shows "no longer
// available" with no way to ask again, which is the exact
// empty-vs-broken confusion DR-10 exists to prevent.
missing = false;
}
});
if (!isOpen || req.key === null) return;
// Nothing to complete: the strip's entry point always has all three.
// Unless the parent is archived — then "complete" and "reachable" are
// different claims, and only a probe settles the second one.
if (!forced && !archivedParent && seedMime && seedSize !== null && seedSize !== undefined) {
return;
}
loading = true;
loadFailed = false;
// A HEAD that never settles is not a failure the fetch layer can report:
// no rejection arrives, so without this the panel sits on "Reading
// details…" forever with no Retry — indistinguishable to the user from
// a hang, and the exact loading-vs-failed confusion DR-10 exists to
// prevent. The request is NOT aborted: if it does eventually answer, it
// is still the truth and still allowed to correct the error state.
const slowTimer = setTimeout(() => {
if (req.stale()) return;
loading = false;
loadFailed = true;
}, METADATA_SLOW_MS);
void (async () => {
// The workspace comes off the TOKEN, not the live prop: the request
// must name the workspace it was issued for even if the panel has
// since moved on.
const result = forced
? await revalidateAttachmentMetadata(req.value.ws, req.value.att, downloadUrl)
: await fetchAttachmentMetadata(req.value.ws, req.value.att, downloadUrl);
clearTimeout(slowTimer);
if (req.stale()) return;
loading = false;
if (result.status === 'ok') {
fetchedMime = result.mime;
fetchedSize = result.size;
missing = false;
loadFailed = false;
} else if (result.status === 'missing') {
// Authoritative. Latch it — the actions go inert below.
missing = true;
loadFailed = false;
} else {
// Says nothing about whether the row exists: keep showing what we
// have and stay retryable.
loadFailed = true;
}
})();
});
/**
* Permission withdrawn while the confirmation is open.
*
* The pane can go peeked (or a role can change) between opening the
* confirmation and answering it. Blocking the eventual request is not
* enough: the user is left looking at a live "Delete file" button for an
* action that can no longer happen, on a surface that is supposed to offer
* no delete at all in that state. So the confirmation is abandoned as a
* rejection, exactly as a subject change abandons it.
*
* Plain latch + `untrack` for the writes: as `$state` this effect would
* depend on what it writes, which aborts the flush and strands unrelated
* reactivity (CONVE-1688).
*/
$effect(() => {
const mayMutate = mutationsEnabled;
untrack(() => {
if (mayMutate || view !== 'delete') return;
settleConfirm(false);
});
});
function retry() {
// ENTRY fence: the clicked row was painted for `paint`'s identity, and
// the live props may already name a different attachment.
if (!paint.isCurrent()) return;
loadFailed = false;
// Goes through the loader effect's revalidate path rather than fetching
// here, so a user Retry and the host's restore signal (DR-14) are ONE
// code path — and both therefore invalidate before refetching, which is
// the whole point of Retry (DR-10).
forceReload += 1;
}
/**
* Ids referenced by THIS item's body. A hit means deleting leaves a
* missing-attachment placeholder in the content, which the user deserves
* to know before confirming. Read at confirm time (not derived) so it sees
* unflushed editor edits.
*/
function referencedHere(): boolean {
let live: string | null = null;
try {
live = liveContent?.() ?? null;
} catch {
live = null;
}
return new Set(attachmentRefsIn(live ?? itemContent ?? '')).has(attachmentId);
}
/**
* The confirmation, as a promise the delete descriptor awaits. The
* descriptor snapshots identity BEFORE this and re-checks permission after
* it resolves, which is the whole reason it is wired this way rather than
* as a bespoke "confirm, then call the API" path here.
*
* The wording comes from the shared `attachmentDeletePrompt` — the same
* two arms the strip's tile shows (DR-18). The hedged arm matters here for
* one EXTRA reason beyond the shared one: the body this checks is the
* HOST's, which is not necessarily the attachment's parent item. The
* open-panel event's `itemId` is ROUTING, not ownership: a chip in a reused
* comment composer's unsubmitted draft correctly routes to the host in
* front of the user even after an item switch.
*/
function confirmDelete(): Promise<boolean> {
deletePrompt = attachmentDeletePrompt(displayName, referencedHere());
return new Promise<boolean>((resolve) => {
// Supersede any confirmation already up — two open at once would
// leave one resolver dangling forever.
pendingConfirm?.(false);
pendingConfirm = resolve;
view = 'delete';
});
}
function settleConfirm(confirmed: boolean) {
const resolve = pendingConfirm;
pendingConfirm = null;
view = 'root';
resolve?.(confirmed);
}
async function runAction(action: ButtonAttachmentAction) {
if (!paint.isCurrent()) return;
if (!action.enabled(ctx)) return;
// Fence 2: a subject change mid-action must not write this action's
// outcome onto a DIFFERENT attachment's panel. The request itself still
// lands — it targets an id, not a view.
const token = viewFence.begin();
const deletesBefore = deleteSignal;
actionError = null;
busy = true;
try {
await action.run(ctx);
if (token.stale()) return;
// Only when the row actually went: `run()` also resolves when the
// user cancelled the confirmation, and closing then would dismiss
// the panel out from under a Cancel.
if (deleteSignal !== deletesBefore) onclose();
} catch (err) {
if (token.stale()) return;
actionError = err instanceof Error ? err.message : `Couldn't ${action.label.toLowerCase()}`;
} finally {
if (!token.stale()) busy = false;
}
}
/**
* Teardown (PLAN-2392, orchestrator review).
*
* The host destroys this component by nulling its request — on an item
* switch, on archive, on close. Without this, a delete that resolves after
* that point still runs its continuation and calls `onclose()`, which
* closes whatever panel the host has open BY THEN: attachment A's delete
* dismissing the panel the user just opened on B. And a confirmation left
* on screen at teardown would never resolve, stranding the descriptor's
* `await` forever.
*
* So teardown does what a subject change does: invalidate the fences so
* every in-flight continuation reads stale, and reject any pending
* confirmation.
*/
onDestroy(() => {
destroyed = true;
clearTimeout(deferredClose);
deferredClose = undefined;
viewFence.invalidate();
loadFence.invalidate();
paint.record(null);
const resolve = pendingConfirm;
pendingConfirm = null;
resolve?.(false);
});
function handleClose() {
// A confirmation still on screen when the panel closes is a rejection:
// leaving the promise unresolved would strand the descriptor's `await`
// forever.
settleConfirm(false);
onclose();
}
/**
* Anchor rows navigate/download by their DEFAULT ACTION, so the close is
* deferred to a macrotask. Closing synchronously detaches the `<a>` during
* its own click handler, and a detached anchor's navigation is cancelled in
* some browsers — the download would silently not happen.
*/
function closeAfterNavigation() {
// Fenced like every other continuation: the timer outlives the click, so
// by the time it fires the panel may have been reopened on a DIFFERENT
// attachment (tap a chip, tap another). Closing then would dismiss a
// panel the user just opened. Cleared on teardown so it cannot fire into
// a destroyed component either.
const token = viewFence.begin();
clearTimeout(deferredClose);
deferredClose = setTimeout(() => {
deferredClose = undefined;
if (destroyed) return;
if (token.stale()) return;
if (!paint.isCurrent()) return;
handleClose();
}, 0);
}
</script>
<Menu
{open}
onclose={handleClose}
trigger={anchor ?? undefined}
mode="portal"
width={272}
sheetOnMobile
sheetTitle={displayName}
ariaLabel={panelLabel}
focusKey={`${attachmentId}:${view}`}
>
{#if view === 'root'}
<!-- Header. `role="presentation"`, like the item menu's confirm note:
a role="menu" owns menuitem / separator / group children, and this
says explicitly that the header is none of them. -->
<div class="ap-header" role="presentation">
<span class="ap-icon" aria-hidden="true"><AttachmentIcon id={iconId} size={22} /></span>
<span class="ap-head-text">
<!-- The full name stays in `title` and in the panel's accessible
name; the ellipsis is visual only (DR-13). -->
<span class="ap-name" title={displayName}>{displayName}</span>
<span class="ap-meta" class:ap-meta-missing={missing} title={mime || undefined}>
{metaLine}
</span>
</span>
</div>
{#if missing}
<div class="ap-note ap-note-missing" role="presentation">
This file is no longer available. It may have been deleted.
</div>
{:else if loadFailed}
<!-- Beside what we already know, never instead of it (DR-10). -->
<div class="ap-note ap-note-error" role="presentation">Couldn't load the file details.</div>
<MenuItem icon="↻" onclick={retry}>Retry</MenuItem>
{/if}
{#if actionError}
<div class="ap-note ap-note-error" role="presentation">{actionError}</div>
{/if}
<div class="menu-divider" role="separator"></div>
{#each actions as action (action.id)}
{#if action.element === 'anchor'}
<MenuItem
icon={action.icon}
href={action.href(ctx)}
download={action.download?.(ctx)}
target={action.target}
rel={action.rel}
title={action.description}
disabled={!action.enabled(ctx) || missing || unreachablePending}
onclick={closeAfterNavigation}
>
{action.label}
</MenuItem>
{:else}
<MenuItem
icon={action.icon}
danger={action.danger}
title={action.description}
disabled={!action.enabled(ctx) || missing || busy || unreachablePending}
onclick={() => runAction(action)}
>
{busy && action.id === 'delete' ? 'Deleting…' : action.label}
</MenuItem>
{/if}
{/each}
{:else}
<!--
Delete confirmation as a drill-down sub-view (DR-18). The shape and
the wording live in the shared component, which the strip's tile
renders too — one confirmation for one object.
-->
<AttachmentDeleteConfirm
prompt={deletePrompt}
{promptId}
oncancel={() => settleConfirm(false)}
onconfirm={() => settleConfirm(true)}
/>
{/if}
</Menu>
<style>
/* Every rule uses LOGICAL properties (DR-13): the panel has to survive a
200-character filename and an RTL locale without pushing its actions
off-screen. */
.ap-header {
display: flex;
align-items: flex-start;
gap: 9px;
padding-block: 7px 8px;
padding-inline: 9px;
}
.ap-icon {
flex: 0 0 auto;
color: var(--text-secondary);
margin-block-start: 1px;
}
/* min-width: 0 on every flex child holding the filename, or the ellipsis
below never engages and the row grows instead. */
.ap-head-text {
display: flex;
flex-direction: column;
gap: 2px;
min-width: 0;
flex: 1 1 auto;
}
.ap-name {
min-width: 0;
font-size: 13px;
font-weight: 600;
color: var(--text-primary);
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.ap-meta {
min-width: 0;
font-size: 11.5px;
color: var(--text-muted);
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
.ap-meta-missing {
color: var(--accent-orange);
}
.ap-note {
padding-block: 4px 6px;
padding-inline: 9px;
font-size: 12px;
line-height: 1.35;
overflow-wrap: anywhere;
}
.ap-note-error {
color: var(--accent-red);
}
.ap-note-missing {
color: var(--text-muted);
}
.menu-divider {
border-block-start: 1px solid var(--border-subtle);
margin-block: 5px;
margin-inline: 4px;
}
</style>
@@ -0,0 +1,198 @@
<!--
AttachmentPanelHost — the ONE consumer of the open-panel channel for one
`ItemDetail` mount (PLAN-2392 DR-8 / DR-14, TASK-2423).
The emitters — strip tiles, editor chip NodeViews, comment-composer chips —
cannot mount a Svelte component (a Tiptap NodeView is imperative DOM), so
they signal through the module-global bus in `$lib/attachments/events`.
Something has to own the panel on the other side. That owner is
`ItemDetail`, which mounts exactly one of these.
WHY A SEPARATE COMPONENT rather than a block inside `ItemDetail`: the
addressing rule below is the load-bearing part of DR-8 and has to be
testable with TWO hosts mounted at once, which is what the pane host
actually does at runtime (a master pane plus a peeked pane). Folded into a
6,000-line component it would be unreachable by any test. `ItemDetail` is
still the host in every sense that matters — it mints the token, it supplies
the permission, and it is the only mount site.
ADDRESSING. A host consumes an event only when BOTH `itemId` and
`hostToken` are its own (`isAttachmentPanelEventForHost`). Matching on the
item alone is not enough — both panes can show the same item — and matching
on the token alone is not enough either, since a host must not open a panel
for an attachment belonging to a different item.
PERMISSION NEVER TRAVELS ON THE EVENT. `mutationsEnabled` is the host's own
`computeMutationsEnabled(canEdit, peeking)`. Not the NodeView's (it has no
mutation context at all), and not `ItemTimeline`'s `canEdit`, which ignores
`peeking` and would let a peeked pane mutate.
PARENT LIFECYCLE (DR-14). Attachment GET/HEAD rejects an archived parent
with a generic 404, while `ItemDetail` keeps the archived item and its
attachment surfaces mounted — so an open panel would keep offering an Open
and a Download that now fail. Archiving therefore CLOSES the panel, and
restoring REVALIDATES it rather than assuming the previous state still
holds. Both arrive here declaratively as `parentArchived`, which the host
derives from the item it already refetches on the SSE lifecycle events.
-->
<script lang="ts">
import { untrack } from 'svelte';
import AttachmentDetailsPanel from './AttachmentDetailsPanel.svelte';
import {
isAttachmentPanelEventForHost,
registerAttachmentDeletionListener,
registerAttachmentPanelListener,
type AttachmentPanelOpenEvent,
} from '$lib/attachments/events';
interface Props {
wsSlug: string;
/** Parent item UUID. Null/undefined while the item is loading or mid-switch. */
itemId: string | null | undefined;
/** This `ItemDetail` mount's identity on the bus. */
hostToken: string;
/** The host's own mutation gate — `canEdit && !peeking`. */
mutationsEnabled: boolean;
/** Persisted item body, for the delete confirmation's contextual warning. */
itemContent?: string | null;
/** Accessor for the editor's live markdown; see the panel's prop docs. */
liveContent?: (() => string | null) | null;
/** Whether the parent item is currently archived (DR-14). */
parentArchived?: boolean;
}
let {
wsSlug,
itemId,
hostToken,
mutationsEnabled,
itemContent = null,
liveContent = null,
parentArchived = false,
}: Props = $props();
let request = $state<AttachmentPanelOpenEvent | null>(null);
/**
* Builds a close handler BOUND to the request it was rendered for, so a
* stale one cannot close a newer panel.
*
* The child is destroyed by nulling `request` (item switch, archive), but a
* continuation it already started — a delete resolving, a deferred close
* after a download — can still call back afterwards. Unbound, that call
* would clear whatever request is current BY THEN, dismissing a panel the
* user just opened on a different attachment.
*
* The child fences its own continuations too; this is the same invariant
* enforced at the boundary, where it holds no matter what any future child
* does with its internals.
*/
function closeRequest(target: AttachmentPanelOpenEvent | null): () => void {
return () => {
if (target && request !== target) return;
request = null;
};
}
let revalidateToken = $state(0);
// Plain `let`, not $state: written and read only inside `untrack`ed effect
// bodies. As $state they would make each effect depend on what it writes,
// which aborts the flush and strands unrelated reactivity (CONVE-1688).
// Seeded from the initial prop DELIBERATELY (hence `untrack`): a host that
// mounts on an already-archived item has nothing to close and nothing to
// revalidate — only a TRANSITION is a lifecycle event.
let wasArchived = untrack(() => parentArchived === true);
let lastItemId = '';
// Subscribe once. `itemId` / `hostToken` are read inside the callback at
// EMIT time, so the comparison always uses the host's current address —
// deliberately not captured, since this component (like `ItemDetail`) can
// outlive an A→B item switch.
$effect(() => {
return registerAttachmentPanelListener((event) => {
if (!isAttachmentPanelEventForHost(event, { itemId, hostToken })) return;
// Opening on an ALREADY-archived parent, not just archiving with the
// panel open (orchestrator's full-diff review). The transition
// handler below cannot see this case, and the strip's event carries
// complete metadata — which is exactly what lets the panel skip its
// probe — so Open, Download and Copy link would render enabled and
// point at endpoints that 404, because attachment reads refuse an
// archived parent (handlers_attachments.go).
//
// So the panel is TOLD the seeds can't be trusted (see its
// `parentArchived` prop) and probes anyway: the 404 comes back as the
// authoritative `missing` state it already knows how to show, and the
// actions go inert through the path that exists for it. One state
// machine, not a second archived-specific one.
//
// Deliberately a prop rather than bumping `revalidateToken` here: the
// panel is created by this same assignment, so it would seed its
// reload stamp from the ALREADY-incremented value and see no change.
request = event;
});
});
// Someone else deleted the attachment this panel is about — the other pane's
// strip, or the panel in a peeked ItemDetail. The strip already reconciles
// on this channel; without it here, the panel keeps offering Download and
// Delete for a row that is gone (orchestrator's full-diff review round 2).
//
// Close rather than latch the missing state: unlike a 404 discovered while
// opening — where the user asked about THIS file and deserves an answer —
// this is an answer to a question nobody asked, and leaving a tombstone
// panel on screen would be stranger than dismissing it.
$effect(() => {
return registerAttachmentDeletionListener((deletedUuid) => {
if (request?.attachmentId === deletedUuid) request = null;
});
});
// A→B item switch: the open panel belongs to the item that is no longer on
// screen, and its Delete would be permissioned by the NEW item's gate.
$effect(() => {
const id = itemId ?? '';
untrack(() => {
if (id === lastItemId) return;
lastItemId = id;
request = null;
});
});
// Archive closes; restore revalidates (DR-14).
$effect(() => {
const archived = parentArchived === true;
untrack(() => {
if (archived === wasArchived) return;
wasArchived = archived;
if (archived) request = null;
else revalidateToken += 1;
});
});
</script>
<!--
The `request?.` guards are load-bearing, not defensive noise: props are
getters the child reads LAZILY, and a delete's own continuation reads them
again (through the panel's view fence) after `onDeleted` has already nulled
`request` — a bare `request.attachmentId` throws there, on the success path.
Reading through to an empty id is the right answer for that read: an
un-addressable identity fails the fence, which is precisely what should
happen to a continuation whose panel is gone.
-->
{#if request}
<AttachmentDetailsPanel
open={true}
{wsSlug}
attachmentId={request?.attachmentId ?? ''}
filename={request?.filename ?? null}
mimeType={request?.mime_type ?? null}
sizeBytes={request?.size_bytes ?? null}
anchor={request?.anchor ?? null}
{mutationsEnabled}
{itemContent}
{liveContent}
{revalidateToken}
parentArchived={parentArchived === true}
onclose={closeRequest(request)}
/>
{/if}
@@ -0,0 +1,702 @@
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<void>>();
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<AttachmentMetadataResult>>();
const revalidateMetaMock = vi.fn<() => Promise<AttachmentMetadataResult>>();
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<typeof import('$lib/attachments/events')>();
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<Parameters<typeof notifyAttachmentPanelOpen>[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 <body>, so queries are document-wide by necessity. */
function panel(): HTMLElement | null {
return document.querySelector<HTMLElement>('[role="menu"]');
}
function rows(): HTMLElement[] {
return Array.from(document.querySelectorAll<HTMLElement>('[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<HostProps>({
wsSlug: 'ws',
itemId: 'item-a',
hostToken: 'host-1',
mutationsEnabled: true,
itemContent: null,
liveContent: null,
parentArchived: false,
});
const propsB = $state<HostProps>({
wsSlug: 'ws',
itemId: 'item-a',
hostToken: 'host-2',
mutationsEnabled: false,
itemContent: null,
liveContent: null,
parentArchived: false,
});
describe('AttachmentPanelHost', () => {
let target: HTMLElement;
const mounted: ReturnType<typeof mount>[] = [];
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 with NO Delete at all — absent
// rather than disabled, matching the strip in the same state.
expect(row('Delete')).toBeUndefined();
// ...and it is genuinely the panel, not an empty menu.
expect(row('Download')).toBeDefined();
});
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
// <a download>/<a target> 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<AttachmentMetadataResult>((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();
// One word for a nameless file across every surface (`displayFilename`):
// the panel used to say "Attachment" while the tile and the delete
// prompt said something else.
expect(panel()?.textContent).toContain('Untitled file');
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. Delete goes ABSENT, not disabled — the strip
// hides its delete control outright in the same state, and two views of
// one object must not show different affordances for one permission.
propsA.mutationsEnabled = false;
flushSync();
expect(row('Delete')).toBeUndefined();
});
it('abandons an open confirmation when permission is withdrawn mid-decision', async () => {
// The pane can go peeked between opening the confirmation and answering
// it. Blocking the eventual request is not enough — the user is left
// looking at a live "Delete file" for an action that can no longer
// happen, on a surface meant to offer no delete at all in that state.
mountHost(propsA);
notifyAttachmentPanelOpen(openEvent());
await settle();
row('Delete')!.click();
await settle();
expect(row('Delete file')).toBeDefined();
propsA.mutationsEnabled = false;
await settle();
expect(row('Delete file')).toBeUndefined();
// Rejected, not merely hidden: nothing was sent.
expect(deleteMock).not.toHaveBeenCalled();
// And Delete is gone from the actions entirely, as in a peeked pane.
expect(row('Delete')).toBeUndefined();
expect(row('Download')).toBeDefined();
});
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('.attachment-delete-prompt');
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('.attachment-delete-prompt')?.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('.attachment-delete-prompt')?.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: reads 404, so the panel
// latches the authoritative missing state. It goes through the
// INVALIDATING path — reachability is an existence question and the
// cache can hold an `ok` from before the archive.
propsA.parentArchived = true;
revalidateMetaMock.mockResolvedValue({ status: 'missing' });
mountHost(propsA);
notifyAttachmentPanelOpen(openEvent({ mime_type: null, size_bytes: null }));
await settle();
expect(panel()?.textContent).toContain('This file is no longer available.');
// Restore does NOT assume the previous state still holds — it re-reads
// through the same path, and the panel comes back to life.
revalidateMetaMock.mockResolvedValue({ status: 'ok', mime: 'application/pdf', size: 1536 });
propsA.parentArchived = false;
await settle();
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}`
);
});
// --- stale-continuation regressions (orchestrator review) -----------------
// All three are the same shape: something the panel started keeps running
// after the host has moved on, and lands on whatever is on screen by then.
it("a delete resolving after an item switch does not close the panel opened since", async () => {
mountHost(propsA);
let releaseDelete: (() => void) | undefined;
deleteMock.mockImplementation(
() => new Promise<void>((resolve) => (releaseDelete = () => resolve()))
);
// Attachment A: confirm the delete, leaving the request in flight.
notifyAttachmentPanelOpen(openEvent());
await settle();
(row('Delete') as HTMLElement).click();
await settle();
(row('Delete file') as HTMLElement).click();
await settle();
// The host switches items, destroying that panel...
propsA.itemId = 'item-b';
flushSync();
expect(panel()).toBeNull();
// ...and a panel opens on a DIFFERENT attachment for the new item.
notifyAttachmentPanelOpen(
openEvent({ itemId: 'item-b', attachmentId: ATT_ID_2, filename: 'notes.txt' })
);
await settle();
expect(panel()?.textContent).toContain('notes.txt');
// Now the first delete finally lands.
releaseDelete?.();
await settle();
// It must not dismiss the panel the user just opened.
expect(panel()).not.toBeNull();
expect(panel()?.textContent).toContain('notes.txt');
});
it('resolves a confirmation left on screen when the host tears the panel down', async () => {
mountHost(propsA);
notifyAttachmentPanelOpen(openEvent());
await settle();
(row('Delete') as HTMLElement).click();
await settle();
expect(panel()?.textContent).toContain('Delete file');
// Archive destroys the child mid-confirmation. The descriptor is
// awaiting that promise; leaving it unresolved strands the await (and
// the closure it holds) forever.
propsA.parentArchived = true;
await settle();
expect(panel()).toBeNull();
// Nothing was deleted, and no late continuation runs.
expect(deleteMock).not.toHaveBeenCalled();
expect(announceMock).not.toHaveBeenCalled();
});
it('closes when another surface deletes the attachment it is showing', async () => {
// The strip already reconciles on this channel. Without it here, a panel
// opened from a peeked pane keeps offering Download and Delete for a row
// the other pane just removed (final review round 2). It CLOSES rather
// than latching the missing state: unlike a 404 found while opening,
// where the user asked about this file and deserves the answer, this is
// an answer to a question nobody asked.
const { notifyAttachmentDeleted } = await import('$lib/attachments/events');
mountHost(propsA);
notifyAttachmentPanelOpen(openEvent());
await settle();
expect(panel()).not.toBeNull();
// A DIFFERENT attachment going away must not disturb it.
notifyAttachmentDeleted(ATT_ID_2);
await settle();
expect(panel()).not.toBeNull();
notifyAttachmentDeleted(ATT_ID);
await settle();
expect(panel()).toBeNull();
});
it('calls a hung metadata read a failure rather than loading forever', async () => {
// A HEAD that never settles produces no rejection, so nothing downstream
// ever fires: without a deadline the panel sits on "Reading details…"
// with no Retry, which to the user is indistinguishable from a hang
// (final review round 4).
vi.useFakeTimers();
try {
mountHost(propsA);
fetchMetaMock.mockReturnValue(new Promise(() => {}));
notifyAttachmentPanelOpen(openEvent({ mime_type: null, size_bytes: null }));
await settle();
expect(panel()?.textContent).toContain('Reading details');
await vi.advanceTimersByTimeAsync(10_000);
flushSync();
expect(panel()?.textContent).not.toContain('Reading details');
expect(panel()?.textContent).toContain("Couldn't load the file details.");
expect(row('Retry')).toBeDefined();
// It still shows what it already knew — never a blank sheet (DR-10).
expect(panel()?.textContent).toContain('spec.pdf');
} finally {
vi.useRealTimers();
}
});
it('probes even when the event carries full metadata, if the parent is archived', async () => {
// The strip's event carries all three fields, which normally lets the
// panel skip the fetch. On an archived parent that would leave Open,
// Download and Copy link enabled against endpoints that 404 — attachment
// reads refuse an archived parent — so opening here must probe anyway
// and land in the authoritative missing state.
propsA.parentArchived = true;
mountHost(propsA);
revalidateMetaMock.mockResolvedValue({ status: 'missing' });
notifyAttachmentPanelOpen(openEvent());
await settle();
expect(revalidateMetaMock).toHaveBeenCalled();
expect(panel()?.textContent).toContain('This file is no longer available.');
// Inert, per the established missing-state contract: a disabled anchor
// renders as a disabled button, so it cannot be activated or navigated.
const download = row('Download') as HTMLButtonElement;
expect(download.tagName).toBe('BUTTON');
expect(download.disabled).toBe(true);
});
it('shows a retryable error when the restore revalidation fails, instead of a dead end', async () => {
// The reachable shape of DR-14's restore path: archiving CLOSES an open
// panel, so a forced revalidation only ever lands on a panel opened
// while the parent was ALREADY archived — where the attachment fetch
// legitimately 404s.
propsA.parentArchived = true;
mountHost(propsA);
revalidateMetaMock.mockResolvedValue({ status: 'missing' });
const partial = { mime_type: null, size_bytes: null };
notifyAttachmentPanelOpen(openEvent(partial));
await settle();
expect(panel()?.textContent).toContain('This file is no longer available.');
// Authoritative-missing offers no Retry, by design.
expect(row('Retry')).toBeUndefined();
// The item is restored, so that 404 no longer necessarily holds — but
// the revalidation itself fails transiently. Leaving `missing` latched
// would show "no longer available" with no way to ask again: the exact
// empty-vs-broken dead end DR-10 exists to prevent.
revalidateMetaMock.mockResolvedValue({ status: 'transient' });
propsA.parentArchived = false;
await settle();
expect(revalidateMetaMock).toHaveBeenCalled();
expect(panel()?.textContent).not.toContain('This file is no longer available.');
expect(row('Retry')).toBeDefined();
});
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);
});
});
+94 -13
View File
@@ -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,108 @@
* destructive confirmation, which is presentational and otherwise
* never announced when the row takes focus (PLAN-2326). */
describedBy?: string;
/** Native tooltip. Explanation, never the only place a fact is stated. */
title?: string;
/** Renders the row as an anchor instead of a button (PLAN-2392 DR-5):
* Download must be a real `<a download>` and Open needs new-tab /
* middle-click semantics. Ignored while `disabled` — see below. */
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,
title,
href,
download,
target,
rel,
onclick,
children
}: Props = $props();
const role = $derived(checked !== undefined ? 'menuitemradio' : 'menuitem');
// A disabled anchor is not a thing: `<a>` ignores `disabled`, is still
// focusable and still navigates. Falling back to a disabled <button> keeps
// the semantics honest AND keeps Menu's keyboard navigation correct — it
// skips rows with `[role^="menuitem"]:not(:disabled)` (Menu.svelte:130),
// which no anchor can ever match.
const asAnchor = $derived(href !== undefined && !disabled);
/**
* A native anchor activates on Enter but NOT on Space, while every other
* row in this menu is a <button>, which activates on both. `role="menuitem"`
* doesn't add the behavior — it only changes what is announced — so without
* this, Space would silently do nothing on exactly the rows a keyboard user
* is most likely to try it on (Download, Open).
*/
function anchorKeydown(e: KeyboardEvent) {
if (e.key !== ' ' && e.key !== 'Spacebar') return;
// Space scrolls the page by default; the menu is the active surface.
e.preventDefault();
(e.currentTarget as HTMLAnchorElement).click();
}
</script>
<button
type="button"
class="mi"
class:danger
role={checked !== undefined ? 'menuitemradio' : 'menuitem'}
aria-checked={checked}
aria-describedby={describedBy}
{disabled}
{onclick}
>
{#if icon}<span class="mi-icon" aria-hidden="true">{icon}</span>{/if}
{#snippet body()}
{#if iconSnippet}
<span class="mi-icon" aria-hidden="true">{@render iconSnippet()}</span>
{:else if icon}
<span class="mi-icon" aria-hidden="true">{icon}</span>
{/if}
<span class="mi-label">{@render children()}</span>
{#if hint}<span class="mi-hint">{hint}</span>{/if}
{#if checked}<span class="mi-check" aria-hidden="true"></span>{/if}
</button>
{/snippet}
{#if asAnchor}
<a
class="mi"
class:danger
{href}
{download}
{target}
{rel}
{role}
{title}
aria-checked={checked}
aria-describedby={describedBy}
{onclick}
onkeydown={anchorKeydown}
>
{@render body()}
</a>
{:else}
<button
type="button"
class="mi"
class:danger
{role}
{title}
aria-checked={checked}
aria-describedby={describedBy}
{disabled}
{onclick}
>
{@render body()}
</button>
{/if}
<style>
.mi {
@@ -53,6 +129,11 @@
align-items: center;
gap: 9px;
width: 100%;
/* The anchor branch is not a button: it needs the border-box sizing
and the link-color/underline reset spelled out, or a row renders
wider than the panel and in the link palette. */
box-sizing: border-box;
text-decoration: none;
padding: 7px 9px;
border: none;
border-radius: var(--radius-sm);
@@ -0,0 +1,164 @@
// Runs in the jsdom vitest project (filename ends `.svelte.test.ts`).
//
// MenuItem grew two capabilities for the attachment options panel
// (PLAN-2392 DR-5 / DR-3b, TASK-2422): an icon SNIPPET alongside the existing
// string icon, and an ANCHOR branch so a row can be a real `<a download>` or
// an Open-in-new-tab link. Both are additive, so the first assertion here is
// that the plain button row every existing call site uses is unchanged.
import { describe, it, expect, vi, afterEach } from 'vitest';
import { render, cleanup } from '@testing-library/svelte';
import { createRawSnippet, tick } from 'svelte';
import MenuItem from './MenuItem.svelte';
const label = createRawSnippet(() => ({ render: () => `<span>Download</span>` }));
const svgIcon = createRawSnippet(() => ({
render: () => `<svg data-testid="icon-svg" viewBox="0 0 24 24"><path d="M0 0h1" /></svg>`,
}));
function row(): HTMLElement {
const el = document.querySelector('.mi');
if (!el) throw new Error('.mi not found');
return el as HTMLElement;
}
/** What Menu.svelte's keyboard navigation actually queries (Menu.svelte:130). */
function menuNavigable(): HTMLElement[] {
return Array.from(
document.querySelectorAll<HTMLElement>('[role^="menuitem"]:not(:disabled)')
);
}
afterEach(() => {
cleanup();
vi.restoreAllMocks();
document.body.innerHTML = '';
});
describe('MenuItem.svelte', () => {
it('still renders the existing string-icon button row unchanged', async () => {
const onclick = vi.fn();
render(MenuItem, { props: { icon: '🗑', hint: '', danger: true, onclick, children: label } });
await tick();
const el = row();
expect(el.tagName).toBe('BUTTON');
expect(el.getAttribute('type')).toBe('button');
expect(el.getAttribute('role')).toBe('menuitem');
expect(el.classList.contains('danger')).toBe(true);
expect(el.querySelector('.mi-icon')?.textContent).toBe('🗑');
expect(el.querySelector('.mi-hint')?.textContent).toBe('');
expect(el.hasAttribute('href')).toBe(false);
(el as HTMLButtonElement).click();
expect(onclick).toHaveBeenCalledTimes(1);
});
it('keeps the menuitemradio + check row for the checked variant', async () => {
render(MenuItem, { props: { checked: true, describedBy: 'desc-1', children: label } });
await tick();
const el = row();
expect(el.getAttribute('role')).toBe('menuitemradio');
expect(el.getAttribute('aria-checked')).toBe('true');
expect(el.getAttribute('aria-describedby')).toBe('desc-1');
expect(el.querySelector('.mi-check')?.textContent).toBe('✓');
});
it('renders an icon snippet as markup, not as literal angle brackets', async () => {
render(MenuItem, { props: { iconSnippet: svgIcon, children: label } });
await tick();
const icon = row().querySelector('.mi-icon');
expect(icon?.getAttribute('aria-hidden')).toBe('true');
// The whole point: an <svg> ELEMENT, not the text "<svg ...>".
expect(icon?.querySelector('svg')).not.toBeNull();
expect(icon?.textContent).not.toContain('<svg');
});
it('prefers the icon snippet when both icon forms are supplied', async () => {
render(MenuItem, { props: { icon: '🗑', iconSnippet: svgIcon, children: label } });
await tick();
const icons = document.querySelectorAll('.mi-icon');
expect(icons).toHaveLength(1);
expect(icons[0].querySelector('svg')).not.toBeNull();
expect(icons[0].textContent).not.toContain('🗑');
});
it('renders an anchor row with href/download and keeps menuitem semantics', async () => {
render(MenuItem, {
props: {
href: '/api/v1/workspaces/ws/attachments/att-1',
download: 'report.pdf',
icon: '⇩',
children: label,
},
});
await tick();
const el = row();
expect(el.tagName).toBe('A');
expect(el.getAttribute('href')).toBe('/api/v1/workspaces/ws/attachments/att-1');
// A REAL download attribute — the server's inline disposition would
// otherwise open the file rather than save it (DR-16).
expect(el.getAttribute('download')).toBe('report.pdf');
expect(el.getAttribute('role')).toBe('menuitem');
expect(el.classList.contains('mi')).toBe(true);
// Reachable by Menu's arrow-key navigation exactly like a button row.
expect(menuNavigable()).toEqual([el]);
});
it('passes target/rel through for the open-in-new-tab anchor', async () => {
render(MenuItem, {
props: {
href: '/api/v1/workspaces/ws/attachments/att-1',
target: '_blank',
rel: 'noopener noreferrer',
children: label,
},
});
await tick();
const el = row();
expect(el.tagName).toBe('A');
expect(el.getAttribute('target')).toBe('_blank');
expect(el.getAttribute('rel')).toBe('noopener noreferrer');
expect(el.hasAttribute('download')).toBe(false);
});
it('activates an anchor row on Space, like every other row in the menu', async () => {
// A native anchor activates on Enter but not Space, and role="menuitem"
// does not add the behavior — so without an explicit handler, Space
// would silently do nothing on Download and Open while working on
// every button row beside them.
const onclick = vi.fn();
render(MenuItem, {
props: { href: '/api/v1/workspaces/ws/attachments/att-1', onclick, children: label },
});
await tick();
const el = row() as HTMLAnchorElement;
const evt = new KeyboardEvent('keydown', { key: ' ', bubbles: true, cancelable: true });
el.dispatchEvent(evt);
await tick();
expect(onclick).toHaveBeenCalledTimes(1);
// Space scrolls the page by default; the open menu is the active surface.
expect(evt.defaultPrevented).toBe(true);
});
it('renders a disabled anchor as a disabled button so keyboard nav skips it', async () => {
render(MenuItem, {
props: { href: '/api/v1/workspaces/ws/attachments/att-1', disabled: true, children: label },
});
await tick();
const el = row();
// `<a>` ignores `disabled`, stays focusable and still navigates — a
// disabled link would be a live link that only LOOKS unavailable.
expect(el.tagName).toBe('BUTTON');
expect((el as HTMLButtonElement).disabled).toBe(true);
expect(el.hasAttribute('href')).toBe(false);
expect(menuNavigable()).toEqual([]);
});
});
+44 -1
View File
@@ -598,12 +598,14 @@
notifyAttachmentImageCapabilitiesChanged,
} from './attachment-image';
import { AttachmentChip } from './attachment-chip';
import type { AttachmentHostAddress } from '$lib/attachments/hostAddress';
import { AttachmentUpload } from './attachment-upload';
let {
content = '',
editable = true,
itemId,
hostToken = '',
ydoc,
awareness,
collabUser,
@@ -621,6 +623,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
@@ -861,6 +873,28 @@
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 => ({
// `wsSlug` is resolved once at mount here (from the route), unlike the
// composer's live prop — reading it through the same reader keeps ONE
// shape for both hosts rather than a per-host special case.
//
// That is SAFE here for a reason worth stating, because it is not the
// reader that provides it: both <Editor> mounts sit inside
// `{#key item.id...}` in ItemDetail, and a workspace switch
// necessarily lands on a different item, so this component is
// remounted and re-reads the route. If that key is ever removed, this
// snapshot goes stale and must become live like the composer's
// (final review round 4).
workspaceSlug: wsSlug,
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.
@@ -914,6 +948,11 @@
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.
address: readHostAddress,
// Initial supportedFormats is empty — server capabilities
// are fetched async below. The toolbar starts disabled
// for all formats until capabilities resolve, then
@@ -934,7 +973,11 @@
}
},
}),
AttachmentChip.configure({ getDownloadUrl: getAttachmentUrl, workspaceSlug: wsSlug }),
AttachmentChip.configure({
getDownloadUrl: getAttachmentUrl,
workspaceSlug: wsSlug,
address: readHostAddress,
}),
// 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
+238 -44
View File
@@ -25,6 +25,15 @@
* MIME upgrades the icon from the filename-extension guess to the
* canonical file-type icon, and the size is rendered alongside the name.
*
* Activation (TASK-2424 / PLAN-2392 DR-2): clicking — or pressing Enter or
* Space on — a live chip opens the shared attachment OPTIONS PANEL, the same
* one the item attachment strip's file tiles open, rather than the old
* `window.open` of the download URL. The NodeView can't mount a Svelte
* component, so it signals the owning `ItemDetail` host through the events
* bus, stamped with the host address its options carry. `renderHTML` below is
* unchanged: it is the non-NodeView / clipboard shape, where an `<a download>`
* is still the honest representation.
*
* Icon and byte formatting are the shared attachment helpers'
* (`$lib/attachments/display` + `$lib/attachments/icons`) as of TASK-2417 —
* this file used to carry its own `iconForMime` / `iconForFilename` /
@@ -40,8 +49,20 @@ import {
type AttachmentVariant,
fetchAttachmentMetadata
} from './attachment-metadata';
import { registerAttachmentDeletionListener } from '$lib/attachments/events';
import { formatBytes, iconForAttachment } from '$lib/attachments/display';
import {
notifyAttachmentPanelOpen,
registerAttachmentDeletionListener
} from '$lib/attachments/events';
import {
type AttachmentHostAddressReader,
readUnaddressed
} from '$lib/attachments/hostAddress';
import {
describeAttachmentType,
displayFilename,
formatBytes,
iconForAttachment,
} from '$lib/attachments/display';
import { iconSvg } from '$lib/attachments/icons/index';
const PAD_ATTACHMENT_PREFIX = 'pad-attachment:';
@@ -54,8 +75,22 @@ export interface AttachmentChipOptions {
HTMLAttributes: Record<string, unknown>;
/** Build the download URL — usually `api.attachments.downloadUrl` from the editor's mount context. */
getDownloadUrl: AttachmentUrlBuilder;
/** Workspace slug used by the metadata HEAD fetcher. Empty disables the fetch. */
/**
* Workspace slug for anything resolved at CONFIGURE time.
*
* NOT the metadata probe's workspace — that reads `address().workspaceSlug`,
* because an editor can outlive a pane workspace switch and this value would
* key the cache under the previous one. Kept for callers that legitimately
* want the mount-time value; a probe is not one of them.
*/
workspaceSlug: string;
/**
* 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`.
*/
address: AttachmentHostAddressReader;
}
declare module '@tiptap/core' {
@@ -84,6 +119,7 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
HTMLAttributes: {},
getDownloadUrl: (uuid: string) => `${PAD_ATTACHMENT_PREFIX}${uuid}`,
workspaceSlug: '',
address: readUnaddressed,
};
},
@@ -153,7 +189,7 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
target: '_blank',
rel: 'noopener noreferrer',
}),
filename || 'attachment',
displayFilename(filename),
];
},
@@ -167,11 +203,28 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
let currentUuid = (node.attrs.uuid as string | null) ?? '';
let currentFilename = (node.attrs.filename as string | null) ?? '';
let currentMime: string | null = null;
// Kept alongside the rendered text because the open-panel event
// carries the three metadata fields the panel displays (DR-2), and
// the accessible name below names the size too. Null until the HEAD
// probe resolves — legitimately so: the panel completes what the
// chip doesn't know rather than waiting for it.
let currentSize: number | null = null;
const wrapper = document.createElement('a');
// A BUTTON, not an anchor (DR-12; orchestrator review of TASK-2424).
// A live chip opens the options panel — it does not navigate — so an
// anchor was announcing it as a link and, worse, left the URL
// reachable by paths the click handler never sees: middle-click and
// aux-click would still open or download it, straight past the
// affordance that exists to stop a tap doing that. Button semantics
// remove the bypass by construction rather than intercepting it, and
// match the strip's file tile, which is the same control.
//
// `renderHTML` stays an `<a download>`: that is the clipboard /
// non-NodeView shape, where a real link IS the honest representation
// and there is no panel to open.
const wrapper = document.createElement('button');
wrapper.type = 'button';
wrapper.className = 'file-chip';
wrapper.target = '_blank';
wrapper.rel = 'noopener noreferrer';
wrapper.contentEditable = 'false';
const iconEl = document.createElement('span');
@@ -187,25 +240,27 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
wrapper.append(iconEl, nameEl, sizeEl);
// The id is the chip's identity for the deletion bus and for the
// panel event; the download URL is the PANEL's business now, so the
// element carries no href to be aux-clicked.
const refreshHref = (): void => {
if (currentUuid) {
wrapper.href = this.options.getDownloadUrl(currentUuid);
wrapper.setAttribute('data-attachment-id', currentUuid);
} else {
wrapper.removeAttribute('href');
wrapper.removeAttribute('data-attachment-id');
}
};
// `data-filename` only: a `download` attribute means nothing on a
// button, and the actual download is the panel's Download action,
// which sets it on a real anchor (DR-16).
const refreshFilenameDom = (): void => {
if (currentFilename) {
wrapper.setAttribute('data-filename', currentFilename);
wrapper.download = currentFilename;
} else {
wrapper.removeAttribute('data-filename');
wrapper.removeAttribute('download');
}
nameEl.textContent = currentFilename || 'attachment';
nameEl.textContent = displayFilename(currentFilename);
};
// Icon resolution is the shared mapper's (TASK-2417): MIME first,
@@ -219,6 +274,37 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
iconEl.innerHTML = iconSvg(iconForAttachment(currentMime, currentFilename));
};
/**
* Accessible name: filename, type AND the action (PLAN-2392 DR-12).
*
* Activating a chip opens the options panel rather than the file, and
* nothing else on the chip says so — the icon is aria-hidden and the
* visible text is just the name. Recomputed whenever any of the three
* inputs changes, since the size and the MIME arrive asynchronously.
*
* Size is included only once known: an unresolved probe should read
* as "Options for notes.txt, Text", not as a confident "0 B".
*
* Deleted-state aware, and deliberately so rather than the dead
* wording living only in `markDeleted`: a filename update on an
* already-dead chip calls back through here, and a name that went
* back to promising options for a row that is gone would be worse
* than no name at all. (`deleted` is declared below and only ever
* read after it is initialised.)
*/
const refreshAccessibleName = (): void => {
const name = displayFilename(currentFilename);
if (deleted) {
wrapper.setAttribute('aria-label', `${name} — this attachment has been deleted`);
return;
}
const parts = [name, describeAttachmentType(currentMime, currentFilename)];
if (Number.isFinite(currentSize) && (currentSize as number) > 0) {
parts.push(formatBytes(currentSize as number));
}
wrapper.setAttribute('aria-label', `Options for ${parts.join(', ')}`);
};
/**
* A deleted attachment leaves this chip looking perfectly valid —
* unlike an <img>, a link makes no request until clicked, so
@@ -239,10 +325,24 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
const markDeleted = (): void => {
deleted = true;
wrapper.classList.add('attachment-missing');
wrapper.removeAttribute('href');
wrapper.removeAttribute('download');
// `disabled` is what makes the dead chip genuinely INERT (DR-12),
// not merely unclickable: a disabled button is unfocusable and
// receives no click or keydown at all, so a keyboard user is
// never handed a focus stop whose Enter and Space do nothing.
// `tabindex` is deliberately never set for the same reason.
//
// Blur it explicitly first: a chip the user is focused on RIGHT
// NOW (they tabbed to it, then another surface deleted the row)
// stays focused in some browsers even once disabled, which would
// strand focus on an element that no longer responds.
if (typeof document !== 'undefined' && document.activeElement === wrapper) {
wrapper.blur();
}
wrapper.disabled = true;
wrapper.title = 'This attachment has been deleted';
currentSize = null;
sizeEl.textContent = '';
refreshAccessibleName();
};
// Set by destroy(). A HEAD probe outlives the NodeView that started
@@ -257,35 +357,100 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
refreshHref();
refreshFilenameDom();
refreshIcon();
refreshAccessibleName();
// Explicit click handler → window.open. Editor.svelte installs a
// global anchor-click suppressor that calls preventDefault on
// every <a> inside the editor (so plain text links don't navigate
// in edit mode); without this handler the chip's anchor
// navigation would also be eaten and clicking the chip would
// silently do nothing. Mirrors the pattern AttachmentImage uses
// for its lightbox click. Reads currentUuid (mutable) so a peer
// Yjs op swapping the chip's target is honoured at click time.
/**
* Open the options panel for this chip (PLAN-2392 DR-2 / TASK-2424).
*
* Replaces the old `window.open` of the download URL: a chip and a
* strip tile are the same attachment and now behave identically —
* metadata first, Download as a deliberate choice.
*
* The address is read AT EMIT TIME (`options.address()`), never
* cached: the comment composer is reused across an item switch, so a
* value captured at configure() would address the previous item's
* host. Tiptap's `options` is a getter returning a fresh spread, so
* pushing a new value onto it after configure() is a silent no-op —
* hence a reader. See `$lib/attachments/hostAddress`.
*
* An UNADDRESSED editor (no host token — no `ItemDetail` above it)
* emits nothing: `notifyAttachmentPanelOpen` drops it rather than
* broadcasting to every mounted host, which is DR-8's whole point.
* Every live mount site threads the address; a surface that doesn't
* has no panel to open.
*
* Reads `currentUuid` (mutable) so a peer Yjs op swapping the chip's
* target is honoured at activation time.
*/
const openPanel = (): void => {
if (!currentUuid || deleted) return;
const address = this.options.address();
notifyAttachmentPanelOpen({
attachmentId: currentUuid,
itemId: address.itemId,
hostToken: address.hostToken,
anchor: wrapper,
filename: currentFilename || null,
mime_type: currentMime,
size_bytes: currentSize,
});
};
// Explicit click handler. Editor.svelte installs a global
// anchor-click suppressor that calls preventDefault on every <a>
// inside the editor (so plain text links don't navigate in edit
// mode); without this handler the chip's activation would also be
// eaten and clicking the chip would silently do nothing. Mirrors the
// pattern AttachmentImage uses for its lightbox click.
//
// This is the MOUSE activation path. Keyboard activation is the
// keydown handler below and never reaches here — see why there.
wrapper.addEventListener('click', (event) => {
if (event.detail > 1) return; // double-click → fall through
if (!currentUuid) return;
// Removing href is not enough: this handler opens the URL
// itself, so a deleted chip would still open a 404 in a new tab
// (Codex round 14). Swallow the click instead.
if (deleted) {
event.preventDefault();
event.stopPropagation();
return;
}
// Removing href is not enough: this handler acts on the URL
// itself, so a deleted chip would still do something (a 404 in a
// new tab, before TASK-2424; a panel for a dead row, after).
// Swallow the click instead (Codex round 14).
event.preventDefault();
event.stopPropagation();
if (typeof window !== 'undefined') {
window.open(
this.options.getDownloadUrl(currentUuid),
'_blank',
'noopener,noreferrer',
);
}
if (deleted) return;
openPanel();
});
/**
* Keyboard activation: Enter AND Space, exactly once each (DR-12).
*
* Enter is handled HERE rather than left to the anchor's native
* "Enter means click", for a reason specific to living inside a
* ProseMirror editor: the chip sits in the editable region, so an
* un-suppressed Enter bubbles to the editor's own keymap, which
* treats it as split-block — it calls `preventDefault` itself, which
* ALSO cancels the anchor's activation click. Relying on the native
* path would mean Enter silently split a paragraph instead of
* opening the panel.
*
* `preventDefault` here is what keeps the count at one: a cancelled
* keydown produces no activation click, so this handler and the
* click handler above are disjoint rather than racing (the exact
* double-fire DR-12 names). It also suppresses Space's page scroll,
* and `stopPropagation` keeps both keys away from the editor keymap.
*/
wrapper.addEventListener('keydown', (event) => {
const isSpace = event.key === ' ' || event.key === 'Spacebar';
if (event.key !== 'Enter' && !isSpace) return;
// A modified key is a shortcut, not an activation.
if (event.ctrlKey || event.metaKey || event.altKey || event.shiftKey) return;
// Suppress BEFORE the deleted/no-uuid bail, not after. A disabled
// button gets no keydown, so this is belt-and-braces — but if a
// dead chip ever did receive one, returning early would let Enter
// through to ProseMirror's keymap and split the paragraph the
// chip sits in, which is a destructive answer to pressing Enter
// on something inert.
event.preventDefault();
event.stopPropagation();
if (!currentUuid || deleted) return;
openPanel();
});
// Async metadata enrichment via HEAD. Server registers HEAD
@@ -301,25 +466,44 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
// not trample state for a NEW uuid that landed via update()
// while we were awaiting HEAD.
const probeMetadata = (forUuid: string): void => {
if (!forUuid || !this.options.workspaceSlug) return;
// Workspace off the READER, not the static option: this editor can
// outlive a workspace switch, and this value keys the metadata
// cache — stale, it asks the previous workspace about this
// workspace's attachment (final review round 3).
const probeWs = this.options.address().workspaceSlug;
if (!forUuid || !probeWs) return;
fetchAttachmentMetadata(
this.options.workspaceSlug,
probeWs,
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;
currentSize = result.size;
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}` : '';
// The name says the type and the size; both just arrived.
refreshAccessibleName();
});
};
@@ -341,16 +525,25 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
if (newUuid !== currentUuid) {
currentUuid = newUuid;
// New target ⇒ the old deletion no longer applies.
// New target ⇒ the old deletion no longer applies. Undoing
// EVERY part of markDeleted() matters: `disabled` is what
// makes a dead chip inert, so leaving it set here would
// give a live chip that announces itself as live and does
// nothing — worse than the dead one, which at least says
// so. Reachable via a peer's uuid swap or a ProseMirror
// node replacement (orchestrator's full-diff review).
deleted = false;
wrapper.disabled = false;
wrapper.classList.remove('attachment-missing');
wrapper.removeAttribute('title');
// New uuid ⇒ stale MIME / size; reset until HEAD probe
// returns for the new identifier.
currentMime = null;
currentSize = null;
sizeEl.textContent = '';
refreshHref();
refreshIcon();
refreshAccessibleName();
probeMetadata(newUuid);
}
if (newFilename !== currentFilename) {
@@ -362,6 +555,7 @@ export const AttachmentChip = Node.create<AttachmentChipOptions>({
// application/octet-stream). Always recomputing is
// idempotent for MIMEs with a definitive icon.
refreshIcon();
refreshAccessibleName();
}
return true;
+190 -27
View File
@@ -28,15 +28,21 @@
import { Node, mergeAttributes } from '@tiptap/core';
import type { Node as ProseMirrorNode } from '@tiptap/pm/model';
import { canOpenInViewer } from '$lib/attachments/display';
import {
type AttachmentUrlBuilder,
type AttachmentVariant,
fetchAttachmentMetadata,
invalidateAttachmentMetadata,
revalidateAttachmentMetadata,
mimeToFormat
} 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.
@@ -96,12 +102,22 @@ export interface AttachmentImageOptions {
*/
getDownloadUrl: AttachmentUrlBuilder;
/**
* Workspace slug used for HEAD-probing the image's MIME (so the
* rotate toolbar can gate buttons on the processor's supported
* formats). Empty string disables the probe — the toolbar still
* shows but skips per-format gating.
* Workspace slug resolved at CONFIGURE time.
*
* The MIME probes that gate the rotate toolbar read
* `address().workspaceSlug` instead: this editor can outlive a pane
* workspace switch, and that value keys the metadata cache. An empty
* address workspace disables the probe — the toolbar still shows, but
* skips per-format gating.
*/
workspaceSlug: string;
/**
* 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.
*/
address: AttachmentHostAddressReader;
/**
* Image formats the server-side processor supports. Drives the
* rotate toolbar's enabled state per attachment: a button is
@@ -161,6 +177,7 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
HTMLAttributes: {},
getDownloadUrl: (uuid: string) => `${PAD_ATTACHMENT_PREFIX}${uuid}`,
workspaceSlug: '',
address: readUnaddressed,
supportedFormats: [] as string[],
transform: async () => {
throw new Error('AttachmentImage: configure({ transform }) is required to use rotate/crop');
@@ -274,6 +291,9 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
// Deletion is authoritative: a load still in flight when it lands
// must not be allowed to paint the image back (Codex round 15).
let deleted = false;
// True once the NodeView is torn down. Async continuations (HEAD
// probes, transform results) must not touch DOM after that.
let destroyed = false;
function showMissing() {
missing.textContent = `📎 ${currentAlt || 'Attachment unavailable'}`;
@@ -283,6 +303,21 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
? 'This attachment has been deleted'
: 'This attachment could not be loaded — it may have been deleted. Click to retry.';
missing.style.cursor = deleted ? 'default' : 'pointer';
// And the INTERACTIVE SEMANTICS go with the copy, not just the
// cursor (DR-12; final review round 5). A confirmed deletion
// makes `retryLoad` a no-op, so leaving role=button + tabindex
// hands a keyboard or screen-reader user a focus stop that
// announces itself as a button and does nothing — the same dead
// stop the file chip's `disabled` closes, on the surface next to
// it. A transient failure IS retryable and keeps both.
if (deleted) {
if (document.activeElement === missing) missing.blur();
missing.removeAttribute('role');
missing.removeAttribute('tabindex');
} else {
missing.setAttribute('role', 'button');
missing.setAttribute('tabindex', '0');
}
if (currentUuid) missing.setAttribute('data-attachment-id', currentUuid);
missing.style.display = '';
img.style.display = 'none';
@@ -310,9 +345,59 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
// 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 (destroyed || 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 <img> `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).
*
* It REVALIDATES rather than reading the cache: the failed load is
* evidence that whatever we last observed about this row is out of
* date, and a cached `ok` from before the deletion would make the
* placeholder permanently unlatchable.
*/
function probeForMissing(forUuid: string) {
// Workspace off the READER, not the static option — it keys the
// metadata cache and this editor can outlive a workspace switch
// (final review round 3).
const probeWs = opts.address().workspaceSlug;
if (!forUuid || !probeWs || deleted || destroyed) return;
void revalidateAttachmentMetadata(probeWs, 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 });
@@ -332,10 +417,11 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
// otherwise fire after the delete and restore the image.
detachLoadListeners();
showMissing();
refresh();
});
missing.setAttribute('role', 'button');
missing.setAttribute('tabindex', '0');
// role/tabindex are set by showMissing(), which knows whether this is
// a retryable failure or a confirmed deletion. Hidden and inert here.
missing.style.cursor = 'pointer';
function retryLoad() {
@@ -357,6 +443,11 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
}
});
// The MIME this node's attachment is KNOWN to have, from a HEAD probe. Null
// until one has answered — the probe is lazy (toolbar construction, uuid
// swap), so "null" means "not yet asked", never "not an image".
let knownMime: string | null = null;
img.addEventListener('click', (event) => {
// In a contenteditable, ProseMirror handles selection on
// mousedown; intercept click so a single click opens the
@@ -368,6 +459,20 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
event.preventDefault();
event.stopPropagation();
if (!currentUuid) return;
// DR-16: the EXACT raster allowlist gates every open-the-viewer
// path, not just the strip's — `image/svg+xml` can carry active
// content, and a node being labelled image/* is not sufficient
// reason to hand it to a viewer.
//
// Gated on what is POSITIVELY KNOWN, deliberately: this node's
// MIME comes from a lazy HEAD probe, so at click time it is often
// simply unasked. Refusing on unknown would stop ordinary images
// opening — a certain regression traded for a marginal risk — so
// an unprobed node keeps today's behaviour and a probed
// non-allowlisted one is refused. Phase 3a's unified viewer
// threads `mime_type` onto the image list itself (DR-16), which
// is what turns this into a complete gate.
if (knownMime && !canOpenInViewer(knownMime)) return;
const fullUrl = opts.getDownloadUrl(currentUuid, 'original');
openImageLightbox(fullUrl, currentAlt);
});
@@ -376,10 +481,23 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
// selected — so non-selected images don't carry the DOM
// cost. Subsequent selections reuse the same toolbar.
let toolbar: HTMLElement | null = null;
let toolbarMime: string | null = null;
let unregisterRefresher: (() => void) | null = null;
const refresh = () => {
if (toolbar) refreshToolbarState(toolbar, toolbarMime, opts.supportedFormats);
if (!toolbar) return;
// A confirmed deletion inertizes the WHOLE node, not just its
// placeholder: rotate and crop against a row that is gone can
// only 404, and leaving them live is the same dead-control gap
// the placeholder's role/tabindex removal closes (round 7).
if (deleted) {
toolbar
.querySelectorAll<HTMLButtonElement>('.attachment-image-toolbar-btn')
.forEach((btn) => {
btn.disabled = true;
btn.title = 'This attachment has been deleted';
});
return;
}
refreshToolbarState(toolbar, knownMime, opts.supportedFormats);
};
const ensureToolbar = (): HTMLElement => {
if (toolbar) return toolbar;
@@ -401,15 +519,25 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
// (e.g. SSR / preview surfaces) — the toolbar's state
// falls back to the supportedFormats list alone, with
// the MIME left null.
if (currentUuid && opts.workspaceSlug) {
const toolbarProbeWs = opts.address().workspaceSlug;
if (currentUuid && toolbarProbeWs) {
const probeUuid = currentUuid;
fetchAttachmentMetadata(opts.workspaceSlug, probeUuid, opts.getDownloadUrl).then(
(meta) => {
// 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;
fetchAttachmentMetadata(toolbarProbeWs, probeUuid, opts.getDownloadUrl).then(
(result) => {
// 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
// well beat the <img> to it.
if (result.status === 'missing') latchMissing(probeUuid);
if (!toolbar) return;
// `transient` leaves the MIME unknown rather than
// wrong: gating falls back to supportedFormats.
knownMime = result.status === 'ok' ? result.mime : null;
refresh();
}
);
@@ -419,6 +547,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
@@ -478,6 +611,12 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
if (currentUuid !== startUuid) return;
swapNodeUuid(result.id);
} catch (err) {
// Same fence as the success path: a failure that lands after
// teardown, or after the node moved to a different image,
// belongs to work the user has already navigated away from.
// Reporting it would alert about attachment A while they are
// looking at B.
if (destroyed || currentUuid !== startUuid) return;
const msg = err instanceof Error ? err.message : 'Rotation failed';
if (opts.onError) opts.onError(msg);
else if (typeof console !== 'undefined') console.error('[attachmentImage] rotate', err);
@@ -530,6 +669,12 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
if (currentUuid !== startUuid) return;
swapNodeUuid(result.id);
} catch (err) {
// Same fence as the success path: a failure that lands after
// teardown, or after the node moved to a different image,
// belongs to work the user has already navigated away from.
// Reporting it would alert about attachment A while they are
// looking at B.
if (destroyed || currentUuid !== startUuid) return;
const msg = err instanceof Error ? err.message : 'Crop failed';
if (opts.onError) opts.onError(msg);
else if (typeof console !== 'undefined') console.error('[attachmentImage] crop', err);
@@ -560,8 +705,9 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
// (e.g. on the same image rendered elsewhere) will
// re-fetch. Triggers for any source of the change:
// local rotate via swapNodeUuid OR a peer Yjs op.
if (currentUuid && opts.workspaceSlug) {
invalidateAttachmentMetadata(opts.workspaceSlug, currentUuid);
const swapWs = opts.address().workspaceSlug;
if (currentUuid && swapWs) {
invalidateAttachmentMetadata(swapWs, currentUuid);
}
currentUuid = newUuid;
// The old uuid's state — whether a 404 placeholder or a
@@ -583,17 +729,21 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
// Toolbar's MIME probe was for the old uuid; reset
// gating to "still loading" + re-probe so per-format
// state stays correct across the swap.
toolbarMime = null;
knownMime = null;
refresh();
if (toolbar && newUuid && opts.workspaceSlug) {
const updateProbeWs = opts.address().workspaceSlug;
if (toolbar && newUuid && updateProbeWs) {
const probeUuid = newUuid;
fetchAttachmentMetadata(
opts.workspaceSlug,
updateProbeWs,
probeUuid,
opts.getDownloadUrl
).then((meta) => {
if (!toolbar || currentUuid !== probeUuid) return;
toolbarMime = meta?.mime ?? null;
).then((result) => {
if (destroyed) return;
if (currentUuid !== probeUuid) return;
if (result.status === 'missing') latchMissing(probeUuid);
if (!toolbar) return;
knownMime = result.status === 'ok' ? result.mime : null;
refresh();
});
}
@@ -624,6 +774,11 @@ export const AttachmentImage = Node.create<AttachmentImageOptions>({
if (toolbar) toolbar.classList.add('attachment-image-toolbar-hidden');
},
destroy() {
// Set FIRST: every async continuation below fences on it, and
// a HEAD probe in flight at teardown would otherwise resolve
// into detached DOM (the chip NodeView has carried this flag
// since it was written; the image one did not).
destroyed = true;
detachLoadListeners();
disposeDeletionListener();
// Tear down the refresher subscription so the
@@ -767,7 +922,13 @@ function refreshToolbarState(
const btns = toolbar.querySelectorAll<HTMLButtonElement>('.attachment-image-toolbar-btn');
const noProcessor = supportedFormats.length === 0;
const format = mime ? mimeToFormat(mime) : null;
const knownUnsupported = !!format && !supportedFormats.includes(format);
// A KNOWN mime that maps to no processor format is unsupported, not
// unknown. `mimeToFormat` returns null both for "never asked" and for
// "asked, and this is not something the processor handles" — treating the
// second as the first left Crop enabled for e.g. image/svg+xml, which
// hands the original to the crop modal for a transform the server will
// refuse (final review round 7).
const knownUnsupported = !!mime && (!format || !supportedFormats.includes(format));
btns.forEach((btn) => {
// Re-derive the original tooltip from the dataset. We keep the
@@ -784,7 +945,9 @@ function refreshToolbarState(
}
if (knownUnsupported) {
btn.disabled = true;
btn.title = `Image editing for ${mime} requires libvips (this build supports ${supportedFormats.join(', ')})`;
btn.title = format
? `Image editing for ${mime} requires libvips (this build supports ${supportedFormats.join(', ')})`
: `Image editing isn't available for ${mime}`;
return;
}
btn.disabled = false;
@@ -0,0 +1,273 @@
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import {
fetchAttachmentMetadata,
invalidateAttachmentMetadata,
revalidateAttachmentMetadata,
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<string, string> = {}): Response {
return new Response(null, { status, headers });
}
let fetchMock: ReturnType<typeof vi.fn>;
// 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<Response>((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('revalidateAttachmentMetadata — existence probes ignore the cache', () => {
// The orchestrator's Codex pass on TASK-2420 caught this: `ok` is cached
// for the page lifetime (correctly — MIME and size are durable facts about
// a content-addressed row), but that makes the cache structurally unable
// to answer "is this row still there?". An <img> whose load just failed
// holds evidence that the cached observation is stale.
it('re-issues the HEAD and observes a 404 that a cached ok would have hidden', async () => {
const uuid = freshUuid();
fetchMock.mockResolvedValueOnce(
head(200, { 'content-type': 'image/png', 'content-length': '10' })
);
expect(await fetchAttachmentMetadata('ws', uuid, url)).toEqual({
status: 'ok',
mime: 'image/png',
size: 10
});
// The row is deleted by someone else; the cached `ok` still says live.
expect(await fetchAttachmentMetadata('ws', uuid, url)).toMatchObject({ status: 'ok' });
expect(fetchMock).toHaveBeenCalledTimes(1);
fetchMock.mockResolvedValueOnce(head(404));
expect(await revalidateAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' });
expect(fetchMock).toHaveBeenCalledTimes(2);
});
it('leaves the authoritative missing result cached for later readers', async () => {
const uuid = freshUuid();
fetchMock.mockResolvedValueOnce(head(404));
expect(await revalidateAttachmentMetadata('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 revalidation, so a retry probes again', async () => {
const uuid = freshUuid();
fetchMock.mockResolvedValueOnce(head(500));
expect(await revalidateAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'transient' });
fetchMock.mockResolvedValueOnce(head(404));
expect(await revalidateAttachmentMetadata('ws', uuid, url)).toEqual({ status: 'missing' });
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();
});
});
@@ -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<string, Promise<AttachmentMetadata | null>>();
/**
* 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<string, Promise<AttachmentMetadataResult>>();
/**
* 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<AttachmentMetadata | null> {
): Promise<AttachmentMetadataResult> {
const key = `${workspaceSlug}:${uuid}`;
const existing = cache.get(key);
if (existing) return existing;
const promise: Promise<AttachmentMetadata | null> = (async () => {
const promise: Promise<AttachmentMetadataResult> = (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;
}
@@ -76,6 +121,34 @@ export function invalidateAttachmentMetadata(workspaceSlug: string, uuid: string
cache.delete(`${workspaceSlug}:${uuid}`);
}
/**
* Ask the server about this attachment RIGHT NOW, ignoring anything
* already cached.
*
* `fetchAttachmentMetadata` answers "what is this attachment?" and a
* cached `ok` is a perfectly good answer — MIME and size are durable
* facts about a content-addressed row. But an EXISTENCE probe asks a
* different question, "is this row still there?", and a page-lifetime
* cache structurally cannot answer it: the cached `ok` is a memory of
* an earlier observation, so a row deleted since would still read as
* live and a permanent placeholder would never latch (found by the
* orchestrator's Codex pass on TASK-2420).
*
* The callers that need this are the ones holding contrary evidence —
* an <img> whose load just failed, or a user pressing Retry (DR-10,
* which requires invalidating before refetching for exactly this
* reason). Use `fetchAttachmentMetadata` for everything else; this one
* costs a round trip every call by design.
*/
export function revalidateAttachmentMetadata(
workspaceSlug: string,
uuid: string,
getDownloadUrl: AttachmentUrlBuilder
): Promise<AttachmentMetadataResult> {
invalidateAttachmentMetadata(workspaceSlug, uuid);
return fetchAttachmentMetadata(workspaceSlug, uuid, getDownloadUrl);
}
/**
* Map a MIME type to its canonical short format name as the server's
* Capabilities reports it ("png" / "jpeg" / "gif" / "bmp" / "tiff" /
@@ -0,0 +1,263 @@
// The live editor chip opens the shared options panel (PLAN-2392 DR-2 / DR-12,
// TASK-2424) — the same panel a strip file tile opens, so an attachment behaves
// the same wherever you meet it.
//
// Driven through a REAL Tiptap editor rather than mocks: the chip is imperative
// NodeView DOM, and the properties under test are all properties of that DOM —
// which element receives the click, whether it is focusable, and how many times
// one activation fires. A hand-built element would pin none of it.
//
// The bus is mocked so the emission can be asserted as a payload rather than
// through a host component, and the metadata probe is mocked so tests stay
// synchronous AND so the workspace it probes under can be asserted directly —
// at the module boundary rather than at fetch level, since the metadata cache
// is module-global and would dedupe across tests.
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { Editor } from '@tiptap/core';
import StarterKit from '@tiptap/starter-kit';
const panelOpenMock = vi.fn<(event: Record<string, unknown>) => void>();
const deletionListeners = new Set<(uuid: string) => void>();
vi.mock('$lib/attachments/events', () => ({
notifyAttachmentPanelOpen: (event: Record<string, unknown>) => panelOpenMock(event),
registerAttachmentDeletionListener: (fn: (uuid: string) => void) => {
deletionListeners.add(fn);
return () => deletionListeners.delete(fn);
},
}));
const probeMock = vi.fn<(ws: string, uuid: string) => Promise<unknown>>();
vi.mock('./attachment-metadata', () => ({
fetchAttachmentMetadata: (ws: string, uuid: string) => probeMock(ws, uuid),
invalidateAttachmentMetadata: () => {},
}));
const { AttachmentChip } = await import('./attachment-chip');
const ADDRESS = { workspaceSlug: '', itemId: 'item-A', hostToken: 'apanel-1' };
function makeEditor(element: HTMLElement, address = () => ADDRESS): Editor {
return new Editor({
element,
extensions: [
StarterKit,
AttachmentChip.configure({
// The default address carries no workspace ⇒ no probe, so MIME and
// size stay null — a LEGITIMATE state the event has to carry
// (DR-2), not a test shortcut. The workspace test below supplies
// one.
workspaceSlug: '',
getDownloadUrl: (uuid: string) => `/api/v1/workspaces/ws/attachments/${uuid}`,
address,
}),
],
content: '<p><a href="pad-attachment:uuid-1">spec.pdf</a></p>',
editable: true,
});
}
describe('editor chip → options panel', () => {
let target: HTMLElement;
let editor: Editor | undefined;
beforeEach(() => {
panelOpenMock.mockReset();
deletionListeners.clear();
target = document.body.appendChild(document.createElement('div'));
});
afterEach(() => {
editor?.destroy();
editor = undefined;
target.remove();
});
// The live NodeView chip is a BUTTON, not an anchor: it opens the options
// panel rather than navigating, and an anchor left the URL reachable by
// middle-click straight past the panel (orchestrator review of TASK-2424).
// `renderHTML` — the clipboard / read-only shape — is still an <a download>.
/**
* Repoint the existing chip node at a different attachment via a
* transaction, which drives the NodeView's `update()` hook — the path a
* collaborative peer's edit takes. Replacing the content instead would
* destroy and rebuild the NodeView and prove nothing about `update()`.
*/
function repointChip(uuid: string, filename: string) {
const ed = (editor ??= makeEditor(target));
let pos = -1;
ed.state.doc.descendants((node, at) => {
if (node.type.name === 'attachmentChip') {
pos = at;
return false;
}
return true;
});
if (pos < 0) throw new Error('no chip node in the document');
const tr = ed.state.tr.setNodeMarkup(pos, undefined, { uuid, filename });
ed.view.dispatch(tr);
}
function chip(): HTMLButtonElement {
editor ??= makeEditor(target);
const el = target.querySelector<HTMLButtonElement>('button.file-chip');
if (!el) throw new Error('chip NodeView did not render');
return el;
}
it('emits the open-panel event instead of opening the file in a new tab', () => {
const el = chip();
const opened = vi.spyOn(window, 'open').mockImplementation(() => null);
el.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 }));
expect(opened).not.toHaveBeenCalled();
expect(panelOpenMock).toHaveBeenCalledTimes(1);
expect(panelOpenMock).toHaveBeenCalledWith({
attachmentId: 'uuid-1',
// Stamped from the address READER at emit time (DR-8).
itemId: 'item-A',
hostToken: 'apanel-1',
anchor: el,
filename: 'spec.pdf',
// Null is legitimate: the chip's HEAD probe may not have resolved,
// and the panel completes what the chip doesn't know (DR-2).
mime_type: null,
size_bytes: null,
});
opened.mockRestore();
});
it('suppresses the click so the editor is not navigated', () => {
const el = chip();
const event = new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 });
el.dispatchEvent(event);
// Editor.svelte's global anchor-click suppressor would eat a plain
// anchor click; the chip stops propagation so its own activation wins.
expect(event.defaultPrevented).toBe(true);
});
it('reads the address at EMIT time, so a reused composer re-addresses', () => {
// The comment composer survives an A→B item switch — its `itemId` prop
// just changes — and Tiptap options cannot be rewritten after
// configure(). A cached address would send B's chips to A's host.
const live = { itemId: 'item-A', hostToken: 'apanel-1' };
editor = makeEditor(target, () => live);
live.itemId = 'item-B';
chip().dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 }));
expect(panelOpenMock.mock.calls[0][0]).toMatchObject({ itemId: 'item-B' });
});
it('activates exactly once from Enter and exactly once from Space', () => {
const el = chip();
// Both keys are handled on keydown and both CANCEL the event. That is
// what holds the count at one: a cancelled keydown produces no
// activation click, so the keydown and click handlers never both run
// for one press (the double-fire DR-12 names). Cancelling is also what
// keeps Enter away from the editor's split-block keymap and Space away
// from scrolling the page.
for (const key of ['Enter', ' ']) {
panelOpenMock.mockReset();
const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true });
el.dispatchEvent(event);
expect(panelOpenMock).toHaveBeenCalledTimes(1);
expect(event.defaultPrevented).toBe(true);
}
});
it('leaves modified Space alone — a shortcut is not an activation', () => {
const el = chip();
el.dispatchEvent(
new KeyboardEvent('keydown', { key: ' ', ctrlKey: true, bubbles: true, cancelable: true })
);
expect(panelOpenMock).not.toHaveBeenCalled();
});
it('names the action, not just the file (DR-12)', () => {
// Type comes from the filename here, since no HEAD probe ran; size is
// omitted rather than reported as a confident "0 B".
expect(chip().getAttribute('aria-label')).toBe('Options for spec.pdf, PDF');
});
it('probes under the CURRENT workspace after a pane workspace switch', async () => {
// The pane switches workspace without remounting its editors, and the
// workspace keys the metadata cache — so a value baked in at configure
// time makes a mounted chip ask the PREVIOUS workspace about this
// workspace's attachment, and cache the answer under the wrong key
// (final review round 3).
probeMock.mockResolvedValue({ status: 'transient' });
let ws = 'ws-a';
editor = makeEditor(target, () => ({ ...ADDRESS, workspaceSlug: ws }));
chip();
await Promise.resolve();
expect(probeMock.mock.calls.map((c) => c[0])).toContain('ws-a');
probeMock.mockClear();
ws = 'ws-b';
repointChip('uuid-3', 'moved.pdf');
await Promise.resolve();
const probedWorkspaces = probeMock.mock.calls.map((c) => c[0]);
expect(probedWorkspaces).toContain('ws-b');
expect(probedWorkspaces).not.toContain('ws-a');
});
it('comes back to life when the node is repointed at a different attachment', () => {
// `disabled` is what makes a dead chip inert, so the uuid-swap path has
// to undo it along with everything else markDeleted() set. Leaving it
// would produce a chip that ANNOUNCES itself as live and does nothing —
// worse than the dead one, which at least says so. Reachable through a
// collaborative peer's edit or a ProseMirror node replacement.
const el = chip();
for (const fn of deletionListeners) fn('uuid-1');
expect(el.disabled).toBe(true);
repointChip('uuid-2', 'other.pdf');
const live = chip();
expect(live.disabled).toBe(false);
expect(live.classList.contains('attachment-missing')).toBe(false);
live.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 }));
expect(panelOpenMock).toHaveBeenCalledTimes(1);
expect(panelOpenMock.mock.calls[0][0]).toMatchObject({ attachmentId: 'uuid-2' });
});
it('offers no URL for a middle-click to bypass the panel with', () => {
// This is why the chip is a button. As an <a href download>, only the
// primary click ran the handler: middle-click and aux-click still opened
// or downloaded the file — exactly the accidental download the panel
// exists to prevent, reachable straight past it. A button has no URL to
// activate, so the bypass cannot exist rather than being intercepted.
const el = chip();
expect(el.tagName).toBe('BUTTON');
expect(el.hasAttribute('href')).toBe(false);
expect(el.hasAttribute('download')).toBe(false);
// type=button: inside a form, a default submit button would be worse
// than a link.
expect(el.getAttribute('type')).toBe('button');
el.dispatchEvent(new MouseEvent('auxclick', { bubbles: true, cancelable: true, button: 1 }));
expect(panelOpenMock).not.toHaveBeenCalled();
});
it('a deleted chip is inert AND unfocusable, not a dead focus stop', () => {
const el = chip();
for (const fn of deletionListeners) fn('uuid-1');
// `disabled` ⇒ not focusable and no events delivered at all, and no
// tabindex is ever set to put the stop back. The name says what
// happened rather than promising options.
expect(el.disabled).toBe(true);
expect(el.hasAttribute('tabindex')).toBe(false);
expect(el.getAttribute('aria-label')).toBe('spec.pdf — this attachment has been deleted');
expect(el.classList.contains('attachment-missing')).toBe(true);
const click = new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 });
el.dispatchEvent(click);
el.dispatchEvent(new KeyboardEvent('keydown', { key: ' ', bubbles: true, cancelable: true }));
expect(panelOpenMock).not.toHaveBeenCalled();
});
});
@@ -0,0 +1,189 @@
// The inline image's missing-attachment placeholder (PLAN-2392 DR-12 / DR-17).
//
// Two causes land on the same element and must NOT look the same to a keyboard
// or screen-reader user:
//
// - a transient load failure — retryable, so the placeholder is a button;
// - a CONFIRMED deletion — `retryLoad` refuses, so a button that announces
// itself and does nothing is a dead focus stop. That is the same failure
// the file chip's `disabled` closes, on the surface right next to it.
//
// Driven through a REAL Tiptap editor, like the chip spec: the placeholder is
// imperative NodeView DOM and its accessibility semantics are properties of
// that DOM, which a hand-built element would not pin.
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { Editor } from '@tiptap/core';
import StarterKit from '@tiptap/starter-kit';
const deletionListeners = new Set<(uuid: string) => void>();
vi.mock('$lib/attachments/events', () => ({
notifyAttachmentPanelOpen: () => {},
registerAttachmentDeletionListener: (fn: (uuid: string) => void) => {
deletionListeners.add(fn);
return () => deletionListeners.delete(fn);
},
}));
// No probe: these tests are about what the placeholder IS, not how it is
// discovered, and a real HEAD would make them asynchronous for no gain.
const probeMock = vi.fn(async () => ({ status: 'transient' as const }));
vi.mock('./attachment-metadata', () => ({
fetchAttachmentMetadata: () => probeMock(),
revalidateAttachmentMetadata: () => probeMock(),
invalidateAttachmentMetadata: () => {},
mimeToFormat: () => null,
}));
const { AttachmentImage } = await import('./attachment-image');
function makeEditor(element: HTMLElement): Editor {
return new Editor({
element,
extensions: [
StarterKit,
AttachmentImage.configure({
workspaceSlug: '',
getDownloadUrl: (uuid: string) => `/api/v1/workspaces/ws/attachments/${uuid}`,
// A workspace IS supplied: the MIME probe is what the viewer gate
// reads, and with an empty one it never runs (which is itself the
// documented "unknown ⇒ keep today's behaviour" path).
address: () => ({ workspaceSlug: 'ws', itemId: 'item-A', hostToken: 'apanel-1' }),
supportedFormats: [],
transform: async () => {
throw new Error('not used');
},
}),
],
content: '<p><img data-attachment-id="uuid-1" src="/api/v1/x" alt="A diagram"></p>',
editable: true,
});
}
describe('inline image missing placeholder', () => {
let target: HTMLElement;
let editor: Editor | undefined;
beforeEach(() => {
deletionListeners.clear();
probeMock.mockClear();
target = document.body.appendChild(document.createElement('div'));
});
afterEach(() => {
editor?.destroy();
editor = undefined;
target.remove();
});
function placeholder(): HTMLElement {
editor ??= makeEditor(target);
const el = target.querySelector<HTMLElement>('.attachment-missing');
if (!el) throw new Error('placeholder did not render');
return el;
}
function failLoad() {
const img = target.querySelector<HTMLImageElement>('img[data-attachment-id]');
if (!img) throw new Error('image NodeView did not render');
img.dispatchEvent(new Event('error'));
}
it('refuses to open a probed non-raster type in the viewer (DR-16)', async () => {
// The allowlist gates EVERY open-the-viewer path, not just the strip's:
// image/svg+xml can carry active content, and a node being labelled
// image/* is not sufficient reason to hand it to a viewer.
probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml' });
editor = makeEditor(target);
const img = target.querySelector<HTMLImageElement>('img[data-attachment-id]');
if (!img) throw new Error('image NodeView did not render');
// Select the node so the lazy MIME probe runs, then let it settle.
editor.commands.setNodeSelection(1);
await Promise.resolve();
await Promise.resolve();
img.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 }));
expect(document.querySelector('dialog.attachment-image-lightbox')).toBeNull();
});
it('still opens an allowlisted raster type', async () => {
// The gate must not cost the common case: a PNG opens as it always did.
probeMock.mockResolvedValue({ status: 'ok', mime: 'image/png' });
editor = makeEditor(target);
const img = target.querySelector<HTMLImageElement>('img[data-attachment-id]');
if (!img) throw new Error('image NodeView did not render');
editor.commands.setNodeSelection(1);
await Promise.resolve();
await Promise.resolve();
img.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 }));
expect(document.querySelector('dialog.attachment-image-lightbox')).not.toBeNull();
document.querySelector('dialog.attachment-image-lightbox')?.remove();
});
it('inertizes the transform toolbar when the attachment is deleted', async () => {
// A confirmed deletion inertizes the WHOLE node. Rotate and crop against
// a row that is gone can only 404, and leaving them live is the same
// dead-control gap the placeholder's role/tabindex removal closes.
probeMock.mockResolvedValue({ status: 'ok', mime: 'image/png' });
editor = makeEditor(target);
editor.commands.setNodeSelection(1);
await Promise.resolve();
await Promise.resolve();
const buttons = () =>
Array.from(
target.querySelectorAll<HTMLButtonElement>('.attachment-image-toolbar-btn')
);
expect(buttons().length).toBeGreaterThan(0);
for (const fn of deletionListeners) fn('uuid-1');
expect(buttons().every((b) => b.disabled)).toBe(true);
expect(buttons()[0].title).toBe('This attachment has been deleted');
});
it('is a focusable button while the failure is merely transient', () => {
editor = makeEditor(target);
failLoad();
const el = placeholder();
expect(el.style.display).not.toBe('none');
// Retryable, so it invites the retry and can be reached to perform it.
expect(el.getAttribute('role')).toBe('button');
expect(el.getAttribute('tabindex')).toBe('0');
expect(el.title).toContain('retry');
});
it('drops its interactive semantics once the deletion is confirmed', async () => {
editor = makeEditor(target);
failLoad();
expect(placeholder().getAttribute('role')).toBe('button');
// Another surface deleted the row: authoritative, and `retryLoad`
// refuses from here on.
for (const fn of deletionListeners) fn('uuid-1');
const el = placeholder();
expect(el.getAttribute('role')).toBeNull();
expect(el.getAttribute('tabindex')).toBeNull();
// The copy stops inviting a retry that cannot happen, too.
expect(el.title).toBe('This attachment has been deleted');
expect(el.title).not.toContain('retry');
});
it('does not leave focus stranded on a placeholder that just went inert', () => {
editor = makeEditor(target);
failLoad();
const el = placeholder();
el.focus();
expect(document.activeElement).toBe(el);
for (const fn of deletionListeners) fn('uuid-1');
// Removing tabindex from the focused element would otherwise leave
// focus on something unreachable by any further keystroke.
expect(document.activeElement).not.toBe(el);
});
});
@@ -32,21 +32,34 @@
* on screen must roll back and toast even if the user hit Retry while
* it was in flight.
* 3. `paint` — "does the CONTROL the user clicked belong to what is on
* screen?" Both control entry points fence on it — `handleDelete` for
* screen?" Both control entry points fence on it — `requestDelete` for
* a tile, `retryLoad` for the error row — at ENTRY, because the other
* two run after an await and no fence can unsend a request.
* two run after an await and no fence can unsend a request. The delete
* confirmation no longer blocks the thread (DR-18 / TASK-2425), so
* `confirmDelete` re-checks the same fence on the far side of it.
*/
import { onDestroy, untrack } from 'svelte';
import { api, PadApiError } from '$lib/api/client';
import type { AttachmentListItem } from '$lib/types';
import { iconForAttachment, formatBytes, isImage } from '$lib/attachments/display';
import {
iconForAttachment,
formatBytes,
canOpenInViewer,
describeAttachmentType,
displayFilename,
} from '$lib/attachments/display';
import AttachmentIcon from '$lib/attachments/icons/AttachmentIcon.svelte';
import Menu from '$lib/components/common/Menu.svelte';
import AttachmentDeleteConfirm, {
attachmentDeletePrompt,
} from '$lib/components/attachments/AttachmentDeleteConfirm.svelte';
import Lightbox, { type LightboxImage } from '$lib/components/common/Lightbox.svelte';
import { attachmentRefsIn } from '$lib/utils/commentAttachments';
import { invalidateAttachmentMetadata } from '$lib/components/editor/attachment-metadata';
import { toastStore } from '$lib/stores/toast.svelte';
import {
announceAttachmentDeleted,
notifyAttachmentPanelOpen,
registerAttachmentDeletionListener,
registerAttachmentUploadListener,
} from '$lib/attachments/events';
@@ -79,6 +92,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 +109,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
@@ -142,6 +165,27 @@
let attachments = $state<StripAttachment[]>([]);
let expanded = $state(false);
let lightbox = $state<{ images: LightboxImage[]; index: number } | null>(null);
/**
* The delete confirmation currently on screen, if any (PLAN-2392 DR-18 /
* TASK-2425). One at a time: opening a second supersedes the first, which
* is what a `<Menu>` anchored to a single trigger can represent anyway.
*
* `anchor` is the tile's own `×` — the menu positions against it and
* returns focus to it on Cancel / Escape, which is what keeps the control
* keyboard-usable end to end.
*
* `prompt` is captured at OPEN time, not derived: the warning reads the
* editor's live markdown, and re-deriving it while the confirmation is up
* would let the message change under the user as they type.
*/
let pendingDelete = $state<{
att: StripAttachment;
anchor: HTMLElement | null;
prompt: string;
} | null>(null);
const uid = $props.id();
const promptId = `attachment-delete-note-${uid}`;
// Three distinguishable states, not two (DR-10). `loadFailed` is what stops
// a fetch failure from rendering as "no attachments"; `showLoading` is the
@@ -263,6 +307,12 @@
attachments = [];
expanded = false;
lightbox = null;
// A confirmation left up for a row the user is no longer looking
// at must go with it — confirming it after the switch would
// DELETE the previous view's attachment from behind the new one
// (the entry fence re-check in `confirmDelete` refuses it, but
// leaving the prompt on screen at all is the wrong picture).
pendingDelete = null;
deletedIds = new Set();
pendingUploads = [];
beyondStripCount = 0;
@@ -472,10 +522,31 @@
// on it would corrupt this item's count. The continuation can therefore
// overstate by one until the next load; that is the safe direction, and it
// self-corrects (Codex round 5).
/**
* Permission withdrawn while the tile's confirmation is open.
*
* `canDelete` is the host's `mutationsEnabled`, so it flips the moment this
* side goes peeked. The delete CONTROL disappears with it (the `{#if
* canDelete}` below), but an already-open confirmation would linger — a
* live "Delete file" prompt anchored to a control that is no longer there.
* Same rejection the panel does, for the same reason.
*/
$effect(() => {
const mayDelete = canDelete;
untrack(() => {
if (mayDelete) return;
pendingDelete = null;
});
});
$effect(() => {
return registerAttachmentDeletionListener((deletedUuid) => {
rememberDeleted(deletedUuid);
attachments = attachments.filter((a) => a.id !== deletedUuid);
// The tile this confirmation is anchored to just went away, so the
// menu would be left pointing at a detached element — and the
// question it is asking has already been answered by someone else.
if (pendingDelete?.att.id === deletedUuid) pendingDelete = null;
});
});
@@ -575,10 +646,17 @@
// Image tiles in strip order, so the lightbox's ←/→ page through the
// item's images (DR-8 — the existing Lightbox, not a second one).
//
// Gated on `canOpenInViewer`, NOT `isImage` (PLAN-2392 DR-16): the viewer
// takes an exact raster allowlist, so an `image/svg+xml` row — legacy,
// mislabelled, or sniffed from XML — renders as a FILE tile and never
// reaches the viewer, because SVG can carry active content. Both this list
// and the tile branch below read the same predicate, so a file tile can
// never be a member of the lightbox's set.
let lightboxImages = $derived<LightboxImage[]>(
attachments
.filter((a) => isImage(a.mime_type))
.map((a) => ({ id: a.id, alt: a.filename }))
.filter((a) => canOpenInViewer(a.mime_type))
.map((a) => ({ id: a.id, alt: displayFilename(a.filename) }))
);
function openLightbox(att: StripAttachment) {
@@ -587,8 +665,49 @@
lightbox = { images: lightboxImages, index };
}
/** What the file IS — the tooltip, and the base of the accessible name. */
function tileLabel(att: StripAttachment): string {
return `${att.filename} (${formatBytes(att.size_bytes)})`;
return `${displayFilename(att.filename)}, ${describeAttachmentType(att.mime_type, att.filename)}, ${formatBytes(att.size_bytes)}`;
}
/**
* The accessible name: `tileLabel` plus the ACTION the tile performs
* (PLAN-2392 DR-12). A file tile no longer downloads on tap, so a name
* that says only what the file is would leave a screen-reader user with
* no way to know what activating it does — and this is the only signpost
* for the changed behavior, since DR-1 deliberately adds no `⋯` control.
*/
function tileActionLabel(att: StripAttachment): string {
const action = canOpenInViewer(att.mime_type) ? 'View' : 'Options for';
return `${action} ${tileLabel(att)}`;
}
/**
* A file tile opens the options panel instead of downloading (DR-1).
*
* ENTRY-fenced for the same reason `requestDelete` is: the clicked tile was
* painted for `paint`'s identity, while `itemId` is live and may already
* name a different view. `itemId` is the event's ROUTING field — which
* `ItemDetail` mount shows the panel — so a stale click would open this
* attachment's panel over a different item's pane.
*
* `anchor` is the tile itself: the panel positions against it and returns
* focus to it on close, which is what makes keyboard activation land
* somewhere sensible.
*/
function openOptions(att: StripAttachment, anchor: HTMLElement | null) {
if (!paint.isCurrent()) return;
notifyAttachmentPanelOpen({
attachmentId: att.id,
itemId: itemId ?? '',
hostToken,
anchor,
// The strip always has all three from its list row — unlike an
// editor chip, whose HEAD probe may not have resolved (DR-2).
filename: att.filename,
mime_type: att.mime_type,
size_bytes: att.size_bytes,
});
}
/**
@@ -612,30 +731,17 @@
}
/**
* Confirm text for a delete (DR-5).
* Open the delete confirmation for a tile (DR-18 / TASK-2425).
*
* The "not referenced here" arm deliberately does NOT claim the attachment
* is unused: a reference can live in another item's content, in an item's
* fields JSON, or in any comment. The server's AttachmentReferenced scan
* covers all three, but none of it is visible client-side — so the wording
* stays honest about what we actually checked.
* This used to raise a browser-native `window.confirm`, which meant one
* object had two confirmation styles — the options panel already drilled
* down to an in-app sub-view. Both surfaces now render the SAME
* `AttachmentDeleteConfirm`, wording included; only the container differs
* (the panel's own menu there, a menu anchored to the `×` here).
*
* The delete REQUEST path below is untouched by that change.
*/
function confirmMessage(att: StripAttachment): string {
if (referencedIds().has(att.id)) {
return (
`Delete ${att.filename}?\n\n` +
"It's still used in this item's content — deleting it will leave a " +
'"missing attachment" placeholder where it appears.'
);
}
return (
`Delete ${att.filename}?\n\n` +
"It isn't referenced in this item's content, but it may still be " +
'referenced by another item or a comment. This cannot be undone.'
);
}
async function handleDelete(att: StripAttachment) {
function requestDelete(att: StripAttachment, anchor: HTMLElement | null) {
if (!canDelete) return;
// ENTRY fence (fence 3 — see the header). The clicked tile was painted
@@ -649,10 +755,51 @@
// confirm and before the call.
if (!paint.isCurrent()) return;
if (typeof window !== 'undefined' && !window.confirm(confirmMessage(att))) return;
// window.confirm blocks the thread, so nothing can have moved between
// the fence above and here.
pendingDelete = {
att,
anchor,
prompt: attachmentDeletePrompt(att.filename, referencedIds().has(att.id)),
};
}
/** Dismissal that isn't an explicit Cancel — Escape, or a click outside. */
function dismissDelete() {
pendingDelete = null;
}
/**
* The Cancel row. Returns focus to the `×` the confirmation was anchored
* to: `window.confirm` restored it for free, and the control is
* opacity-hidden unless its cell has focus-within, so a keyboard user who
* cancels would otherwise be dropped on <body> with the control they came
* from now invisible.
*
* Deliberately NOT wired to `Menu`'s `onclose`: Escape already refocuses
* the trigger inside `Menu`, and an outside click must not have focus
* yanked back off whatever the user just clicked.
*/
function cancelDelete() {
const anchor = pendingDelete?.anchor;
pendingDelete = null;
anchor?.focus();
}
/**
* The user confirmed. `window.confirm` blocked the thread, so the entry
* fence taken when the `×` was clicked was still true by definition when it
* returned; an in-app confirmation does NOT block, and the user can switch
* item or workspace while it is up. So the fence — and the permission — are
* re-checked HERE, at the point that actually sends the request.
*/
function confirmDelete() {
const pending = pendingDelete;
pendingDelete = null;
if (!pending) return;
if (!canDelete || !paint.isCurrent()) return;
void performDelete(pending.att);
}
async function performDelete(att: StripAttachment) {
// Capture identity BEFORE the await (fence 2): a switch mid-delete must
// not roll the tile back into a DIFFERENT item's strip, and must not
// toast over it. The DELETE itself still lands — it targets an id, not a
@@ -731,8 +878,8 @@
}
toastStore.show(
code === 'forbidden'
? `You don't have permission to delete ${att.filename}.`
: `Couldn't delete ${att.filename}.`,
? `You don't have permission to delete ${displayFilename(att.filename)}.`
: `Couldn't delete ${displayFilename(att.filename)}.`,
'error'
);
}
@@ -768,15 +915,15 @@
{#if attachments.length > 0 || hasMoreThanStrip}
<div class="strip-row">
{#each visible as att (att.id)}
<!-- The delete control can't nest inside the tile's own button /
anchor, so each tile gets a positioned wrapper. -->
<!-- The delete control can't nest inside the tile's own
button, so each tile gets a positioned wrapper. -->
<div class="att-cell">
{#if isImage(att.mime_type)}
{#if canOpenInViewer(att.mime_type)}
<button
type="button"
class="att-tile"
title={tileLabel(att)}
aria-label={tileLabel(att)}
aria-label={tileActionLabel(att)}
onclick={() => openLightbox(att)}
>
<img
@@ -786,18 +933,30 @@
/>
</button>
{:else}
<a
<!--
A real <button>, not an <a download> (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).
-->
<button
type="button"
class="att-tile"
href={api.attachments.downloadUrl(wsSlug, att.id)}
download={att.filename}
title={tileLabel(att)}
aria-label={tileLabel(att)}
aria-label={tileActionLabel(att)}
onclick={(e) => openOptions(att, e.currentTarget)}
>
<span class="att-icon">
<AttachmentIcon id={iconForAttachment(att.mime_type, att.filename)} />
</span>
<span class="att-name" aria-hidden="true">{att.filename}</span>
</a>
<span class="att-name" aria-hidden="true">{displayFilename(att.filename)}</span>
</button>
{/if}
{#if canDelete}
@@ -807,9 +966,9 @@
<button
type="button"
class="att-delete"
title="Delete {att.filename}"
aria-label="Delete {att.filename}"
onclick={() => handleDelete(att)}
title="Delete {displayFilename(att.filename)}"
aria-label="Delete {displayFilename(att.filename)}"
onclick={(e) => requestDelete(att, e.currentTarget)}
>
×
</button>
@@ -846,6 +1005,43 @@
</section>
{/if}
<!--
The delete confirmation (DR-18). The same `Menu` presentation the options
panel uses — popover on desktop, BottomSheet at the mobile breakpoint — so
ESC ordering, outside-click, portal placement and focus return are the
app's existing behaviours rather than a second implementation. Focus
returns to the `×` the menu is anchored to, and Cancel is its first row, so
Enter on arrival can never delete.
-->
{#if pendingDelete}
<!--
Every prop below is read through `?.` even though the block only exists
while `pendingDelete` is set: `Menu` places itself in a `tick().then()`,
which can run AFTER the confirmation was dismissed and this block torn
down, and a prop expression is re-evaluated on every read. Reading
`pendingDelete.anchor` there throws an unhandled rejection — real, and
observed in the suite.
-->
<Menu
open
onclose={dismissDelete}
trigger={pendingDelete?.anchor ?? undefined}
mode="portal"
width={272}
sheetOnMobile
sheetTitle="Delete {displayFilename(pendingDelete?.att.filename)}"
ariaLabel="Delete {displayFilename(pendingDelete?.att.filename)}"
focusKey={pendingDelete?.att.id}
>
<AttachmentDeleteConfirm
prompt={pendingDelete?.prompt ?? ''}
{promptId}
oncancel={cancelDelete}
onconfirm={confirmDelete}
/>
</Menu>
{/if}
{#if lightbox}
<Lightbox
images={lightbox.images}
@@ -984,6 +1180,12 @@
text-decoration: none;
overflow: hidden;
cursor: pointer;
/* Both tiles are <button>s as of TASK-2424, and the file tile is the
one with TEXT in it. Without this the UA's button font (13.33px
Arial) replaces the inherited one, and `.att-name`'s 0.6em would be
measured against it — a visibly smaller, differently-faced filename
than the anchor rendered. */
font: inherit;
}
.att-tile:hover {
border-color: var(--accent, var(--border));
@@ -49,11 +49,17 @@ function broadcastUpload(itemId: string, a: UploadedAttachment) {
for (const fn of uploadListeners) fn(itemId, a);
}
// TASK-2424: the strip is now also an EMITTER on the open-panel channel, so
// the mocked module has to carry that export too (a vi.mock factory replaces
// the whole module — a missing export is an import error, not a silent hole).
const panelOpenMock = vi.fn<(event: Record<string, unknown>) => void>();
vi.mock('$lib/attachments/events', () => ({
announceAttachmentDeleted: (ws: string, uuid: string) => {
notifyDeletedMock(uuid);
invalidateMock(ws, uuid);
},
notifyAttachmentPanelOpen: (event: Record<string, unknown>) => panelOpenMock(event),
registerAttachmentDeletionListener: (fn: (uuid: string) => void) => {
deletionListeners.add(fn);
return () => deletionListeners.delete(fn);
@@ -119,6 +125,7 @@ const props = $state<{
canDelete: boolean;
itemContent: string | null;
liveContent: (() => string | null) | null;
hostToken: string;
}>({
wsSlug: 'ws',
username: 'dave',
@@ -126,6 +133,7 @@ const props = $state<{
canDelete: false,
itemContent: null,
liveContent: null,
hostToken: 'host-1',
});
describe('ItemAttachmentStrip', () => {
@@ -140,6 +148,8 @@ describe('ItemAttachmentStrip', () => {
notifyDeletedMock.mockReset();
invalidateMock.mockReset();
invalidateMetadataMock.mockReset();
panelOpenMock.mockReset();
props.hostToken = 'host-1';
props.wsSlug = 'ws';
props.username = 'dave';
props.itemId = null;
@@ -201,7 +211,12 @@ describe('ItemAttachmentStrip', () => {
expect(target.querySelector('.fields-header')?.textContent).toBe('Attachments · 2');
});
it('labels tiles with filename + human size and links non-images to a download', async () => {
// TASK-2424 (PLAN-2392 DR-1 / DR-12) deliberately falsifies the previous
// version of this test, which pinned the non-image tile as an
// `<a href download>`: a tap used to put the file straight in Downloads.
// The tile is now a real button that opens the options panel, and Download
// is a deliberate choice inside it.
it('renders non-image tiles as panel-trigger buttons, not download links', async () => {
listMock.mockResolvedValue(
response([
att({ id: 'doc', mime_type: 'application/pdf', filename: 'spec.pdf', size_bytes: 1536 }),
@@ -211,11 +226,111 @@ describe('ItemAttachmentStrip', () => {
await settle();
const tile = tiles()[0];
expect(tile.tagName).toBe('A');
expect(tile.getAttribute('aria-label')).toBe('spec.pdf (1.5 KB)');
expect(tile.getAttribute('title')).toBe('spec.pdf (1.5 KB)');
expect(tile.getAttribute('href')).toBe('/api/v1/workspaces/ws/attachments/doc');
expect(tile.getAttribute('download')).toBe('spec.pdf');
expect(tile.tagName).toBe('BUTTON');
expect(tile.getAttribute('type')).toBe('button');
// Nothing downloads on tap any more.
expect(tile.getAttribute('href')).toBeNull();
expect(tile.getAttribute('download')).toBeNull();
// The accessible name carries filename, TYPE and the ACTION (DR-12) —
// the only signpost for the changed behaviour, since DR-1 adds no `⋯`.
expect(tile.getAttribute('aria-label')).toBe('Options for spec.pdf, PDF, 1.5 KB');
expect(tile.getAttribute('title')).toBe('spec.pdf, PDF, 1.5 KB');
});
it('emits the open-panel event with the anchor and all three metadata fields', async () => {
listMock.mockResolvedValue(
response([
att({ id: 'doc', mime_type: 'application/pdf', filename: 'spec.pdf', size_bytes: 1536 }),
])
);
mountStrip('item-a');
await settle();
const tile = tiles()[0];
tile.click();
flushSync();
expect(panelOpenMock).toHaveBeenCalledTimes(1);
expect(panelOpenMock).toHaveBeenCalledWith({
attachmentId: 'doc',
// Routing: which ItemDetail mount shows the panel (DR-8).
itemId: 'item-a',
hostToken: 'host-1',
anchor: tile,
// The strip always has all three from its list row, unlike a chip.
filename: 'spec.pdf',
mime_type: 'application/pdf',
size_bytes: 1536,
});
});
it('adds no keydown handler that would race the UA activation click', async () => {
// NAMED for what it can prove. "Activates exactly once per key press" is
// the requirement (DR-12), but jsdom does not synthesise a button's
// activation click, so no jsdom test can demonstrate it — the browser
// suite does (web/e2e/item-attachment-strip.spec.ts). What IS falsifiable
// here is the failure mode DR-12 names: a hand-rolled keydown handler
// firing ALONGSIDE the UA's click and opening the panel twice.
listMock.mockResolvedValue(
response([att({ id: 'doc', mime_type: 'application/pdf', filename: 'spec.pdf' })])
);
mountStrip('item-a');
await settle();
const tile = tiles()[0] as HTMLButtonElement;
// DR-12 wants Enter AND Space to activate, exactly once each. A native
// <button> is how that is guaranteed: the UA converts both keys into a
// single `click` and already suppresses Space's page scroll. The thing
// a test can actually falsify is the failure mode DR-12 names — a
// hand-rolled keydown handler firing ALONGSIDE the UA's click, opening
// the panel twice. jsdom does not synthesise the activation click, so
// the key press alone must produce nothing...
for (const key of ['Enter', ' ']) {
const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true });
tile.dispatchEvent(event);
flushSync();
expect(panelOpenMock).not.toHaveBeenCalled();
// ...and Space must not be swallowed as a scroll suppressor either:
// the UA's own default handling is what we are relying on.
expect(event.defaultPrevented).toBe(false);
}
// The UA's click is then the one and only activation path.
tile.click();
flushSync();
expect(panelOpenMock).toHaveBeenCalledTimes(1);
});
it('renders an SVG as a FILE tile and keeps it out of the viewer (DR-16)', async () => {
// `isImage` would say yes to this — the viewer takes an exact raster
// allowlist instead, because SVG can carry active content.
listMock.mockResolvedValue(
response([
att({ id: 'svg', mime_type: 'image/svg+xml', filename: 'logo.svg' }),
att({ id: 'png', mime_type: 'image/png', filename: 'shot.png' }),
])
);
mountStrip('item-a');
await settle();
const [svgTile, pngTile] = tiles();
// The SVG got the file path: an icon + name, no thumbnail request.
expect(svgTile.querySelector('img')).toBeNull();
expect(svgTile.getAttribute('aria-label')).toContain('Options for logo.svg');
svgTile.click();
flushSync();
expect(document.querySelector('.lightbox-backdrop')).toBeNull();
expect(panelOpenMock).toHaveBeenCalledTimes(1);
// ...and it isn't a member of the lightbox's set either, so the PNG's
// viewer holds ONE image rather than paging into the SVG (the counter
// only renders for a multi-image set, so its absence IS the assertion).
panelOpenMock.mockClear();
pngTile.click();
flushSync();
expect(panelOpenMock).not.toHaveBeenCalled();
expect(document.querySelector('.lightbox-backdrop')).not.toBeNull();
expect(document.querySelector('.lightbox-counter')).toBeNull();
});
it('renders images as thumb-sm buttons that open the lightbox', async () => {
@@ -713,17 +828,63 @@ describe('ItemAttachmentStrip', () => {
});
// ── 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<HTMLButtonElement>('.att-delete'));
}
/** Portaled to <body> like every other Menu, so queried document-wide. */
function confirmPanel(): HTMLElement | null {
return document.querySelector<HTMLElement>('[role="menu"]');
}
function confirmRows(): HTMLElement[] {
return Array.from(document.querySelectorAll<HTMLElement>('[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;
@@ -754,6 +915,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
// <body> 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
@@ -765,16 +974,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 () => {
@@ -784,16 +991,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 () => {
@@ -802,19 +1006,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 <img> 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 () => {
@@ -831,18 +1078,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 () => {
@@ -855,15 +1100,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 () => {
@@ -873,8 +1140,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);
@@ -882,7 +1149,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 () => {
@@ -901,9 +1167,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.
@@ -919,7 +1184,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 () => {
@@ -939,11 +1203,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'));
@@ -954,7 +1217,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 () => {
@@ -966,13 +1228,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 () => {
@@ -993,9 +1254,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';
@@ -1012,7 +1272,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 () => {
@@ -1030,9 +1289,8 @@ describe('ItemAttachmentStrip', () => {
failDelete = reject;
})
);
const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true);
deleteButtons()[0].click();
flushSync();
openConfirm();
clickConfirm();
broadcastDeletion('a1');
flushSync();
@@ -1042,7 +1300,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 () => {
@@ -1055,13 +1312,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 () => {
@@ -1071,13 +1327,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 () => {
@@ -1101,9 +1356,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.
@@ -1119,7 +1373,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 () => {
@@ -1140,9 +1393,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 —
@@ -1161,7 +1413,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 () => {
@@ -1181,9 +1432,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';
@@ -1200,7 +1450,6 @@ describe('ItemAttachmentStrip', () => {
expect(tiles()).toHaveLength(0);
expect(toastMock).not.toHaveBeenCalled();
confirmSpy.mockRestore();
});
// ── Upload refresh (TASK-2385) ────────────────────────────────────────
@@ -1546,12 +1795,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 () => {
@@ -1565,11 +1811,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");
});
});
+68 -13
View File
@@ -42,6 +42,8 @@
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';
import { isSamePaneTarget, breadcrumbParentTarget } from '$lib/collections/paneTarget';
@@ -772,6 +774,43 @@
// (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();
/**
* 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();
@@ -5012,21 +5051,34 @@
{wsSlug}
{username}
itemId={itemMatchesRef ? item?.id : null}
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}
/>
<!-- The attachment options panel's host (PLAN-2392 DR-8 / TASK-2423).
ONE per ItemDetail mount, and the only consumer of the open-panel
channel for this mount's token — the panel it opens is
permissioned by THIS host's `mutationsEnabled`, never by the
emitting strip tile / editor chip, and never by the timeline's
`canEdit` (which ignores `peeking`).
Sits beside the strip, OUTSIDE the {#key itemSlug} block, for the
same reason: it must not remount on an A→B switch — it closes the
panel itself when `itemId` changes.
parentArchived (DR-14): an archived parent's attachment fetch
404s, so an open panel would keep offering an Open and a Download
that both fail. `isArchived` follows the SSE item_archived /
item_restored refetches above, so archive closes the panel and
restore revalidates it. -->
<AttachmentPanelHost
{wsSlug}
itemId={itemMatchesRef ? item?.id : null}
hostToken={attachmentHostToken}
{mutationsEnabled}
itemContent={itemMatchesRef ? item?.content : null}
liveContent={liveEditorMarkdown}
parentArchived={itemMatchesRef && isArchived}
/>
<!-- Content editor — OUTSIDE the {#key itemSlug} above: the collab
@@ -5276,6 +5328,7 @@
onUpdate={handleContentUpdate}
editable={false}
itemId={item.id}
hostToken={attachmentHostToken}
onEditor={(e) => editorInstance = e}
onImportInserted={handleImportInserted}
/>
@@ -5333,6 +5386,7 @@
onUpdate={handleContentUpdate}
editable={!peeking}
itemId={item.id}
hostToken={attachmentHostToken}
ydoc={ydoc}
awareness={collabProvider?.awareness}
collabUser={collabUserState}
@@ -5535,6 +5589,7 @@
onRestore={handleVersionRestore}
flushBeforeRestore={flushCollabBeforeRestore}
itemId={item.id}
hostToken={attachmentHostToken}
collectionId={item.collection_id}
frozen={false}
restoreFrozen={peeking}
+148 -16
View File
@@ -12,13 +12,23 @@
WorkspaceStorageInfo
} from '$lib/types';
import { toastStore } from '$lib/stores/toast.svelte';
import { iconForAttachment, formatBytes, isImage } from '$lib/attachments/display';
import {
iconForAttachment,
formatBytes,
isImage,
canOpenInViewer,
displayFilename,
} from '$lib/attachments/display';
import {
buildStorageFilters,
hasActiveStorageFilters,
type StorageFilterSelections
} from '$lib/attachments/storageFilters';
import AttachmentIcon from '$lib/attachments/icons/AttachmentIcon.svelte';
import Menu from '$lib/components/common/Menu.svelte';
import AttachmentDeleteConfirm, {
workspaceAttachmentDeletePrompt,
} from '$lib/components/attachments/AttachmentDeleteConfirm.svelte';
import { viewIdentity, createFence, createPaintFence } from '$lib/attachments/viewFence';
// ── Props ────────────────────────────────────────────────────────────────
@@ -49,6 +59,22 @@
let loading = $state(true);
let usage = $state<WorkspaceStorageInfo | null>(null);
let attachments = $state<AttachmentListItem[]>([]);
/**
* 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 +274,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 +430,64 @@
// ── 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: workspaceAttachmentDeletePrompt(att.filename),
};
}
/** 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.
*/
/**
* Ids with a DELETE in flight. A plain Set, not `$state`: nothing renders
* from it, and it is read by the guard in `confirmDelete` which needs the
* value as of right now rather than a reactive snapshot.
*/
const deletingIds = new Set<string>();
function confirmDelete() {
const pending = pendingDelete;
pendingDelete = null;
if (!pending) return;
if (!paint.isCurrent()) return;
// One delete at a time. `confirm()` blocked the thread, so a second
// confirmation could not be raised while the first request was in
// flight; an in-app one can, and this list does not remove the row
// optimistically the way the item strip does, so the same row stays
// clickable throughout. Two DELETEs for one row means the second gets a
// 404 and the user sees a success and an "already deleted" for a single
// action (orchestrator's full-diff review round 2).
if (deletingIds.has(pending.att.id)) 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 +505,7 @@
// 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;
deletingIds.add(att.id);
try {
await api.attachments.delete(reqWsSlug, att.id);
// Same broadcast the item attachment strip does (PLAN-2382 /
@@ -448,7 +533,7 @@
if (paint.isCurrent() && !painted.changed()) await reload();
return;
}
toastStore.show(`Deleted ${att.filename}`, 'success');
toastStore.show(`Deleted ${displayFilename(att.filename)}`, 'success');
await reload();
} catch (err) {
// A 404 is authoritative that the row is gone — the list was simply
@@ -465,13 +550,17 @@
if (paint.isCurrent() && !painted.changed()) await reload();
return;
}
toastStore.show(`${att.filename} was already deleted`, 'info');
toastStore.show(`${displayFilename(att.filename)} was already deleted`, 'info');
await reload();
return;
}
if (req.stale()) return;
const msg = err instanceof Error ? err.message : 'Failed to delete attachment';
toastStore.show(msg, 'error');
} finally {
// Every arm above returns from inside the try/catch, so the release
// has to be here or a failed delete would block the row forever.
deletingIds.delete(att.id);
}
}
@@ -664,17 +753,29 @@
<div class="att-list">
{#each attachments as att (att.id)}
<div class="att-row card">
<a
<!-- DR-16: the THUMBNAIL still renders for anything image-ish
(it is an <img> either way), but the link that hands the
ORIGINAL to the browser is gated on the exact raster
allowlist — `image/svg+xml` can carry active content and
the download handler defaults unknown types to an inline
disposition. A non-allowlisted row keeps the row, loses
the open-in-new-tab. -->
<svelte:element
this={canOpenInViewer(att.mime_type) ? 'a' : 'div'}
class="att-thumb"
href={api.attachments.downloadUrl(wsSlug, att.id)}
target="_blank"
rel="noopener"
aria-label="Open {att.filename}"
href={canOpenInViewer(att.mime_type)
? api.attachments.downloadUrl(wsSlug, att.id)
: undefined}
target={canOpenInViewer(att.mime_type) ? '_blank' : undefined}
rel={canOpenInViewer(att.mime_type) ? 'noopener' : undefined}
aria-label={canOpenInViewer(att.mime_type)
? `Open ${displayFilename(att.filename)}`
: undefined}
>
{#if isImage(att.mime_type)}
<img
src={api.attachments.downloadUrl(wsSlug, att.id, 'thumb-sm')}
alt={att.filename}
alt={displayFilename(att.filename)}
loading="lazy"
/>
{:else}
@@ -682,11 +783,13 @@
<AttachmentIcon id={iconForAttachment(att.mime_type, att.filename)} />
</span>
{/if}
</a>
</svelte:element>
<div class="att-meta">
<div class="att-line1">
<span class="att-filename" title={att.filename}>{att.filename}</span>
<span class="att-filename" title={displayFilename(att.filename)}
>{displayFilename(att.filename)}</span
>
</div>
<div class="att-line2">
<span class="att-size">{formatBytes(att.size_bytes)}</span>
@@ -716,7 +819,9 @@
<button
type="button"
class="btn btn-small btn-remove"
onclick={() => handleDelete(att)}
onclick={(e) => requestDelete(att, e.currentTarget)}
title="Delete {displayFilename(att.filename)}"
aria-label="Delete {displayFilename(att.filename)}"
>
Delete
</button>
@@ -751,6 +856,33 @@
{/if}
</div>
<!--
The delete confirmation (PLAN-2392 DR-18 / TASK-2425) — the same in-app
drill-down the item strip and the options panel show, anchored to the row's
own Delete button. Props are read through `?.` because `Menu` places itself
in a `tick().then()` that can run after this block is torn down.
-->
{#if pendingDelete}
<Menu
open
onclose={dismissDelete}
trigger={pendingDelete?.anchor ?? undefined}
mode="portal"
width={272}
sheetOnMobile
sheetTitle="Delete {displayFilename(pendingDelete?.att.filename)}"
ariaLabel="Delete {displayFilename(pendingDelete?.att.filename)}"
focusKey={pendingDelete?.att.id}
>
<AttachmentDeleteConfirm
prompt={pendingDelete?.prompt ?? ''}
{promptId}
oncancel={cancelDelete}
onconfirm={confirmDelete}
/>
</Menu>
{/if}
<style>
/* ── Local copies of the parent settings page primitives ────────────────
* Svelte 5 scopes styles to the component, so a child cannot reach the
@@ -141,6 +141,27 @@ describe('StorageTab workspace switching', () => {
return target.textContent ?? '';
}
/**
* The delete confirmation is an in-app drill-down as of TASK-2425
* (PLAN-2392 DR-18) the same one the item strip and the options panel
* show rather than a browser-native `confirm()`. It is portaled to
* <body> like every other Menu, so it is queried document-wide.
*/
function confirmPanel(): HTMLElement | null {
return document.querySelector<HTMLElement>('[role="menu"]');
}
/** Click a row's Delete, then the destructive row of the confirmation. */
function deleteAndConfirm(index = 0) {
Array.from(target.querySelectorAll<HTMLButtonElement>('.btn-remove'))[index].click();
flushSync();
const confirmRow = Array.from(
document.querySelectorAll<HTMLElement>('[role="menu"] [role="menuitem"]')
).find((el) => el.querySelector('.mi-label')?.textContent?.trim() === 'Delete file');
confirmRow!.click();
flushSync();
}
it('loads exactly once on mount', async () => {
mountTab();
await settle();
@@ -277,9 +298,7 @@ describe('StorageTab workspace switching', () => {
const slowDelete = deferred<void>();
deleteMock.mockReturnValueOnce(slowDelete.promise);
const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true);
target.querySelector<HTMLButtonElement>('.btn-remove')?.click();
flushSync();
deleteAndConfirm();
expect(deleteMock).toHaveBeenCalledWith('ws-a', 'a1');
listMock.mockResolvedValueOnce(response([att({ id: 'b1', filename: 'from-b.pdf' })]));
@@ -299,7 +318,6 @@ describe('StorageTab workspace switching', () => {
expect(toastMock).not.toHaveBeenCalled();
expect(listMock.mock.calls.length).toBe(listCalls);
expect(text()).toContain('from-b.pdf');
confirmSpy.mockRestore();
});
it('still suppresses a delete continuation after an A→B→A round trip', async () => {
@@ -313,9 +331,7 @@ describe('StorageTab workspace switching', () => {
const slowDelete = deferred<void>();
deleteMock.mockReturnValueOnce(slowDelete.promise);
const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true);
target.querySelector<HTMLButtonElement>('.btn-remove')?.click();
flushSync();
deleteAndConfirm();
expect(deleteMock).toHaveBeenCalledWith('ws-a', 'a1');
props.wsSlug = 'ws-b';
@@ -339,7 +355,6 @@ describe('StorageTab workspace switching', () => {
// against ws-a (Codex round 4).
expect(listMock.mock.calls.length).toBe(listCalls + 1);
expect(listMock).toHaveBeenLastCalledWith('ws-a', expect.anything());
confirmSpy.mockRestore();
});
it('does not toast or refetch for a delete that resolves after unmount', async () => {
@@ -349,9 +364,7 @@ describe('StorageTab workspace switching', () => {
const slowDelete = deferred<void>();
deleteMock.mockReturnValueOnce(slowDelete.promise);
const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true);
target.querySelector<HTMLButtonElement>('.btn-remove')?.click();
flushSync();
deleteAndConfirm();
unmount(instance!);
instance = undefined;
@@ -366,7 +379,6 @@ describe('StorageTab workspace switching', () => {
expect(announceMock).toHaveBeenCalledWith('ws-a', 'a1');
expect(toastMock).not.toHaveBeenCalled();
expect(listMock.mock.calls.length).toBe(listCalls);
confirmSpy.mockRestore();
});
it('does not toast for a list or usage request that fails after unmount', async () => {
@@ -401,7 +413,6 @@ describe('StorageTab workspace switching', () => {
mountTab();
await settle();
const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true);
props.wsSlug = 'ws-b';
// No flushSync between the switch and the click: that IS the window.
target.querySelector<HTMLButtonElement>('.btn-remove')?.click();
@@ -409,8 +420,63 @@ describe('StorageTab workspace switching', () => {
await settle();
expect(deleteMock).not.toHaveBeenCalled();
expect(confirmSpy).not.toHaveBeenCalled();
confirmSpy.mockRestore();
// Not even prompted — the row the user aimed at belongs to a workspace
// this tab is no longer showing.
expect(confirmPanel()).toBeNull();
});
it('sends one DELETE even if the row is confirmed twice while the first is in flight', async () => {
// `confirm()` blocked the thread, so a second confirmation could not be
// raised while a request was in flight. An in-app one can — and unlike
// the item strip, this list does not remove the row optimistically, so
// the same row stays clickable throughout. Two DELETEs for one row means
// the second 404s and the user sees a success AND an "already deleted"
// for a single action (final review round 2).
listMock.mockResolvedValueOnce(response([att({ id: 'a1', filename: 'doc.pdf' })]));
mountTab();
await settle();
const slowDelete = deferred<void>();
deleteMock.mockReturnValueOnce(slowDelete.promise);
deleteAndConfirm();
expect(deleteMock).toHaveBeenCalledTimes(1);
deleteAndConfirm();
expect(deleteMock).toHaveBeenCalledTimes(1);
// Fail it: the row stays in the list, which is exactly the case where a
// guard that is never released would strand it as undeletable forever.
listMock.mockResolvedValueOnce(response([att({ id: 'a1', filename: 'doc.pdf' })]));
slowDelete.reject(new Error('network'));
await settle();
expect(rows()).toHaveLength(1);
deleteMock.mockResolvedValueOnce(undefined);
listMock.mockResolvedValueOnce(response([]));
deleteAndConfirm();
expect(deleteMock).toHaveBeenCalledTimes(2);
});
it('treats a 404 as authoritative, exactly like a success', async () => {
// The 404 arm claims parity with the success arm — same broadcast, same
// refresh — and nothing tested it: the shared descriptor's 404 test
// covers a different implementation (final review round 2).
listMock.mockResolvedValueOnce(response([att({ id: 'a1', filename: 'gone.pdf' })]));
mountTab();
await settle();
deleteMock.mockRejectedValueOnce(new FakeApiError('not_found'));
listMock.mockResolvedValueOnce(response([]));
deleteAndConfirm();
await settle();
// Broadcast like a success, so other surfaces reconcile...
expect(announceMock).toHaveBeenCalledWith('ws-a', 'a1');
// ...told as information, not as a failure...
expect(toastMock).toHaveBeenCalledWith(expect.stringContaining('already deleted'), 'info');
expect(toastMock).not.toHaveBeenCalledWith(expect.anything(), 'error');
// ...and the stale row is gone from the list.
expect(rows()).toHaveLength(0);
});
it('ignores a superseded usage response and its error toast', async () => {
@@ -68,9 +68,20 @@
* callers, so existing usage is unaffected.
*/
flushBeforeRestore?: () => Promise<void>;
/**
* 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
@@ -120,7 +131,23 @@
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 could never ask again for the
// rest of the mount (PLAN-2392 DR-17). Note what this does and
// does not buy: clearing the mark makes the attachment eligible
// again on the NEXT run of the probe effect (a new comment, an
// edit, a remount), it does not schedule a retry of its own. A
// proactive retry belongs with the timeline's deletion
// subscription in phase 3c, not here. 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
@@ -471,6 +498,7 @@
<CommentEditor
{wsSlug}
{itemId}
{hostToken}
placeholder="Write a comment… (paste or drop an image to attach)"
submitLabel="Comment"
{submitting}
@@ -509,6 +537,7 @@
{canEdit}
{frozen}
{isAdmin}
{hostToken}
{attachmentResolver}
onDelete={handleDelete}
onReply={handleReply}
@@ -38,6 +38,14 @@
attachmentResolver?: AttachmentResolver;
/** True when the current user is a platform admin (can edit any comment). */
isAdmin?: boolean;
/**
* Identity of the `ItemDetail` mount that owns this card
* (PLAN-2392 DR-8 / TASK-2421). Forwarded to the edit / reply
* CommentEditors so their attachment chips address the ONE host that
* owns them — master and peeked panes share a module-global bus.
* Empty (the default) disables addressing.
*/
hostToken?: string;
onDelete: (commentId: string) => void;
onReply: (commentId: string, body: string) => void | Promise<void>;
/** 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 @@
<CommentEditor
{wsSlug}
itemId={comment.item_id}
{hostToken}
content={comment.body}
placeholder="Edit comment…"
submitLabel="Save"
@@ -272,6 +281,7 @@
<CommentEditor
{wsSlug}
itemId={comment.item_id}
{hostToken}
placeholder="Write a reply… (paste or drop an image to attach)"
submitLabel="Reply"
autofocus
@@ -327,6 +337,7 @@
<CommentEditor
{wsSlug}
itemId={comment.item_id}
{hostToken}
content={reply.body}
placeholder="Edit reply…"
submitLabel="Save"