From 850d3559b4b4b7ca2d03c815ec6daaae2efb207e Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 4 Aug 2026 19:14:40 +0000 Subject: [PATCH] fix(attachments): reconcile the panel on deletion, guard the storage delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final review round 2, probing what a user can do that the code did not anticipate. - An open panel kept offering Download and Delete for an attachment another surface had just deleted. The strip already reconciles on that broadcast; the panel now does too, and CLOSES rather than latching the missing state — unlike a 404 found while opening, where the user asked about this file and deserves the answer, this is an answer to a question nobody asked, and a tombstone panel would be stranger than dismissing it. - Storage could send two DELETEs for one row. `confirm()` blocked the thread, so a second confirmation could not be raised while a request was in flight; an in-app one can, and unlike the item strip this list does not remove the row optimistically, so it stays clickable throughout. The user's single action produced a success AND an "already deleted". Guarded per id, released in a `finally` so a FAILED delete does not strand the row as permanently undeletable. - Storage's 404 arm claimed parity with its success arm — same broadcast, same refresh — and nothing tested it; the shared descriptor's 404 test covers a different implementation. Now tested. Both guards are mutation-tested. --- .../attachments/AttachmentPanelHost.svelte | 16 ++++++ .../AttachmentPanelHost.svelte.test.ts | 23 ++++++++ .../lib/components/settings/StorageTab.svelte | 20 +++++++ .../settings/StorageTab.svelte.test.ts | 54 +++++++++++++++++++ 4 files changed, 113 insertions(+) diff --git a/web/src/lib/components/attachments/AttachmentPanelHost.svelte b/web/src/lib/components/attachments/AttachmentPanelHost.svelte index 02e699f3..54b251a1 100644 --- a/web/src/lib/components/attachments/AttachmentPanelHost.svelte +++ b/web/src/lib/components/attachments/AttachmentPanelHost.svelte @@ -40,6 +40,7 @@ import AttachmentDetailsPanel from './AttachmentDetailsPanel.svelte'; import { isAttachmentPanelEventForHost, + registerAttachmentDeletionListener, registerAttachmentPanelListener, type AttachmentPanelOpenEvent, } from '$lib/attachments/events'; @@ -131,6 +132,21 @@ }); }); + // Someone else deleted the attachment this panel is about — the other pane's + // strip, or the panel in a peeked ItemDetail. The strip already reconciles + // on this channel; without it here, the panel keeps offering Download and + // Delete for a row that is gone (orchestrator's full-diff review round 2). + // + // Close rather than latch the missing state: unlike a 404 discovered while + // opening — where the user asked about THIS file and deserves an answer — + // this is an answer to a question nobody asked, and leaving a tombstone + // panel on screen would be stranger than dismissing it. + $effect(() => { + return registerAttachmentDeletionListener((deletedUuid) => { + if (request?.attachmentId === deletedUuid) request = null; + }); + }); + // A→B item switch: the open panel belongs to the item that is no longer on // screen, and its Delete would be permissioned by the NEW item's gate. $effect(() => { diff --git a/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts b/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts index ae1dc0e5..b064de89 100644 --- a/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts +++ b/web/src/lib/components/attachments/AttachmentPanelHost.svelte.test.ts @@ -559,6 +559,29 @@ describe('AttachmentPanelHost', () => { expect(announceMock).not.toHaveBeenCalled(); }); + it('closes when another surface deletes the attachment it is showing', async () => { + // The strip already reconciles on this channel. Without it here, a panel + // opened from a peeked pane keeps offering Download and Delete for a row + // the other pane just removed (final review round 2). It CLOSES rather + // than latching the missing state: unlike a 404 found while opening, + // where the user asked about this file and deserves the answer, this is + // an answer to a question nobody asked. + const { notifyAttachmentDeleted } = await import('$lib/attachments/events'); + mountHost(propsA); + notifyAttachmentPanelOpen(openEvent()); + await settle(); + expect(panel()).not.toBeNull(); + + // A DIFFERENT attachment going away must not disturb it. + notifyAttachmentDeleted(ATT_ID_2); + await settle(); + expect(panel()).not.toBeNull(); + + notifyAttachmentDeleted(ATT_ID); + await settle(); + expect(panel()).toBeNull(); + }); + it('probes even when the event carries full metadata, if the parent is archived', async () => { // The strip's event carries all three fields, which normally lets the // panel skip the fetch. On an archived parent that would leave Open, diff --git a/web/src/lib/components/settings/StorageTab.svelte b/web/src/lib/components/settings/StorageTab.svelte index 21d8093d..03a87f57 100644 --- a/web/src/lib/components/settings/StorageTab.svelte +++ b/web/src/lib/components/settings/StorageTab.svelte @@ -459,11 +459,26 @@ * 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. */ + /** + * Ids with a DELETE in flight. A plain Set, not `$state`: nothing renders + * from it, and it is read by the guard in `confirmDelete` which needs the + * value as of right now rather than a reactive snapshot. + */ + const deletingIds = new Set(); + function confirmDelete() { const pending = pendingDelete; pendingDelete = null; if (!pending) return; if (!paint.isCurrent()) return; + // One delete at a time. `confirm()` blocked the thread, so a second + // confirmation could not be raised while the first request was in + // flight; an in-app one can, and this list does not remove the row + // optimistically the way the item strip does, so the same row stays + // clickable throughout. Two DELETEs for one row means the second gets a + // 404 and the user sees a success and an "already deleted" for a single + // action (orchestrator's full-diff review round 2). + if (deletingIds.has(pending.att.id)) return; void handleDelete(pending.att); } @@ -484,6 +499,7 @@ // continuation toast and refetch (Codex round 3). const req = viewFence.begin(); + deletingIds.add(att.id); try { await api.attachments.delete(reqWsSlug, att.id); // Same broadcast the item attachment strip does (PLAN-2382 / @@ -535,6 +551,10 @@ if (req.stale()) return; const msg = err instanceof Error ? err.message : 'Failed to delete attachment'; toastStore.show(msg, 'error'); + } finally { + // Every arm above returns from inside the try/catch, so the release + // has to be here or a failed delete would block the row forever. + deletingIds.delete(att.id); } } diff --git a/web/src/lib/components/settings/StorageTab.svelte.test.ts b/web/src/lib/components/settings/StorageTab.svelte.test.ts index 2dc059f7..cdd7d8ad 100644 --- a/web/src/lib/components/settings/StorageTab.svelte.test.ts +++ b/web/src/lib/components/settings/StorageTab.svelte.test.ts @@ -425,6 +425,60 @@ describe('StorageTab workspace switching', () => { expect(confirmPanel()).toBeNull(); }); + it('sends one DELETE even if the row is confirmed twice while the first is in flight', async () => { + // `confirm()` blocked the thread, so a second confirmation could not be + // raised while a request was in flight. An in-app one can — and unlike + // the item strip, this list does not remove the row optimistically, so + // the same row stays clickable throughout. Two DELETEs for one row means + // the second 404s and the user sees a success AND an "already deleted" + // for a single action (final review round 2). + listMock.mockResolvedValueOnce(response([att({ id: 'a1', filename: 'doc.pdf' })])); + mountTab(); + await settle(); + + const slowDelete = deferred(); + deleteMock.mockReturnValueOnce(slowDelete.promise); + deleteAndConfirm(); + expect(deleteMock).toHaveBeenCalledTimes(1); + + deleteAndConfirm(); + expect(deleteMock).toHaveBeenCalledTimes(1); + + // Fail it: the row stays in the list, which is exactly the case where a + // guard that is never released would strand it as undeletable forever. + listMock.mockResolvedValueOnce(response([att({ id: 'a1', filename: 'doc.pdf' })])); + slowDelete.reject(new Error('network')); + await settle(); + expect(rows()).toHaveLength(1); + + deleteMock.mockResolvedValueOnce(undefined); + listMock.mockResolvedValueOnce(response([])); + deleteAndConfirm(); + expect(deleteMock).toHaveBeenCalledTimes(2); + }); + + it('treats a 404 as authoritative, exactly like a success', async () => { + // The 404 arm claims parity with the success arm — same broadcast, same + // refresh — and nothing tested it: the shared descriptor's 404 test + // covers a different implementation (final review round 2). + listMock.mockResolvedValueOnce(response([att({ id: 'a1', filename: 'gone.pdf' })])); + mountTab(); + await settle(); + + deleteMock.mockRejectedValueOnce(new FakeApiError('not_found')); + listMock.mockResolvedValueOnce(response([])); + deleteAndConfirm(); + await settle(); + + // Broadcast like a success, so other surfaces reconcile... + expect(announceMock).toHaveBeenCalledWith('ws-a', 'a1'); + // ...told as information, not as a failure... + expect(toastMock).toHaveBeenCalledWith(expect.stringContaining('already deleted'), 'info'); + expect(toastMock).not.toHaveBeenCalledWith(expect.anything(), 'error'); + // ...and the stale row is gone from the list. + expect(rows()).toHaveLength(0); + }); + it('ignores a superseded usage response and its error toast', async () => { const slowA = deferred(); usageMock.mockReturnValueOnce(slowA.promise);