From c4189b1dc66b0ede91347ac0a91671a7f4170c40 Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 4 Aug 2026 13:48:05 +0000 Subject: [PATCH] fix(attachments): one in-app delete confirmation, everywhere (TASK-2425) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The strip's hover `×` raised a browser-native `window.confirm` while the options panel — and the rest of the item UI — drilled down to an in-app sub-view. Two confirmation styles for one object is exactly what DR-18 exists to prevent, and the settings Storage tab's row Delete was on a native `confirm()` too. All three now render one shared `AttachmentDeleteConfirm`: prompt as `role="presentation"` carrying an id, `aria-describedby` back-reference from the destructive row, Cancel FIRST so the focus handoff can never land Enter on Delete, destructive row last. It renders rows only — each surface supplies its own `Menu`, so ESC ordering, outside-click, portal placement, focus return and the mobile sheet swap stay the app's existing behaviours rather than a second implementation. Both warning arms carry through verbatim, from one shared builder: the referenced arm and the hedged one, which stays hedged because the check can only ever speak for the item it has. The Storage tab keeps its own wording (the GC grace period) — a reference check has no meaning in a workspace-wide list — but shares the shape. The delete REQUEST paths are untouched: same entry paint fence, same `viewFence.begin()`, same optimistic removal and single-row rollback, same 404-is-authoritative arm, same `announceAttachmentDeleted`. One addition each: `window.confirm` blocked the thread, so the entry fence was still true by definition when it returned — an in-app confirmation does not, so the fence is re-checked where the request is actually sent, and an open confirmation is abandoned when the view changes under it or another surface deletes the row. Also fixes an unhandled rejection the suite surfaced: `Menu` places itself in a `tick().then()` that can run after its block is torn down, so every prop expression reading the pending state needs `?.`. Tests: the ~18 strip tests (and 4 Storage tab tests) that spied on `window.confirm` now drive the real rows; every message-arm, fence and rollback assertion is preserved. New coverage for the confirmation's shape, Cancel's focus return, the confirm-time fence, and abandonment on switch / external delete. The e2e strip spec drops its `dialog` handler and pins the 24×24 target size in a real browser. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC --- web/e2e/item-attachment-strip.spec.ts | 25 +- web/src/lib/attachments/actions.ts | 9 +- .../AttachmentDeleteConfirm.svelte | 100 +++++++ .../attachments/AttachmentDetailsPanel.svelte | 59 ++-- .../AttachmentPanelHost.svelte.test.ts | 6 +- .../items/ItemAttachmentStrip.svelte | 160 ++++++++-- .../items/ItemAttachmentStrip.svelte.test.ts | 274 +++++++++++++----- .../lib/components/settings/StorageTab.svelte | 98 ++++++- .../settings/StorageTab.svelte.test.ts | 42 ++- 9 files changed, 603 insertions(+), 170 deletions(-) create mode 100644 web/src/lib/components/attachments/AttachmentDeleteConfirm.svelte 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} +