gsadmin/pad
1
0
Fork 0
mirror of https://github.com/PerpetualSoftware/pad.git synced 2026-09-24 19:32:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(attachments): route inline images through the unified viewer (TASK-2433)

Browse Source 
The inline `pad-attachment:` image NodeView opened its own hand-rolled
`<dialog>`: `openImageLightbox` appended one to `document.body` and
`showModal()`d it. It worked, and everything the modal contract is made of
came free from the platform — top layer, inertness, focus trap, focus
restore, Escape. This commit deletes it and emits on the viewer channel
instead, so the `Lightbox` that `AttachmentViewerHost` mounts is the only
viewer on this route, with the lease-stacked backdrop, the escape ordering
and the DR-16 filter re-applied over the whole set. Shaped as a pure swap
so the deletion is reviewable on its own.

THE MIME IS RESOLVED BEFORE ANYTHING IS EMITTED, revising the
decomposition's "keep today's positively-known gate". TASK-2431 made
`Lightbox` fail closed on an unresolved MIME, so an event carrying
`mime_type: null` is not "let the viewer decide" — it is a viewer that
mounts and renders no image. Activation now awaits `fetchAttachmentMetadata`
(a cache hit in the common case) and emits only on a positively-known
allowlisted answer. That also closes a mid-phase bypass: the old gate read
`knownMime` only when truthy, so a click landing before the lazy probe
resolved opened the original file, and a later unsafe answer did not close
it. What it costs, deliberately and temporarily, is that a `transient`
probe now opens nothing — the four-branch matrix that makes the gate total
is TASK-2434's.

AND THE CHANNEL ENFORCES IT, because a rule only one producer follows is a
convention, not an invariant — and the failure mode is silent precisely
because the viewer fails closed. `notifyViewerOpen` now takes a set whose
`mime_type` is non-nullable (`ViewerReadyImage` / `ViewerOpenRequest`), so a
forgetful producer is a compile error rather than an image that does not
open, and it refuses a set at runtime unless every entry is positively
allowlisted, the way it already no-ops on a missing address. The WHOLE
emission is dropped rather than the offending entry: `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
activated. `LightboxImage` is untouched — the consumer side stays nullable,
because its records are live and `size_bytes` / `width` / `height` must
remain optional for 3b. `events.test.ts` asserted the permissive behaviour;
it now asserts the refusal, with a control so a gate that refused
everything cannot pass.

The await is new, so the fences are too: the gate's premises are re-checked
on the far side (teardown, a uuid swap, a deletion, and a host that moved —
the comment composer's address is live across an item switch), and one
activation at a time. That latch is generation-stamped: it is released by
the resolution's own finalizer AND by a uuid swap, which this NodeView
outlives, so an unconditional release would let a superseded request unlock
the one that replaced it, and a HEAD that never settles would otherwise
leave the new image permanently unopenable.

The `.attachment-image-lightbox` CSS goes with it; it lived in `app.css`,
not the TS file, where a JS-only sweep would have left it.

Tests assert the emitted PAYLOAD, not the dialog's absence — an
implementation that deleted the dialog and emitted nothing satisfies "no
dialog" — and a new spec drives the WHOLE route with nothing stubbed but
the network: real NodeView, real bus, real `AttachmentViewerHost`, real
`Lightbox`, asserting a viewer in the document, addressed to the right
host, showing the un-variant original. Verified by mutation: emit-nothing,
emit-unresolved-MIME, drop-the-allowlist-check at either the producer or
the channel, gate the set on its first entry only, drop the one-at-a-time
latch or release it unconditionally, drop any post-await fence, null the
invoker, and append an overlay to `documentElement` each fail at least one
test. Two of those needed cases the existing specs could not reach: every
prior DR-16 test selected the node first, which pre-resolves the MIME
through `canActivate()` and leaves the activation path's own check
unexercised — precisely the state a body image is in when it is clicked.

Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
This commit is contained in:
xarmian
2026-08-05 21:28:50 +00:00
parent 092498da23
commit 30fa9fc3dd
11 changed files with 1346 additions and 180 deletions
Download Patch File Download Diff File Expand all files Collapse all files
web/src/app.css
-49
View File
@@ -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 <dialog> 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;
web/src/lib/attachments/events.test.ts
+132 -8
View File
@@ -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> = {}): LightboxImage {
};
}
function viewerEvent(over: Partial<AttachmentViewerOpenEvent> = {}): 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<LightboxImage> = {}): ViewerReadyImage {
return image({ mime_type: 'image/png', ...over }) as ViewerReadyImage;
}
function viewerEvent(over: Partial<ViewerOpenRequest> = {}): 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));
web/src/lib/attachments/events.ts
+64 -5
View File
@@ -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<LightboxImage, 'mime_type'> & { mime_type: string };
/** What `notifyViewerOpen` accepts: the event, with the set already resolved. */
export interface ViewerOpenRequest extends Omit<AttachmentViewerOpenEvent, 'images'> {
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);
}
web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts
+20 -3
View File
@@ -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> = {}): 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<LightboxImage> = {}): ViewerReadyImage {
return {
id: ATT_ID,
alt: 'a diagram',
@@ -56,10 +73,10 @@ function image(over: Partial<LightboxImage> = {}): LightboxImage {
width: 800,
height: 600,
...over,
};
} as ViewerReadyImage;
}
function openEvent(over: Partial<ViewerEvent> = {}): ViewerEvent {
function openEvent(over: Partial<ViewerRequest> = {}): ViewerRequest {
return {
attachmentId: ATT_ID,
workspaceSlug: 'ws',
web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts
+11 -1
View File
@@ -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> = {}): ViewerEvent {
function openEvent(over: Partial<ViewerRequest> = {}): ViewerRequest {
return {
attachmentId: ATT_ID,
workspaceSlug: 'ws',
web/src/lib/components/editor/attachment-image.ts
+175 -44
View File
@@ -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<AttachmentImageOptions>({
// 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<AttachmentImageOptions>({
* 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<AttachmentImageOptions>({
* 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 <img> 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<AttachmentImageOptions>({
// 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 <dialog> 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
* `<dialog>` 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();
}
web/src/lib/components/editor/attachmentImageKeyboard.svelte.test.ts
+411 -46
View File
@@ -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<Record<string, unknown>> = [];
vi.mock('$lib/attachments/events', () => ({
notifyAttachmentPanelOpen: () => {},
notifyViewerOpen: (event: Record<string, unknown>) => {
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<ProbeResult>>(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 = '<p><img data-attachment-id="uuid-1" src="/api/v1/x" alt="A diagram"></p>';
/**
* 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<number> {
// 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 `<dialog>`. 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 <img> 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<never>(() => {});
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', () => {
web/src/lib/components/editor/attachmentImageMissing.svelte.test.ts
+96 -15
View File
@@ -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<Record<string, unknown>> = [];
vi.mock('$lib/attachments/events', () => ({
notifyAttachmentPanelOpen: () => {},
notifyViewerOpen: (event: Record<string, unknown>) => {
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<ProbeResult>>(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<HTMLImageElement>('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<HTMLImageElement>('img[data-attachment-id]');
if (!img) throw new Error('image NodeView did not render');
// Select the node so the lazy MIME probe runs, then let it settle.
editor.commands.setNodeSelection(1);
await Promise.resolve();
await Promise.resolve();
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 `<dialog>` 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<HTMLImageElement>('img[data-attachment-id]');
if (!img) throw new Error('image NodeView did not render');
editor.commands.setNodeSelection(1);
await Promise.resolve();
await Promise.resolve();
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<HTMLImageElement>('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();
web/src/lib/components/editor/attachmentImageNoOverlay.test.ts
+143
View File
@@ -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 `<dialog>` 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([]);
});
});
web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts
+211
View File
@@ -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 `<dialog>` 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: `<p><img data-attachment-id="${UUID}" src="/api/v1/x" alt="A diagram"></p>`,
editable: true,
});
}
describe('inline image → viewer host → Lightbox', () => {
let editorTarget: HTMLElement;
let hostTarget: HTMLElement;
let editor: Editor | undefined;
let host: Record<string, unknown> | null = null;
function mountHost(props: { itemId: string; hostToken: string }) {
host = mount(AttachmentViewerHost, { target: hostTarget, props }) as Record<string, unknown>;
flushSync();
}
/** Viewers actually on screen — the real component's root. */
function viewers(): HTMLElement[] {
return Array.from(document.body.querySelectorAll<HTMLElement>('.lightbox-backdrop'));
}
function image(): HTMLImageElement {
const el = editorTarget.querySelector<HTMLImageElement>('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 `<body>`, showing the attachment that was clicked. The
// deleted `<dialog>` 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<HTMLImageElement>('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 `<Editor>`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);
});
});
web/src/lib/components/timeline/timelineEditorOwnedImages.svelte.test.ts
+83 -9
View File
@@ -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 `<dialog>`; 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<string, unknown>;
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 });
});
});