From 03e0edb60532db39f48bdca3c03499fe11c0ae45 Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 5 Aug 2026 23:57:09 +0000 Subject: [PATCH] feat(attachments): complete the MIME matrix and address fence (TASK-2434) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TASK-2433 made the inline-image activation path resolve the attachment's MIME before emitting and refuse anything not positively allowlisted. That refusal was binary: `ok` + raster opened the viewer and every other result returned silently, leaving two states where a focused, button-announced image swallowed the gesture — a `transient` probe, and a resolved MIME the viewer will not take. This completes the four-branch matrix, each arm with a destination: - `ok` + allowlisted raster → the viewer, unchanged. - `ok` + anything else → the options PANEL (DR-7). A REDIRECT, not a refusal: an SVG or a PDF referenced as an inline image is a real attachment with real options, it is just not something to hand a viewer that would execute it. The image therefore stays a real activation target and its accessible name names the panel — taking the semantics off (as the binary gate did) would hide a working control, and would make the redirect fire exactly once before the recorded MIME closed the gate on every later tap. - `missing` (authoritative 404) → the permanent placeholder, latched, nothing opened. - `transient` → the RETRYABLE placeholder. Never an open and never a latch: only a 404 is authoritative (DR-17). The fence is hardened to the FULL address. The whole address is captured before the await and compared after, and both emissions stamp the CAPTURED values — the reader is live (`CommentEditor` is reused across an item switch) so a re-read can address the wrong host. The continuation also re-checks `deleted` on its own terms: a delete does not change which attachment the node points at, so a probe issued before it can resolve `ok` afterwards with the uuid still current. Check and emit stay adjacent and synchronous — no timer, no microtask between them. Also adds the minimal pending contract the await needs: `aria-busy` plus a wait cursor while the MIME resolves, cleared by the resolution's finalizer and by a uuid swap. No new chrome. Seam with PLAN-2411, stated in comments and deliberately not built: the `deleted` latch is cleared ONLY by an authoritative restore signal on 2411's channel, never by editor undo — DR-17 requires Ctrl-Z to leave an inert placeholder rather than resurrect a working attachment. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC --- .../lib/components/editor/attachment-image.ts | 345 ++++++++-- .../attachmentImageKeyboard.svelte.test.ts | 615 +++++++++++++++++- .../attachmentImageViewerHost.svelte.test.ts | 93 +++ 3 files changed, 1001 insertions(+), 52 deletions(-) diff --git a/web/src/lib/components/editor/attachment-image.ts b/web/src/lib/components/editor/attachment-image.ts index 9ae90c67..fa205799 100644 --- a/web/src/lib/components/editor/attachment-image.ts +++ b/web/src/lib/components/editor/attachment-image.ts @@ -38,7 +38,11 @@ import { mimeToFormat } from './attachment-metadata'; import { openCropModal, type CropResult } from './attachment-crop-modal'; -import { notifyViewerOpen, registerAttachmentDeletionListener } from '$lib/attachments/events'; +import { + notifyAttachmentPanelOpen, + notifyViewerOpen, + registerAttachmentDeletionListener +} from '$lib/attachments/events'; import { type AttachmentHostAddressReader, readUnaddressed @@ -290,6 +294,16 @@ export const AttachmentImage = Node.create({ // Latched by a confirmed deletion (not by a mere load failure). // Deletion is authoritative: a load still in flight when it lands // must not be allowed to paint the image back (Codex round 15). + // + // SEAM WITH PLAN-2411 (stated, not built here): this latch is cleared + // ONLY by an authoritative RESTORE signal on 2411's channel — never by + // editor undo. DR-17 requires Ctrl-Z to leave an inert placeholder + // rather than resurrect a working attachment: the delete was a REST row + // mutation that Tiptap/Yjs history cannot roll back, so an undo that + // re-inserted the node would otherwise present a live-looking image for + // a row the server no longer has. `deleted` is closure-private and the + // bus is deletion-only, so 2411 must extend both; that is its work, and + // nothing in THIS file may clear the latch on a document event. let deleted = false; // True once the NodeView is torn down. Async continuations (HEAD // probes, transform results) must not touch DOM after that. @@ -329,22 +343,24 @@ export const AttachmentImage = Node.create({ * role+tabindex would be a focus stop that announces itself as a * button and does nothing. (The placeholder itself carries the * retry affordance while the failure is still transient.) - * - a KNOWN MIME outside the viewer allowlist — activate() refuses it - * (DR-16), so the same dead stop applies. An UNPROBED node keeps the - * semantics: "not yet asked" is not "not viewable", and the probe is - * lazy. This mirrors ItemTimeline.svelte's semantics pass, which is - * likewise able to take the semantics back OFF when a probe resolves. - * - * Consequence, accepted deliberately: because the probe is lazy, a - * refused image can be FOCUSED before its MIME resolves and lose - * focus when it does. The alternative is leaving an SVG as a focus - * stop that announces itself as a button and does nothing, which is - * the failure this whole rule exists to prevent — and the same - * trade the placeholder below already makes. + * A KNOWN MIME outside the viewer allowlist is NOT one of them, as of + * TASK-2434. It used to be: the gate was binary — viewer or nothing — + * so a resolved `image/svg+xml` made the image a control that refused, + * and the semantics had to come off. The matrix replaced that refusal + * with a REDIRECT (DR-7): a non-allowlisted attachment is in the + * options PANEL's scope, so the image stays a perfectly real activation + * target — only its destination changes. Taking the semantics off now + * would hide a working control instead of retiring a dead one, and + * would break it for the mouse too, since click and key share one gate: + * the first tap would open the panel, resolve the MIME, and leave the + * image inert for every tap after it. * * The accessible name comes from alt with a GENERIC fallback: there is * no filename on the node's attrs and the HEAD metadata does not carry * one either, so the filename form DR-12 sketches has no source here. + * It also has to name the DESTINATION rather than always promising a + * viewer — a resolved non-raster type announces the panel it actually + * opens. */ /** * Is this image an activation target right now? @@ -356,6 +372,13 @@ export const AttachmentImage = Node.create({ * below was, briefly, only in the semantics half — so the placeholder * hid the image and stripped its role, while a stale or synthetic * event on the hidden still opened a viewer. + * + * It answers "does activation DO something", not "does it open the + * viewer". The MIME decides WHICH surface (see activate()), and it is + * deliberately absent from here: this predicate is also what a + * post-await continuation re-checks, and folding a viewer-only clause + * into it would make the panel branch unreachable the moment it wrote + * the MIME it was branching on. */ function canActivate(): boolean { return ( @@ -364,11 +387,20 @@ export const AttachmentImage = Node.create({ // Hidden means the placeholder has taken over: either a // confirmed deletion or a load failure. Neither has an image // to show, so neither has anything to open. - img.style.display !== 'none' && - !(knownMime && !canOpenInViewer(knownMime)) + img.style.display !== 'none' ); } + /** + * What activation would open, as far as anything KNOWN says. Unprobed + * reads as the viewer — "not yet asked" is not "not viewable", and the + * probe is lazy — which is exactly what the name has to say before the + * HEAD lands. activate() never trusts it: it resolves the MIME itself. + */ + function announcesPanel(): boolean { + return !!knownMime && !canOpenInViewer(knownMime); + } + function applyImageSemantics() { if (!canActivate()) { // Removing tabindex from the focused element would strand focus on @@ -383,10 +415,37 @@ export const AttachmentImage = Node.create({ img.setAttribute('tabindex', '0'); img.setAttribute( 'aria-label', - currentAlt ? `View image: ${currentAlt}` : 'View attachment image' + announcesPanel() + ? currentAlt + ? `Attachment options: ${currentAlt}` + : 'Attachment options' + : currentAlt + ? `View image: ${currentAlt}` + : 'View attachment image' ); } + /** + * The pending contract, minimal (TASK-2434). + * + * Activation awaits a HEAD. On a cache hit that is one microtask and + * nothing is visible; cold, it is a round trip during which a click + * that does nothing reads as broken. This NodeView has no + * metadata-pending affordance — the placeholder below is for LOAD + * failure, a different thing — so the smallest honest one: `aria-busy` + * for assistive tech and a wait cursor for everyone else. No spinner + * chrome in a parity commit. + */ + function setActivationPending(pending: boolean): void { + if (pending) { + img.setAttribute('aria-busy', 'true'); + img.style.cursor = 'progress'; + return; + } + img.removeAttribute('aria-busy'); + img.style.cursor = ''; + } + function showMissing() { missing.textContent = `📎 ${currentAlt || 'Attachment unavailable'}`; // Distinct copy per cause: a confirmed deletion is permanent and @@ -533,6 +592,48 @@ export const AttachmentImage = Node.create({ const base = opts.getDownloadUrl(currentUuid, 'thumb-md'); loadImage(`${base}${base.includes('?') ? '&' : '?'}retry=${Date.now()}`); } + /** + * The `transient` arm of the matrix (TASK-2434 / DR-17). + * + * `transient` says NOTHING about whether the row exists — a 5xx, a + * proxy hiccup, a network throw — so it must never latch and must + * never open. Before this task it also did nothing at all, which left + * the worst of the three: a focused image announcing itself as a button + * whose activation silently returned, with no way for the user to learn + * that anything failed or to try again. Repeat presses would have gone + * on doing nothing forever. + * + * So it hands over to the RETRYABLE placeholder — the one affordance + * this NodeView already has for "this did not work, click to retry", + * reached here by a different route than a failed `load`. Nothing is + * latched: `deleted` stays false, the transient result is not cached + * (`fetchAttachmentMetadata` evicts it on settle), and Retry re-issues + * both the image load and, on a second failure, the HEAD. + * + * Retry does NOT resume the activation, deliberately. It is the + * placeholder's existing affordance and it means "load this image + * again", not "open it" — a reload control that opened a viewer would + * be doing something the user did not ask for. The user gets the image + * back and activates it again if they still want to, and that second + * gesture really does re-probe: a transient result is evicted from the + * metadata cache as it settles, so nothing replays the failure. + * + * The cost, accepted: an image that had rendered FINE is replaced by + * the placeholder when only its HEAD failed. It is one click to undo + * and it is recoverable; a control that silently does nothing is + * neither. Anything gentler would be a third failure state — an inline + * error, a toast — which is new chrome this commit does not add. + */ + function showTransientProbeFailure(): void { + // Keyboard activation is the case that matters: showMissing() takes + // the semantics off the and blurs it, so without this the + // keypress that reported the failure also drops focus to . + // The placeholder is the retry control, so focus belongs on it. + const hadFocus = document.activeElement === img; + showMissing(); + if (hadFocus) missing.focus(); + } + missing.addEventListener('click', retryLoad); missing.addEventListener('keydown', (event) => { if (event.key === 'Enter' || event.key === ' ') { @@ -570,8 +671,12 @@ export const AttachmentImage = Node.create({ * 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. + * we await one HEAD, and we ask the VIEWER for only a + * positively-known allowlisted answer. (Since TASK-2434 a + * positively-known NON-allowlisted answer is not dropped — it goes to + * the options panel instead. The rule the viewer cares about is + * unchanged: nothing unresolved and nothing outside the allowlist + * reaches it.) * * The channel enforces the same rule at the boundary as of TASK-2433 — * `notifyViewerOpen` takes a set whose `mime_type` is non-nullable and @@ -580,14 +685,26 @@ export const AttachmentImage = Node.create({ * 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. + * TASK-2434 made the gate TOTAL rather than binary. Every probe result + * now has a destination, and none of them is "return quietly": + * + * - `ok` + allowlisted raster → the viewer. + * - `ok` + anything else → the options PANEL (DR-7). A + * REDIRECT, not a refusal: an SVG or a PDF referenced as an inline + * image is a real attachment with real options, it is just not + * something to hand a viewer that would execute it. + * - `missing` (an authoritative 404) → the permanent placeholder, + * latched. Nothing opens. + * - `transient` → the RETRYABLE placeholder. Never an open, and + * never a latch: only a 404 is authoritative (DR-17). + * + * The two that were dead focus stops before this — `transient`, and a + * resolved non-allowlisted MIME — are the two the matrix exists for. + * + * The one remaining silent return is a surface with NO WORKSPACE to + * probe with (SSR / preview). It is not a dead stop in practice: those + * surfaces have no host mounted to receive either event, so there is + * nothing to route to and nothing to say. * * It also closes a mid-phase bypass Codex found: the old gate read * `knownMime` only when truthy, so a click landing before the lazy @@ -608,13 +725,26 @@ export const AttachmentImage = Node.create({ // 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(); + // + // DESTRUCTURED INTO PRIMITIVES, not held as the returned object. + // `opts.address()` is a reader the HOST supplies, and both live + // implementations happen to build a fresh object literal per call — + // so holding the reference would be safe today. It would be safe + // only for that reason. A host that returned a stable object it + // mutated in place would rewrite the very snapshot this fence + // compares against, and the comparison would pass unconditionally + // while looking exactly as it looks now. Three string copies buy + // independence from a property no interface states and no test + // could plausibly catch. + const { workspaceSlug: fromWs, itemId: fromItem, hostToken: fromHost } = + 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; + if (!fromWs) return; activating = true; const seq = ++activationSeq; - void fetchAttachmentMetadata(from.workspaceSlug, forUuid, opts.getDownloadUrl) + setActivationPending(true); + void fetchAttachmentMetadata(fromWs, 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 @@ -629,32 +759,145 @@ export const AttachmentImage = Node.create({ // finds its own uuid in place and emits for a gesture two // swaps ago. if (activationSeq !== seq) return; + // DELETION, checked explicitly and BEFORE the result is read. + // It is the one invalidation the uuid comparison structurally + // cannot catch: a delete does not change which attachment the + // node points at, so a probe issued before it can resolve `ok` + // afterwards with `forUuid` still current — describing a row + // the server has since dropped. `canActivate()` restates this + // below; it is spelled out here because the branches between + // the two must not act on that answer either. + // + // The two guards happen to be equivalent TODAY — every path + // that sets `deleted` also hides the image — but nothing + // enforces that, and inferring "the row is gone" from "the + // placeholder is showing" would silently stop holding the + // moment a deletion state exists that does not hide. + if (deleted) return; + + // AUTHORITATIVE 404. The row is gone: latch the permanent + // placeholder (the same end state the deletion broadcast + // reaches) and open nothing. This runs even when the image is + // already hidden behind a retryable placeholder — upgrading a + // transient failure to a confirmed one is exactly what a 404 + // is for, so it deliberately precedes the presentability + // check below. + if (result.status === 'missing') { + latchMissing(forUuid); + return; + } + // NOT AUTHORITATIVE. Stay retryable, latch nothing, open + // nothing — and stop being a control that silently does + // nothing (see showTransientProbeFailure). + if (result.status === 'transient') { + showTransientProbeFailure(); + return; + } + + // A positively-known MIME. Record it: the semantics pass and + // the transform toolbar both read `knownMime`, and this + // activation is often the FIRST thing to learn it (the + // toolbar's own probe only runs once the node is selected). + // Without this the image would keep announcing "View image" + // for something that opens the panel. + // + // The SEMANTICS follow, and deliberately nothing else. The + // rotate/crop toolbar reads `knownMime` too, but refreshing + // it from here would settle its per-format gating earlier + // than it does today — a behaviour change this task's + // contract does not ask for. It is also unnecessary: a + // toolbar only exists once the node has been selected, and + // selection runs its own probe for the same uuid which + // refreshes on arrival. The window is a moment of staleness + // that closes itself. + knownMime = result.mime; + applyImageSemantics(); + + // The gesture-time gate, restated on state that may have + // moved: a load failure inside the await window hands over to + // the placeholder, and there is then no image to act on. 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 + // surface 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. + // + // Read ONCE, compared against the FULL captured address, and + // every emission below uses the CAPTURED values — never a + // re-read. The check and the emit are adjacent and + // synchronous; deferring either behind a timer or a microtask + // would reopen the window this closes. + // + // WHAT THIS FENCE DOES NOT CATCH, stated because it is a real + // gap and not an oversight: it compares VALUES, so an address + // that leaves and RETURNS (A→B→A) reads as unchanged. That is + // reachable — the pane's `ItemDetail` has no `{#key}` (PLAN-2105 + // / TASK-2112), so an A→B→A item switch keeps one host token, + // and the comment composer it owns is reused across it. + // + // It is left as-is deliberately, on two grounds. First, the + // outcome differs from what the fence exists to prevent: the + // user has NOT left the pane (they are back on it), and the + // gesture is NOT re-attributed (same node, same attachment, + // same host), so what opens is the image they clicked over the + // pane they are looking at. Compare the uuid A→B→A case just + // below, which IS fenced by `activationSeq` — there the + // SUBJECT of the gesture changes, which is a different and + // worse thing than a destination that round-trips to itself. + // + // Second, it is not fixable from inside this NodeView. Telling + // A→B→A from A needs a generation that advances on every + // address CHANGE, and this NodeView cannot observe one: it + // only READS the address (`AttachmentHostAddressReader` is a + // getter, with no subscription and no epoch), so a B that + // arrives and leaves between the two reads is invisible here + // by construction. Closing it means adding an epoch to + // `AttachmentHostAddress` and bumping it in every host — a + // cross-surface API change that also lands on the chip + // NodeView, which is outside this task's contract. Tracked as + // a fence-completeness follow-up rather than smuggled in here. const to = opts.address(); if ( - to.workspaceSlug !== from.workspaceSlug || - to.itemId !== from.itemId || - to.hostToken !== from.hostToken + to.workspaceSlug !== fromWs || + to.itemId !== fromItem || + to.hostToken !== fromHost ) { return; } + + // DR-16 / DR-7, on the resolved answer rather than the absence + // of one: `image/svg+xml` can carry active content, so it does + // not go to the viewer — but it is still an attachment the + // user just activated, and the options panel is the surface + // that owns everything the viewer will not take. A redirect, + // not a refusal. + // + // `filename` is null for the same reason it is on the viewer + // payload: the node's attrs carry none and the HEAD does not + // either. The panel fetches what it needs itself — its three + // metadata fields are nullable precisely for emitters like + // this one. + if (!canOpenInViewer(result.mime)) { + notifyAttachmentPanelOpen({ + attachmentId: forUuid, + itemId: fromItem, + hostToken: fromHost, + anchor: img, + filename: null, + mime_type: result.mime, + size_bytes: result.size, + }); + return; + } notifyViewerOpen({ attachmentId: forUuid, - workspaceSlug: from.workspaceSlug, - itemId: from.itemId, - hostToken: from.hostToken, + workspaceSlug: fromWs, + itemId: fromItem, + hostToken: fromHost, // 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 @@ -688,8 +931,19 @@ export const AttachmentImage = Node.create({ // 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; + // replaced it — nor clear a pending state the request that + // replaced it is still entitled to show. + if (activationSeq !== seq) return; + activating = false; + // The DOM write, unlike the latch release, is subject to + // this file's teardown rule: `destroyed` means every async + // continuation stops touching the node. Harmless in + // practice — the element is detached — but the `.then()` + // above fences on it and a finalizer that does not is the + // kind of asymmetry that stops being harmless the first + // time someone puts something real in here. + if (destroyed) return; + setActivationPending(false); }); } @@ -998,6 +1252,11 @@ export const AttachmentImage = Node.create({ // the user made two swaps ago. activationSeq += 1; activating = false; + // And the pending affordance goes with the latch: the bump + // above retires the old request's finalizer, so leaving + // `aria-busy` on would strand the NEW image as permanently + // busy. + setActivationPending(false); resetMissing(); if (newUuid) { loadImage(opts.getDownloadUrl(newUuid, 'thumb-md')); diff --git a/web/src/lib/components/editor/attachmentImageKeyboard.svelte.test.ts b/web/src/lib/components/editor/attachmentImageKeyboard.svelte.test.ts index f4425e9a..ed2e739d 100644 --- a/web/src/lib/components/editor/attachmentImageKeyboard.svelte.test.ts +++ b/web/src/lib/components/editor/attachmentImageKeyboard.svelte.test.ts @@ -40,8 +40,16 @@ 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> = []; +// Open-the-PANEL requests, same treatment. TASK-2434 makes this NodeView a +// producer on BOTH channels — a non-allowlisted MIME is redirected to the +// options panel rather than refused — so a spec that captured only the viewer +// channel could not tell a redirect from a silent drop, which is exactly the +// distinction the matrix is about. +const panelEmitted: Array> = []; vi.mock('$lib/attachments/events', () => ({ - notifyAttachmentPanelOpen: () => {}, + notifyAttachmentPanelOpen: (event: Record) => { + panelEmitted.push(event); + }, notifyViewerOpen: (event: Record) => { emitted.push(event); }, @@ -66,9 +74,16 @@ type ProbeResult = const probeMock = vi.fn<(ws?: string, uuid?: string) => Promise>(async () => ({ status: 'transient', })); +// The load-failure path's REVALIDATION, separable from the cached read above. +// It delegates to `probeMock` by default, so for almost every test the two are +// one mock and answer alike. Exactly one test needs them to differ: proving +// that a 404 reaching the ACTIVATION branch upgrades an already-hidden +// placeholder requires the error path's own probe not to be the thing that +// latches it, or the assertion could not tell the two routes apart. +const revalidateMock = vi.fn<(ws?: string, uuid?: string) => Promise>(); vi.mock('./attachment-metadata', () => ({ fetchAttachmentMetadata: (ws: string, uuid: string) => probeMock(ws, uuid), - revalidateAttachmentMetadata: (ws: string, uuid: string) => probeMock(ws, uuid), + revalidateAttachmentMetadata: (ws: string, uuid: string) => revalidateMock(ws, uuid), invalidateAttachmentMetadata: () => {}, mimeToFormat: () => null, })); @@ -133,7 +148,15 @@ function makeCommentEditor(element: HTMLElement): Editor { }); } -/** How many times activation actually fired. One request per open. */ +/** + * How many times activation asked for the VIEWER. One request per open. + * + * Deliberately not "how many times activation fired": since TASK-2434 an + * activation can instead land on the panel channel, which `panelEmitted` + * counts. Reading this as a count of activations would make every redirect + * look like a dropped gesture — the exact confusion the two arrays exist to + * keep apart. + */ function openCount(): number { return emitted.length; } @@ -208,7 +231,10 @@ describe('inline body image — keyboard activation (DR-12)', () => { // `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 }); + revalidateMock.mockClear(); + revalidateMock.mockImplementation((ws?: string, uuid?: string) => probeMock(ws, uuid)); emitted.length = 0; + panelEmitted.length = 0; host = document.body.appendChild(document.createElement('div')); target = host.appendChild(document.createElement('div')); }); @@ -218,6 +244,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { editor = undefined; host.remove(); emitted.length = 0; + panelEmitted.length = 0; document.querySelectorAll('.timeline-viewer-stub').forEach((d) => d.remove()); }); @@ -465,7 +492,7 @@ describe('inline body image — keyboard activation (DR-12)', () => { expect(await opened()).toBe(0); }); - it('refuses a probed non-raster type through the KEYBOARD, not just the mouse', async () => { + it('keeps a probed non-raster type OUT of the viewer 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. @@ -488,13 +515,571 @@ describe('inline body image — keyboard activation (DR-12)', () => { press(image(), 'Enter'); expect(await opened()).toBe(0); - // And it stops being a focus stop at all, rather than announcing itself - // as a button that does nothing. - expect(image().getAttribute('role')).toBeNull(); - expect(image().getAttribute('tabindex')).toBeNull(); - expect(image().getAttribute('aria-label')).toBeNull(); + // TASK-2434: it is a REDIRECT, not a refusal. Asserting only "the viewer + // did not open" would be satisfied by an implementation that dropped the + // gesture on the floor — which is what this used to do, and the dead + // focus stop the matrix exists to close. + expect(panelEmitted).toHaveLength(1); + // So it stays a real control, and it says where it goes. + expect(image().getAttribute('role')).toBe('button'); + expect(image().getAttribute('tabindex')).toBe('0'); + expect(image().getAttribute('aria-label')).toBe('Attachment options: A diagram'); }); + it('redirects a non-allowlisted MIME to the panel, with the whole payload', async () => { + // The `ok` + not-allowlisted arm of the matrix, asserted on what is + // EMITTED. A count alone is satisfied by an event of any shape, and the + // panel's routing fields are exactly what a producer gets wrong: the + // address decides which of two mounted hosts opens it, and the three + // metadata fields are what the panel renders before its own fetch lands. + probeMock.mockResolvedValue({ status: 'ok', mime: 'application/pdf', size: 1234 }); + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + expect(await opened()).toBe(0); + + expect(panelEmitted).toEqual([ + { + attachmentId: 'uuid-1', + itemId: 'item-A', + hostToken: 'apanel-1', + anchor: img, + // No filename anywhere on this surface — the node's attrs carry + // none and the HEAD does not either. Null, not a fabricated one. + filename: null, + mime_type: 'application/pdf', + size_bytes: 1234, + }, + ]); + // It ASKED. A payload alone is satisfiable by an implementation that + // skipped the probe and assumed a MIME — which is precisely the gate + // this whole path exists to close. + expect(probeMock).toHaveBeenCalled(); + // And the pending affordance is cleared on THIS branch too. The + // finalizer is shared, but a clear moved into the viewer branch would + // leave every redirect permanently `aria-busy`. + expect(img.getAttribute('aria-busy')).toBeNull(); + expect(img.style.cursor).toBe(''); + }); + + it('keeps redirecting on every activation, not just the first', async () => { + // The regression the semantics rewrite exists to prevent, and the one an + // attribute-only assertion would miss entirely. Activation now WRITES the + // resolved MIME onto the node. If the activation gate still refused a + // known non-allowlisted MIME — as it did before this task — the first tap + // would open the panel and every tap after it would silently do nothing, + // because the gate would be reading the answer the first tap recorded. + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml', size: 10 }); + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + await opened(); + expect(panelEmitted).toHaveLength(1); + + // The premise: the MIME really is on the node now (the label proves it, + // and it is the same state the gate would have been reading). + expect(img.getAttribute('aria-label')).toBe('Attachment options: A diagram'); + + press(img, 'Enter'); + await opened(); + click(img); + await opened(); + + expect(panelEmitted).toHaveLength(3); + expect(await opened()).toBe(0); + // The LAST one, not just the count: a redirect that kept firing with a + // payload frozen at the first gesture would be a count of three and a + // panel that opens on whatever the node used to be. + expect(panelEmitted[2]).toMatchObject({ + attachmentId: 'uuid-1', + itemId: 'item-A', + hostToken: 'apanel-1', + anchor: img, + mime_type: 'image/svg+xml', + size_bytes: 10, + }); + }); + + it('never opens the panel for a MIME the viewer WILL take', async () => { + // The other direction of the redirect, which a one-sided spec would leave + // free: an implementation that emitted BOTH events would satisfy every + // panel assertion above and open two surfaces from one gesture. + editor = makeEditor(target); + + press(image(), 'Enter'); + + expect(await opened()).toBe(1); + expect(panelEmitted).toEqual([]); + // Reached by ASKING, not by assuming: an implementation that skipped the + // probe for images it liked the look of would pass the two assertions + // above and be exactly the bypass TASK-2433 closed. + expect(probeMock).toHaveBeenCalled(); + }); + + it('latches the permanent placeholder on an authoritative 404, and opens nothing', async () => { + // The `missing` arm. `missing` is the ONLY result that may latch (DR-17), + // and the latch is what keeps editor undo from resurrecting a deleted + // attachment as a live-looking node. + probeMock.mockResolvedValue({ status: 'missing' }); + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + await opened(); + + expect(await opened()).toBe(0); + expect(panelEmitted).toEqual([]); + + const placeholder = target.querySelector('.attachment-missing'); + expect(img.style.display).toBe('none'); + expect(placeholder?.style.display).not.toBe('none'); + // PERMANENT, and the copy says so: a deleted row is not retryable, so the + // placeholder is deliberately NOT a control. A retryable placeholder here + // would invite a click that can only 404. + expect(placeholder?.title).toBe('This attachment has been deleted'); + expect(placeholder?.getAttribute('role')).toBeNull(); + expect(placeholder?.getAttribute('tabindex')).toBeNull(); + // And the latch holds against BOTH ways back. A click cannot retry it... + placeholder?.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(img.style.display).toBe('none'); + // ...and neither can a `load` that was already in flight when the 404 + // landed. The implementation stops that twice over — the latch detaches + // the listener AND `resetMissing` refuses to run once `deleted` — so this + // pins the OUTCOME rather than either mechanism, which is the honest + // claim to make about a guard with a redundant partner. + img.dispatchEvent(new Event('load')); + expect(img.style.display).toBe('none'); + expect(img.getAttribute('aria-busy')).toBeNull(); + }); + + it('leaves a transient failure RETRYABLE — no open, no latch', async () => { + // The `transient` arm, and the dead focus stop TASK-2433 left behind: it + // emitted nothing and did nothing, so a focused image announced itself as + // a button and silently swallowed every press. + probeMock.mockResolvedValue({ status: 'transient' }); + editor = makeEditor(target); + const img = image(); + img.focus(); + + press(img, 'Enter'); + await opened(); + + expect(await opened()).toBe(0); + expect(panelEmitted).toEqual([]); + + const placeholder = target.querySelector('.attachment-missing'); + expect(img.style.display).toBe('none'); + expect(placeholder?.style.display).not.toBe('none'); + // RETRYABLE, not latched — the copy, the semantics and the focus all say + // the same thing. This is the assertion that separates it from `missing`: + // an implementation that treated the two alike would pass every "did not + // open" check above and permanently strand a row that is perfectly fine. + expect(placeholder?.title).toContain('Click to retry'); + expect(placeholder?.getAttribute('role')).toBe('button'); + expect(placeholder?.getAttribute('tabindex')).toBe('0'); + // The keypress that reported the failure must not drop focus to . + expect(document.activeElement).toBe(placeholder); + + // And the latch really is absent: Retry restores the image, which + // `missing` above cannot do. + expect(img.getAttribute('aria-busy')).toBeNull(); + const srcBefore = img.getAttribute('src'); + placeholder?.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(img.style.display).toBe(''); + expect(placeholder?.style.display).toBe('none'); + // Retry means a NEW REQUEST, not just an un-hidden element: without the + // cache-busting query the browser replays the failed entry and the retry + // is theatre. Restoring the DOM alone would pass the two lines above. + expect(img.getAttribute('src')).not.toBe(srcBefore); + expect(img.getAttribute('src')).toContain('retry='); + }); + + it('does not let a transient failure latch permanently across a recovery', async () => { + // DR-17's rule stated over TIME rather than over one result: only an + // authoritative 404 latches, so a blip followed by a healthy probe must + // leave the image fully openable again. An implementation that reused the + // `missing` path for `transient` would fail here and NOWHERE else — every + // single-shot assertion above would still pass. + probeMock.mockResolvedValue({ status: 'transient' }); + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + await opened(); + const placeholder = target.querySelector('.attachment-missing'); + // The premise, and it is load-bearing: without it this passes against an + // implementation whose `transient` arm does nothing at all — there would + // be no latch to survive, and the recovery below would prove nothing. + expect(img.style.display).toBe('none'); + expect(placeholder?.getAttribute('role')).toBe('button'); + placeholder?.dispatchEvent(new MouseEvent('click', { bubbles: true })); + + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/png', size: 4096 }); + press(img, 'Enter'); + + expect(await opened()).toBe(1); + }); + + it('shows a pending affordance while the MIME is still resolving', async () => { + // A cold probe is a round trip, and a click that does nothing for its + // duration reads as broken. Deliberately minimal: `aria-busy` and a + // cursor, no new chrome. + let release: () => void = () => {}; + probeMock.mockImplementation( + () => + new Promise((resolve) => { + release = () => resolve({ status: 'ok', mime: 'image/png', size: 4096 }); + }) + ); + editor = makeEditor(target); + const img = image(); + expect(img.getAttribute('aria-busy')).toBeNull(); + + press(img, 'Enter'); + + // Set SYNCHRONOUSLY with the gesture — a pending state that waits for a + // tick is not covering the wait it exists for. + expect(img.getAttribute('aria-busy')).toBe('true'); + expect(img.style.cursor).toBe('progress'); + + release(); + expect(await opened()).toBe(1); + // And cleared, or the image announces itself as permanently busy. + expect(img.getAttribute('aria-busy')).toBeNull(); + expect(img.style.cursor).toBe(''); + }); + + it('clears the pending affordance when a swap supersedes the request', async () => { + // The finalizer only runs for the request that still holds the latch, so + // a swap has to clear the pending state itself — otherwise a HEAD that + // never settles leaves the NEW image permanently `aria-busy`. + probeMock.mockImplementation(() => new Promise(() => {})); + editor = makeEditor(target); + press(image(), 'Enter'); + expect(image().getAttribute('aria-busy')).toBe('true'); + + // The SAME element across the swap: this NodeView deliberately survives a + // uuid change, and reacquiring by selector would let a destroy/recreate + // implementation pass without ever clearing anything. + const before = image(); + editor.commands.setNodeSelection(1); + editor.commands.updateAttributes('attachmentImage', { uuid: 'uuid-2' }); + + expect(image()).toBe(before); + expect(before.getAttribute('aria-busy')).toBeNull(); + expect(before.style.cursor).toBe(''); + }); + + it('opens NOTHING when a delete lands mid-probe and the answer comes back ok', async () => { + // The race the uuid comparison structurally cannot catch: a delete does + // not change which attachment the node points at, so `forUuid` is still + // current when the probe resolves — with a perfectly valid `ok` describing + // a row the server has since dropped. + // + // The implementation states this TWICE — an explicit `deleted` re-check + // and `canActivate()`'s hidden-placeholder clause — and mutation testing + // confirms this spec fails only when BOTH are gone. That is the honest + // claim: it pins the BEHAVIOUR, not either line. (The two are equivalent + // today only because every path that sets `deleted` also hides the image; + // nothing enforces that, which is why the check is stated on its own + // terms rather than inferred from the placeholder.) + // + // A test that let the probe resolve BEFORE the delete would pass against + // an implementation with no `deleted` check at all, so the ordering here + // is the whole point: the answer is held until after the broadcast. + let release: () => void = () => {}; + probeMock.mockImplementation( + () => + new Promise((resolve) => { + release = () => resolve({ status: 'ok', mime: 'image/png', size: 4096 }); + }) + ); + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + for (const fn of deletionListeners) fn('uuid-1'); + // The premise: the node still points at the very attachment the probe is + // about. Without this the drop could be the uuid fence doing the work. + expect(img.getAttribute('data-attachment-id')).toBe('uuid-1'); + release(); + + expect(await opened()).toBe(0); + // BOTH channels. A `deleted` check placed after the allowlist branch + // would still leak the redirect. + expect(panelEmitted).toEqual([]); + }); + + it('opens NO PANEL either when a delete lands mid-probe on a non-raster type', async () => { + // The same race down the redirect branch, which is new surface: the + // `missing`/`transient`/`ok` split gave the continuation three more places + // to act on a row that is gone. + let release: () => void = () => {}; + probeMock.mockImplementation( + () => + new Promise((resolve) => { + release = () => resolve({ status: 'ok', mime: 'image/svg+xml', size: 10 }); + }) + ); + editor = makeEditor(target); + + press(image(), 'Enter'); + for (const fn of deletionListeners) fn('uuid-1'); + release(); + + await opened(); + expect(panelEmitted).toEqual([]); + expect(await opened()).toBe(0); + + // THE CONTROL, and it is the whole test: `expect no panel` is satisfied + // by an implementation that has no redirect at all — the pre-TASK-2434 + // binary refusal passes it outright. So prove the branch works on an + // undeleted node under the identical probe. + editor.destroy(); + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml', size: 10 }); + editor = makeEditor(target); + press(image(), 'Enter'); + await opened(); + expect(panelEmitted).toHaveLength(1); + }); + + // The address fence, restated on the PANEL branch. It is a second emission + // site with its own copy of the captured address, so the three comparisons + // have to hold for it independently — a fence that only guarded the viewer + // would let a redirect open a panel over a pane the user has left. + 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 PANEL redirect when the ${label} moves mid-resolution`, async () => { + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml', size: 10 }); + editor = makeCommentEditor(target); + + press(image(), 'Enter'); + address = { ...moved }; + + await opened(); + expect(panelEmitted).toEqual([]); + + // The control: settled, the next gesture DOES redirect — so the drop + // above is the fence, not a branch that never worked. + press(image(), 'Enter'); + await opened(); + expect(panelEmitted).toHaveLength(1); + expect(panelEmitted[0].itemId).toBe(moved.itemId); + expect(panelEmitted[0].hostToken).toBe(moved.hostToken); + }); + } + + it('emits with the address CAPTURED at the gesture, even if the reader mutates in place', async () => { + // The fence holds three PRIMITIVES, not the object the reader returned. + // Both live readers build a fresh object literal per call, so holding the + // reference would be safe — for that reason alone, which no interface + // states. This pins the independence: a reader that hands back ONE object + // and mutates it in place would, if the fence held the reference, rewrite + // the snapshot it compares against and pass unconditionally. + const shared = { workspaceSlug: 'ws', itemId: 'item-A', hostToken: 'apanel-1' }; + address = shared; + let release: () => void = () => {}; + probeMock.mockImplementation( + () => + new Promise((resolve) => { + release = () => resolve({ status: 'ok', mime: 'image/png', size: 4096 }); + }) + ); + editor = makeCommentEditor(target); + + press(image(), 'Enter'); + // The host "moves" by mutating the object the reader keeps handing out. + shared.itemId = 'item-B'; + shared.hostToken = 'apanel-2'; + release(); + + // Dropped. Holding the reference would compare item-B against item-B. + expect(await opened()).toBe(0); + }); + + it('DOES emit when the address leaves and returns (A→B→A) — documented, accepted', async () => { + // The fence compares VALUES, so an address that round-trips reads as + // unchanged. Reachable: the pane's `ItemDetail` has no `{#key}` + // (PLAN-2105 / TASK-2112), so an A→B→A item switch keeps one host token + // and the composer it owns is reused across it. + // + // PINNED AS THE CURRENT BEHAVIOUR, not asserted as ideal. It is accepted + // because the outcome differs from what the fence prevents — the user is + // back on the pane, and the gesture is not re-attributed: the same node, + // the same attachment, the same host. Contrast the uuid A→B→A case below, + // which IS dropped, because there the SUBJECT of the gesture changed. + // + // Telling A→B→A from A needs an epoch on `AttachmentHostAddress` that + // every host bumps; this NodeView only READS the address, so a B between + // the two reads is invisible to it by construction. When that epoch + // lands, this expectation flips to 0 and this comment is its changelog. + let release: () => void = () => {}; + probeMock.mockImplementation( + () => + new Promise((resolve) => { + release = () => resolve({ status: 'ok', mime: 'image/png', size: 4096 }); + }) + ); + editor = makeCommentEditor(target); + const original = { ...address }; + + press(image(), 'Enter'); + address = { workspaceSlug: 'ws', itemId: 'item-B', hostToken: 'apanel-1' }; + address = { ...original }; + release(); + + expect(await opened()).toBe(1); + // And it emits at A — not at the B it passed through. + expect(emitted[0].itemId).toBe(original.itemId); + expect(emitted[0].hostToken).toBe(original.hostToken); + }); + + it('UPGRADES an already-hidden retryable placeholder when a 404 arrives', async () => { + // The transient→missing transition, and the only DR-17 boundary that had + // no test. The mutant it exists for is exact: an implementation that + // processed `missing` only when `canActivate()` is true would pass every + // other test in this file, because the placeholder-showing state is + // precisely where `canActivate()` is already false. That is why the + // `missing` branch deliberately runs BEFORE the presentability check — a + // 404 is authoritative whatever the node happens to be showing. + // + // Reaching that state takes care. An activation cannot START while the + // placeholder is up (the gesture-time gate refuses), and Retry UN-hides + // the image — so a naive "fail, retry, 404" sequence lands the 404 on a + // VISIBLE image and tests nothing new. The image has to go behind the + // placeholder DURING the await, which is what the load `error` does. + let release: () => void = () => {}; + probeMock.mockImplementation( + () => + new Promise((resolve) => { + release = () => resolve({ status: 'missing' }); + }) + ); + // The error path runs its OWN revalidation, which latches on a 404 too. + // Keeping it transient is what makes the latch below attributable to the + // activation branch rather than to the load failure's probe. + revalidateMock.mockResolvedValue({ status: 'transient' }); + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + img.dispatchEvent(new Event('error')); + await opened(); + + const placeholder = target.querySelector('.attachment-missing'); + // The premise, and the whole point: hidden, and still RETRYABLE. Without + // both, the 404 below would be landing on a state some other test covers. + expect(img.style.display).toBe('none'); + expect(placeholder?.getAttribute('role')).toBe('button'); + expect(placeholder?.title).toContain('Click to retry'); + + // The 404 lands while the placeholder is up. + release(); + await opened(); + + // Upgraded: permanent, inert, no longer inviting a retry that can only + // 404 again. + expect(placeholder?.title).toBe('This attachment has been deleted'); + expect(placeholder?.getAttribute('role')).toBeNull(); + expect(placeholder?.getAttribute('tabindex')).toBeNull(); + // And the latch holds — a retry click cannot undo an authoritative 404. + placeholder?.dispatchEvent(new MouseEvent('click', { bubbles: true })); + expect(img.style.display).toBe('none'); + expect(await opened()).toBe(0); + expect(panelEmitted).toEqual([]); + }); + + it('opens nothing when a load failure hides the image mid-probe and the MIME is FINE', async () => { + // The post-await `canActivate()` guard, which nothing else reaches. The + // spec below ("will not ACTIVATE while the image is showing a + // load-failure placeholder") fails the load FIRST and gestures after, so + // it is stopped by the GESTURE-time gate and would pass with the + // post-await check deleted entirely. + // + // This is the other order, and it is the reachable one: the gesture lands + // on a healthy image, the load fails while the HEAD is in flight, and the + // HEAD comes back with a perfectly good `image/png`. Nothing is deleted + // and nothing is missing — the only reason not to open is that there is + // no longer an image on screen to open. Emitting here would put a viewer + // over a placeholder. + let release: () => void = () => {}; + probeMock.mockImplementation( + () => + new Promise((resolve) => { + release = () => resolve({ status: 'ok', mime: 'image/png', size: 4096 }); + }) + ); + // The error path's own revalidation stays transient, so the placeholder + // remains the RETRYABLE one — this test is about a healthy MIME meeting a + // hidden image, not about a latch. + revalidateMock.mockResolvedValue({ status: 'transient' }); + editor = makeEditor(target); + const img = image(); + + press(img, 'Enter'); + img.dispatchEvent(new Event('error')); + const placeholder = target.querySelector('.attachment-missing'); + // The premise: hidden, retryable, NOT deleted — so every other guard in + // the continuation (uuid, generation, `deleted`) is satisfied and only + // presentability is left to do the work. + expect(img.style.display).toBe('none'); + expect(placeholder?.getAttribute('role')).toBe('button'); + + release(); + await opened(); + + expect(await opened()).toBe(0); + expect(panelEmitted).toEqual([]); + // And it stayed retryable: refusing to open must not cost the user the + // retry affordance. + expect(placeholder?.getAttribute('role')).toBe('button'); + }); + + it('emits SYNCHRONOUSLY with the fence check, not on a later turn', async () => { + // The fence's correctness rests on the check and the emit being + // adjacent: anything queued between them — a timer, a microtask hop — is + // a new window for the address to go stale across, which is the whole + // hazard the fence exists for. Nothing else in this file would notice a + // refactor that queued delivery, because every other assertion goes + // through `opened()`, which waits two MACROtask turns and would happily + // observe a `setTimeout(0)` emission. + // + // So: resolve the probe and read the count after a bounded number of + // MICROtasks. A delivery queued behind a timer cannot be observed here; + // a synchronous one always is. + let release: () => void = () => {}; + probeMock.mockImplementation( + () => + new Promise((resolve) => { + release = () => resolve({ status: 'ok', mime: 'image/png', size: 4096 }); + }) + ); + editor = makeEditor(target); + + press(image(), 'Enter'); + release(); + + // Three microtasks is generous for the implementation's own `.then` + // chain and still strictly inside the current macrotask. + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + expect(emitted).toHaveLength(1); + }); + + // NOTE, deliberately not a test: "stamps the CAPTURED address rather than a + // re-read one" is UNOBSERVABLE from outside this function, and writing a test + // that appears to prove it would be worse than leaving the gap named. The + // fence refuses to emit whenever the two differ, so every emission happens in + // a state where captured and re-read are equal — a spec that moved the address + // away and back would assert the same values under either implementation. The + // enforceable half is the drop, and that is asserted per field above. + 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 @@ -520,6 +1105,10 @@ describe('inline body image — keyboard activation (DR-12)', () => { // And it did ask — a refusal reached by never probing at all would be // the same count for the wrong reason. expect(probeMock).toHaveBeenCalled(); + // TASK-2434: and neither gesture was DROPPED. Asserting only "no viewer" + // is satisfied by the silent return this task replaced; the unprobed + // gesture has to reach the panel exactly as the probed one does. + expect(panelEmitted).toHaveLength(2); }); it('emits once when two gestures land inside one resolution window', async () => { @@ -587,6 +1176,10 @@ describe('inline body image — keyboard activation (DR-12)', () => { press(image(), 'Enter'); expect(await opened()).toBe(0); + // And nothing else opened either — `missing` is the one result with no + // destination at all. An implementation that fell through to the panel + // redirect would offer options for a row that is gone. + expect(panelEmitted).toEqual([]); }); it('drops the request when the NodeView is torn down mid-resolution', async () => { @@ -629,6 +1222,10 @@ describe('inline body image — keyboard activation (DR-12)', () => { // working. press(image(), 'Enter'); expect(await opened()).toBe(1); + // The probe ran under the address the GESTURE happened at — the + // workspace keys the metadata cache, so probing under the wrong one + // answers a question about a different workspace's row. + expect(probeMock).toHaveBeenLastCalledWith(moved.workspaceSlug, 'uuid-1'); expect(emitted[0].workspaceSlug).toBe(moved.workspaceSlug); expect(emitted[0].itemId).toBe(moved.itemId); expect(emitted[0].hostToken).toBe(moved.hostToken); diff --git a/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts b/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts index 7143c620..6bbd5a4e 100644 --- a/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts +++ b/web/src/lib/components/editor/attachmentImageViewerHost.svelte.test.ts @@ -24,6 +24,11 @@ import { Editor } from '@tiptap/core'; import StarterKit from '@tiptap/starter-kit'; import { __resetViewerBackdropForTests } from '$lib/a11y/viewerBackdrop'; import { _resetEscapeStackForTests } from '$lib/stores/escapeStack'; +import { + isAttachmentPanelEventForHost, + registerAttachmentPanelListener, + type AttachmentPanelOpenEvent, +} from '$lib/attachments/events'; const UUID = '11111111-1111-4111-8111-111111111111'; const ITEM_ID = 'item-A'; @@ -43,6 +48,9 @@ const { AttachmentImage } = await import('./attachment-image'); const { default: AttachmentViewerHost } = await import( '$lib/components/attachments/AttachmentViewerHost.svelte' ); +const { default: AttachmentPanelHost } = await import( + '$lib/components/attachments/AttachmentPanelHost.svelte' +); let address = { workspaceSlug: 'ws', itemId: ITEM_ID, hostToken: HOST_TOKEN }; @@ -194,6 +202,91 @@ describe('inline image → viewer host → Lightbox', () => { expect(viewers()).toHaveLength(0); }); + it('REDIRECTS a non-allowlisted type into the real panel host', async () => { + // The whole route for the redirect arm, with nothing between the NodeView + // and the panel stubbed: the real bus, the real `AttachmentPanelHost`, the + // real panel it mounts. The producer specs next door mock the bus, so a + // host that stopped consuming — or an address that could not route — + // would leave them green and leave the user with an image that does + // nothing, which is the exact failure this task replaced. + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml', size: 100 } as never); + mountHost({ itemId: ITEM_ID, hostToken: HOST_TOKEN }); + const panelHost = mount(AttachmentPanelHost, { + target: hostTarget, + props: { + wsSlug: 'ws', + itemId: ITEM_ID, + hostToken: HOST_TOKEN, + mutationsEnabled: false, + }, + }) as Record; + try { + editor = makeEditor(editorTarget); + + image().dispatchEvent( + new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 }) + ); + await settle(); + + // No viewer — the security half. + expect(viewers()).toHaveLength(0); + // And a real panel on screen, for the attachment that was activated — + // the completeness half. A redirect nobody consumes is still a tap + // that does nothing. + const panel = document.body.querySelector('[role="menu"] .ap-header'); + expect(panel).not.toBeNull(); + // And it is about THIS attachment, not merely present: the MIME the + // probe returned and a download target carrying the uuid. A panel that + // opened on the wrong row would satisfy a presence check. + expect(panel?.querySelector('.ap-meta')?.getAttribute('title')).toBe('image/svg+xml'); + expect( + document.body.querySelector('[role="menu"] a[download]')?.getAttribute('href') + ).toContain(UUID); + } finally { + unmount(panelHost); + } + }); + + it('REDIRECTS a non-allowlisted type onto the real panel channel', async () => { + // TASK-2434's redirect, asserted through the REAL bus rather than a mock. + // The producer specs next door mock `notifyAttachmentPanelOpen`, so they + // see the call and are blind to what the channel does with it — and the + // channel drops any emission it judges unaddressable. An event that never + // leaves the bus is indistinguishable, from the producer's side, from the + // silent refusal this task replaced. + const seen: AttachmentPanelOpenEvent[] = []; + const dispose = registerAttachmentPanelListener((e) => seen.push(e)); + try { + probeMock.mockResolvedValue({ status: 'ok', mime: 'image/svg+xml', size: 100 } as never); + mountHost({ itemId: ITEM_ID, hostToken: HOST_TOKEN }); + editor = makeEditor(editorTarget); + + image().dispatchEvent( + new MouseEvent('click', { bubbles: true, cancelable: true, detail: 1 }) + ); + await settle(); + + // No viewer — the security half. + expect(viewers()).toHaveLength(0); + // And it went SOMEWHERE — the completeness half. + expect(seen).toHaveLength(1); + expect(seen[0].attachmentId).toBe(UUID); + expect(seen[0].mime_type).toBe('image/svg+xml'); + // Addressed well enough for a host to claim it. The channel's own + // predicate, not a re-implementation of it: a payload that reached a + // raw subscriber but that no host would match is still a tap that + // does nothing. + expect( + isAttachmentPanelEventForHost(seen[0], { itemId: ITEM_ID, hostToken: HOST_TOKEN }) + ).toBe(true); + expect( + isAttachmentPanelEventForHost(seen[0], { itemId: ITEM_ID, hostToken: 'another-mount' }) + ).toBe(false); + } finally { + dispose(); + } + }); + 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