From 884ddc575e79692c30f93fa56e7ed13e828e953c Mon Sep 17 00:00:00 2001 From: xarmian Date: Wed, 5 Aug 2026 05:36:00 +0000 Subject: [PATCH] feat(attachments): add viewer open channel and per-host viewer host (TASK-2428) Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC --- web/src/lib/attachments/events.test.ts | 234 ++++++++++ web/src/lib/attachments/events.ts | 150 +++++++ .../lib/attachments/viewerResource.svelte.ts | 59 +++ .../lib/attachments/viewerResource.test.ts | 82 ++++ web/src/lib/attachments/viewerResource.ts | 48 ++ .../viewerResourceGen.svelte.test.ts | 185 ++++++++ .../attachments/AttachmentViewerHost.svelte | 170 +++++++ .../AttachmentViewerHost.svelte.test.ts | 419 ++++++++++++++++++ .../AttachmentViewerHostClose.svelte.test.ts | 155 +++++++ .../attachments/fixtures/LightboxStub.svelte | 27 ++ .../attachments/fixtures/lightboxStub.ts | 16 + .../lib/components/items/ItemDetail.svelte | 55 +++ 12 files changed, 1600 insertions(+) create mode 100644 web/src/lib/attachments/viewerResource.svelte.ts create mode 100644 web/src/lib/attachments/viewerResource.test.ts create mode 100644 web/src/lib/attachments/viewerResource.ts create mode 100644 web/src/lib/attachments/viewerResourceGen.svelte.test.ts create mode 100644 web/src/lib/components/attachments/AttachmentViewerHost.svelte create mode 100644 web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts create mode 100644 web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts create mode 100644 web/src/lib/components/attachments/fixtures/LightboxStub.svelte create mode 100644 web/src/lib/components/attachments/fixtures/lightboxStub.ts diff --git a/web/src/lib/attachments/events.test.ts b/web/src/lib/attachments/events.test.ts index fe98a63b..b91f8926 100644 --- a/web/src/lib/attachments/events.test.ts +++ b/web/src/lib/attachments/events.test.ts @@ -2,9 +2,14 @@ import { describe, expect, it } from 'vitest'; import { createAttachmentHostToken, isAttachmentPanelEventForHost, + isAttachmentViewerEventForHost, notifyAttachmentPanelOpen, + notifyViewerOpen, registerAttachmentPanelListener, + registerAttachmentViewerListener, type AttachmentPanelOpenEvent, + type AttachmentViewerOpenEvent, + type LightboxImage, } from './events'; /** @@ -166,3 +171,232 @@ describe('the panel channel with two live hosts', () => { expect(seen).toHaveLength(1); }); }); + +/** + * The addressing layer for the image viewer (PLAN-2392 phase 3a / TASK-2428). + * + * Same shape of test as the panel channel above, and for the same reason: the + * bus is module-global while `ItemDetail` is mounted twice (master + peeked + * pane), so every case below has a concrete two-viewer bug behind it. The two + * channels are exercised separately because they are separate predicates — + * a shared rule stated twice is exactly what regresses in one place only. + */ +function image(over: Partial = {}): LightboxImage { + return { + id: 'att-1', + alt: 'a diagram', + filename: 'diagram.png', + mime_type: 'image/png', + size_bytes: 4096, + width: 800, + height: 600, + ...over, + }; +} + +function viewerEvent(over: Partial = {}): AttachmentViewerOpenEvent { + return { + attachmentId: 'att-1', + workspaceSlug: 'ws-1', + itemId: 'item-1', + hostToken: 'host-a', + images: [image()], + index: 0, + invoker: null, + ...over, + }; +} + +describe('isAttachmentViewerEventForHost', () => { + const host = { itemId: 'item-1', hostToken: 'host-a' }; + + it('matches when BOTH the item and the token are the host’s', () => { + expect(isAttachmentViewerEventForHost(viewerEvent(), host)).toBe(true); + }); + + it('ignores an event that matches only the item (the two-panes-one-item case)', () => { + expect(isAttachmentViewerEventForHost(viewerEvent({ hostToken: 'host-b' }), host)).toBe( + false + ); + }); + + it('ignores an event that matches only the token', () => { + expect(isAttachmentViewerEventForHost(viewerEvent({ itemId: 'item-2' }), host)).toBe(false); + }); + + it('never matches on a missing half, on either side', () => { + // Two absences are not a match: a surface given no token must not be + // able to address every host at once, and a host without one must not + // consume unaddressed events. + expect(isAttachmentViewerEventForHost(viewerEvent({ hostToken: '' }), host)).toBe(false); + expect(isAttachmentViewerEventForHost(viewerEvent({ itemId: '' }), host)).toBe(false); + expect( + isAttachmentViewerEventForHost(viewerEvent(), { itemId: 'item-1', hostToken: '' }) + ).toBe(false); + expect(isAttachmentViewerEventForHost(viewerEvent(), { itemId: null, hostToken: null })).toBe( + false + ); + expect( + isAttachmentViewerEventForHost(viewerEvent({ hostToken: '', itemId: '' }), { + itemId: '', + hostToken: '', + }) + ).toBe(false); + }); + + it('tolerates a null/undefined event', () => { + expect(isAttachmentViewerEventForHost(null, host)).toBe(false); + expect(isAttachmentViewerEventForHost(undefined, host)).toBe(false); + }); +}); + +describe('viewer open channel', () => { + it('delivers to the addressed host only, with two hosts subscribed', () => { + const master = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + const peeked = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + const masterSeen: AttachmentViewerOpenEvent[] = []; + const peekedSeen: AttachmentViewerOpenEvent[] = []; + const offMaster = registerAttachmentViewerListener((e) => { + if (isAttachmentViewerEventForHost(e, master)) masterSeen.push(e); + }); + const offPeeked = registerAttachmentViewerListener((e) => { + if (isAttachmentViewerEventForHost(e, peeked)) peekedSeen.push(e); + }); + try { + notifyViewerOpen(viewerEvent({ hostToken: peeked.hostToken })); + expect(masterSeen).toHaveLength(0); + expect(peekedSeen).toHaveLength(1); + + notifyViewerOpen(viewerEvent({ attachmentId: 'att-2', hostToken: master.hostToken })); + expect(masterSeen).toHaveLength(1); + expect(masterSeen[0].attachmentId).toBe('att-2'); + expect(peekedSeen).toHaveLength(1); + } finally { + offMaster(); + offPeeked(); + } + }); + + it('carries the emit-time workspace, the set, the index and the invoker through unchanged', () => { + // The workspace is CAPTURED, not resolved by the host: the pane can + // switch workspace without remounting, so a host that read it live + // could serve a viewer opened in ws1 from ws2's endpoint. + const host = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + // 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' })]; + let received: AttachmentViewerOpenEvent | null = null; + const off = registerAttachmentViewerListener((e) => { + if (isAttachmentViewerEventForHost(e, host)) received = e; + }); + try { + notifyViewerOpen( + viewerEvent({ + attachmentId: 'att-2', + workspaceSlug: 'other-ws', + hostToken: host.hostToken, + images, + index: 1, + invoker, + }) + ); + } finally { + off(); + } + const got = received as AttachmentViewerOpenEvent | null; + expect(got).not.toBeNull(); + expect(got!.workspaceSlug).toBe('other-ws'); + expect(got!.images).toEqual(images); + expect(got!.index).toBe(1); + // The event's own invariant: the index names the attachment it opened on. + expect(got!.images[got!.index]?.id).toBe(got!.attachmentId); + 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. + const host = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + let received: AttachmentViewerOpenEvent | null = null; + const off = registerAttachmentViewerListener((e) => { + if (isAttachmentViewerEventForHost(e, host)) received = e; + }); + try { + notifyViewerOpen( + viewerEvent({ + hostToken: host.hostToken, + images: [ + image({ + filename: null, + mime_type: null, + size_bytes: null, + width: null, + height: null, + }), + ], + }) + ); + } finally { + off(); + } + const got = received as AttachmentViewerOpenEvent | null; + expect(got).not.toBeNull(); + expect(got!.images[0].filename).toBeNull(); + expect(got!.images[0].width).toBeNull(); + }); + + it('drops an unaddressable or empty emission rather than broadcasting it', () => { + const seen: AttachmentViewerOpenEvent[] = []; + const off = registerAttachmentViewerListener((e) => seen.push(e)); + try { + notifyViewerOpen(viewerEvent({ hostToken: '' })); + notifyViewerOpen(viewerEvent({ itemId: '' })); + notifyViewerOpen(viewerEvent({ attachmentId: '' })); + // A full-screen viewer with nothing to show is worse than no viewer. + notifyViewerOpen(viewerEvent({ images: [] })); + // The slug is a path segment of every image URL, and the host does + // not substitute its own: without it the viewer opens on 404s. + notifyViewerOpen(viewerEvent({ workspaceSlug: '' })); + } finally { + off(); + } + expect(seen).toHaveLength(0); + }); + + it('returns a disposer that stops delivery', () => { + const host = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + const seen: AttachmentViewerOpenEvent[] = []; + const off = registerAttachmentViewerListener((e) => { + if (isAttachmentViewerEventForHost(e, host)) seen.push(e); + }); + notifyViewerOpen(viewerEvent({ hostToken: host.hostToken })); + expect(typeof off).toBe('function'); + off(); + notifyViewerOpen(viewerEvent({ hostToken: host.hostToken })); + expect(seen).toHaveLength(1); + // Disposing twice is a no-op, not a throw — teardown paths can double-fire. + expect(() => off()).not.toThrow(); + }); + + it('keeps the two channels separate', () => { + // One host, both channels: a panel emission must not open a viewer and + // a viewer emission must not open a panel, even though the identity + // fields (and the TOKEN — one per host, not one per channel) are shared. + const host = { itemId: 'item-1', hostToken: createAttachmentHostToken() }; + const viewerSeen: AttachmentViewerOpenEvent[] = []; + const panelSeen: AttachmentPanelOpenEvent[] = []; + const offViewer = registerAttachmentViewerListener((e) => viewerSeen.push(e)); + const offPanel = registerAttachmentPanelListener((e) => panelSeen.push(e)); + try { + notifyAttachmentPanelOpen(event({ hostToken: host.hostToken })); + notifyViewerOpen(viewerEvent({ hostToken: host.hostToken })); + } finally { + offViewer(); + offPanel(); + } + expect(viewerSeen).toHaveLength(1); + expect(panelSeen).toHaveLength(1); + expect(viewerSeen[0].attachmentId).toBe('att-1'); + }); +}); diff --git a/web/src/lib/attachments/events.ts b/web/src/lib/attachments/events.ts index 414dc4ff..ea76ebbc 100644 --- a/web/src/lib/attachments/events.ts +++ b/web/src/lib/attachments/events.ts @@ -247,3 +247,153 @@ export function notifyAttachmentPanelOpen(event: AttachmentPanelOpenEvent): void if (!event?.attachmentId || !event.itemId || !event.hostToken) return; for (const fn of panelListeners) fn(event); } + +/** + * Image viewer (PLAN-2392 phase 3a, TASK-2428). + * + * Tapping an image — an inline editor image, and in phase 3c the surfaces that + * still mount their own viewer — opens the full-screen `Lightbox`. Same problem + * as the panel channel above, same answer: the emitters include Tiptap + * NodeViews, which are imperative DOM and cannot mount a Svelte component, so + * they signal through this bus and an `ItemDetail`-owned host does the mounting. + * + * ADDRESSING is DR-8's, unchanged and shared: a host consumes an event only + * when BOTH `itemId` and `hostToken` are its own, because the bus is global + * while `ItemDetail` is mounted more than once (master pane + peeked pane). + * The token is the SAME one the panel channel uses — one token per HOST, not + * one per channel — so `createAttachmentHostToken` is not duplicated here. + * + * WHY THE STRIP AND THE TIMELINE ARE NOT ON THIS CHANNEL: they mount `Lightbox` + * directly and keep doing so. The a11y contract lives in the component, and the + * lease stack makes coexisting mounts safe, so consolidating producers buys + * nothing until 3c gives them a single surface to consolidate ONTO. A decision, + * not an omission. + * + * `mutationsEnabled` is deliberately absent from this channel and from the + * host: 3a's viewer has no mutating action, so it would be a dead prop. It is a + * hard prerequisite of 3c's Delete, and when it arrives its source must be the + * host's own gate. + */ +export interface LightboxImage { + id: string; + alt: string; + /** + * Metadata the viewer may caption with, all NULLABLE for the same reason + * the panel's three are: an emitter knows only what its own surface gives + * it, and an inline image's HEAD probe may not have completed or may have + * failed. Structurally a superset of the `Lightbox` component's own + * `LightboxImage` ({id, alt}), so a set built for this channel is passed + * straight through to it. + */ + filename: string | null; + mime_type: string | null; + size_bytes: number | null; + width: number | null; + height: number | null; +} + +export interface AttachmentViewerOpenEvent { + /** UUID of the attachment the viewer opens ON. */ + attachmentId: string; + /** + * Workspace the images are read from, CAPTURED AT EMIT — never read live + * from the host. The pane switches workspace without remounting, so a host + * that resolved the slug itself at render time could serve a viewer opened + * in ws1 from ws2's endpoint. The emitter knows which workspace the click + * happened in; that is the answer the viewer must keep. + */ + workspaceSlug: string; + /** + * UUID of the item whose `ItemDetail` mount should SHOW the viewer. + * + * ROUTING, not ownership — see `AttachmentPanelOpenEvent.itemId` for the + * full argument. It names the host in front of the user; it asserts nothing + * about which item the attachment belongs to, and nothing about permission. + */ + itemId: string; + /** Identity of the `ItemDetail` mount that owns the emitting surface. */ + hostToken: string; + /** + * The set the viewer's ←/→ page through, in the emitting surface's own + * order. Readonly because the viewer must not reorder or mutate a set the + * emitter still owns. + */ + images: readonly LightboxImage[]; + /** Index to open at. `images[index]?.id === attachmentId` at emit. */ + index: number; + /** + * The element the viewer returns focus to on close. Null when the emitter + * has no stable element to offer. + */ + invoker: HTMLElement | null; +} + +const viewerListeners = new Set<(event: AttachmentViewerOpenEvent) => void>(); + +/** + * "Is this event mine?" — the single predicate every viewer host must use. + * + * Deliberately a separate function from the panel's rather than one generic + * over both: the two events are different shapes, and a shared predicate would + * have to be typed loosely enough to accept anything with two string fields. + * The RULE is identical and stated once, in `isAddressable`: both sides must be + * fully addressable before a comparison means anything — two empty tokens are + * not a match, they are two absences. + * + * The event parameter accepts `null | undefined` where the panel's does not. + * That is the signature TASK-2428 specifies, and it matches what both + * functions have always DONE at runtime (`if (!event) return false`) — the + * panel's type is simply narrower than its behaviour. Widening the panel's to + * match is a change to a shipped surface and belongs to whoever next touches + * it, not to this task. + */ +export function isAttachmentViewerEventForHost( + event: AttachmentViewerOpenEvent | null | undefined, + host: { itemId: string | null | undefined; hostToken: string | null | undefined } +): boolean { + if (!event) return false; + const from = { itemId: event.itemId, hostToken: event.hostToken }; + const to = { itemId: host?.itemId ?? '', hostToken: host?.hostToken ?? '' }; + if (!isAddressable(from) || !isAddressable(to)) return false; + return from.itemId === to.itemId && from.hostToken === to.hostToken; +} + +/** + * Subscribe to open-viewer requests. Returns a dispose function — call it from + * the host's teardown, or the listener leaks and fires into a dead component. + * Listeners receive EVERY emission; filter with + * `isAttachmentViewerEventForHost`. + */ +export function registerAttachmentViewerListener( + fn: (event: AttachmentViewerOpenEvent) => void +): () => void { + viewerListeners.add(fn); + return () => viewerListeners.delete(fn); +} + +/** + * Request that the owning host open the image viewer. + * + * No-op when the event can't address a host, can't be fetched, or carries no + * images: an emission missing any identity field would either reach nobody or + * invite a "matches anything" reading of the predicate; one without a workspace + * would open a viewer whose every image URL 404s (the slug is a path segment, + * and the host deliberately does not substitute its own); and an empty set + * would open a full-screen viewer showing nothing. + * + * 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. + * + * (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 { + if (!event?.attachmentId || !event.itemId || !event.hostToken) return; + if (!event.workspaceSlug) return; + if (!event.images?.length) return; + for (const fn of viewerListeners) fn(event); +} diff --git a/web/src/lib/attachments/viewerResource.svelte.ts b/web/src/lib/attachments/viewerResource.svelte.ts new file mode 100644 index 00000000..99ee7e20 --- /dev/null +++ b/web/src/lib/attachments/viewerResource.svelte.ts @@ -0,0 +1,59 @@ +import { untrack } from 'svelte'; +import { nextViewerResourceGen, viewerResourceKey } from './viewerResource'; + +/** + * The viewer-resource generation as a live counter (TASK-2428). + * + * The pure rule lives next door in `viewerResource.ts`; this is the reactive + * wrapper `ItemDetail` actually uses. It is a module rather than four lines + * inline in a 7,000-line component for one reason: the `$effect` below is the + * subtlest thing in this task, and inline it could only ever be exercised + * through `AttachmentViewerHost`'s props — which are handed the answer and so + * cannot tell a correct counter from one that never moves, nor a healthy flush + * from a self-invalidating one (Codex round 6). + * + * SELF-WRITE DISCIPLINE (CONVE-1688). The effect writes `gen`, which it must + * therefore never READ in its tracked scope: an `$effect` that depends on what + * it writes aborts the flush, and an aborted flush strands unrelated reactivity + * elsewhere in the same batch while reporting nothing in a production build. + * So the tracked scope reads ONLY the caller's inputs, and both the comparison + * and the write happen inside `untrack`. `lastKey` is a plain `let` for the + * same reason — as `$state` it would reintroduce exactly that dependency. + * + * Must be called during component initialisation, like any `$effect` owner. + */ +export interface ViewerResourceInput { + /** Workspace the CURRENTLY LOADED item belongs to, not the route's. */ + workspaceSlug: string; + /** UUID of the loaded item. */ + itemId: string | null | undefined; + /** Whether the loaded item matches the requested ref (the switch boundary). */ + loaded: boolean; +} + +export interface ViewerResourceGen { + /** Advances only on a real loaded-item resource change. */ + readonly current: number; +} + +export function createViewerResourceGen(read: () => ViewerResourceInput): ViewerResourceGen { + let gen = $state(0); + let lastKey = ''; + + $effect(() => { + const input = read(); + const key = viewerResourceKey(input.workspaceSlug, input.itemId, input.loaded); + untrack(() => { + const next = nextViewerResourceGen(key, lastKey, gen); + if (next === gen) return; + lastKey = key; + gen = next; + }); + }); + + return { + get current() { + return gen; + }, + }; +} diff --git a/web/src/lib/attachments/viewerResource.test.ts b/web/src/lib/attachments/viewerResource.test.ts new file mode 100644 index 00000000..d1cbcd62 --- /dev/null +++ b/web/src/lib/attachments/viewerResource.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from 'vitest'; +import { nextViewerResourceGen, viewerResourceKey } from './viewerResource'; + +/** + * The generation half of the viewer's lifecycle rule (TASK-2428). + * + * Extracted from `ItemDetail` precisely so these sequences can be stated: a + * host-level test cannot distinguish a correct generation from one that simply + * never moves, because all it ever sees is the number it was handed. + * + * The sequences below are the ones `ItemDetail` actually produces — the key is + * `itemMatchesRef ? {ws, item.id} : ''`, which reads empty mid-switch. + */ +describe('viewerResourceKey', () => { + it('is empty until the loaded item matches the requested ref', () => { + expect(viewerResourceKey('ws', 'item-a', false)).toBe(''); + expect(viewerResourceKey('ws', undefined, true)).toBe(''); + expect(viewerResourceKey('ws', 'item-a', true)).not.toBe(''); + }); + + it('includes the workspace, so the same ref in two workspaces is two resources', () => { + // A reused pane can navigate ws1→ws2 carrying `?item=` where both + // workspaces own that ref (IDEA-2135). + expect(viewerResourceKey('ws1', 'item-a', true)).not.toBe( + viewerResourceKey('ws2', 'item-a', true) + ); + }); + + it('contains no control characters', () => { + // A NUL separator makes the whole source file binary to grep — caught + // in review on the first draft of this rule. + expect(viewerResourceKey('ws', 'item-a', true)).toMatch(/^[\x20-\x7e]+$/); + }); +}); + +describe('nextViewerResourceGen', () => { + /** Replays a key sequence, returning the generation after each step. */ + function replay(keys: string[]): number[] { + let gen = 0; + let lastKey = ''; + return keys.map((key) => { + const next = nextViewerResourceGen(key, lastKey, gen); + if (next !== gen) { + lastKey = key; + gen = next; + } + return gen; + }); + } + + it('advances once when the first item loads', () => { + expect(replay(['', 'ws A'])).toEqual([0, 1]); + }); + + it('does NOT advance on a same-item reload', () => { + // The whole point: a collection schema edit refetches the item already + // on screen. `loadGeneration` moves; this must not. + expect(replay(['ws A', 'ws A', 'ws A'])).toEqual([1, 1, 1]); + }); + + it('counts an A→B switch once, through the empty mid-switch key', () => { + expect(replay(['ws A', '', 'ws B'])).toEqual([1, 1, 2]); + }); + + it('does not count a boundary flap that lands back on the same item', () => { + expect(replay(['ws A', '', 'ws A'])).toEqual([1, 1, 1]); + }); + + it('counts A→B→A as three distinct resources, not two', () => { + // Rapid j/k paging. Coming BACK to A is a resource change too: whatever + // was open belonged to B. + expect(replay(['ws A', '', 'ws B', '', 'ws A'])).toEqual([1, 1, 2, 2, 3]); + }); + + it('counts a cross-workspace switch that keeps the ref', () => { + expect(replay(['ws1 A', 'ws2 A'])).toEqual([1, 2]); + }); + + it('never advances on the empty key alone', () => { + expect(replay(['', '', ''])).toEqual([0, 0, 0]); + }); +}); diff --git a/web/src/lib/attachments/viewerResource.ts b/web/src/lib/attachments/viewerResource.ts new file mode 100644 index 00000000..7ff95780 --- /dev/null +++ b/web/src/lib/attachments/viewerResource.ts @@ -0,0 +1,48 @@ +/** + * The viewer's resource-identity rule (PLAN-2392 phase 3a, TASK-2428). + * + * `AttachmentViewerHost` closes an open viewer on a RESOURCE SWITCH and on + * nothing else. Half of that is the host's own `itemId`; the other half is a + * generation counter, because the id alone cannot tell a same-item resource + * change from a same-item RELOAD — and `ItemDetail` reloads constantly (a + * collection schema edit refetches the item it is already showing). + * + * The two counters `ItemDetail` already has are both wrong for this: + * `loadGeneration` is a non-reactive fence bumped by EVERY `loadData()`, and + * `itemGen` is bumped by every optimistic item write. Keying a viewer on either + * tears it down on a refresh that changed nothing the user can see. + * + * This module is the rule itself, extracted from the component for one reason: + * `ItemDetail` is 7,000 lines with no unit harness, so a rule left inline is + * only ever tested through the host's props — which cannot see whether the + * generation was computed correctly in the first place. + */ + +/** Identity of a loaded item resource, or `''` for "nothing loaded". */ +export function viewerResourceKey( + workspaceSlug: string | null | undefined, + itemId: string | null | undefined, + loaded: boolean +): string { + if (!loaded || !itemId) return ''; + // The workspace is part of the identity, not decoration: a reused pane can + // navigate ws1→ws2 carrying the same `?item=` where both workspaces + // own that ref (IDEA-2135). A plain `::` separator, deliberately: the first + // draft used a NUL, which makes the whole source file binary to grep. + return `${workspaceSlug ?? ''}::${itemId}`; +} + +/** + * The next generation for an observed key. Returns the CURRENT generation + * unchanged when nothing advanced, so the caller's write is a no-op. + * + * An EMPTY key never advances it. Empty is "no loaded resource right now" — + * what the switch boundary reads mid-load — so A→''→B is ONE transition and + * counts once, at B. (Going empty still closes an open viewer: that is the + * host's `itemId` arm, which fires immediately rather than waiting for a + * replacement that may never arrive.) + */ +export function nextViewerResourceGen(key: string, lastKey: string, gen: number): number { + if (!key || key === lastKey) return gen; + return gen + 1; +} diff --git a/web/src/lib/attachments/viewerResourceGen.svelte.test.ts b/web/src/lib/attachments/viewerResourceGen.svelte.test.ts new file mode 100644 index 00000000..5b05040f --- /dev/null +++ b/web/src/lib/attachments/viewerResourceGen.svelte.test.ts @@ -0,0 +1,185 @@ +import { describe, it, expect, afterEach } from 'vitest'; +import { flushSync } from 'svelte'; +import { createViewerResourceGen } from './viewerResource.svelte'; + +/** + * The generation EFFECT (TASK-2428) — the production one `ItemDetail` calls, + * not a copy of it. + * + * Two things are under test, and the second is why this file exists at all: + * + * 1. Which transitions advance the counter. Driven through the same inputs + * `ItemDetail` supplies (`loadedItemWsSlug`, `item?.id`, `itemMatchesRef`), + * replaying the sequences a real session produces — a route alias, a + * schema-edit reload, an A→B switch through the empty mid-switch boundary, + * a cross-workspace nav that keeps the ref. + * + * 2. That the effect keeps the FLUSH alive. An `$effect` that reads a `$state` + * it also writes in its tracked scope aborts the flush and strands + * unrelated reactivity in the same batch, reporting nothing in a production + * build (CONVE-1688). A test that only checks the counter's VALUE passes + * through that, because the counter is what the aborted effect already + * wrote. So every case below runs beside an independent probe effect that + * must keep observing — the neighbour is the assertion. + */ + +// One reactive input object, the shape ItemDetail's reader closes over. +const input = $state({ workspaceSlug: '', itemId: null as string | null, loaded: false }); + +interface Harness { + gen: () => number; + /** Values the neighbouring effect observed, in order. */ + probeSeen: number[]; + /** How many times the neighbour re-ran. */ + probeRuns: () => number; + stop: () => void; +} + +let running: Harness | null = null; + +function start(): Harness { + const probeSeen: number[] = []; + let probeRuns = 0; + let read!: () => number; + const stop = $effect.root(() => { + const resource = createViewerResourceGen(() => ({ + workspaceSlug: input.workspaceSlug, + itemId: input.itemId, + loaded: input.loaded, + })); + read = () => resource.current; + // The neighbour. It depends on the counter, so a stranded flush shows up + // as a value it never sees — and it is a SEPARATE effect, so an aborted + // batch takes it down with the one that aborted. + $effect(() => { + probeSeen.push(resource.current); + probeRuns += 1; + }); + }); + flushSync(); + running = { gen: () => read(), probeSeen, probeRuns: () => probeRuns, stop }; + return running; +} + +/** Applies one observable state of the loaded item, then settles. */ +function set(next: Partial) { + Object.assign(input, next); + flushSync(); +} + +/** The mid-switch boundary: `itemMatchesRef` false, item not yet adopted. */ +const MID_SWITCH = { loaded: false }; + +afterEach(() => { + running?.stop(); + running = null; + Object.assign(input, { workspaceSlug: '', itemId: null, loaded: false }); +}); + +describe('createViewerResourceGen', () => { + it('advances once when the first item loads, and the neighbour sees it', () => { + const h = start(); + expect(h.gen()).toBe(0); + + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + expect(h.gen()).toBe(1); + // Not just the value: the downstream effect actually re-ran with it. + expect(h.probeSeen).toEqual([0, 1]); + }); + + it('does NOT advance on a collection-only or username-only route alias', () => { + // `/dave/ws/tasks/TASK-1` → `/dave/ws/bugs/TASK-1` (or a different + // username for the same workspace). The route change triggers a + // `loadData()`, which re-adopts the SAME item under the SAME workspace, + // so every input this effect reads is re-asserted unchanged. That is + // the whole reason the route is not one of its inputs. + const h = start(); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + const runsBefore = h.probeRuns(); + + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + + expect(h.gen()).toBe(1); + // The neighbour did not re-run either: nothing downstream was disturbed + // by a refresh that changed nothing. + expect(h.probeRuns()).toBe(runsBefore); + }); + + it('does NOT advance on the reload that follows a collection schema edit', () => { + // `loadData()` flips `loading` and re-adopts the same item. On this + // path `itemMatchesRef` never goes false (the item is not nulled), so + // the inputs never leave the loaded state. + const h = start(); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + + expect(h.gen()).toBe(1); + expect(h.probeSeen).toEqual([0, 1]); + }); + + it('counts an A→B switch ONCE, through the empty mid-switch boundary', () => { + const h = start(); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + + set(MID_SWITCH); + expect(h.gen()).toBe(1); // the gap is not a resource + set({ workspaceSlug: 'ws', itemId: 'item-b', loaded: true }); + + expect(h.gen()).toBe(2); + expect(h.probeSeen).toEqual([0, 1, 2]); + }); + + it('counts a boundary flap that lands back on the same item as NOTHING', () => { + const h = start(); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + const runsBefore = h.probeRuns(); + + set(MID_SWITCH); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + + expect(h.gen()).toBe(1); + expect(h.probeRuns()).toBe(runsBefore); + }); + + it('advances on a same-ref DIFFERENT-WORKSPACE change', () => { + // A reused pane navigating ws1→ws2 while carrying `?item=`, where + // both workspaces own that ref (IDEA-2135). The plain item id would + // miss it if the two happened to resolve to the same id; the workspace + // is part of the identity precisely so it cannot. + const h = start(); + set({ workspaceSlug: 'ws1', itemId: 'item-a', loaded: true }); + set({ workspaceSlug: 'ws2', itemId: 'item-a', loaded: true }); + + expect(h.gen()).toBe(2); + expect(h.probeSeen).toEqual([0, 1, 2]); + }); + + it('counts A→B→A as three resources, keeping the neighbour live throughout', () => { + // Rapid j/k paging. Coming back to A is a change too: whatever was open + // belonged to B. This is also the sequence a self-invalidating effect + // survives longest, because its first write is a no-op. + const h = start(); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + set(MID_SWITCH); + set({ workspaceSlug: 'ws', itemId: 'item-b', loaded: true }); + set(MID_SWITCH); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: true }); + + expect(h.gen()).toBe(3); + expect(h.probeSeen).toEqual([0, 1, 2, 3]); + // Still reacting after five transitions — the flush was never aborted. + set(MID_SWITCH); + set({ workspaceSlug: 'ws', itemId: 'item-c', loaded: true }); + expect(h.probeSeen).toEqual([0, 1, 2, 3, 4]); + }); + + it('never advances while nothing is loaded', () => { + const h = start(); + set({ workspaceSlug: 'ws', itemId: 'item-a', loaded: false }); + set({ workspaceSlug: 'ws', itemId: null, loaded: false }); + + expect(h.gen()).toBe(0); + expect(h.probeSeen).toEqual([0]); + }); +}); diff --git a/web/src/lib/components/attachments/AttachmentViewerHost.svelte b/web/src/lib/components/attachments/AttachmentViewerHost.svelte new file mode 100644 index 00000000..ae3c9d1c --- /dev/null +++ b/web/src/lib/components/attachments/AttachmentViewerHost.svelte @@ -0,0 +1,170 @@ + + + + +{#key request} + {#if request} + + {/if} +{/key} diff --git a/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts b/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts new file mode 100644 index 00000000..1aea27cc --- /dev/null +++ b/web/src/lib/components/attachments/AttachmentViewerHost.svelte.test.ts @@ -0,0 +1,419 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { flushSync, mount, unmount } from 'svelte'; + +// TASK-2428. The viewer is exercised THROUGH its host, because the host is +// where the two rules that matter live: an event is consumed only when both +// `itemId` and `hostToken` are this host's own (DR-8), and an open viewer is +// closed by a RESOURCE SWITCH and by nothing else. Nothing routes into it yet — +// the emitters land in the next task — so these tests emit on the bus directly, +// which is also the only way to drive a NodeView-originated open. +// +// The bus stays REAL: addressing is the thing under test. +// +// What jsdom CANNOT prove here, and is therefore the browser suite's: focus +// entry, background inertness, and the viewer's real stacking against a panel +// or a menu opened at the same time. + +// The bus stays REAL, with ONE wrapper: the registration is counted and its +// disposer is tracked, because "an unmounted host stops receiving events" is +// otherwise untestable through the DOM — a destroyed component renders nothing +// whether or not its listener leaked, so a leak would pass silently. +const subs = vi.hoisted(() => ({ registered: 0, disposed: 0 })); +vi.mock('$lib/attachments/events', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + registerAttachmentViewerListener: (fn: Parameters< + typeof actual.registerAttachmentViewerListener + >[0]) => { + subs.registered += 1; + const off = actual.registerAttachmentViewerListener(fn); + return () => { + subs.disposed += 1; + off(); + }; + }, + }; +}); + +const { notifyViewerOpen } = await import('$lib/attachments/events'); +type ViewerEvent = import('$lib/attachments/events').AttachmentViewerOpenEvent; +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 { + return { + id: ATT_ID, + alt: 'a diagram', + filename: 'diagram.png', + mime_type: 'image/png', + size_bytes: 4096, + width: 800, + height: 600, + ...over, + }; +} + +function openEvent(over: Partial = {}): ViewerEvent { + return { + attachmentId: ATT_ID, + workspaceSlug: 'ws', + itemId: 'item-a', + hostToken: 'host-1', + images: [image()], + index: 0, + invoker: null, + ...over, + }; +} + +/** + * The viewer is a fixed-position overlay rendered into its host's own mount + * container, which is what lets a two-host test say WHICH host opened rather + * than only how many viewers exist. + */ +function viewers(scope: ParentNode = document): HTMLElement[] { + return Array.from(scope.querySelectorAll('.lightbox-backdrop')); +} + +function viewer(): HTMLElement | null { + return viewers()[0] ?? null; +} + +function viewerImage(): HTMLImageElement | null { + return document.querySelector('.lightbox-image'); +} + +interface HostProps { + itemId: string | null; + hostToken: string; + resourceGen: number; +} + +// Two reactive props objects, declared at the top level because `$state(...)` +// may only initialize a declaration. Two of them because the pane host runs a +// master and a peeked ItemDetail at once, which is exactly what DR-8's +// addressing exists for. +const propsA = $state({ itemId: 'item-a', hostToken: 'host-1', resourceGen: 1 }); +const propsB = $state({ itemId: 'item-a', hostToken: 'host-2', resourceGen: 1 }); + +describe('AttachmentViewerHost', () => { + let target: HTMLElement; + const mounted: ReturnType[] = []; + + beforeEach(() => { + subs.registered = 0; + subs.disposed = 0; + Object.assign(propsA, { itemId: 'item-a', hostToken: 'host-1', resourceGen: 1 }); + Object.assign(propsB, { itemId: 'item-a', hostToken: 'host-2', resourceGen: 1 }); + target = document.body.appendChild(document.createElement('div')); + }); + + afterEach(() => { + while (mounted.length) unmount(mounted.pop()!); + target.remove(); + document.querySelectorAll('.viewer-host-target').forEach((el) => el.remove()); + }); + + /** Mounts a host in its OWN container, and hands that container back. */ + function mountHost(props: HostProps): HTMLElement { + const container = target.appendChild(document.createElement('div')); + container.className = 'viewer-host-target'; + mounted.push(mount(AttachmentViewerHost, { target: container, props })); + flushSync(); + return container; + } + + it('opens for an event addressed to it, on the event’s workspace', () => { + mountHost(propsA); + notifyViewerOpen(openEvent()); + flushSync(); + + expect(viewer()).not.toBeNull(); + // The workspace is the one CAPTURED at emit, not one the host resolved: + // the pane switches workspace without remounting. + expect(viewerImage()?.getAttribute('src')).toContain('/workspaces/ws/attachments/'); + expect(viewerImage()?.getAttribute('alt')).toBe('a diagram'); + }); + + it('serves the emit-time workspace even when it is not the host’s current one', () => { + mountHost(propsA); + notifyViewerOpen(openEvent({ workspaceSlug: 'other-ws' })); + flushSync(); + + expect(viewerImage()?.getAttribute('src')).toContain('/workspaces/other-ws/attachments/'); + }); + + it('ignores an event addressed to the OTHER host, with both mounted', () => { + const containerA = mountHost(propsA); + const containerB = mountHost(propsB); + + // Same item, other host token: exactly one viewer may open, and it must + // be the ADDRESSED one — counting viewers alone would pass if the wrong + // host opened. Matching on itemId alone would open two. + notifyViewerOpen(openEvent({ hostToken: 'host-2' })); + flushSync(); + + expect(viewers()).toHaveLength(1); + expect(viewers(containerB)).toHaveLength(1); + expect(viewers(containerA)).toHaveLength(0); + + // ...and the reverse direction, so neither host is simply inert. + notifyViewerOpen(openEvent({ hostToken: 'host-1' })); + flushSync(); + expect(viewers(containerA)).toHaveLength(1); + expect(viewers()).toHaveLength(2); + }); + + it('ignores an event for a different item on its own token', () => { + mountHost(propsA); + notifyViewerOpen(openEvent({ itemId: 'item-b' })); + flushSync(); + + expect(viewer()).toBeNull(); + }); + + it('opens at the requested index and pages through the whole set', () => { + mountHost(propsA); + notifyViewerOpen( + openEvent({ + attachmentId: ATT_ID_2, + images: [image(), image({ id: ATT_ID_2, alt: 'second' })], + index: 1, + }) + ); + flushSync(); + + expect(viewerImage()?.getAttribute('src')).toContain(ATT_ID_2); + document.querySelector('.lightbox-nav.prev')!.click(); + flushSync(); + expect(viewerImage()?.getAttribute('src')).toContain(ATT_ID); + }); + + it('REMOUNTS on a second open, so the new index is honoured', () => { + // Lightbox seeds `current` once through `untrack`, so a prop update + // would leave the second open showing the first image. + mountHost(propsA); + const images = [image(), image({ id: ATT_ID_2, alt: 'second' })]; + notifyViewerOpen(openEvent({ images, index: 0 })); + flushSync(); + expect(viewerImage()?.getAttribute('src')).toContain(ATT_ID); + + notifyViewerOpen(openEvent({ attachmentId: ATT_ID_2, images, index: 1 })); + flushSync(); + expect(viewers()).toHaveLength(1); + expect(viewerImage()?.getAttribute('src')).toContain(ATT_ID_2); + }); + + it('closes on the viewer’s own close control', () => { + mountHost(propsA); + notifyViewerOpen(openEvent()); + flushSync(); + document.querySelector('.lightbox-close')!.click(); + flushSync(); + + expect(viewer()).toBeNull(); + }); + + it('returns focus to the invoker on close', () => { + const invoker = target.appendChild(document.createElement('button')); + mountHost(propsA); + notifyViewerOpen(openEvent({ invoker })); + flushSync(); + document.querySelector('.lightbox-close')!.click(); + flushSync(); + + expect(document.activeElement).toBe(invoker); + }); + + it('does not throw when the invoker is gone by the time the viewer closes', () => { + // An editor NodeView is re-rendered on any document change, so the + // element that opened the viewer can be detached by now. + const invoker = target.appendChild(document.createElement('button')); + mountHost(propsA); + notifyViewerOpen(openEvent({ invoker })); + flushSync(); + invoker.remove(); + expect(() => + document.querySelector('.lightbox-close')!.click() + ).not.toThrow(); + flushSync(); + expect(viewer()).toBeNull(); + }); + + it('closes when the host switches item', () => { + mountHost(propsA); + notifyViewerOpen(openEvent()); + flushSync(); + expect(viewer()).not.toBeNull(); + + propsA.itemId = 'item-b'; + flushSync(); + expect(viewer()).toBeNull(); + }); + + it('closes when the loaded resource changes under a stable item id', () => { + // The generation is the arm that survives a mid-switch where the id + // prop never visibly settles on a different value. + mountHost(propsA); + notifyViewerOpen(openEvent()); + flushSync(); + + propsA.resourceGen = 2; + flushSync(); + expect(viewer()).toBeNull(); + }); + + it('does NOT close on a same-resource refresh', () => { + // The whole reason for a dedicated generation: ItemDetail's + // `loadGeneration` is bumped by every loadData(), including the + // same-item reload after a collection schema edit. Keying on that would + // tear the viewer down on a refresh that changed nothing it can see. + // + // This is the shape a refresh has at this boundary: `loadData()` does + // not null `item`, and `itemMatchesRef` ignores the collection and + // username segments, so both a reload and a collection-only route + // change leave the id AND the generation exactly where they were — + // re-asserted, never absent. (The `loading` flip that used to destroy + // this host outright is why it is mounted at ItemDetail's top level.) + mountHost(propsA); + notifyViewerOpen(openEvent()); + flushSync(); + + propsA.itemId = 'item-a'; + propsA.resourceGen = 1; + flushSync(); + + expect(viewer()).not.toBeNull(); + expect(viewerImage()?.getAttribute('src')).toContain(ATT_ID); + }); + + it('closes when the item goes away without another arriving', () => { + // The id is `itemMatchesRef ? item?.id : null`, so null means the item + // this viewer belongs to is no longer the one on screen. Waiting for the + // next non-empty id instead would strand the viewer over a skeleton or + // an error page whenever the incoming item never loads. + mountHost(propsA); + notifyViewerOpen(openEvent()); + flushSync(); + expect(viewer()).not.toBeNull(); + + propsA.itemId = null; + flushSync(); + expect(viewer()).toBeNull(); + }); + + it('opens normally on a host that mounted with an already-loaded resource', () => { + // The lifecycle rule is about TRANSITIONS. A host seeded from its + // initial props has nothing to clear, and must not swallow the first + // event because its own mount looked like a change. + mountHost(propsA); + flushSync(); + notifyViewerOpen(openEvent()); + flushSync(); + expect(viewer()).not.toBeNull(); + }); + + it('unsubscribes from the bus on unmount, so a dead host receives nothing', () => { + // Asserted through the disposer, not the DOM: a destroyed component + // renders nothing whether or not its listener leaked, so a DOM-only + // check here would pass with the subscription still live and firing + // into a dead view for the rest of the session. + const before = subs.registered; + mountHost(propsA); + notifyViewerOpen(openEvent()); + flushSync(); + expect(viewer()).not.toBeNull(); + expect(subs.registered).toBe(before + 1); + expect(subs.disposed).toBe(0); + + unmount(mounted.pop()!); + flushSync(); + expect(subs.disposed).toBe(1); + + notifyViewerOpen(openEvent()); + flushSync(); + expect(viewer()).toBeNull(); + }); + + it('subscribes ONCE and still addresses on its CURRENT item after a switch', () => { + // The registration effect reads `itemId` / `hostToken` inside the + // callback at EMIT time rather than in its tracked scope. Reading them + // outside would both re-run the effect on every item switch (tearing the + // listener down and re-adding it) AND capture the address, so the host + // would keep answering for the item it mounted on. Counting + // registrations catches the first half; the emissions below catch the + // second, which is the one that silently breaks a tap after a switch. + const before = subs.registered; + mountHost(propsA); + propsA.itemId = 'item-b'; + propsA.resourceGen = 2; + flushSync(); + + // The address it mounted with is now stale and must be ignored... + notifyViewerOpen(openEvent({ itemId: 'item-a' })); + flushSync(); + expect(viewer()).toBeNull(); + + // ...while the item it is showing NOW is answered. + notifyViewerOpen(openEvent({ itemId: 'item-b' })); + flushSync(); + expect(viewer()).not.toBeNull(); + + expect(subs.registered).toBe(before + 1); + expect(subs.disposed).toBe(0); + }); + + it('keeps the flush alive: a teardown does not strand neighbouring reactivity', () => { + // The self-write hazard (CONVE-1688): an $effect that writes a $state it + // also reads in its tracked scope ABORTS the flush, which strands + // unrelated reactivity elsewhere in the same flush and reports nothing + // in a production build. A test that only checks this host's own + // counter can pass while the component is quietly breaking its + // neighbours, so the assertion has to be about a NEIGHBOUR. + // + // Two hosts driven by ONE props object: they share a flush, so if the + // first one's lifecycle effect aborts it, the second's teardown never + // runs and its viewer stays on screen. Exercised open → mutate → + // teardown → RE-open → mutate, because a hazard that no-ops on its + // first write only shows once the state has actually moved. + const first = mountHost(propsA); + const neighbour = mountHost(propsA); + + notifyViewerOpen(openEvent()); + flushSync(); + expect(viewers(first)).toHaveLength(1); + expect(viewers(neighbour)).toHaveLength(1); + + propsA.resourceGen = 2; + flushSync(); + expect(viewers(first)).toHaveLength(0); + expect(viewers(neighbour)).toHaveLength(0); + + // Re-open on the same (now current) resource and drive it again. + notifyViewerOpen(openEvent()); + flushSync(); + expect(viewers(neighbour)).toHaveLength(1); + + propsA.itemId = 'item-b'; + flushSync(); + expect(viewers(first)).toHaveLength(0); + expect(viewers(neighbour)).toHaveLength(0); + + // And the neighbour is still LIVE, not merely emptied: it answers the + // new address, which it could not do if its effects had been stranded. + notifyViewerOpen(openEvent({ itemId: 'item-b' })); + flushSync(); + expect(viewers(neighbour)).toHaveLength(1); + }); + + // The bound-close invariant is NOT tested here: driving it needs the close + // callback of a viewer the host has already destroyed, and a click on the + // detached button never reaches Svelte's delegated root handler — the + // version of this test written that way passed with the guard deleted. + // It lives in `AttachmentViewerHostClose.svelte.test.ts`, against a stubbed + // Lightbox that hands the callback back. +}); diff --git a/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts b/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts new file mode 100644 index 00000000..947d7e32 --- /dev/null +++ b/web/src/lib/components/attachments/AttachmentViewerHostClose.svelte.test.ts @@ -0,0 +1,155 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { flushSync, mount, unmount } from 'svelte'; + +/** + * The bound-close invariant, on its own because it needs `Lightbox` stubbed + * (TASK-2428). + * + * `AttachmentViewerHost` hands each viewer a close handler BOUND to the request + * it was rendered for, so a continuation from a viewer the host has already + * destroyed cannot dismiss the one the user opened since. Driving that through + * the real component is impossible in jsdom: the old close button is detached + * by then, and a click on a detached node never reaches Svelte's delegated root + * handler — a test written that way passes with the guard deleted (Codex round + * 4). Holding the callback directly is the only honest way to fire it. + */ +vi.mock('$lib/components/common/Lightbox.svelte', async () => ({ + default: (await import('./fixtures/LightboxStub.svelte')).default, +})); + +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; + +const ATT_ID = '11111111-2222-4333-8444-555555555555'; +const ATT_ID_2 = '99999999-8888-4777-8666-555555555555'; + +function openEvent(over: Partial = {}): ViewerEvent { + return { + attachmentId: ATT_ID, + workspaceSlug: 'ws', + itemId: 'item-a', + hostToken: 'host-1', + images: [ + { + id: ATT_ID, + alt: 'a diagram', + filename: 'diagram.png', + mime_type: 'image/png', + size_bytes: 4096, + width: 800, + height: 600, + }, + ], + index: 0, + invoker: null, + ...over, + }; +} + +function stub(): HTMLElement | null { + return document.querySelector('.lightbox-stub'); +} + +const props = $state({ itemId: 'item-a' as string | null, hostToken: 'host-1', resourceGen: 1 }); + +describe('AttachmentViewerHost — bound close', () => { + let target: HTMLElement; + const mounted: ReturnType[] = []; + + beforeEach(() => { + lightboxStubCalls.length = 0; + Object.assign(props, { itemId: 'item-a', hostToken: 'host-1', resourceGen: 1 }); + target = document.body.appendChild(document.createElement('div')); + mounted.push(mount(AttachmentViewerHost, { target, props })); + flushSync(); + }); + + afterEach(() => { + while (mounted.length) unmount(mounted.pop()!); + target.remove(); + }); + + it('remounts the viewer per open, rather than re-using one instance', () => { + // Lightbox seeds its index once through `untrack`, so a re-used instance + // would silently keep showing the first image. + notifyViewerOpen(openEvent()); + flushSync(); + notifyViewerOpen(openEvent({ attachmentId: ATT_ID_2, images: [{ ...openEvent().images[0], id: ATT_ID_2 }] })); + flushSync(); + + expect(lightboxStubCalls).toHaveLength(2); + expect(stub()?.dataset.attachmentId).toBe(ATT_ID_2); + }); + + it('a close from a DESTROYED viewer cannot dismiss the newer one', () => { + notifyViewerOpen(openEvent()); + flushSync(); + const staleClose = lightboxStubCalls[0].onClose; + + notifyViewerOpen(openEvent({ attachmentId: ATT_ID_2, images: [{ ...openEvent().images[0], id: ATT_ID_2 }] })); + flushSync(); + + staleClose(); + flushSync(); + + expect(stub()).not.toBeNull(); + expect(stub()?.dataset.attachmentId).toBe(ATT_ID_2); + }); + + it('the CURRENT viewer’s close still closes it', () => { + // The guard must not be so broad that it makes closing impossible. + notifyViewerOpen(openEvent()); + flushSync(); + lightboxStubCalls[0].onClose(); + flushSync(); + + expect(stub()).toBeNull(); + }); + + it('a close from a viewer torn down by a resource switch does not close the next one', () => { + notifyViewerOpen(openEvent()); + flushSync(); + const staleClose = lightboxStubCalls[0].onClose; + + // The host switches item, destroying that viewer... + props.itemId = 'item-b'; + props.resourceGen = 2; + flushSync(); + expect(stub()).toBeNull(); + + // ...and one opens on the new item. + notifyViewerOpen(openEvent({ itemId: 'item-b', attachmentId: ATT_ID_2 })); + flushSync(); + expect(stub()).not.toBeNull(); + + staleClose(); + flushSync(); + expect(stub()).not.toBeNull(); + }); + + it('returns focus to the invoker only on the bound close', () => { + const invoker = target.appendChild(document.createElement('button')); + const other = target.appendChild(document.createElement('button')); + other.focus(); + + notifyViewerOpen(openEvent({ invoker })); + flushSync(); + lightboxStubCalls[0].onClose(); + flushSync(); + expect(document.activeElement).toBe(invoker); + + // A stale close returns nothing: it did not close anything, so moving + // the user's focus would be a jump out of whatever they are now in. + notifyViewerOpen(openEvent({ invoker })); + flushSync(); + const staleClose = lightboxStubCalls[1].onClose; + notifyViewerOpen(openEvent({ attachmentId: ATT_ID_2, invoker: null })); + flushSync(); + other.focus(); + staleClose(); + flushSync(); + expect(document.activeElement).toBe(other); + }); +}); diff --git a/web/src/lib/components/attachments/fixtures/LightboxStub.svelte b/web/src/lib/components/attachments/fixtures/LightboxStub.svelte new file mode 100644 index 00000000..51776ead --- /dev/null +++ b/web/src/lib/components/attachments/fixtures/LightboxStub.svelte @@ -0,0 +1,27 @@ + + + +
diff --git a/web/src/lib/components/attachments/fixtures/lightboxStub.ts b/web/src/lib/components/attachments/fixtures/lightboxStub.ts new file mode 100644 index 00000000..f8ad9c33 --- /dev/null +++ b/web/src/lib/components/attachments/fixtures/lightboxStub.ts @@ -0,0 +1,16 @@ +/** + * Recording surface for `LightboxStub.svelte` (TASK-2428). + * + * Exists so a test can hold a viewer's `onClose` AFTER that viewer has been + * destroyed — the only way to drive the stale-continuation case, since a click + * on a detached button never reaches Svelte's delegated root handler and so + * proves nothing (Codex round 4 found the click-based version vacuous). + */ +export interface LightboxStubCall { + images: { id: string }[]; + index: number; + wsSlug: string; + onClose: () => void; +} + +export const lightboxStubCalls: LightboxStubCall[] = []; diff --git a/web/src/lib/components/items/ItemDetail.svelte b/web/src/lib/components/items/ItemDetail.svelte index 48bce3bd..bfd2bf13 100644 --- a/web/src/lib/components/items/ItemDetail.svelte +++ b/web/src/lib/components/items/ItemDetail.svelte @@ -43,7 +43,9 @@ import CopyItemDialog from '$lib/components/items/CopyItemDialog.svelte'; import ItemAttachmentStrip from '$lib/components/items/ItemAttachmentStrip.svelte'; import AttachmentPanelHost from '$lib/components/attachments/AttachmentPanelHost.svelte'; + import AttachmentViewerHost from '$lib/components/attachments/AttachmentViewerHost.svelte'; import { createAttachmentHostToken } from '$lib/attachments/events'; + import { createViewerResourceGen } from '$lib/attachments/viewerResource.svelte'; import { copyToClipboard } from '$lib/utils/clipboard'; import { repairDeadItemLastRoute } from '$lib/collections/paneUrlParams'; import { isSamePaneTarget, breadcrumbParentTarget } from '$lib/collections/paneTarget'; @@ -410,6 +412,36 @@ onReady?.(scrollReady); }); + // Viewer-resource generation (PLAN-2392 / TASK-2428). A DEDICATED reactive + // counter that advances only when the LOADED item resource actually changes + // — the item this instance is showing, workspace included (IDEA-2135's + // same-ref-different-workspace case), and only once it matches the requested + // ref rather than mid-switch. + // + // It exists because neither of the two counters already here can be used: + // `loadGeneration` is a plain non-reactive fence bumped by EVERY loadData() + // — including the same-item reload after a collection schema edit — so a + // consumer keyed on it would tear down on a refresh that changed nothing it + // can see, and `itemGen` is bumped by every optimistic item write. The route + // is not usable either: a collection-only or username-only URL change + // preserves the item. + // + // Derived-then-observed rather than incremented at each `item = ...` site: + // there are ~30 of those and most are same-item writes. One key comparison + // states the rule once and cannot be forgotten by a future assignment. + // + // Both the rule and the effect that applies it live in + // `$lib/attachments/viewerResource*` so they are unit-testable: inline here + // they could only be exercised through the host's props, which are handed the + // answer and so cannot tell a correct counter from one that never moves + // (Codex round 6). The self-write discipline this effect needs — never read + // `gen` in the tracked scope — is documented and pinned there. + const viewerResource = createViewerResourceGen(() => ({ + workspaceSlug: loadedItemWsSlug, + itemId: item?.id, + loaded: itemMatchesRef && item !== null, + })); + // Resolved identity for the host's `?item == master` guard (TASK-2173). Non- // reactive-write $derived (CONVE-1688: the emit effect below only READS it): // the loaded item's {id, ref, slug} once it matches the requested ref/slug, @@ -5816,6 +5848,29 @@ {/if} {/if} + + +