mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-25 03:42:06 +00:00
fix(attachments): reconcile the panel on deletion, guard the storage delete
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.
This commit is contained in:
@@ -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(() => {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string>();
|
||||
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<void>();
|
||||
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<WorkspaceStorageInfo>();
|
||||
usageMock.mockReturnValueOnce(slowA.promise);
|
||||
|
||||
Reference in New Issue
Block a user