diff --git a/web/src/app.css b/web/src/app.css index c422e7ce..8dd5ccb0 100644 --- a/web/src/app.css +++ b/web/src/app.css @@ -637,55 +637,6 @@ dialog.attachment-crop-modal::backdrop { filter: brightness(1.1); } -/* Lightbox dialog — appended to document.body when an attachment image - is clicked, so styles must be global rather than scoped to Editor.svelte. - Uses the native element; the ::backdrop pseudo handles the - overlay shade. The dialog content fills the viewport with a centered - image and a single close button. */ -dialog.attachment-image-lightbox { - border: none; - background: transparent; - padding: 0; - max-width: 100vw; - max-height: 100vh; - overflow: visible; - color: #fff; -} -dialog.attachment-image-lightbox::backdrop { - background: rgba(0, 0, 0, 0.85); - backdrop-filter: blur(2px); -} -dialog.attachment-image-lightbox .attachment-image-lightbox-img { - max-width: 95vw; - max-height: 95vh; - display: block; - margin: 0 auto; - box-shadow: 0 12px 40px rgba(0, 0, 0, 0.55); - cursor: zoom-out; - border-radius: var(--radius); -} -dialog.attachment-image-lightbox .attachment-image-lightbox-close { - position: fixed; - top: 16px; - right: 16px; - width: 40px; - height: 40px; - border-radius: 50%; - border: none; - background: rgba(0, 0, 0, 0.6); - color: #fff; - font-size: 28px; - line-height: 1; - cursor: pointer; - display: flex; - align-items: center; - justify-content: center; - z-index: 1; -} -dialog.attachment-image-lightbox .attachment-image-lightbox-close:hover { - background: rgba(0, 0, 0, 0.85); -} - /* Light mode */ [data-theme="light"] { --bg-primary: #f5f5f9; diff --git a/web/src/lib/attachments/events.test.ts b/web/src/lib/attachments/events.test.ts index b91f8926..b48f1420 100644 --- a/web/src/lib/attachments/events.test.ts +++ b/web/src/lib/attachments/events.test.ts @@ -10,6 +10,8 @@ import { type AttachmentPanelOpenEvent, type AttachmentViewerOpenEvent, type LightboxImage, + type ViewerOpenRequest, + type ViewerReadyImage, } from './events'; /** @@ -194,13 +196,28 @@ function image(over: Partial = {}): LightboxImage { }; } -function viewerEvent(over: Partial = {}): AttachmentViewerOpenEvent { +// Typed as the EMITTER's shape (`ViewerOpenRequest`), not the event's: the +// producer half of this channel must resolve the MIME before it asks for a +// viewer, and that is now a type, so the fixture has to satisfy it. The one +// case that deliberately violates it casts, and asserts the runtime guard. +/** + * The same member in the PRODUCER's shape. The cast is confined to this one + * helper: every field but `mime_type` is identical, and the default IS a + * resolved allowlisted type, so the assertion is true of everything it returns. + * A case that deliberately wants an unresolved or refused MIME calls `image()` + * and casts at ITS call site, where the violation is visible. + */ +function viewable(over: Partial = {}): ViewerReadyImage { + return image({ mime_type: 'image/png', ...over }) as ViewerReadyImage; +} + +function viewerEvent(over: Partial = {}): ViewerOpenRequest { return { attachmentId: 'att-1', workspaceSlug: 'ws-1', itemId: 'item-1', hostToken: 'host-a', - images: [image()], + images: [viewable()], index: 0, invoker: null, ...over, @@ -285,7 +302,7 @@ describe('viewer open channel', () => { // A stand-in element: this suite runs in the node environment, and the // bus only ever carries the reference — the host is where it is used. const invoker = { tagName: 'IMG' } as unknown as HTMLElement; - const images = [image(), image({ id: 'att-2', alt: 'second' })]; + const images = [viewable(), viewable({ id: 'att-2', alt: 'second' })]; let received: AttachmentViewerOpenEvent | null = null; const off = registerAttachmentViewerListener((e) => { if (isAttachmentViewerEventForHost(e, host)) received = e; @@ -314,9 +331,13 @@ describe('viewer open channel', () => { expect(got!.invoker).toBe(invoker); }); - it('passes incomplete image metadata through as nulls', () => { - // An inline image knows only what its NodeView options give it, and its - // HEAD probe may not have completed. The viewer opens anyway. + it('passes the CAPTION metadata through as nulls', () => { + // An inline image knows only what its NodeView options give it: there is + // no filename on the node's attrs and the HEAD probe carries no + // dimensions. Those are captions — absent is a fact about the producer, + // not a reason to refuse — so the viewer opens anyway. `mime_type` is + // deliberately NOT in this list; it is the gate, and the next test is + // about it. const host = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; let received: AttachmentViewerOpenEvent | null = null; const off = registerAttachmentViewerListener((e) => { @@ -327,9 +348,8 @@ describe('viewer open channel', () => { viewerEvent({ hostToken: host.hostToken, images: [ - image({ + viewable({ filename: null, - mime_type: null, size_bytes: null, width: null, height: null, @@ -346,6 +366,110 @@ describe('viewer open channel', () => { expect(got!.images[0].width).toBeNull(); }); + it('refuses a set whose MIME is unresolved or not allowlisted', () => { + // THE GATE, at the boundary rather than in each producer (TASK-2433). + // + // This test used to assert the opposite — that a null `mime_type` was + // passed through — on the reasoning that what is viewable is the + // emitter's judgement. TASK-2431 ended that: `Lightbox` FAILS CLOSED on + // an unresolved MIME, so an emission the viewer will filter to nothing + // is not a permissive channel, it is an image that does not open with + // nothing thrown and nothing logged. A producer that forgets is now + // refused here, where every producer passes. + // + // The casts are the point, not a workaround: `notifyViewerOpen` takes + // `ViewerOpenRequest`, so each of these is ALREADY a compile error at an + // honest call site. What is asserted here is the runtime half — the one + // that still holds for a JS caller, an `any`, or a value that arrived + // from outside the type system. + const seen: AttachmentViewerOpenEvent[] = []; + const off = registerAttachmentViewerListener((e) => seen.push(e)); + try { + // Unresolved: the probe never answered, or the producer never asked. + notifyViewerOpen(viewerEvent({ images: [image({ mime_type: null }) as ViewerReadyImage] })); + // Resolved, and refused: `image/svg+xml` can carry active content + // (DR-16). `image/*` is not sufficient reason to display something. + notifyViewerOpen( + viewerEvent({ images: [image({ mime_type: 'image/svg+xml' }) as ViewerReadyImage] }) + ); + notifyViewerOpen( + viewerEvent({ images: [image({ mime_type: 'application/pdf' }) as ViewerReadyImage] }) + ); + // ONE bad entry poisons the whole emission rather than being filtered + // out of it: `index` and `attachmentId` name a position in the set the + // PRODUCER built, and silently renumbering it would open the viewer on + // a different image than the one the user activated. + notifyViewerOpen( + viewerEvent({ + images: [viewable(), image({ id: 'att-2', mime_type: null }) as ViewerReadyImage], + }) + ); + } finally { + off(); + } + expect(seen).toHaveLength(0); + }); + + it('refuses a malformed set instead of throwing out of a notify call', () => { + // A boundary's input is only as good as its caller, and a THROW here is + // worse than a drop: it unwinds out of a notify function into whatever + // the producer was doing — for the inline image NodeView, into the + // `.then` of its MIME resolution, as an unhandled rejection. + // + // The sparse case is the one a type cannot catch and `.some` silently + // permits: holes are SKIPPED by `.some`, so a hole-only array passes an + // every-entry check that is written with it and arrives at a viewer with + // nothing to show. + const seen: AttachmentViewerOpenEvent[] = []; + const off = registerAttachmentViewerListener((e) => seen.push(e)); + const bad = (images: unknown) => + notifyViewerOpen({ ...viewerEvent(), images } as unknown as ViewerOpenRequest); + try { + expect(() => bad({ length: 1 })).not.toThrow(); + expect(() => bad(new Array(1))).not.toThrow(); + expect(() => bad([undefined])).not.toThrow(); + expect(() => bad([{ id: 'att-1', alt: '', mime_type: 42 }])).not.toThrow(); + expect(() => bad('image/png')).not.toThrow(); + expect(() => bad(null)).not.toThrow(); + } finally { + off(); + } + expect(seen).toHaveLength(0); + }); + + it('delivers a FROZEN set unchanged — readonly is the contract, not a rejection', () => { + // The set is `readonly` on the event because the viewer must not reorder + // or mutate something the emitter still owns, so a producer freezing its + // own array is the contract being honoured, not a malformed input. + const seen: AttachmentViewerOpenEvent[] = []; + const off = registerAttachmentViewerListener((e) => seen.push(e)); + const images = Object.freeze([viewable()]); + try { + notifyViewerOpen(viewerEvent({ images })); + } finally { + off(); + } + expect(seen).toHaveLength(1); + expect(seen[0].images).toBe(images); + }); + + it('still delivers a set whose every entry is allowlisted', () => { + // The control. A gate that refused everything would satisfy the test + // above and close the channel. + const seen: AttachmentViewerOpenEvent[] = []; + const off = registerAttachmentViewerListener((e) => seen.push(e)); + try { + notifyViewerOpen( + viewerEvent({ + images: [viewable(), viewable({ id: 'att-2', mime_type: 'image/webp' })], + }) + ); + } finally { + off(); + } + expect(seen).toHaveLength(1); + }); + it('drops an unaddressable or empty emission rather than broadcasting it', () => { const seen: AttachmentViewerOpenEvent[] = []; const off = registerAttachmentViewerListener((e) => seen.push(e)); diff --git a/web/src/lib/attachments/events.ts b/web/src/lib/attachments/events.ts index 8d520cc0..ea48fd36 100644 --- a/web/src/lib/attachments/events.ts +++ b/web/src/lib/attachments/events.ts @@ -22,6 +22,7 @@ import { invalidateAttachmentMetadata } from '$lib/components/editor/attachment-metadata'; import type { AttachmentUploadResult } from '$lib/types'; import { isAddressable } from '$lib/attachments/hostAddress'; +import { canOpenInViewer } from '$lib/attachments/display'; const listeners = new Set<(uuid: string) => void>(); @@ -338,6 +339,31 @@ export interface AttachmentViewerOpenEvent { invoker: HTMLElement | null; } +/** + * An image a producer has RESOLVED as viewable — same shape as `LightboxImage` + * with the one field that is a GATE rather than a caption made non-nullable. + * + * Two types rather than one, deliberately, because the two directions have + * genuinely different obligations. A PRODUCER must know the MIME before it + * asks for a viewer (TASK-2433), so `notifyViewerOpen` takes this and a + * `mime_type: null` emission is a compile error at the call site rather than a + * silent no-op at runtime. A CONSUMER must keep accepting the nullable shape: + * `Lightbox` is mounted directly by the strip and the timeline as well, its + * records are live and can lose their MIME, and its own filter is what covers + * a row that turns unsafe after the viewer is already open. + * + * The other three nullable fields stay nullable in both directions — phase 3b's + * loading policy wants `width` / `height` when a producer has them and must + * still work when it does not, and no producer has a filename for an inline + * body image at all. + */ +export type ViewerReadyImage = Omit & { mime_type: string }; + +/** What `notifyViewerOpen` accepts: the event, with the set already resolved. */ +export interface ViewerOpenRequest extends Omit { + images: readonly ViewerReadyImage[]; +} + const viewerListeners = new Set<(event: AttachmentViewerOpenEvent) => void>(); /** @@ -391,19 +417,52 @@ export function registerAttachmentViewerListener( * and the host deliberately does not substitute its own); and an empty set * would open a full-screen viewer showing nothing. * + * IT ALSO POLICES THE MIME (TASK-2433). This channel used to leave that to the + * emitter, on the reasoning that what is viewable is a judgement about the + * surface it is emitted from. That reasoning stopped holding when TASK-2431 + * made `Lightbox` FAIL CLOSED on an unresolved MIME: a set the viewer will + * filter to nothing does not produce an error, it produces an image that does + * not open, with nothing thrown and nothing logged. "Resolve the MIME before + * you emit" was then a convention, and a convention the next producer breaks + * silently is not an invariant — so it is enforced here, where every producer + * passes. + * + * THE WHOLE EMISSION IS DROPPED, not the offending entry. Filtering the set + * would desynchronize it from `index` and `attachmentId` — the event's own + * stated invariant is `images[index]?.id === attachmentId`, and a bus that + * quietly renumbered a producer's set would open the viewer on a different + * image than the one that was activated. `Lightbox`'s own `$derived` filter + * stays as the second line: it re-applies the gate over the live records, so a + * row that becomes unsafe AFTER the viewer opened is still dropped. + * * What it deliberately does NOT police: the `index` (the viewer clamps, and * dropping the whole emission over an off-by-one would be a silent no-op where - * showing the neighbouring image is harmless), and the MIME types in the set — - * what is viewable is the EMITTER's judgement, made against the surface it is - * emitting from, not a rule this channel can state for every future producer. + * showing the neighbouring image is harmless). * * (Named per TASK-2428's normative signature — `notifyViewerOpen`, without the * `Attachment` infix the sibling emitters carry. The task spells the exported * surface out explicitly, so it wins over the local naming rhyme.) */ -export function notifyViewerOpen(event: AttachmentViewerOpenEvent): void { +export function notifyViewerOpen(event: ViewerOpenRequest): void { if (!event?.attachmentId || !event.itemId || !event.hostToken) return; if (!event.workspaceSlug) return; - if (!event.images?.length) return; + // `Array.isArray` rather than a truthy `length`, and an INDEXED loop rather + // than `.some`, because this is a boundary and its input is only as good as + // the caller. An array-like `{length: 1}` would make `.some` throw — out of + // a notify function, into a producer's `.then`, as an unhandled rejection — + // and a SPARSE array's holes are skipped by `.some` entirely, so `new + // Array(1)` would sail through the gate and reach a viewer with nothing in + // it. A type is not a runtime guarantee for a shared module. + if (!Array.isArray(event.images) || event.images.length === 0) return; + for (let i = 0; i < event.images.length; i++) { + const img = event.images[i]; + // POSITIVELY allowlisted, every entry. `canOpenInViewer` answers false + // for null and undefined alike, so "unresolved" and "resolved to + // something we will not display" are refused by the same call — which is + // the point: to the user they are the same non-event, and only one of + // them was ever spelled out in a producer's comments. + if (!img || typeof img.mime_type !== 'string') return; + if (!canOpenInViewer(img.mime_type)) return; + } for (const fn of viewerListeners) fn(event); } diff --git a/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts b/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts index 04dfb779..e22909ce 100644 --- a/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts +++ b/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts @@ -40,13 +40,30 @@ vi.mock('$lib/attachments/events', async (importOriginal) => { const { notifyViewerOpen } = await import('$lib/attachments/events'); type ViewerEvent = import('$lib/attachments/events').AttachmentViewerOpenEvent; +// What `notifyViewerOpen` ACCEPTS, which is narrower than what it delivers: a +// producer must have resolved the MIME (TASK-2433), so the emitter's set is +// `ViewerReadyImage` while the host still consumes the nullable event shape. +// The fixture below is an emitter, so it is typed as the request. +// +// This file is `*.svelte.test.ts`, which `tsconfig.json` EXCLUDES, so +// `npm run check` would not have caught the mismatch — a reason to keep the +// annotation honest by hand rather than a reason it does not matter. +type ViewerRequest = import('$lib/attachments/events').ViewerOpenRequest; +type ViewerReadyImage = import('$lib/attachments/events').ViewerReadyImage; type LightboxImage = import('$lib/attachments/events').LightboxImage; const { default: AttachmentViewerHost } = await import('./AttachmentViewerHost.svelte'); const ATT_ID = '11111111-2222-4333-8444-555555555555'; const ATT_ID_2 = '99999999-8888-4777-8666-555555555555'; -function image(over: Partial = {}): LightboxImage { +/** + * A member of an emitted set, in the PRODUCER's shape: `notifyViewerOpen` + * requires a resolved MIME (TASK-2433). The cast is confined here and the + * default IS a resolved allowlisted type, so the assertion holds for + * everything this returns; overrides stay in the nullable shape so a case that + * wants an unresolved one is written — and read — as a deliberate exception. + */ +function image(over: Partial = {}): ViewerReadyImage { return { id: ATT_ID, alt: 'a diagram', @@ -56,10 +73,10 @@ function image(over: Partial = {}): LightboxImage { width: 800, height: 600, ...over, - }; + } as ViewerReadyImage; } -function openEvent(over: Partial = {}): ViewerEvent { +function openEvent(over: Partial = {}): ViewerRequest { return { attachmentId: ATT_ID, workspaceSlug: 'ws', diff --git a/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts b/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts index 9ff42129..aa55d091 100644 --- a/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts +++ b/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts @@ -21,11 +21,21 @@ const { notifyViewerOpen } = await import('$lib/attachments/events'); const { lightboxStubCalls } = await import('./fixtures/lightboxStub'); const { default: AttachmentViewerHost } = await import('./AttachmentViewerHost.svelte'); type ViewerEvent = import('$lib/attachments/events').AttachmentViewerOpenEvent; +// What `notifyViewerOpen` ACCEPTS, which is narrower than what it delivers: a +// producer must have resolved the MIME (TASK-2433), so the emitter's set is +// `ViewerReadyImage` while the host still consumes the nullable event shape. +// The fixture below is an emitter, so it is typed as the request. +// +// This file is `*.svelte.test.ts`, which `tsconfig.json` EXCLUDES, so +// `npm run check` would not have caught the mismatch — a reason to keep the +// annotation honest by hand rather than a reason it does not matter. +type ViewerRequest = import('$lib/attachments/events').ViewerOpenRequest; +type ViewerReadyImage = import('$lib/attachments/events').ViewerReadyImage; const ATT_ID = '11111111-2222-4333-8444-555555555555'; const ATT_ID_2 = '99999999-8888-4777-8666-555555555555'; -function openEvent(over: Partial = {}): ViewerEvent { +function openEvent(over: Partial = {}): ViewerRequest { return { attachmentId: ATT_ID, workspaceSlug: 'ws', diff --git a/web/src/lib/components/editor/attachment-image.ts b/web/src/lib/components/editor/attachment-image.ts index cd6a4530..9ae90c67 100644 --- a/web/src/lib/components/editor/attachment-image.ts +++ b/web/src/lib/components/editor/attachment-image.ts @@ -38,7 +38,7 @@ import { mimeToFormat } from './attachment-metadata'; import { openCropModal, type CropResult } from './attachment-crop-modal'; -import { registerAttachmentDeletionListener } from '$lib/attachments/events'; +import { notifyViewerOpen, registerAttachmentDeletionListener } from '$lib/attachments/events'; import { type AttachmentHostAddressReader, readUnaddressed @@ -304,6 +304,19 @@ export const AttachmentImage = Node.create({ // without a temporal-dead-zone hazard. let knownMime: string | null = null; + // True while an activation's MIME resolution is in flight. See + // activate() for why one is enough — and why "exactly once" now needs + // a latch rather than only the key-repeat guard. + // + // The counter is what makes RELEASING it safe. The latch is dropped in + // two places — the resolution's own finalizer, and a uuid swap, which + // must not leave the NEW image latched behind the old one's request — + // and an unconditional finalizer would let a superseded activation + // release a latch that a LATER one is holding. Each activation stamps + // its own number and releases only if it is still the current holder. + let activating = false; + let activationSeq = 0; + /** * DR-12: the inline body image is an activation target, so it carries a * button's semantics — but ONLY while it is actually one. @@ -536,8 +549,9 @@ export const AttachmentImage = Node.create({ * property of the MOUSE rather than of activation: a keyboard path that * opened on its own would have bypassed it entirely. One function owns * the gate so there is exactly one place a route can be added to and - * exactly one place the gate can be strengthened (the viewer bus and its - * fence land on this same seam in the tasks that follow). + * exactly one place the gate can be strengthened — which is what + * TASK-2433 then did on this same seam: the viewer bus, the MIME + * resolution and the address fence all live inside this one function. * * The gate itself is `canActivate()`, shared with the semantics pass so * an image can never be openable and un-announced, or announced and @@ -548,16 +562,135 @@ export const AttachmentImage = Node.create({ * 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 activation 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. + * THE MIME IS RESOLVED BEFORE ANYTHING IS EMITTED (TASK-2433, revising + * the decomposition's "keep the positively-known gate"). `Lightbox` + * FAILS CLOSED on an unresolved MIME as of TASK-2431 — it filters the + * set it was handed and admits only allowlisted entries — so an event + * carrying `mime_type: null` is not "let the viewer decide", it is a + * viewer that mounts and renders no image. The producer's half of that + * contract is this function: either the cache answers (the common + * case — the toolbar probe and the strip both warm the same entry) or + * we await one HEAD, and we emit ONLY on a positively-known + * allowlisted answer. + * + * The channel enforces the same rule at the boundary as of TASK-2433 — + * `notifyViewerOpen` takes a set whose `mime_type` is non-nullable and + * refuses one that is not allowlisted — so this is no longer the only + * thing between a forgetful producer and an image that quietly does not + * open. It is still where the answer is OBTAINED, and the payload needs + * it either way. + * + * What that COSTS, deliberately and temporarily: an image whose probe + * comes back `transient` (or whose surface has no workspace to probe + * with) keeps its button semantics and does not open. That is a dead + * focus stop, and it is TASK-2434's — the four-branch matrix + * (ok → viewer, unsafe → panel redirect, missing → inert placeholder, + * transient → retryable) is what makes this gate TOTAL. This task is + * the surface swap, and the swap must not be the thing that reopens + * the hole TASK-2431 closed. + * + * It also closes a mid-phase bypass Codex found: the old gate read + * `knownMime` only when truthy, so a click landing before the lazy + * probe resolved opened the original file in the legacy dialog, and a + * later `unsafe` answer did not close it. */ function activate(): void { if (!canActivate()) return; - const fullUrl = opts.getDownloadUrl(currentUuid, 'original'); - openImageLightbox(fullUrl, currentAlt); + // One activation at a time. The MIME resolution is asynchronous + // even on a cache hit (a settled promise still resolves on a + // microtask), so two gestures inside one tick would otherwise both + // clear the gate and emit — and "fires exactly once" has to survive + // the await that was just introduced, not only the key repeat. + if (activating) return; + const forUuid = currentUuid; + // The address the GESTURE happened at. The workspace half is what + // keys the metadata cache and what the viewer reads every image URL + // from, so probing under one workspace and emitting under another + // would serve ws1's click from ws2's endpoint. Snapshot once, then + // re-check at emit (below) rather than re-reading and trusting it. + const from = opts.address(); + // No workspace ⇒ no probe ⇒ nothing can be positively known. An + // SSR/preview surface simply does not open a viewer. + if (!from.workspaceSlug) return; + activating = true; + const seq = ++activationSeq; + void fetchAttachmentMetadata(from.workspaceSlug, forUuid, opts.getDownloadUrl) + .then((result) => { + // Everything the gate asserted at gesture time has to still + // hold at emit time: the NodeView can be torn down, the node + // can be pointed at a different attachment (rotate/crop, a + // peer's op), and the deletion broadcast can land — all + // inside the await window. + if (destroyed || currentUuid !== forUuid) return; + // And this must still be the CURRENT activation. Comparing + // the uuid is not enough on its own: the node can be pointed + // away and back again (a rotate the user undoes, a peer's op + // reverted), and then a request from before the round trip + // finds its own uuid in place and emits for a gesture two + // swaps ago. + if (activationSeq !== seq) return; + if (!canActivate()) return; + // `transient` and `missing` are both "not positively known". + if (result.status !== 'ok') return; + // DR-16, restated on the resolved answer rather than on the + // absence of one: `image/svg+xml` can carry active content. + if (!canOpenInViewer(result.mime)) return; + // The host may have MOVED while the HEAD was in flight — the + // comment composer is deliberately reused across an item + // switch (see hostAddress.ts), so its address is live. The + // gesture belonged to the old address: emitting there opens a + // viewer over a pane the user has left, and emitting at the + // new one attributes the gesture to a different item. Neither + // is what the user did, so drop it. + const to = opts.address(); + if ( + to.workspaceSlug !== from.workspaceSlug || + to.itemId !== from.itemId || + to.hostToken !== from.hostToken + ) { + return; + } + notifyViewerOpen({ + attachmentId: forUuid, + workspaceSlug: from.workspaceSlug, + itemId: from.itemId, + hostToken: from.hostToken, + // A single-image set: this NodeView knows about ITS node + // and nothing else. The body's other images are a set the + // editor could offer, but assembling one here would make + // ←/→ page through a list this surface does not own. + images: [ + { + id: forUuid, + alt: currentAlt, + // The node's attrs carry no filename and the HEAD + // metadata does not either — the same absence + // `applyImageSemantics` names its fallback for. + filename: null, + mime_type: result.mime, + size_bytes: result.size, + // 3b's pixel-based loading policy wants these; the + // HEAD probe has no source for them today. + width: null, + height: null, + }, + ], + index: 0, + // Where the viewer aims focus on close. The is a real + // focus stop as of TASK-2432, so this is a stable target + // rather than whatever happened to hold focus at open — + // though `Lightbox` still falls back to the body if the + // element has since become unfocusable or detached. + invoker: img, + }); + }) + .finally(() => { + // Only if this activation is still the one holding the latch. + // A uuid swap bumps the counter, so a stale resolution + // landing afterwards cannot unlock the request that + // replaced it. + if (activationSeq === seq) activating = false; + }); } img.addEventListener('click', (event) => { @@ -851,6 +984,20 @@ export const AttachmentImage = Node.create({ // The old uuid's state — whether a 404 placeholder or a // confirmed deletion — says nothing about the new one. deleted = false; + // Nor does an activation still resolving for it. That + // request is already fenced (its continuation compares + // `currentUuid`), so the latch is guarding nothing but the + // NEW image — and this NodeView deliberately outlives the + // swap, so a HEAD that never settles would leave the image + // in front of the user permanently unopenable. + // + // The bump is what retires any request still resolving for + // the old attachment: its continuation checks the generation + // before emitting, so a swap AWAY AND BACK cannot let it + // find its own uuid in place and open a viewer for a gesture + // the user made two swaps ago. + activationSeq += 1; + activating = false; resetMissing(); if (newUuid) { loadImage(opts.getDownloadUrl(newUuid, 'thumb-md')); @@ -1103,38 +1250,22 @@ function refreshToolbarState( }); } -/** - * Open a centered showing the full-resolution attachment. - * Closes on backdrop click, the close button, or the Esc key. +/* + * THERE IS NO LIGHTBOX IN THIS FILE, AND THERE MUST NOT BE ONE AGAIN. + * + * `openImageLightbox` / `closeLightbox` used to live here: a hand-rolled + * `` this NodeView appended to `document.body` and `showModal()`d. + * TASK-2433 deleted them. Inline images now emit on the viewer channel + * (`notifyViewerOpen`, in activate() above) and the `AttachmentViewerHost` that + * `ItemDetail` owns mounts the `Lightbox` for THIS route — which is where the + * modal contract lives: the lease-stacked backdrop, the focus trap and restore, + * the Escape ordering, and the DR-16 filter re-applied over the whole set. (The + * strip and the timeline still mount `Lightbox` themselves, by decision rather + * than omission — see the channel's own note in `$lib/attachments/events`.) + * + * A NodeView cannot mount a Svelte component, which is the entire reason the + * bus exists. Re-adding an imperative overlay here would not be a shortcut, it + * would be a second viewer with none of that contract — so + * `attachmentImageNoOverlay.test.ts` asserts, statically, that this file + * creates no dialog and calls no `showModal`. */ -function openImageLightbox(fullUrl: string, alt: string): void { - if (typeof document === 'undefined') return; - const dialog = document.createElement('dialog'); - dialog.className = 'attachment-image-lightbox'; - - const closeBtn = document.createElement('button'); - closeBtn.type = 'button'; - closeBtn.className = 'attachment-image-lightbox-close'; - closeBtn.setAttribute('aria-label', 'Close image preview'); - closeBtn.textContent = '×'; - closeBtn.addEventListener('click', () => closeLightbox(dialog)); - - const img = document.createElement('img'); - img.className = 'attachment-image-lightbox-img'; - img.src = fullUrl; - if (alt) img.alt = alt; - // Prevent clicks on the image itself from bubbling to the backdrop - // handler below (which closes the dialog). - img.addEventListener('click', (event) => event.stopPropagation()); - - dialog.append(closeBtn, img); - dialog.addEventListener('click', () => closeLightbox(dialog)); - dialog.addEventListener('close', () => dialog.remove()); - - document.body.appendChild(dialog); - dialog.showModal(); -} - -function closeLightbox(dialog: HTMLDialogElement): void { - if (dialog.open) dialog.close(); -} diff --git a/web/src/lib/components/editor/attachmentImageKeyboard.svelte.test.ts b/web/src/lib/components/editor/attachmentImageKeyboard.svelte.test.ts index 6bd1f64f..f4425e9a 100644 --- a/web/src/lib/components/editor/attachmentImageKeyboard.svelte.test.ts +++ b/web/src/lib/components/editor/attachmentImageKeyboard.svelte.test.ts @@ -6,16 +6,28 @@ // no name. This spec pins the button contract that closes that, and — more // importantly — pins the two ways the contract is easy to get WRONG: // -// - activation firing TWICE. A keyed remount or a fast dialog can hide a -// duplicate visually, so every activation assertion here is a COUNT -// (`dialog.attachment-image-lightbox` elements in the document), never a -// truthiness check. `toBe(1)` fails on 2; `not.toBeNull()` does not. +// - activation firing TWICE. A duplicate is easy to hide visually, so every +// activation assertion here is a COUNT of open-viewer REQUESTS on the bus, +// never a truthiness check. `toBe(1)` fails on 2; `not.toBeNull()` does not. +// Counting requests rather than DOM is also what survived TASK-2433: the +// NodeView no longer opens anything itself, so a spec that counted overlays +// would now assert 0 forever and pass against an implementation that emits +// nothing at all. // // - the KEYBOARD path bypassing the MIME gate. The gate used to live inside // the click handler, which made it a property of the mouse. Both routes now // call one `activate()`, and the refusal is asserted through the KEYBOARD, // which is the assertion that would have caught a second emitter. // +// THIS FILE IS THE PRODUCER LAYER, and its counts are counts of REQUESTS on the +// bus, not of viewers on screen: `notifyViewerOpen` is mocked, so "opens once" +// here means "asks once". That is the right layer for the keyboard contract — +// which is about how many times activation fires, and under which gestures — +// but it is deliberately blind to whether anything is listening. The end of the +// route (real bus → real `AttachmentViewerHost` → real `Lightbox`, and a real +// viewer in the document) is pinned next door in +// `attachmentImageViewerHost.svelte.test.ts`. +// // Driven through a REAL Tiptap editor, like the placeholder spec next door: the // semantics live on imperative NodeView DOM and a hand-built element would pin // nothing about the code under test. @@ -25,18 +37,38 @@ import StarterKit from '@tiptap/starter-kit'; import { isEditorOwnedImage } from '$lib/attachments/editorOwnedImage'; const deletionListeners = new Set<(uuid: string) => void>(); +// Open-viewer requests, captured RAW — before the channel's addressability +// filter, so what is asserted is what THIS NodeView produced. +const emitted: Array> = []; vi.mock('$lib/attachments/events', () => ({ notifyAttachmentPanelOpen: () => {}, + notifyViewerOpen: (event: Record) => { + emitted.push(event); + }, registerAttachmentDeletionListener: (fn: (uuid: string) => void) => { deletionListeners.add(fn); return () => deletionListeners.delete(fn); }, })); -const probeMock = vi.fn(async () => ({ status: 'transient' as const })); +// The probe's full result union, spelled out. Without it `vi.fn` infers the +// type of the DEFAULT implementation alone, and every `mockResolvedValue` for a +// different arm is a type error — invisible under `npm run check`, which +// excludes `*.svelte.test.ts`, and a trap for whoever widens that exclude. +type ProbeResult = + | { status: 'ok'; mime: string; size: number } + | { status: 'missing' } + | { status: 'transient' }; + +// Args are passed through so a test can answer PER ATTACHMENT — the uuid-swap +// case below needs one image's probe to still be in flight while another's +// answers. +const probeMock = vi.fn<(ws?: string, uuid?: string) => Promise>(async () => ({ + status: 'transient', +})); vi.mock('./attachment-metadata', () => ({ - fetchAttachmentMetadata: () => probeMock(), - revalidateAttachmentMetadata: () => probeMock(), + fetchAttachmentMetadata: (ws: string, uuid: string) => probeMock(ws, uuid), + revalidateAttachmentMetadata: (ws: string, uuid: string) => probeMock(ws, uuid), invalidateAttachmentMetadata: () => {}, mimeToFormat: () => null, })); @@ -45,6 +77,14 @@ const { AttachmentImage } = await import('./attachment-image'); const BODY_CONTENT = '

A diagram

'; +/** + * The address every editor below reads through. MUTABLE, because the real + * reader is: `CommentEditor` is deliberately reused across an item switch and + * its address changes under a mounted NodeView (see hostAddress.ts). Activation + * now spans an await, so that switch can land mid-flight. + */ +let address = { workspaceSlug: 'ws', itemId: 'item-A', hostToken: 'apanel-1' }; + /** The item-body configuration (Editor.svelte): transforms enabled. */ function makeEditor(element: HTMLElement, content: string = BODY_CONTENT): Editor { return new Editor({ @@ -55,7 +95,7 @@ function makeEditor(element: HTMLElement, content: string = BODY_CONTENT): Edito workspaceSlug: 'ws', getDownloadUrl: (uuid: string, variant?: string) => `/api/v1/workspaces/ws/attachments/${uuid}?variant=${variant ?? 'thumb-md'}`, - address: () => ({ workspaceSlug: 'ws', itemId: 'item-A', hostToken: 'apanel-1' }), + address: () => address, supportedFormats: ['png'], transform: async () => { throw new Error('not used'); @@ -81,7 +121,7 @@ function makeCommentEditor(element: HTMLElement): Editor { AttachmentImage.configure({ getDownloadUrl: (uuid: string) => `/api/v1/workspaces/ws/attachments/${uuid}`, workspaceSlug: 'ws', - address: () => ({ workspaceSlug: 'ws', itemId: 'item-A', hostToken: 'apanel-1' }), + address: () => address, supportedFormats: [] as string[], transform: async () => { throw new Error('Image transforms are not available in comments.'); @@ -93,9 +133,25 @@ function makeCommentEditor(element: HTMLElement): Editor { }); } -/** How many times activation actually fired. One dialog per open. */ +/** How many times activation actually fired. One request per open. */ function openCount(): number { - return document.querySelectorAll('dialog.attachment-image-lightbox').length; + return emitted.length; +} + +/** + * Activation resolves the image's MIME BEFORE emitting (TASK-2433), so it is + * asynchronous even on a cache hit. Every post-gesture count goes through here; + * a synchronous read would be 0 regardless of what the implementation did. + */ +async function opened(): Promise { + // TWO turns of the macrotask queue, not one. A count read too early is + // conservative for the "opens once" cases (an emission that had not + // happened yet reads as 0 and FAILS the assertion) but not for the "opens + // nothing" ones, where an implementation that emitted one tick later would + // pass. The second await closes that. + await new Promise((resolve) => setTimeout(resolve, 0)); + await new Promise((resolve) => setTimeout(resolve, 0)); + return emitted.length; } /** @@ -126,9 +182,13 @@ function timelineDelegation( // not the only surface that could ever delegate over one. if (opts.ownership && isEditorOwnedImage(img)) return; onFire(); - const dialog = document.createElement('dialog'); - dialog.className = 'attachment-image-lightbox'; - document.body.appendChild(dialog); + // A stand-in for the timeline's OWN viewer, deliberately a different + // element from anything the NodeView produces: these tests are about + // two viewers from one gesture, which is only observable if the two + // are distinguishable. + const stub = document.createElement('div'); + stub.className = 'timeline-viewer-stub'; + document.body.appendChild(stub); }; } @@ -139,8 +199,16 @@ describe('inline body image — keyboard activation (DR-12)', () => { beforeEach(() => { deletionListeners.clear(); + address = { workspaceSlug: 'ws', itemId: 'item-A', hostToken: 'apanel-1' }; probeMock.mockClear(); - probeMock.mockResolvedValue({ status: 'transient' as const }); + // The ORDINARY case is an image whose MIME resolves to an allowlisted + // raster type, and as of TASK-2433 that is a PRECONDITION of opening at + // all: activation resolves the MIME first and emits only on a positive + // answer, because `Lightbox` fails closed on an unresolved one. A + // `transient` default would make every "opens exactly once" test below + // assert 0 for a reason that has nothing to do with the keyboard. + probeMock.mockResolvedValue({ status: 'ok' as const, mime: 'image/png', size: 4096 }); + emitted.length = 0; host = document.body.appendChild(document.createElement('div')); target = host.appendChild(document.createElement('div')); }); @@ -149,7 +217,8 @@ describe('inline body image — keyboard activation (DR-12)', () => { editor?.destroy(); editor = undefined; host.remove(); - document.querySelectorAll('dialog.attachment-image-lightbox').forEach((d) => d.remove()); + emitted.length = 0; + document.querySelectorAll('.timeline-viewer-stub').forEach((d) => d.remove()); }); function image(): HTMLImageElement { @@ -200,7 +269,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { expect(image().getAttribute('aria-label')).toBe('View attachment image'); }); - it('opens on Enter, exactly once', () => { + it('opens on Enter, exactly once', async () => { editor = makeEditor(target); expect(openCount()).toBe(0); @@ -209,22 +278,60 @@ describe('inline body image — keyboard activation (DR-12)', () => { // A COUNT, not a truthiness check: a second emitter (a keyboard path that // activated on its own AND a synthesized click) shows up here as 2 and is // invisible to `not.toBeNull()`. - expect(openCount()).toBe(1); + expect(await opened()).toBe(1); }); - it('opens on Space exactly once and suppresses the page scroll', () => { + it('emits the viewer request the deleted dialog used to open', async () => { + // TASK-2433 deleted this NodeView's hand-rolled ``. Every other + // test in this file counts activations, and a count is satisfied by an + // emission of any shape — so exactly one test has to pin the CONTENT, or + // "the dialog is gone" would be provable by an implementation that + // replaced it with nothing. + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + await opened(); + + expect(emitted).toEqual([ + { + attachmentId: 'uuid-1', + workspaceSlug: 'ws', + itemId: 'item-A', + hostToken: 'apanel-1', + images: [ + { + id: 'uuid-1', + alt: 'A diagram', + filename: null, + mime_type: 'image/png', + size_bytes: 4096, + width: null, + height: null, + }, + ], + index: 0, + // The focus-restore target. The is a real focus stop as of + // TASK-2432, so the keyboard path has a stable one to offer — + // which is the whole reason `invoker` is on the event. + invoker: img, + }, + ]); + }); + + it('opens on Space exactly once and suppresses the page scroll', async () => { editor = makeEditor(target); expect(openCount()).toBe(0); const ev = press(image(), ' '); - expect(openCount()).toBe(1); + expect(await opened()).toBe(1); // Unhandled Space scrolls the document — and inside a contenteditable it // would also be taken as text input against the selected atom. expect(ev.defaultPrevented).toBe(true); }); - it('accepts the legacy "Spacebar" key name too', () => { + it('accepts the legacy "Spacebar" key name too', async () => { // Same alias the file chip next door accepts — older engines report Space // under this name, and an image that ignored it would be inert there. editor = makeEditor(target); @@ -232,22 +339,22 @@ describe('inline body image — keyboard activation (DR-12)', () => { const ev = press(image(), 'Spacebar'); - expect(openCount()).toBe(1); + expect(await opened()).toBe(1); expect(ev.defaultPrevented).toBe(true); }); - it('opens on a mouse click, exactly once', () => { + it('opens on a mouse click, exactly once', async () => { editor = makeEditor(target); expect(openCount()).toBe(0); click(image()); - expect(openCount()).toBe(1); + expect(await opened()).toBe(1); }); // Both activation keys, because the propagation stop is per-key: a handler // that stopped for Enter and not Space would pass an Enter-only test and // still double-open on the key most people press. for (const key of ['Enter', ' ']) { - it(`does not double-open when ${key === ' ' ? 'Space' : key} lands inside a delegated container`, () => { + it(`does not double-open when ${key === ' ' ? 'Space' : key} lands inside a delegated container`, async () => { // ItemTimeline delegates thumbnail click/keydown across its whole entry // list, and that list CONTAINS live CommentEditor instances whose bodies // render this NodeView — an `img[data-attachment-id]`, which is exactly @@ -269,7 +376,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { press(image(), key); expect(delegated).toBe(0); - expect(openCount()).toBe(1); + expect(await opened()).toBe(1); }); } @@ -283,7 +390,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { ['Shift+Enter', { shiftKey: true }], ['Alt+Space', { altKey: true }], ] as const) { - it(`treats ${label} as a shortcut, not an activation`, () => { + it(`treats ${label} as a shortcut, not an activation`, async () => { let seen = 0; let timelineOpened = 0; host.addEventListener('keydown', () => (seen += 1)); @@ -305,7 +412,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { // instead of the handler under test. const solo = new KeyboardEvent('keydown', { key, cancelable: true, ...mods }); image().dispatchEvent(solo); - expect(openCount()).toBe(0); + expect(await opened()).toBe(0); expect(solo.defaultPrevented).toBe(false); // Then bubbling, for the other half: the event must still REACH the @@ -314,12 +421,12 @@ describe('inline body image — keyboard activation (DR-12)', () => { // just as thoroughly as one that opened a viewer. press(image(), key, mods); expect(seen).toBe(1); - expect(openCount()).toBe(0); + expect(await opened()).toBe(0); expect(timelineOpened).toBe(0); }); } - it('opens once for a HELD key, not once per repeat', () => { + it('opens once for a HELD key, not once per repeat', async () => { // Every repeat is another keydown. Without a guard, leaning on Enter // stacks a viewer per repeat — "exactly once" has to mean once per // gesture. A truthiness check would not see this at all. @@ -334,14 +441,14 @@ describe('inline body image — keyboard activation (DR-12)', () => { press(img, 'Enter'); for (let i = 0; i < 4; i++) press(img, 'Enter', { repeat: true }); - expect(openCount()).toBe(1); + expect(await opened()).toBe(1); // And the repeats stay suppressed: they belong to the activation the // user made once, so letting them escape would hand the surrounding // surface four keypresses that never happened. expect(delegated).toBe(0); }); - it('still reaches the delegated container for keys it does not handle', () => { + it('still reaches the delegated container for keys it does not handle', async () => { // The propagation stop is scoped to the activation keys; swallowing // everything would break Escape, Tab-adjacent handlers and the editor's // own key handling on the surrounding surface. @@ -355,19 +462,22 @@ describe('inline body image — keyboard activation (DR-12)', () => { // Nor is it swallowed: an Escape the node consumed would never reach the // surface that closes a pane or a dialog. expect(ev.defaultPrevented).toBe(false); - expect(openCount()).toBe(0); + expect(await opened()).toBe(0); }); it('refuses a probed non-raster type through the KEYBOARD, not just the mouse', async () => { // The gate used to live inside the click handler. A keyboard path that // emitted on its own would have sailed straight past it, so the refusal // is asserted on the route that would have bypassed it. - probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml' }); + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml' , size: 4096 }); editor = makeEditor(target); - // BEFORE the probe answers, this is an ordinary activatable button (the - // documented "unknown ⇒ keep today's behaviour" path). Pinning that here - // is what makes the refusal below attributable to the RESOLVED MIME + // BEFORE the probe answers, this still LOOKS like an ordinary button: + // the semantics pass keeps role/tabindex while the MIME is merely + // unasked, because "not yet asked" is not "not viewable". (Activation + // itself no longer trusts that — TASK-2433 made it resolve the MIME + // first — but the two are separate claims and this one is the premise.) + // Pinning it here is what makes the refusal below attributable to the RESOLVED MIME // rather than to any other inertness route — an empty uuid, a latched // deletion, a hidden image — all of which would already be true now. expect(image().getAttribute('role')).toBe('button'); @@ -376,7 +486,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { expect(probeMock).toHaveBeenCalled(); press(image(), 'Enter'); - expect(openCount()).toBe(0); + expect(await opened()).toBe(0); // And it stops being a focus stop at all, rather than announcing itself // as a button that does nothing. @@ -385,10 +495,265 @@ describe('inline body image — keyboard activation (DR-12)', () => { expect(image().getAttribute('aria-label')).toBeNull(); }); + it('refuses an UNPROBED non-raster type — the gesture that beats the lazy probe', async () => { + // The bypass this task's revised gate closes, and the one the test above + // cannot see: `settleProbe()` selects the node, which builds the toolbar + // and runs the lazy HEAD, so by then `canActivate()` already knows the + // MIME and refuses before activation does any work of its own. An + // UNSELECTED body image — the state every image is in until the user + // touches it — has `knownMime === null`, and the old gate read it only + // when truthy: the click sailed past and opened the ORIGINAL file. + // + // So: never selected, no toolbar, nothing probed. The refusal here can + // only come from activation resolving the MIME itself before emitting. + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml' , size: 4096 }); + editor = makeEditor(target); + + // The premise: nothing has asked yet, so the image still looks openable. + expect(probeMock).not.toHaveBeenCalled(); + expect(image().getAttribute('role')).toBe('button'); + + press(image(), 'Enter'); + expect(await opened()).toBe(0); + click(image()); + expect(await opened()).toBe(0); + // And it did ask — a refusal reached by never probing at all would be + // the same count for the wrong reason. + expect(probeMock).toHaveBeenCalled(); + }); + + it('emits once when two gestures land inside one resolution window', async () => { + // Resolving the MIME before emitting introduced an await where there was + // none, and "fires exactly once" has to survive it: two activations + // entering that window would each clear the gate and each emit, which is + // two viewers from one user intent. The key-repeat guard does not cover + // this — these are two distinct, unrepeated gestures. + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + press(img, 'Enter'); + click(img); + + expect(await opened()).toBe(1); + + // And the latch RELEASES: a gesture after the window closes opens again, + // so this is not "the second one is swallowed forever". + press(img, 'Enter'); + expect(await opened()).toBe(2); + }); + + it('drops the request when the attachment is deleted mid-resolution', async () => { + // The await opened a window, and everything the gate checked at gesture + // time can stop being true inside it. A deletion is the sharpest case: + // it is authoritative, it arrives from another surface at any moment, and + // a request emitted after it opens a viewer on a row that is gone. + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + // Same tick as the gesture, before the HEAD resolves. + for (const fn of deletionListeners) fn('uuid-1'); + + expect(await opened()).toBe(0); + }); + + it('never probes, and never emits, on a surface with no workspace', async () => { + // An SSR / preview surface configures the extension with the unaddressed + // reader. There is no workspace to probe under, so nothing can be + // positively known — and the rule is "emit only on a positive answer", + // not "emit when we could not check". + address = { workspaceSlug: '', itemId: 'item-A', hostToken: 'apanel-1' }; + editor = makeEditor(target); + + press(image(), 'Enter'); + click(image()); + + expect(await opened()).toBe(0); + // Not merely refused after asking — never asked. A probe keyed on an + // empty workspace is a cache entry under the wrong key. + expect(probeMock).not.toHaveBeenCalled(); + }); + + it('does not emit for a row the probe says is GONE', async () => { + // `missing` is authoritative and distinct from `transient`: one is "the + // row is not there", the other is "we could not tell". Neither is a + // positively-known MIME, so neither opens — but they are separate + // branches of the result union and a gate written as + // `status === 'transient'` would let this one through. + probeMock.mockResolvedValue({ status: 'missing' }); + editor = makeEditor(target); + + press(image(), 'Enter'); + + expect(await opened()).toBe(0); + }); + + it('drops the request when the NodeView is torn down mid-resolution', async () => { + // The await outlives the editor: an item switch or a pane remount + // destroys the view while the HEAD is in flight, and a request emitted + // afterwards asks a host to open a viewer for a surface that is gone. + editor = makeEditor(target); + + press(image(), 'Enter'); + editor.destroy(); + editor = undefined; + + expect(await opened()).toBe(0); + }); + + // Each field SEPARATELY, because the fence is three comparisons and a test + // that moved only one leaves the other two free to be deleted. The workspace + // is what every image URL is read from, `itemId` is which pane shows the + // viewer, and `hostToken` is which of two concurrently-mounted panes it is. + for (const [label, moved] of [ + ['workspace', { workspaceSlug: 'ws2', itemId: 'item-A', hostToken: 'apanel-1' }], + ['item', { workspaceSlug: 'ws', itemId: 'item-B', hostToken: 'apanel-1' }], + ['owning mount', { workspaceSlug: 'ws', itemId: 'item-A', hostToken: 'apanel-2' }], + ] as const) { + it(`drops the request when the ${label} moves mid-resolution`, async () => { + // `CommentEditor` is reused across an item switch — its address + // changes under a mounted NodeView. The gesture belonged to the OLD + // address: emitting there opens a viewer over a pane the user has + // left, and emitting at the new one attributes the gesture to a + // different item. + editor = makeCommentEditor(target); + + press(image(), 'Enter'); + address = { ...moved }; + + expect(await opened()).toBe(0); + + // The control: with the address settled, the very next gesture emits + // — so the drop above is the fence, not a composer that stopped + // working. + press(image(), 'Enter'); + expect(await opened()).toBe(1); + expect(emitted[0].workspaceSlug).toBe(moved.workspaceSlug); + expect(emitted[0].itemId).toBe(moved.itemId); + expect(emitted[0].hostToken).toBe(moved.hostToken); + }); + } + + it('does not leave the NEW image dead when a swap lands mid-resolution', async () => { + // The one-at-a-time latch is per NodeView, and this NodeView deliberately + // SURVIVES a uuid swap (rotate/crop, or a peer's op) rather than being + // recreated. So a latch taken for the old attachment would still be held + // when the new one is clicked — and since a HEAD has no timeout, an + // activation that never settles would leave the image in front of the + // user permanently unopenable. + const pending = new Promise(() => {}); + probeMock.mockImplementation((_ws?: string, uuid?: string) => + uuid === 'uuid-1' + ? (pending as unknown as Promise<{ status: 'transient' }>) + : Promise.resolve({ status: 'ok', mime: 'image/png', size: 4096 } as never) + ); + editor = makeEditor(target); + + press(image(), 'Enter'); + expect(await opened()).toBe(0); + + // The swap the NodeView is built to survive. + editor.commands.setNodeSelection(1); + editor.commands.updateAttributes('attachmentImage', { uuid: 'uuid-2' }); + await opened(); + + press(image(), 'Enter'); + const requests = await opened(); + expect(requests).toBe(1); + expect(emitted[0].attachmentId).toBe('uuid-2'); + }); + + it('does not let a superseded activation unlock the one that replaced it', async () => { + // The other half of the swap fix, and the one it is easy to get wrong: + // the latch is released in TWO places now — the resolution's finalizer + // and the swap — so an unconditional release lets the OLD image's HEAD, + // landing late, unlock a request the NEW image is still holding. Two + // gestures then both emit, which is the duplicate the latch exists to + // prevent. + // ONE promise per attachment, handed to every caller — the real cache + // does exactly this (`fetchAttachmentMetadata` installs the in-flight + // promise and shares it), and it is load-bearing here: a fresh promise + // per call would leave the duplicate activation's continuation pending + // forever, hiding the very duplicate this test is about. + let releaseOld: () => void = () => {}; + let releaseNew: () => void = () => {}; + const oldProbe = new Promise((resolve) => { + releaseOld = () => resolve({ status: 'ok', mime: 'image/png', size: 1 }); + }); + const newProbe = new Promise((resolve) => { + releaseNew = () => resolve({ status: 'ok', mime: 'image/png', size: 4096 }); + }); + probeMock.mockImplementation( + (_ws?: string, uuid?: string) => (uuid === 'uuid-1' ? oldProbe : newProbe) as never + ); + editor = makeEditor(target); + + // A's activation goes in flight and is then superseded by a swap. + press(image(), 'Enter'); + editor.commands.setNodeSelection(1); + editor.commands.updateAttributes('attachmentImage', { uuid: 'uuid-2' }); + await opened(); + + // B's activation takes the latch... + press(image(), 'Enter'); + await opened(); + // ...and A's late answer must not hand it back. + releaseOld(); + await opened(); + + // A second gesture while B is still resolving. With the latch correctly + // held this is a no-op; with it wrongly released, this starts a SECOND + // resolution and both emit. + press(image(), 'Enter'); + releaseNew(); + + expect(await opened()).toBe(1); + expect(emitted[0].attachmentId).toBe('uuid-2'); + }); + + it('drops a request whose image was swapped AWAY AND BACK', async () => { + // Comparing the uuid alone is not enough, and this is the interleaving + // that shows it: a rotate the user immediately undoes, or a peer's op + // reverted, puts the ORIGINAL attachment back under a request that is + // still resolving for it. It would then find its own uuid in place and + // open a viewer for a gesture made two swaps ago — against an image the + // user has since acted on twice. + let releaseFirst: () => void = () => {}; + const firstProbe = new Promise((resolve) => { + releaseFirst = () => resolve({ status: 'ok', mime: 'image/png', size: 1 }); + }); + probeMock.mockImplementation( + (_ws?: string, uuid?: string) => + (uuid === 'uuid-1' + ? firstProbe + : Promise.resolve({ status: 'ok', mime: 'image/png', size: 4096 })) as never + ); + editor = makeEditor(target); + + press(image(), 'Enter'); + expect(await opened()).toBe(0); + + editor.commands.setNodeSelection(1); + editor.commands.updateAttributes('attachmentImage', { uuid: 'uuid-2' }); + await opened(); + editor.commands.setNodeSelection(1); + editor.commands.updateAttributes('attachmentImage', { uuid: 'uuid-1' }); + await opened(); + // The premise: the ORIGINAL attachment really is back under the node. + // Without it the stale request would be blocked by the uuid comparison + // and this test would pass for the wrong reason. + expect(image().getAttribute('data-attachment-id')).toBe('uuid-1'); + + releaseFirst(); + + expect(await opened()).toBe(0); + }); + it('still opens an allowlisted raster type after the probe resolves', async () => { // The control for the refusal above: same code path, same probe, same // number of awaits — only the MIME differs. - probeMock.mockResolvedValue({ status: 'ok', mime: 'image/png' }); + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/png' , size: 4096 }); editor = makeEditor(target); await settleProbe(); expect(probeMock).toHaveBeenCalled(); @@ -396,10 +761,10 @@ describe('inline body image — keyboard activation (DR-12)', () => { expect(image().getAttribute('role')).toBe('button'); expect(openCount()).toBe(0); press(image(), 'Enter'); - expect(openCount()).toBe(1); + expect(await opened()).toBe(1); }); - it('makes a deleted image inert rather than a dead focus stop', () => { + it('makes a deleted image inert rather than a dead focus stop', async () => { editor = makeEditor(target); const img = image(); expect(img.getAttribute('role')).toBe('button'); @@ -414,7 +779,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { // no longer exists. press(img, 'Enter'); click(img); - expect(openCount()).toBe(0); + expect(await opened()).toBe(0); }); it('does not strand focus on an image that just went inert', () => { @@ -430,7 +795,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { expect(document.activeElement).not.toBe(img); }); - it('gives a comment editor the same contract as an item body', () => { + it('gives a comment editor the same contract as an item body', async () => { // CommentEditor.svelte configures AttachmentImage independently, so // "works in the body" is not evidence it works in a comment. editor = makeCommentEditor(target); @@ -442,10 +807,10 @@ describe('inline body image — keyboard activation (DR-12)', () => { expect(openCount()).toBe(0); press(img, 'Enter'); - expect(openCount()).toBe(1); + expect(await opened()).toBe(1); }); - it('will not ACTIVATE while the image is showing a load-failure placeholder', () => { + it('will not ACTIVATE while the image is showing a load-failure placeholder', async () => { // Attributes are not the contract — activation is. Stripping role and // tabindex hides the image from Tab, but a stale event still in flight, a // synthetic one, or focus that predates the failure all reach the @@ -460,7 +825,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { press(img, ' '); click(img); - expect(openCount()).toBe(0); + expect(await opened()).toBe(0); }); it('is not a focus stop while the image is showing a load-failure placeholder', () => { diff --git a/web/src/lib/components/editor/attachmentImageMissing.svelte.test.ts b/web/src/lib/components/editor/attachmentImageMissing.svelte.test.ts index 010359a4..11367e1b 100644 --- a/web/src/lib/components/editor/attachmentImageMissing.svelte.test.ts +++ b/web/src/lib/components/editor/attachmentImageMissing.svelte.test.ts @@ -11,22 +11,42 @@ // 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. +// +// The viewer assertions here are PRODUCER-level — `notifyViewerOpen` is mocked, +// so "opens" means "asks". A broken or absent host would be invisible to them; +// that half of the route is `attachmentImageViewerHost.svelte.test.ts`'s. 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>(); +// Every open-the-viewer request this NodeView makes, captured RAW — before the +// channel's own addressability filter. The gate under test is the NodeView's; +// routing is `events.ts`'s and has its own spec. +const emitted: Array> = []; vi.mock('$lib/attachments/events', () => ({ notifyAttachmentPanelOpen: () => {}, + notifyViewerOpen: (event: Record) => { + emitted.push(event); + }, 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 })); +// The probe's full result union, spelled out. Without it `vi.fn` infers the +// type of the DEFAULT implementation alone, and every `mockResolvedValue` for a +// different arm is a type error — invisible under `npm run check`, which +// excludes `*.svelte.test.ts`, and a trap for whoever widens that exclude. +type ProbeResult = + | { status: 'ok'; mime: string; size: number } + | { status: 'missing' } + | { status: 'transient' }; + +// No probe by default: 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<() => Promise>(async () => ({ status: 'transient' })); vi.mock('./attachment-metadata', () => ({ fetchAttachmentMetadata: () => probeMock(), revalidateAttachmentMetadata: () => probeMock(), @@ -65,6 +85,7 @@ describe('inline image missing placeholder', () => { beforeEach(() => { deletionListeners.clear(); + emitted.length = 0; probeMock.mockClear(); target = document.body.appendChild(document.createElement('div')); }); @@ -82,6 +103,16 @@ describe('inline image missing placeholder', () => { return el; } + /** + * Let the lazy HEAD probe AND the activation's own MIME resolution settle. + * Activation is asynchronous as of TASK-2433 — it resolves the MIME before + * emitting — so a synchronous assertion after a click would read `[]` no + * matter what the implementation does. + */ + async function settle() { + await new Promise((resolve) => setTimeout(resolve, 0)); + } + function failLoad() { const img = target.querySelector('img[data-attachment-id]'); if (!img) throw new Error('image NodeView did not render'); @@ -92,41 +123,91 @@ describe('inline image missing placeholder', () => { // 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' }); + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml' , size: 4096 }); editor = makeEditor(target); const img = target.querySelector('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(); + await settle(); img.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 })); - expect(document.querySelector('dialog.attachment-image-lightbox')).toBeNull(); + await settle(); + // NOTHING is emitted — not an event carrying the unsafe MIME for the + // viewer to reject at the other end. The gate is the producer's, because + // a request that reached the bus would be visible to any future consumer. + expect(emitted).toEqual([]); }); - 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' }); + it('emits a fully-stamped viewer request for an allowlisted raster type', async () => { + // The gate must not cost the common case: a PNG opens as it always did — + // and this is the assertion the deletion of the old `` rests on. + // "The dialog is gone" is satisfied by an implementation that opens + // NOTHING, so the claim has to be about what is EMITTED. + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/png', size: 4096 }); editor = makeEditor(target); const img = target.querySelector('img[data-attachment-id]'); if (!img) throw new Error('image NodeView did not render'); editor.commands.setNodeSelection(1); - await Promise.resolve(); - await Promise.resolve(); + await settle(); 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(); + await settle(); + + // The WHOLE payload, field by field. Each one is load-bearing and each + // has a plausible wrong value: the address routes the event to one of + // several mounted hosts (DR-8), `workspaceSlug` is what every image URL + // is read from, `mime_type` is what lets `Lightbox` re-state the DR-16 + // gate over the set (it FAILS CLOSED on null as of TASK-2431, so an + // event with an unresolved MIME mounts a viewer that renders no image), + // and `invoker` is where the viewer aims focus on close. + expect(emitted).toEqual([ + { + attachmentId: 'uuid-1', + workspaceSlug: 'ws', + itemId: 'item-A', + hostToken: 'apanel-1', + images: [ + { + id: 'uuid-1', + alt: 'A diagram', + filename: null, + mime_type: 'image/png', + size_bytes: 4096, + width: null, + height: null, + }, + ], + index: 0, + invoker: img, + }, + ]); + }); + + it('emits nothing at all when the MIME cannot be resolved', async () => { + // The revised rule for this task (TASK-2431's adversarial round): a + // `transient` probe is NOT "unknown, so keep today's behaviour" any more. + // `Lightbox` fails closed on an unresolved MIME, so emitting one would be + // a request that opens nothing while looking, on the bus, exactly like a + // request that does. The retryable branch is TASK-2434's. + probeMock.mockResolvedValue({ status: 'transient' }); + editor = makeEditor(target); + const img = target.querySelector('img[data-attachment-id]'); + if (!img) throw new Error('image NodeView did not render'); + + img.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 })); + await settle(); + + expect(emitted).toEqual([]); }); 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' }); + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/png' , size: 4096 }); editor = makeEditor(target); editor.commands.setNodeSelection(1); await Promise.resolve(); diff --git a/web/src/lib/components/editor/attachmentImageNoOverlay.test.ts b/web/src/lib/components/editor/attachmentImageNoOverlay.test.ts new file mode 100644 index 00000000..960952e1 --- /dev/null +++ b/web/src/lib/components/editor/attachmentImageNoOverlay.test.ts @@ -0,0 +1,143 @@ +// The inline image NodeView owns NO overlay (PLAN-2392 phase 3a / TASK-2433). +// +// This is a STATIC check, deliberately, because the thing it guards is an +// absence and a behavioural test cannot see one. TASK-2433 deleted +// `openImageLightbox` / `closeLightbox` — a hand-rolled `` the NodeView +// appended to `document.body` and `showModal()`d — and routed activation onto +// the viewer channel instead, so the `Lightbox` that `AttachmentViewerHost` +// mounts is the only viewer on THIS route. Everything the modal contract is +// made of lives there: the lease-stacked backdrop, the focus trap and restore, the Escape +// ordering, and the DR-16 filter re-applied over the whole set. +// +// A future edit that "just pops a quick preview" from the NodeView would +// silently reintroduce a second viewer with none of that — and it would pass +// every PRODUCER spec in this directory, because those assert what the NodeView +// emits and a stray overlay emits nothing. `attachmentImageViewerHost.svelte.test.ts` +// asserts a real viewer appears, which is the other half, but it counts +// `.lightbox-backdrop` roots and would not see an extra overlay of a different +// shape either. Hence: read the source. +// +// The CSS half is checked here too. `.attachment-image-lightbox` lived in +// `app.css`, not in the TS file, so a JS-only sweep would have left a rule for +// a dialog nothing constructs — dead weight that also reads, to the next +// person, as evidence the dialog is still a supported surface. +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const SRC = fileURLToPath(new URL('../../..', import.meta.url)); +const NODEVIEW = join(SRC, 'lib/components/editor/attachment-image.ts'); + +/** Source with block and line comments stripped — the prose above mentions + * every banned token by name, and a check that matched its own warning label + * would be unfalsifiable. */ +function code(path: string): string { + return readFileSync(path, 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/^\s*\/\/.*$/gm, ''); +} + +function walk(dir: string, out: string[] = []): string[] { + for (const name of readdirSync(dir)) { + if (name === 'node_modules' || name === '.svelte-kit') continue; + const full = join(dir, name); + if (statSync(full).isDirectory()) walk(full, out); + else if (/\.(ts|js|svelte|css)$/.test(name)) out.push(full); + } + return out; +} + +describe('attachment-image.ts defines no overlay of its own', () => { + const source = code(NODEVIEW); + + it('constructs no dialog and opens no modal', () => { + expect(source).not.toMatch(/createElement\(\s*['"]dialog['"]\s*\)/); + expect(source).not.toMatch(/\bshowModal\b/); + expect(source).not.toMatch(/\bHTMLDialogElement\b/); + }); + + it('reaches for nothing on the document but the two things it needs', () => { + // An ALLOWLIST, not a list of forbidden names. `document.body` was the + // deleted lightbox's route out of this subtree, but `document['body']`, + // `document.querySelector('body')` and `document.getElementById(…)` are + // the same move spelled differently, and a blacklist only ever catches + // the spellings someone thought of. This NodeView legitimately needs + // exactly two members — one to build its own DOM, one to check where + // focus is — so anything else is a reach for a root it does not own. + const allowedMembers = new Set(['createElement', 'activeElement']); + const members = Array.from(source.matchAll(/\bdocument\s*\.\s*([A-Za-z_$][\w$]*)/g)).map( + (m) => m[1] + ); + expect(members.length).toBeGreaterThan(0); + expect(members.filter((m) => !allowedMembers.has(m))).toEqual([]); + // Computed access would slip straight past the allowlist above. + expect(source).not.toMatch(/\bdocument\s*\[/); + + // Named roots are only the obvious half. The check that survives a + // creative reimplementation is on the RECEIVER of every insertion — + // `someParent.appendChild(overlay)`, a portal into a captured root — + // where a blacklist of names would only catch the ones already thought + // of. + // + // The rule is DERIVED, not a list of blessed identifiers: every receiver + // must be an element this file CREATED. Combined with the member + // allowlist above, that is what confines the whole NodeView to its own + // subtree — a locally created element has no route into the document + // except the `dom` this NodeView returns, so anything appended into one + // is inside what ProseMirror mounts. Renaming `wrapper` to `container` + // keeps passing; appending into anything that arrived from elsewhere + // does not. + const created = new Set( + Array.from(source.matchAll(/\b(?:const|let|var)\s+([A-Za-z_$][\w$]*)[^=\n]*=\s*document\.createElement\(/g)).map((m) => m[1]) + ); + const INSERTERS = 'append|appendChild|prepend|insertBefore|replaceChildren|insertAdjacentElement'; + // The receiver is the WHOLE expression, dots included, so a reach through + // something this file was handed (`opts.host.appendChild(overlay)`) is a + // receiver of `opts.host` — not in `created`, and not silently skipped + // the way a bare-identifier pattern would skip it. + const receivers = Array.from( + source.matchAll(new RegExp(String.raw`([A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)*)\.(?:${INSERTERS})\(`, 'g')) + ).map((m) => m[1]); + expect(created.size).toBeGreaterThan(0); + expect(receivers.length).toBeGreaterThan(0); + expect(receivers.filter((r) => !created.has(r))).toEqual([]); + // And the same call spelled computed (`x['appendChild'](overlay)`), which + // no dot-pattern can see. + expect(source).not.toMatch(new RegExp(String.raw`\[\s*['"\`](?:${INSERTERS})['"\`]\s*\]`)); + // WHAT THIS CANNOT SEE, stated so nobody mistakes it for a proof. It is + // a regex over source: it has no scopes, no bindings and no call graph. + // An overlay built by an imported helper is invisible to it, and so is + // a name that `created` knows about being REASSIGNED to something + // foreign before it is appended into. Closing those needs an AST pass + // with binding resolution, which is a disproportionate amount of + // machinery for a guard whose job is to make a deliberate re-addition + // of the deleted dialog fail loudly. The narrower claim it does make — + // this file appends only into elements it created, and reaches for + // nothing on the document but `createElement` and `activeElement` — is + // the one that would have caught the code this task removed. + }); + + it('names the deleted lightbox nowhere', () => { + expect(source).not.toMatch(/openImageLightbox|closeLightbox|attachment-image-lightbox/); + }); + + it('routes activation through the viewer channel instead', () => { + // The other half of the same claim: "no overlay" is only the right + // absence if something else opens the viewer. A file that deleted the + // dialog and emitted nothing satisfies every assertion above. + expect(source).toMatch(/notifyViewerOpen\(/); + }); + + it('leaves no reference to the lightbox class anywhere in the app', () => { + // Including `app.css`, whose rules a JS-only sweep does not see, and the + // specs — a test still counting `dialog.attachment-image-lightbox` would + // be counting an element nothing can create, i.e. asserting 0 forever. + const offenders = walk(SRC) + // This file, which cannot check for a name without naming it. + .filter((f) => f !== fileURLToPath(import.meta.url)) + .filter((f) => readFileSync(f, 'utf8').includes('attachment-image-lightbox')) + .map((f) => f.slice(SRC.length)); + expect(offenders).toEqual([]); + }); +}); diff --git a/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts b/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts new file mode 100644 index 00000000..7143c620 --- /dev/null +++ b/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts @@ -0,0 +1,211 @@ +// The whole route: inline image NodeView → the REAL bus → the REAL +// `AttachmentViewerHost` → the REAL `Lightbox` (PLAN-2392 phase 3a / TASK-2433). +// +// Every other spec for this change stops at the producer: they mock +// `notifyViewerOpen` and assert the payload, which is the right shape for +// asking "did the NodeView emit the correct request" and is deliberately blind +// to whether anything is listening. That leaves the commit's actual headline +// claim — a deleted `` REPLACED by the shared viewer, not removed — +// resting on two halves nobody joins. A host that stopped consuming, an +// address that could not route, a payload the viewer filters back out: all of +// them keep the producer specs green and leave the user with an image that +// does nothing. +// +// So nothing here is stubbed but the network. The bus is real, the host is +// real, `Lightbox` is real, and the assertion is a viewer in the document with +// the clicked attachment in it. +// +// What jsdom still cannot prove — real inertness, real Tab traversal, layout +// and stacking — is `Lightbox`'s own contract and belongs to the browser suite +// (TASK-2436), not to a jsdom test that would pass vacuously here. +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { flushSync, mount, unmount } from 'svelte'; +import { Editor } from '@tiptap/core'; +import StarterKit from '@tiptap/starter-kit'; +import { __resetViewerBackdropForTests } from '$lib/a11y/viewerBackdrop'; +import { _resetEscapeStackForTests } from '$lib/stores/escapeStack'; + +const UUID = '11111111-1111-4111-8111-111111111111'; +const ITEM_ID = 'item-A'; +const HOST_TOKEN = 'apanel-1'; + +// The one stub: the HEAD probe. Everything the viewer path is made of stays +// real, because the point of this file is the wiring between the parts. +const probeMock = vi.fn(async () => ({ status: 'ok' as const, mime: 'image/png', size: 4096 })); +vi.mock('./attachment-metadata', () => ({ + fetchAttachmentMetadata: () => probeMock(), + revalidateAttachmentMetadata: () => probeMock(), + invalidateAttachmentMetadata: () => {}, + mimeToFormat: () => null, +})); + +const { AttachmentImage } = await import('./attachment-image'); +const { default: AttachmentViewerHost } = await import( + '$lib/components/attachments/AttachmentViewerHost.svelte' +); + +let address = { workspaceSlug: 'ws', itemId: ITEM_ID, hostToken: HOST_TOKEN }; + +function makeEditor(element: HTMLElement): Editor { + return new Editor({ + element, + extensions: [ + StarterKit, + AttachmentImage.configure({ + workspaceSlug: 'ws', + getDownloadUrl: (uuid: string, variant?: string) => + `/api/v1/workspaces/ws/attachments/${uuid}?variant=${variant ?? 'thumb-md'}`, + address: () => address, + supportedFormats: ['png'], + transform: async () => { + throw new Error('not used'); + }, + }), + ], + content: `

A diagram

`, + editable: true, + }); +} + +describe('inline image → viewer host → Lightbox', () => { + let editorTarget: HTMLElement; + let hostTarget: HTMLElement; + let editor: Editor | undefined; + let host: Record | null = null; + + function mountHost(props: { itemId: string; hostToken: string }) { + host = mount(AttachmentViewerHost, { target: hostTarget, props }) as Record; + flushSync(); + } + + /** Viewers actually on screen — the real component's root. */ + function viewers(): HTMLElement[] { + return Array.from(document.body.querySelectorAll('.lightbox-backdrop')); + } + + function image(): HTMLImageElement { + const el = editorTarget.querySelector('img[data-attachment-id]'); + if (!el) throw new Error('image NodeView did not render'); + return el; + } + + /** Activation resolves the MIME before emitting, so it spans a task. */ + async function settle() { + await new Promise((resolve) => setTimeout(resolve, 0)); + await new Promise((resolve) => setTimeout(resolve, 0)); + flushSync(); + } + + beforeEach(() => { + _resetEscapeStackForTests(); + __resetViewerBackdropForTests(); + address = { workspaceSlug: 'ws', itemId: ITEM_ID, hostToken: HOST_TOKEN }; + probeMock.mockClear(); + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/png', size: 4096 }); + editorTarget = document.body.appendChild(document.createElement('div')); + hostTarget = document.body.appendChild(document.createElement('div')); + }); + + afterEach(() => { + editor?.destroy(); + editor = undefined; + if (host) unmount(host); + host = null; + editorTarget.remove(); + hostTarget.remove(); + document.body.querySelectorAll('.lightbox-backdrop').forEach((el) => el.remove()); + }); + + // Run against TWO different addresses. One fixed pair cannot tell a producer + // that STAMPS the address from one that hard-codes the constants this file + // happens to use — and a hard-coded address is exactly what DR-8 exists to + // prevent, since it would route every editor's images at one pane. + for (const [label, addr] of [ + ['the default address', { itemId: ITEM_ID, hostToken: HOST_TOKEN }], + ['a different item and mount', { itemId: 'item-Z', hostToken: 'apanel-9' }], + ] as const) { + it(`opens the shared viewer on the image the user activated — ${label}`, async () => { + address = { workspaceSlug: 'ws', ...addr }; + mountHost(addr); + editor = makeEditor(editorTarget); + expect(viewers()).toHaveLength(0); + + image().dispatchEvent( + new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 }) + ); + await settle(); + + // ONE viewer, and it is the real component: a `role="dialog"` root + // portaled to ``, showing the attachment that was clicked. The + // deleted `` is not merely gone — something took its place. + expect(viewers()).toHaveLength(1); + const viewer = viewers()[0]; + expect(viewer.getAttribute('role')).toBe('dialog'); + expect(viewer.parentElement).toBe(document.body); + const shown = viewer.querySelector('img.lightbox-image'); + expect(shown?.getAttribute('src')).toContain(UUID); + // The full-resolution blob, as the deleted dialog loaded: the viewer + // asks for the canonical URL with no `variant`. + expect(shown?.getAttribute('src')).not.toContain('variant='); + }); + } + + it('opens on the KEYBOARD too, exactly once', async () => { + // The route the deleted dialog never had. A count, so a second emitter + // anywhere along the chain is visible. + mountHost({ itemId: ITEM_ID, hostToken: HOST_TOKEN }); + editor = makeEditor(editorTarget); + + image().dispatchEvent( + new KeyboardEvent('keydown', { key: 'Enter', bubbles: true, cancelable: true }) + ); + await settle(); + + expect(viewers()).toHaveLength(1); + }); + + it('reaches only the host it is ADDRESSED to', async () => { + // DR-8's rule, asserted end to end rather than on the channel alone: a + // master pane and a peeked pane are both mounted, and a NodeView's event + // must open the viewer over the one that owns it. A producer that + // stamped a constant, or a host that consumed everything, both show up + // here as a viewer in the wrong place. + mountHost({ itemId: ITEM_ID, hostToken: 'a-different-mount' }); + editor = makeEditor(editorTarget); + + image().dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 })); + await settle(); + + expect(viewers()).toHaveLength(0); + }); + + it('opens nothing at all when no host is mounted', async () => { + // The honest statement of what this surface now depends on: an editor + // outside an `ItemDetail` announces a button and, with nobody listening, + // opens nothing. Every real mount threads a token (both ``s and + // every `CommentEditor` come from `ItemDetail`), so this is the + // boundary, not a live bug — pinned so a future reusable-editor surface + // discovers it here rather than as a dead control in front of a user. + editor = makeEditor(editorTarget); + + image().dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 })); + await settle(); + + expect(viewers()).toHaveLength(0); + }); + + it('does not open for a MIME the viewer would filter back out', async () => { + // The producer's gate and the viewer's are the same gate, stated twice + // on purpose (TASK-2431). If the producer ever emitted an SVG, the + // viewer would filter it and mount an empty shell — the failure this + // asserts is absent. + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml', size: 100 }); + mountHost({ itemId: ITEM_ID, hostToken: HOST_TOKEN }); + editor = makeEditor(editorTarget); + + image().dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 })); + await settle(); + + expect(viewers()).toHaveLength(0); + }); +}); diff --git a/web/src/lib/components/timeline/timelineEditorOwnedImages.svelte.test.ts b/web/src/lib/components/timeline/timelineEditorOwnedImages.svelte.test.ts index 028533bf..7020c0fb 100644 --- a/web/src/lib/components/timeline/timelineEditorOwnedImages.svelte.test.ts +++ b/web/src/lib/components/timeline/timelineEditorOwnedImages.svelte.test.ts @@ -21,12 +21,20 @@ // this file exists rather than another stand-in. // // The two viewers are distinguishable, which is what makes "exactly once" -// observable: the NodeView opens its own `dialog.attachment-image-lightbox`, -// the timeline opens the `Lightbox` component (`.lightbox-backdrop`). +// observable: the NodeView emits an open-viewer REQUEST on the shared channel +// (TASK-2433 deleted its hand-rolled ``; an `AttachmentViewerHost` owned +// by `ItemDetail` mounts the one `Lightbox` in response, and no `ItemDetail` is +// mounted here), while the timeline mounts `Lightbox` itself +// (`.lightbox-backdrop`). Requests are counted off the REAL bus, addressability +// filter included — this file's whole premise is that nothing is restated. import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { flushSync, mount, unmount, tick } from 'svelte'; import type { Comment, TimelineEntry, TimelineResponse } from '$lib/types'; import { __resetViewerBackdropForTests } from '$lib/a11y/viewerBackdrop'; +import { + isAttachmentViewerEventForHost, + registerAttachmentViewerListener, +} from '$lib/attachments/events'; import { _resetEscapeStackForTests } from '$lib/stores/escapeStack'; const PNG = '11111111-1111-4111-8111-111111111111'; @@ -129,6 +137,11 @@ const props = $state({ currentContent: '', itemId: 'item-a', collectionId: 'coll-1', + // The identity `ItemDetail` mints per mount and threads down to every + // CommentEditor. Without it the NodeView's events are unaddressable and the + // channel drops them — which would make every `node: 1` below unreachable + // for a reason that has nothing to do with the delegation under test. + hostToken: 'host-1', visibleKinds: undefined as Array<'comment' | 'activity' | 'version'> | undefined, }); @@ -156,14 +169,39 @@ function bodyImage(): HTMLElement { return el; } -/** Viewers currently open, counted per owner so a double-open is visible. */ +/** Open-viewer requests the NodeView put on the bus, in this test's lifetime. */ +let nodeRequests: unknown[] = []; +let disposeViewerListener: (() => void) | null = null; + +/** + * Counted per owner so a double-open is visible. NOTE THE ASYMMETRY, which is + * the shape of the change rather than a shortcut: `timeline` counts viewers + * actually mounted, because ItemTimeline still mounts `Lightbox` itself, while + * `node` counts open-viewer REQUESTS addressed to this host, because the + * NodeView no longer mounts anything — an `ItemDetail`-owned + * `AttachmentViewerHost` does, and no `ItemDetail` is mounted in this file. + * A request that would reach that host is the strongest statement available + * here; the request-to-viewer half is + * `editor/attachmentImageViewerHost.svelte.test.ts`'s. + */ function viewers() { return { - node: document.querySelectorAll('dialog.attachment-image-lightbox').length, + node: nodeRequests.length, timeline: document.querySelectorAll('.lightbox-backdrop').length, }; } +/** + * Activation resolves the image's MIME before emitting (TASK-2433), so it is + * asynchronous even on a cache hit. Counting without this reads 0 whatever the + * implementation does — the exact shape of vacuous green this phase keeps + * producing. + */ +async function flushActivation() { + await new Promise((resolve) => setTimeout(resolve, 0)); + flushSync(); +} + function press(el: HTMLElement, key: string, mods: KeyboardEventInit = {}) { el.dispatchEvent( new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true, ...mods }) @@ -193,6 +231,20 @@ describe('ItemTimeline vs. a live editor inside it', () => { commentUpdateMock.mockReset(); commentUpdateMock.mockResolvedValue(undefined); props.visibleKinds = undefined; + nodeRequests = []; + // The REAL channel, not a mock: an event that fails its addressability + // check reaches no host at runtime either, and counting pre-filter + // emissions would score a misaddressed request as a viewer the user can + // see. `hostToken` is set on the props for the same reason. + disposeViewerListener = registerAttachmentViewerListener((event) => { + // Addressed to THIS host, by the same predicate `AttachmentViewerHost` + // uses. A raw count would score a misaddressed request as a viewer the + // user can see, when at runtime it reaches nobody. + if (!isAttachmentViewerEventForHost(event, { itemId: 'item-a', hostToken: 'host-1' })) { + return; + } + nodeRequests.push(event); + }); host = document.body.appendChild(document.createElement('div')); app = mount(ItemTimeline, { target: host, props }) as Record; await settle(); @@ -201,10 +253,11 @@ describe('ItemTimeline vs. a live editor inside it', () => { afterEach(() => { if (app) unmount(app); app = null; + disposeViewerListener?.(); + disposeViewerListener = null; + nodeRequests = []; host.remove(); - document - .querySelectorAll('dialog.attachment-image-lightbox, .lightbox-backdrop') - .forEach((d) => d.remove()); + document.querySelectorAll('.lightbox-backdrop').forEach((d) => d.remove()); }); it('mounts a live editor whose image already carries the NodeView contract', async () => { @@ -233,6 +286,7 @@ describe('ItemTimeline vs. a live editor inside it', () => { expect(viewers()).toEqual({ node: 0, timeline: 0 }); press(editorImage(), 'Enter'); + await flushActivation(); expect(viewers()).toEqual({ node: 1, timeline: 0 }); }); @@ -240,12 +294,14 @@ describe('ItemTimeline vs. a live editor inside it', () => { it('opens exactly one viewer on Space inside the editor', async () => { await enterEditMode(); press(editorImage(), ' '); + await flushActivation(); expect(viewers()).toEqual({ node: 1, timeline: 0 }); }); it('opens exactly one viewer on a click inside the editor', async () => { await enterEditMode(); click(editorImage()); + await flushActivation(); expect(viewers()).toEqual({ node: 1, timeline: 0 }); }); @@ -263,6 +319,7 @@ describe('ItemTimeline vs. a live editor inside it', () => { press(img, 'Enter', { metaKey: true }); await settle(); + await flushActivation(); expect(viewers()).toEqual({ node: 0, timeline: 0 }); expect(commentUpdateMock).toHaveBeenCalledTimes(1); @@ -282,6 +339,7 @@ describe('ItemTimeline vs. a live editor inside it', () => { img.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 })); img.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, detail: 2 })); flushSync(); + await flushActivation(); expect(viewers()).toEqual({ node: 1, timeline: 0 }); }); @@ -316,9 +374,21 @@ describe('ItemTimeline vs. a live editor inside it', () => { expect(img.getAttribute('role')).toBe('button'); expect(img.getAttribute('tabindex')).toBe('0'); expect(img.getAttribute('aria-label')).toBe('View image: a sketch'); - // And it still activates — semantics without activation is the dead stop - // in its other direction. + // Activation, however, is now gated on a RESOLVED MIME (TASK-2433), and + // this fixture's whole point is an image whose probe never resolves one: + // `Lightbox` fails closed on `mime_type: null`, so emitting for it would + // be a request that opens nothing. It stays shut here — and the dead + // focus stop that leaves (announced as a button, refuses to open) is + // exactly what TASK-2434's four-branch matrix is for, where a transient + // probe becomes retryable rather than silent. press(img, 'Enter'); + await flushActivation(); + expect(viewers()).toEqual({ node: 0, timeline: 0 }); + + // The control, so the assertion above is not just "activation is broken": + // the PROBED image in the same live editor still opens. + press(editorImage(PNG), 'Enter'); + await flushActivation(); expect(viewers()).toEqual({ node: 1, timeline: 0 }); }); @@ -334,11 +404,13 @@ describe('ItemTimeline vs. a live editor inside it', () => { expect(img.getAttribute('tabindex')).toBe('0'); click(img); + await flushActivation(); expect(viewers()).toEqual({ node: 0, timeline: 1 }); }); it('still opens its OWN rendered thumbnail by keyboard', async () => { press(bodyImage(), 'Enter'); + await flushActivation(); expect(viewers()).toEqual({ node: 0, timeline: 1 }); }); @@ -346,9 +418,11 @@ describe('ItemTimeline vs. a live editor inside it', () => { // The modifier guard is not scoped to editor-owned DOM: a shortcut is a // shortcut wherever it is pressed. press(bodyImage(), 'Enter', { metaKey: true }); + await flushActivation(); expect(viewers()).toEqual({ node: 0, timeline: 0 }); press(bodyImage(), 'Enter'); + await flushActivation(); expect(viewers()).toEqual({ node: 0, timeline: 1 }); }); });