diff --git a/web/e2e/item-attachment-strip.spec.ts b/web/e2e/item-attachment-strip.spec.ts index a840bed5..7d57ff33 100644 --- a/web/e2e/item-attachment-strip.spec.ts +++ b/web/e2e/item-attachment-strip.spec.ts @@ -209,14 +209,27 @@ test.describe('item attachment strip', () => { const del = page.locator(DELETE_BTN).first(); await del.focus(); await expect(del).toBeFocused(); + // WCAG 2.2 target size (2.5.8), from PLAN-2382. Only a real browser + // applies the scoped CSS, so this is the only place it can be checked. + const box = await del.boundingBox(); + expect(box?.width).toBeGreaterThanOrEqual(24); + expect(box?.height).toBeGreaterThanOrEqual(24); - page.once('dialog', (dialog) => { - // The attachment IS embedded in the body (the drop inserted it), so - // the confirm must say so rather than hedging. - expect(dialog.message()).toContain("still used in this item's content"); - void dialog.accept(); - }); + // TASK-2425 / DR-18: the confirmation is the app's own drill-down, not + // a browser dialog. A native `confirm()` would hang this click until + // Playwright auto-dismissed it, so the absence of a dialog handler is + // itself part of the assertion. await del.click(); + const confirmMenu = page.locator('[role="menu"]'); + await expect(confirmMenu).toBeVisible(); + // The attachment IS embedded in the body (the drop inserted it), so the + // prompt must say so rather than hedging. + await expect(confirmMenu.locator('.attachment-delete-prompt')).toContainText( + "still used in this item's content" + ); + // Cancel is first, so the focus handoff can never land Enter on Delete. + await expect(confirmMenu.getByRole('menuitem').first()).toContainText('Cancel'); + await confirmMenu.getByRole('menuitem', { name: 'Delete file' }).click(); await expect(page.locator(TILE)).toHaveCount(0); // ...and the strip disappears entirely once empty. diff --git a/web/src/lib/attachments/actions.ts b/web/src/lib/attachments/actions.ts index c0c3a7b5..3b7d7601 100644 --- a/web/src/lib/attachments/actions.ts +++ b/web/src/lib/attachments/actions.ts @@ -70,10 +70,11 @@ export interface AttachmentActionContext { */ origin?: string; /** - * Confirmation gate for Delete. The surface owns the wording and the - * modality (the strip uses `window.confirm`), so a descriptor never - * invents one — but when supplied, returning false aborts before any - * request is sent. + * Confirmation gate for Delete. The surface owns the modality, so a + * descriptor never invents one — but when supplied, returning false aborts + * before any request is sent. Every surface now resolves this through the + * shared `AttachmentDeleteConfirm` drill-down (DR-18); the native + * `window.confirm` the strip used to raise is gone. */ confirmDelete?: (attachment: AttachmentActionSubject) => boolean | Promise; /** Called after the server confirms the row is gone (204 or 404). */ diff --git a/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte b/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte new file mode 100644 index 00000000..9dcae214 --- /dev/null +++ b/web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte @@ -0,0 +1,100 @@ + + + + + + +Cancel + +Delete file + + diff --git a/web/src/lib/components/attachments/AttachmentDetailsPanel.svelte b/web/src/lib/components/attachments/AttachmentDetailsPanel.svelte index d27d25a6..a92f79bd 100644 --- a/web/src/lib/components/attachments/AttachmentDetailsPanel.svelte +++ b/web/src/lib/components/attachments/AttachmentDetailsPanel.svelte @@ -39,15 +39,14 @@ refetching — a plain refetch would replay the cached failure and look broken (DR-10). - THE DELETE CONFIRMATION IS AN IN-APP DRILL-DOWN (DR-18), copying the item - menu's shape exactly: the prompt is `role="presentation"` with an - `aria-describedby` back-reference from the destructive row, Cancel comes - FIRST so the focus handoff can never land Enter on Delete, and the - contextual "still used in this item's content" warning is carried through - from the strip. It is wired as the descriptor's `confirmDelete` promise - rather than as a bespoke delete path, so the descriptor's own - identity-snapshot and permission re-check across the confirmation stay in - force. + THE DELETE CONFIRMATION IS AN IN-APP DRILL-DOWN (DR-18) — and it is the + SAME one the strip's tile shows, `AttachmentDeleteConfirm`, rows and prompt + text both. This panel supplies the sub-view slot; the confirmation owns the + shape (prompt as `role="presentation"`, `aria-describedby` back-reference, + Cancel first, destructive row last) and the two warning arms. It is wired as + the descriptor's `confirmDelete` promise rather than as a bespoke delete + path, so the descriptor's own identity-snapshot and permission re-check + across the confirmation stay in force. SWITCH-SAFETY. The host swaps this component's props from one attachment to another without a `{#key}` remount (a second tap while the panel is @@ -65,6 +64,9 @@ import Menu from '$lib/components/common/Menu.svelte'; import MenuItem from '$lib/components/common/MenuItem.svelte'; import AttachmentIcon from '$lib/attachments/icons/AttachmentIcon.svelte'; + import AttachmentDeleteConfirm, { + attachmentDeletePrompt, + } from './AttachmentDeleteConfirm.svelte'; import { attachmentActionsFor, type AttachmentActionContext, @@ -382,20 +384,16 @@ * it resolves, which is the whole reason it is wired this way rather than * as a bespoke "confirm, then call the API" path here. * - * The "not referenced here" arm deliberately does NOT claim the attachment - * is unused, and stays hedged word-for-word with the strip's. Two - * independent reasons: a reference can live in another item's content, in - * fields JSON, or in any comment — the server's scan covers all three and - * none of it is visible client-side — and the body this checks is the + * The wording comes from the shared `attachmentDeletePrompt` — the same + * two arms the strip's tile shows (DR-18). The hedged arm matters here for + * one EXTRA reason beyond the shared one: the body this checks is the * HOST's, which is not necessarily the attachment's parent item. The * open-panel event's `itemId` is ROUTING, not ownership: a chip in a reused * comment composer's unsubmitted draft correctly routes to the host in * front of the user even after an item switch. */ function confirmDelete(): Promise { - deletePrompt = referencedHere() - ? `Delete ${displayName}? It's still used in this item's content — deleting it will leave a "missing attachment" placeholder where it appears.` - : `Delete ${displayName}? It isn't referenced in this item's content, but it may still be referenced by another item or a comment. This cannot be undone.`; + deletePrompt = attachmentDeletePrompt(displayName, referencedHere()); return new Promise((resolve) => { // Supersede any confirmation already up — two open at once would // leave one resolver dangling forever. @@ -565,18 +563,16 @@ {/each} {:else} - - settleConfirm(false)}>Cancel - - settleConfirm(true)}> - Delete file - + settleConfirm(false)} + onconfirm={() => settleConfirm(true)} + /> {/if} @@ -636,16 +632,9 @@ padding-inline: 9px; font-size: 12px; line-height: 1.35; - /* Wraps rather than ellipsizes: the delete prompt carries the filename - and must stay readable in full. */ overflow-wrap: anywhere; } - .ap-note-warn { - font-weight: 500; - color: var(--accent-orange); - } - .ap-note-error { color: var(--accent-red); } diff --git a/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts b/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts index c760aebd..574e0784 100644 --- a/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts +++ b/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts @@ -350,7 +350,7 @@ describe('AttachmentPanelHost', () => { await settle(); // The confirmation is a sub-view of the panel, not a window.confirm. - const prompt = document.querySelector('.ap-note-warn'); + const prompt = document.querySelector('.attachment-delete-prompt'); expect(prompt?.getAttribute('role')).toBe('presentation'); expect(prompt?.textContent).toContain("still used in this item's content"); const confirmRows = rows(); @@ -381,7 +381,7 @@ describe('AttachmentPanelHost', () => { row('Delete')!.click(); await settle(); - expect(document.querySelector('.ap-note-warn')?.textContent).toContain( + expect(document.querySelector('.attachment-delete-prompt')?.textContent).toContain( "isn't referenced in this item's content" ); }); @@ -397,7 +397,7 @@ describe('AttachmentPanelHost', () => { row('Delete')!.click(); await settle(); - expect(document.querySelector('.ap-note-warn')?.textContent).toContain( + expect(document.querySelector('.attachment-delete-prompt')?.textContent).toContain( "still used in this item's content" ); }); diff --git a/web/src/lib/components/items/ItemAttachmentStrip.svelte b/web/src/lib/components/items/ItemAttachmentStrip.svelte index f2735ca4..52302ca5 100644 --- a/web/src/lib/components/items/ItemAttachmentStrip.svelte +++ b/web/src/lib/components/items/ItemAttachmentStrip.svelte @@ -32,9 +32,11 @@ * on screen must roll back and toast even if the user hit Retry while * it was in flight. * 3. `paint` — "does the CONTROL the user clicked belong to what is on - * screen?" Both control entry points fence on it — `handleDelete` for + * screen?" Both control entry points fence on it — `requestDelete` for * a tile, `retryLoad` for the error row — at ENTRY, because the other - * two run after an await and no fence can unsend a request. + * two run after an await and no fence can unsend a request. The delete + * confirmation no longer blocks the thread (DR-18 / TASK-2425), so + * `confirmDelete` re-checks the same fence on the far side of it. */ import { onDestroy, untrack } from 'svelte'; import { api, PadApiError } from '$lib/api/client'; @@ -46,6 +48,10 @@ describeAttachmentType, } from '$lib/attachments/display'; import AttachmentIcon from '$lib/attachments/icons/AttachmentIcon.svelte'; + import Menu from '$lib/components/common/Menu.svelte'; + import AttachmentDeleteConfirm, { + attachmentDeletePrompt, + } from '$lib/components/attachments/AttachmentDeleteConfirm.svelte'; import Lightbox, { type LightboxImage } from '$lib/components/common/Lightbox.svelte'; import { attachmentRefsIn } from '$lib/utils/commentAttachments'; import { invalidateAttachmentMetadata } from '$lib/components/editor/attachment-metadata'; @@ -158,6 +164,27 @@ let attachments = $state([]); let expanded = $state(false); let lightbox = $state<{ images: LightboxImage[]; index: number } | null>(null); + /** + * The delete confirmation currently on screen, if any (PLAN-2392 DR-18 / + * TASK-2425). One at a time: opening a second supersedes the first, which + * is what a `` anchored to a single trigger can represent anyway. + * + * `anchor` is the tile's own `×` — the menu positions against it and + * returns focus to it on Cancel / Escape, which is what keeps the control + * keyboard-usable end to end. + * + * `prompt` is captured at OPEN time, not derived: the warning reads the + * editor's live markdown, and re-deriving it while the confirmation is up + * would let the message change under the user as they type. + */ + let pendingDelete = $state<{ + att: StripAttachment; + anchor: HTMLElement | null; + prompt: string; + } | null>(null); + + const uid = $props.id(); + const promptId = `attachment-delete-note-${uid}`; // Three distinguishable states, not two (DR-10). `loadFailed` is what stops // a fetch failure from rendering as "no attachments"; `showLoading` is the @@ -279,6 +306,12 @@ attachments = []; expanded = false; lightbox = null; + // A confirmation left up for a row the user is no longer looking + // at must go with it — confirming it after the switch would + // DELETE the previous view's attachment from behind the new one + // (the entry fence re-check in `confirmDelete` refuses it, but + // leaving the prompt on screen at all is the wrong picture). + pendingDelete = null; deletedIds = new Set(); pendingUploads = []; beyondStripCount = 0; @@ -492,6 +525,10 @@ return registerAttachmentDeletionListener((deletedUuid) => { rememberDeleted(deletedUuid); attachments = attachments.filter((a) => a.id !== deletedUuid); + // The tile this confirmation is anchored to just went away, so the + // menu would be left pointing at a detached element — and the + // question it is asking has already been answered by someone else. + if (pendingDelete?.att.id === deletedUuid) pendingDelete = null; }); }); @@ -630,7 +667,7 @@ /** * A file tile opens the options panel instead of downloading (DR-1). * - * ENTRY-fenced for the same reason `handleDelete` is: the clicked tile was + * ENTRY-fenced for the same reason `requestDelete` is: the clicked tile was * painted for `paint`'s identity, while `itemId` is live and may already * name a different view. `itemId` is the event's ROUTING field — which * `ItemDetail` mount shows the panel — so a stale click would open this @@ -676,30 +713,17 @@ } /** - * Confirm text for a delete (DR-5). + * Open the delete confirmation for a tile (DR-18 / TASK-2425). * - * The "not referenced here" arm deliberately does NOT claim the attachment - * is unused: a reference can live in another item's content, in an item's - * fields JSON, or in any comment. The server's AttachmentReferenced scan - * covers all three, but none of it is visible client-side — so the wording - * stays honest about what we actually checked. + * This used to raise a browser-native `window.confirm`, which meant one + * object had two confirmation styles — the options panel already drilled + * down to an in-app sub-view. Both surfaces now render the SAME + * `AttachmentDeleteConfirm`, wording included; only the container differs + * (the panel's own menu there, a menu anchored to the `×` here). + * + * The delete REQUEST path below is untouched by that change. */ - function confirmMessage(att: StripAttachment): string { - if (referencedIds().has(att.id)) { - return ( - `Delete ${att.filename}?\n\n` + - "It's still used in this item's content — deleting it will leave a " + - '"missing attachment" placeholder where it appears.' - ); - } - return ( - `Delete ${att.filename}?\n\n` + - "It isn't referenced in this item's content, but it may still be " + - 'referenced by another item or a comment. This cannot be undone.' - ); - } - - async function handleDelete(att: StripAttachment) { + function requestDelete(att: StripAttachment, anchor: HTMLElement | null) { if (!canDelete) return; // ENTRY fence (fence 3 — see the header). The clicked tile was painted @@ -713,10 +737,51 @@ // confirm and before the call. if (!paint.isCurrent()) return; - if (typeof window !== 'undefined' && !window.confirm(confirmMessage(att))) return; - // window.confirm blocks the thread, so nothing can have moved between - // the fence above and here. + pendingDelete = { + att, + anchor, + prompt: attachmentDeletePrompt(att.filename, referencedIds().has(att.id)), + }; + } + /** Dismissal that isn't an explicit Cancel — Escape, or a click outside. */ + function dismissDelete() { + pendingDelete = null; + } + + /** + * The Cancel row. Returns focus to the `×` the confirmation was anchored + * to: `window.confirm` restored it for free, and the control is + * opacity-hidden unless its cell has focus-within, so a keyboard user who + * cancels would otherwise be dropped on with the control they came + * from now invisible. + * + * Deliberately NOT wired to `Menu`'s `onclose`: Escape already refocuses + * the trigger inside `Menu`, and an outside click must not have focus + * yanked back off whatever the user just clicked. + */ + function cancelDelete() { + const anchor = pendingDelete?.anchor; + pendingDelete = null; + anchor?.focus(); + } + + /** + * The user confirmed. `window.confirm` blocked the thread, so the entry + * fence taken when the `×` was clicked was still true by definition when it + * returned; an in-app confirmation does NOT block, and the user can switch + * item or workspace while it is up. So the fence — and the permission — are + * re-checked HERE, at the point that actually sends the request. + */ + function confirmDelete() { + const pending = pendingDelete; + pendingDelete = null; + if (!pending) return; + if (!canDelete || !paint.isCurrent()) return; + void performDelete(pending.att); + } + + async function performDelete(att: StripAttachment) { // Capture identity BEFORE the await (fence 2): a switch mid-delete must // not roll the tile back into a DIFFERENT item's strip, and must not // toast over it. The DELETE itself still lands — it targets an id, not a @@ -885,7 +950,7 @@ class="att-delete" title="Delete {att.filename}" aria-label="Delete {att.filename}" - onclick={() => handleDelete(att)} + onclick={(e) => requestDelete(att, e.currentTarget)} > × @@ -922,6 +987,43 @@ {/if} + +{#if pendingDelete} + + + + +{/if} + {#if lightbox} { }); - // ── Delete (TASK-2384) ──────────────────────────────────────────────── + // ── Delete (TASK-2384, confirmation reworked in TASK-2425) ──────────── // // The affordance is gated on ItemDetail's `mutationsEnabled` // (canEdit && !peeking) per PLAN-2382 DR-6, and the confirm text has to // stay honest about what was actually checked (DR-5): "referenced in this // item's content" is knowable client-side; "unused anywhere" is not. + // + // TASK-2425 (PLAN-2392 DR-18) replaced the browser-native `window.confirm` + // these tests used to spy on with the SAME in-app drill-down the options + // panel shows — so they now drive the real rows. That is not a cosmetic + // change to the tests: the native confirm blocked the thread, so nothing + // could move between the entry fence and the request, while the in-app one + // leaves a window in which the user can switch item or workspace. Every + // fence and rollback assertion below is preserved, and the confirmation is + // driven through the rows a user would actually click. function deleteButtons(): HTMLButtonElement[] { return Array.from(target.querySelectorAll('.att-delete')); } + /** Portaled to like every other Menu, so queried document-wide. */ + function confirmPanel(): HTMLElement | null { + return document.querySelector('[role="menu"]'); + } + + function confirmRows(): HTMLElement[] { + return Array.from(document.querySelectorAll('[role="menu"] [role="menuitem"]')); + } + + /** By VISIBLE label — MenuItem's icon span is part of `textContent`. */ + function confirmRow(label: string): HTMLElement | undefined { + return confirmRows().find( + (el) => el.querySelector('.mi-label')?.textContent?.trim() === label + ); + } + + function promptText(): string { + return document.querySelector('.attachment-delete-prompt')?.textContent ?? ''; + } + + /** Click a tile's `×`. Opens the confirmation; sends nothing. */ + function openConfirm(index = 0) { + deleteButtons()[index].click(); + flushSync(); + } + + /** The destructive row — the only thing that issues a DELETE. */ + function clickConfirm() { + confirmRow('Delete file')!.click(); + flushSync(); + } + + function clickCancel() { + confirmRow('Cancel')!.click(); + flushSync(); + } + it('offers no delete control when canDelete is false', async () => { listMock.mockResolvedValue(response([att({ id: 'a1' })])); props.canDelete = false; @@ -863,6 +909,54 @@ describe('ItemAttachmentStrip', () => { expect(buttons[0].disabled).toBe(false); }); + it('confirms in-app, never with a browser dialog, Cancel first (DR-18)', async () => { + // The shape the item menu establishes and the options panel already + // used: prompt as `role="presentation"` (a role="menu" owns only + // menuitem / separator / group children), an aria-describedby + // back-reference from the destructive row so the otherwise-unannounced + // prompt is read out, Cancel FIRST so the menu's focus handoff can + // never land Enter on Delete. + const nativeConfirm = vi.spyOn(window, 'confirm').mockReturnValue(true); + listMock.mockResolvedValue(response([att({ id: 'a1' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + openConfirm(); + + expect(nativeConfirm).not.toHaveBeenCalled(); + const prompt = document.querySelector('.attachment-delete-prompt'); + expect(prompt?.getAttribute('role')).toBe('presentation'); + const rows = confirmRows(); + const labelOf = (el: HTMLElement) => el.querySelector('.mi-label')?.textContent?.trim(); + expect(labelOf(rows[0])).toBe('Cancel'); + expect(labelOf(rows[rows.length - 1])).toBe('Delete file'); + expect(rows[rows.length - 1].getAttribute('aria-describedby')).toBe(prompt?.id); + // Opening the confirmation is not a delete. + expect(deleteMock).not.toHaveBeenCalled(); + nativeConfirm.mockRestore(); + }); + + it('cancelling sends nothing, keeps the tile, and refocuses the × control', async () => { + listMock.mockResolvedValue(response([att({ id: 'a1' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + const closeBtn = deleteButtons()[0]; + openConfirm(); + clickCancel(); + await settle(); + + expect(deleteMock).not.toHaveBeenCalled(); + expect(tiles()).toHaveLength(1); + expect(confirmPanel()).toBeNull(); + // `window.confirm` restored focus for free. The control is also + // opacity-hidden unless its cell has focus-within, so dropping focus to + // would make the affordance vanish under a keyboard user. + expect(document.activeElement).toBe(closeBtn); + }); + it('warns that the attachment is still used in this item content', async () => { // A canonical UUID: attachmentRefsIn() is anchored to that shape (the // ids the upload endpoint returns), so the reference scan only matches @@ -874,16 +968,14 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); - deleteButtons()[0].click(); - await settle(); + openConfirm(); - expect(confirmSpy).toHaveBeenCalledOnce(); - expect(confirmSpy.mock.calls[0][0]).toContain("still used in this item's content"); + expect(promptText()).toContain("still used in this item's content"); // Declined → nothing deleted, tile stays. + clickCancel(); + await settle(); expect(deleteMock).not.toHaveBeenCalled(); expect(tiles()).toHaveLength(1); - confirmSpy.mockRestore(); }); it('never claims an unreferenced attachment is unused', async () => { @@ -893,16 +985,13 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); - deleteButtons()[0].click(); - await settle(); + openConfirm(); - const message = String(confirmSpy.mock.calls[0][0]); + const message = promptText(); // Comment bodies and other items are NOT scanned client-side (DR-5), // so the copy must hedge rather than assert non-use. expect(message).toContain('may still be referenced'); expect(message).not.toContain('not used'); - confirmSpy.mockRestore(); }); it('removes the tile optimistically and calls the API on confirm', async () => { @@ -911,19 +1000,62 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(deleteMock).toHaveBeenCalledWith('ws', 'a1'); expect(tiles()).toHaveLength(1); expect(toastMock).not.toHaveBeenCalled(); + // The confirmation goes with the row it was asking about. + expect(confirmPanel()).toBeNull(); // An already painted in the editor never re-requests, so the // NodeView has to be told or the body keeps showing a deleted image // until reload (Codex round 12). expect(notifyDeletedMock).toHaveBeenCalledWith('a1'); expect(invalidateMock).toHaveBeenCalledWith('ws', 'a1'); - confirmSpy.mockRestore(); + }); + + it('abandons an open confirmation when the item switches under it', async () => { + // The in-app confirmation does NOT block the thread the way + // `window.confirm` did, so this window exists at all only as of + // TASK-2425: the prompt can still be up when the strip repaints for a + // different item. Leaving it there would delete the PREVIOUS item's + // attachment from behind the new one. + listMock.mockResolvedValueOnce(response([att({ id: 'a1' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + openConfirm(); + expect(confirmPanel()).not.toBeNull(); + + listMock.mockResolvedValueOnce(response([att({ id: 'b1' })])); + props.itemId = 'item-b'; + flushSync(); + await settle(); + + expect(confirmPanel()).toBeNull(); + expect(deleteMock).not.toHaveBeenCalled(); + }); + + it('drops an open confirmation when another surface deletes that row', async () => { + // The tile the confirmation is anchored to has just been unmounted, so + // the menu would be left pointing at a detached element — and the + // question it is asking has already been answered. + listMock.mockResolvedValue(response([att({ id: 'a1' }), att({ id: 'a2' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + openConfirm(); + expect(confirmPanel()).not.toBeNull(); + + broadcastDeletion('a1'); + flushSync(); + + expect(confirmPanel()).toBeNull(); + expect(deleteMock).not.toHaveBeenCalled(); }); it('refuses a delete click that lands after the ITEM already switched', async () => { @@ -940,18 +1072,16 @@ describe('ItemAttachmentStrip', () => { expect(deleteButtons()).toHaveLength(1); listMock.mockResolvedValueOnce(response([att({ id: 'b1' })])); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); props.itemId = 'item-b'; // No flushSync between the switch and the click: that IS the window. deleteButtons()[0].click(); await settle(); // Not even prompted — the tile the user aimed at no longer exists. - expect(confirmSpy).not.toHaveBeenCalled(); + expect(confirmPanel()).toBeNull(); expect(deleteMock).not.toHaveBeenCalled(); expect(notifyDeletedMock).not.toHaveBeenCalled(); expect(tiles()[0].getAttribute('aria-label')).toContain('b1.png'); - confirmSpy.mockRestore(); }); it('refuses a delete click that lands after the WORKSPACE already switched', async () => { @@ -964,15 +1094,37 @@ describe('ItemAttachmentStrip', () => { await settle(); listMock.mockResolvedValueOnce(response([att({ id: 'ws2-row' })])); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); props.wsSlug = 'ws2'; deleteButtons()[0].click(); await settle(); - expect(confirmSpy).not.toHaveBeenCalled(); + expect(confirmPanel()).toBeNull(); expect(deleteMock).not.toHaveBeenCalled(); expect(tiles()[0].getAttribute('aria-label')).toContain('ws2-row.png'); - confirmSpy.mockRestore(); + }); + + it('refuses a CONFIRMATION that lands after the item already switched', async () => { + // The window `window.confirm` did not have: it blocked the thread, so + // the entry fence taken when the `×` was clicked was still true by + // definition when it returned. An in-app confirmation can sit on screen + // across a switch, so the fence is re-checked at the point that + // actually sends the request (TASK-2425). + listMock.mockResolvedValueOnce(response([att({ id: 'a1' })])); + props.canDelete = true; + mountStrip('item-a'); + await settle(); + + openConfirm(); + + listMock.mockResolvedValueOnce(response([att({ id: 'b1' })])); + props.itemId = 'item-b'; + // No flushSync: the prompt is still up and the props already read B. + clickConfirm(); + await settle(); + + expect(deleteMock).not.toHaveBeenCalled(); + expect(notifyDeletedMock).not.toHaveBeenCalled(); + expect(tiles()[0].getAttribute('aria-label')).toContain('b1.png'); }); it('rolls the tile back and toasts when the delete fails', async () => { @@ -982,8 +1134,8 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(tiles()).toHaveLength(2); @@ -991,7 +1143,6 @@ describe('ItemAttachmentStrip', () => { expect(notifyDeletedMock).not.toHaveBeenCalled(); expect(toastMock).toHaveBeenCalledOnce(); expect(String(toastMock.mock.calls[0][0])).toContain('a1.png'); - confirmSpy.mockRestore(); }); it('does not roll a failed delete back into a DIFFERENT item strip', async () => { @@ -1010,9 +1161,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); expect(tiles()).toHaveLength(0); // optimistic removal happened // Switch to B before the delete settles. @@ -1028,7 +1178,6 @@ describe('ItemAttachmentStrip', () => { expect(names.some((n) => n?.includes('a1.png'))).toBe(false); expect(names.some((n) => n?.includes('b1.png'))).toBe(true); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('rolls back only the failed row, never resurrecting a concurrent success', async () => { @@ -1048,11 +1197,10 @@ describe('ItemAttachmentStrip', () => { failFirst = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - - deleteButtons()[0].click(); // a1 — in flight, will fail - flushSync(); - deleteButtons()[0].click(); // now b1 — resolves immediately + openConfirm(); // a1 — in flight, will fail + clickConfirm(); + openConfirm(); // now b1 — resolves immediately + clickConfirm(); await settle(); failFirst(new Error('boom')); @@ -1063,7 +1211,6 @@ describe('ItemAttachmentStrip', () => { expect(names.some((n) => n.includes('b1.png'))).toBe(false); // stays deleted // ...and restored at its original position, not appended. expect(names[0]).toContain('a1.png'); - confirmSpy.mockRestore(); }); it('still announces the deletion when the delete 404s', async () => { @@ -1075,13 +1222,12 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(notifyDeletedMock).toHaveBeenCalledWith('a1'); expect(invalidateMock).toHaveBeenCalledWith('ws', 'a1'); - confirmSpy.mockRestore(); }); it('still announces a 404 delete when the view switched under it', async () => { @@ -1102,9 +1248,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); listMock.mockResolvedValue(response([att({ id: 'b1' })])); props.itemId = 'item-b'; @@ -1121,7 +1266,6 @@ describe('ItemAttachmentStrip', () => { expect(names.some((n) => n.includes('a1.png'))).toBe(false); expect(names.some((n) => n.includes('b1.png'))).toBe(true); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('does not roll back a failed delete that another surface already announced', async () => { @@ -1139,9 +1283,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); broadcastDeletion('a1'); flushSync(); @@ -1151,7 +1294,6 @@ describe('ItemAttachmentStrip', () => { const names = tiles().map((el) => el.getAttribute('aria-label') ?? ''); expect(names.some((n) => n.includes('a1.png'))).toBe(false); - confirmSpy.mockRestore(); }); it('keeps the tile removed when the delete 404s (already gone)', async () => { @@ -1164,13 +1306,12 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(tiles()).toHaveLength(1); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('names permission as the reason on a 403, and restores the tile', async () => { @@ -1180,13 +1321,12 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); + openConfirm(); + clickConfirm(); await settle(); expect(tiles()).toHaveLength(1); expect(String(toastMock.mock.calls[0][0])).toContain("don't have permission"); - confirmSpy.mockRestore(); }); it('still rolls back and toasts when a Retry re-ran the load mid-delete', async () => { @@ -1210,9 +1350,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); expect(tiles()).toHaveLength(0); // optimistic removal // Retry while the delete is still in flight. @@ -1228,7 +1367,6 @@ describe('ItemAttachmentStrip', () => { expect(toastMock).toHaveBeenCalledOnce(); expect(String(toastMock.mock.calls[0][0])).toContain('survivor.png'); expect(notifyDeletedMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('suppresses a failed delete when the WORKSPACE changed under it', async () => { @@ -1249,9 +1387,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); expect(tiles()).toHaveLength(0); // Retry clicked, THEN the workspace swapped before the effect flushed — @@ -1270,7 +1407,6 @@ describe('ItemAttachmentStrip', () => { expect(names.some((n) => n.includes('survivor.png'))).toBe(false); expect(names.some((n) => n.includes('other-ws.png'))).toBe(true); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); it('still suppresses a failed delete after an A→B→A round trip', async () => { @@ -1290,9 +1426,8 @@ describe('ItemAttachmentStrip', () => { failDelete = reject; }) ); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true); - deleteButtons()[0].click(); - flushSync(); + openConfirm(); + clickConfirm(); listMock.mockResolvedValue(response([att({ id: 'b1' })])); props.itemId = 'item-b'; @@ -1309,7 +1444,6 @@ describe('ItemAttachmentStrip', () => { expect(tiles()).toHaveLength(0); expect(toastMock).not.toHaveBeenCalled(); - confirmSpy.mockRestore(); }); // ── Upload refresh (TASK-2385) ──────────────────────────────────────── @@ -1655,12 +1789,9 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); - deleteButtons()[0].click(); - await settle(); + openConfirm(); - expect(String(confirmSpy.mock.calls[0][0])).toContain("still used in this item's content"); - confirmSpy.mockRestore(); + expect(promptText()).toContain("still used in this item's content"); }); it('falls back to persisted content when the live read throws', async () => { @@ -1674,11 +1805,8 @@ describe('ItemAttachmentStrip', () => { mountStrip('item-a'); await settle(); - const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false); - deleteButtons()[0].click(); - await settle(); + openConfirm(); - expect(String(confirmSpy.mock.calls[0][0])).toContain("still used in this item's content"); - confirmSpy.mockRestore(); + expect(promptText()).toContain("still used in this item's content"); }); }); diff --git a/web/src/lib/components/settings/StorageTab.svelte b/web/src/lib/components/settings/StorageTab.svelte index dbc3835e..93664447 100644 --- a/web/src/lib/components/settings/StorageTab.svelte +++ b/web/src/lib/components/settings/StorageTab.svelte @@ -19,6 +19,8 @@ type StorageFilterSelections } from '$lib/attachments/storageFilters'; import AttachmentIcon from '$lib/attachments/icons/AttachmentIcon.svelte'; + import Menu from '$lib/components/common/Menu.svelte'; + import AttachmentDeleteConfirm from '$lib/components/attachments/AttachmentDeleteConfirm.svelte'; import { viewIdentity, createFence, createPaintFence } from '$lib/attachments/viewFence'; // ── Props ──────────────────────────────────────────────────────────────── @@ -49,6 +51,22 @@ let loading = $state(true); let usage = $state(null); let attachments = $state([]); + /** + * The delete confirmation currently on screen (PLAN-2392 DR-18 / + * TASK-2425). This row used to raise a browser-native `confirm()`; every + * attachment delete now goes through the same in-app drill-down — Cancel + * first, destructive row last, prompt back-referenced by + * `aria-describedby`. The WORDING stays this surface's own: the strip's + * "referenced in this item's content" check has no meaning in a + * workspace-wide list, and what matters here is the GC grace period. + */ + let pendingDelete = $state<{ + att: AttachmentListItem; + anchor: HTMLElement | null; + prompt: string; + } | null>(null); + const uid = $props.id(); + const promptId = `storage-delete-note-${uid}`; let total = $state(0); let limit = $state(50); let offset = $state(0); @@ -248,6 +266,10 @@ attachments = []; total = 0; usage = null; + // A confirmation left up for a row from the previous workspace + // goes with it: the button it is anchored to has just been + // unmounted, and `confirmDelete`'s fence would refuse it anyway. + pendingDelete = null; void loadWorkspaceView(); return; } @@ -400,6 +422,49 @@ // ── Actions ────────────────────────────────────────────────────────────── + /** + * Open the delete confirmation (PLAN-2392 DR-18 / TASK-2425). + * + * ENTRY fence, for the same reason `handleDelete` re-takes it below: the + * clicked row was painted for the workspace this tab has LOADED, which + * during the prop-update → effect-flush window is not necessarily the one + * `wsSlug` already names. + */ + function requestDelete(att: AttachmentListItem, anchor: HTMLElement | null) { + if (!paint.isCurrent()) return; + pendingDelete = { + att, + anchor, + prompt: `Delete ${att.filename}? The blob is reclaimed by garbage collection after a grace period.`, + }; + } + + /** Escape / outside-click. `Menu` handles the Escape refocus itself. */ + function dismissDelete() { + pendingDelete = null; + } + + /** The Cancel row — returns focus to the button it was anchored to. */ + function cancelDelete() { + const anchor = pendingDelete?.anchor; + pendingDelete = null; + anchor?.focus(); + } + + /** + * The user confirmed. `confirm()` blocked the thread, so the entry fence + * was still true by definition when it returned; an in-app confirmation + * does not, and the workspace can change while it is up — so the fence is + * re-checked at the point that actually sends the request. + */ + function confirmDelete() { + const pending = pendingDelete; + pendingDelete = null; + if (!pending) return; + if (!paint.isCurrent()) return; + void handleDelete(pending.att); + } + async function handleDelete(att: AttachmentListItem) { // ENTRY fence (fence 3). The clicked row was painted for the workspace // this tab has LOADED, which during the prop-update → effect-flush window @@ -417,10 +482,6 @@ // continuation toast and refetch (Codex round 3). const req = viewFence.begin(); - const ok = confirm( - `Delete ${att.filename}? The blob is reclaimed by garbage collection after a grace period.` - ); - if (!ok) return; try { await api.attachments.delete(reqWsSlug, att.id); // Same broadcast the item attachment strip does (PLAN-2382 / @@ -716,7 +777,7 @@ @@ -751,6 +812,33 @@ {/if} + +{#if pendingDelete} + + + +{/if} +