mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-24 19:32:10 +00:00
Merge branch 'task/2425-inapp-confirm' into feat/attachment-options-panel
This commit is contained in:
xarmian
@@ -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.
|
||||
|
||||
@@ -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<boolean>;
|
||||
/** Called after the server confirms the row is gone (204 or 404). */
|
||||
|
||||
@@ -0,0 +1,100 @@
|
||||
<!--
|
||||
AttachmentDeleteConfirm — the ONE delete confirmation for an attachment
|
||||
(PLAN-2392 DR-18, TASK-2425).
|
||||
|
||||
One object must not have two confirmation styles. The options panel drilled
|
||||
down to an in-app sub-view while the strip's hover `×` still raised a
|
||||
browser-native `window.confirm` — different chrome, different focus
|
||||
behaviour, different copy shape, for the same DELETE. This component is what
|
||||
both surfaces render instead, so the shape can only ever be changed in one
|
||||
place.
|
||||
|
||||
THE SHAPE, copied from the item menu's confirm (ItemDetail, PLAN-2326 DR-6):
|
||||
|
||||
- The prompt is `role="presentation"`. A `role="menu"` owns menuitem /
|
||||
separator / group children, and this says explicitly that the prompt is
|
||||
none of them — but presentational text is then never announced on its
|
||||
own, hence...
|
||||
- ...the `aria-describedby` back-reference from the destructive row, which
|
||||
is how a screen-reader user hears WHY they are being asked.
|
||||
- Cancel comes FIRST. The menu's focus handoff lands on the first row, so
|
||||
any other order would put Enter-on-arrival on Delete.
|
||||
- The destructive row comes LAST, `danger`-styled.
|
||||
|
||||
It renders rows only — no `role="menu"` container of its own. Each caller
|
||||
supplies its own `Menu` (the panel drills down inside the one it already
|
||||
has; the strip opens one anchored to the tile's `×`), so ESC ordering,
|
||||
outside-click, portal placement, focus return and the mobile sheet swap are
|
||||
the app's existing behaviours on both surfaces rather than a second
|
||||
implementation.
|
||||
|
||||
The PROMPT TEXT is `attachmentDeletePrompt` below, shared for the same
|
||||
reason the markup is: the hedged arm's honesty is the substance of DR-5, and
|
||||
two copies of it drift.
|
||||
-->
|
||||
<script lang="ts" module>
|
||||
/**
|
||||
* The two arms of the delete warning — the only place either is written.
|
||||
*
|
||||
* `referencedHere` can only ever speak for the body the caller has. So the
|
||||
* "not referenced" 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 and none of it is visible client-side, so the wording
|
||||
* stays hedged (DR-5). Do not "tighten" it.
|
||||
*/
|
||||
export function attachmentDeletePrompt(
|
||||
displayName: string,
|
||||
referencedHere: boolean
|
||||
): string {
|
||||
return 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.`;
|
||||
}
|
||||
</script>
|
||||
|
||||
<script lang="ts">
|
||||
import MenuItem from '$lib/components/common/MenuItem.svelte';
|
||||
|
||||
interface Props {
|
||||
/** The warning line — build it with `attachmentDeletePrompt`. */
|
||||
prompt: string;
|
||||
/**
|
||||
* Unique id for the prompt element. Owned by the caller because it is
|
||||
* the caller that may have several of these on a page; the destructive
|
||||
* row's `aria-describedby` points at it.
|
||||
*/
|
||||
promptId: string;
|
||||
oncancel: () => void;
|
||||
onconfirm: () => void;
|
||||
}
|
||||
|
||||
let { prompt, promptId, oncancel, onconfirm }: Props = $props();
|
||||
</script>
|
||||
|
||||
<div class="attachment-delete-prompt" role="presentation" id={promptId}>{prompt}</div>
|
||||
<MenuItem icon="‹" onclick={oncancel}>Cancel</MenuItem>
|
||||
<div class="attachment-delete-divider" role="separator"></div>
|
||||
<MenuItem icon="🗑" danger describedBy={promptId} onclick={onconfirm}>Delete file</MenuItem>
|
||||
|
||||
<style>
|
||||
/* Logical properties throughout: the prompt carries a filename, which may
|
||||
be 200 characters or right-to-left. */
|
||||
.attachment-delete-prompt {
|
||||
padding-block: 4px 6px;
|
||||
padding-inline: 9px;
|
||||
font-size: 12px;
|
||||
line-height: 1.35;
|
||||
font-weight: 500;
|
||||
color: var(--accent-orange);
|
||||
/* Wraps rather than ellipsizes: the prompt carries the filename and has
|
||||
to stay readable in full. */
|
||||
overflow-wrap: anywhere;
|
||||
}
|
||||
|
||||
.attachment-delete-divider {
|
||||
border-block-start: 1px solid var(--border-subtle);
|
||||
margin-block: 5px;
|
||||
margin-inline: 4px;
|
||||
}
|
||||
</style>
|
||||
@@ -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<boolean> {
|
||||
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<boolean>((resolve) => {
|
||||
// Supersede any confirmation already up — two open at once would
|
||||
// leave one resolver dangling forever.
|
||||
@@ -565,18 +563,16 @@
|
||||
{/each}
|
||||
{:else}
|
||||
<!--
|
||||
Delete confirmation as a drill-down sub-view (DR-18), the same shape
|
||||
as the item menu's: the prompt is presentational, so it is never
|
||||
announced on its own — hence the aria-describedby back-reference
|
||||
from the destructive row — and Cancel comes FIRST so the focusKey
|
||||
handoff can never land Enter on Delete.
|
||||
Delete confirmation as a drill-down sub-view (DR-18). The shape and
|
||||
the wording live in the shared component, which the strip's tile
|
||||
renders too — one confirmation for one object.
|
||||
-->
|
||||
<div class="ap-note ap-note-warn" role="presentation" id={promptId}>{deletePrompt}</div>
|
||||
<MenuItem icon="‹" onclick={() => settleConfirm(false)}>Cancel</MenuItem>
|
||||
<div class="menu-divider" role="separator"></div>
|
||||
<MenuItem icon="🗑" danger describedBy={promptId} onclick={() => settleConfirm(true)}>
|
||||
Delete file
|
||||
</MenuItem>
|
||||
<AttachmentDeleteConfirm
|
||||
prompt={deletePrompt}
|
||||
{promptId}
|
||||
oncancel={() => settleConfirm(false)}
|
||||
onconfirm={() => settleConfirm(true)}
|
||||
/>
|
||||
{/if}
|
||||
</Menu>
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
});
|
||||
|
||||
@@ -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<StripAttachment[]>([]);
|
||||
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 `<Menu>` 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 <body> 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)}
|
||||
>
|
||||
×
|
||||
</button>
|
||||
@@ -922,6 +987,43 @@
|
||||
</section>
|
||||
{/if}
|
||||
|
||||
<!--
|
||||
The delete confirmation (DR-18). The same `Menu` presentation the options
|
||||
panel uses — popover on desktop, BottomSheet at the mobile breakpoint — so
|
||||
ESC ordering, outside-click, portal placement and focus return are the
|
||||
app's existing behaviours rather than a second implementation. Focus
|
||||
returns to the `×` the menu is anchored to, and Cancel is its first row, so
|
||||
Enter on arrival can never delete.
|
||||
-->
|
||||
{#if pendingDelete}
|
||||
<!--
|
||||
Every prop below is read through `?.` even though the block only exists
|
||||
while `pendingDelete` is set: `Menu` places itself in a `tick().then()`,
|
||||
which can run AFTER the confirmation was dismissed and this block torn
|
||||
down, and a prop expression is re-evaluated on every read. Reading
|
||||
`pendingDelete.anchor` there throws an unhandled rejection — real, and
|
||||
observed in the suite.
|
||||
-->
|
||||
<Menu
|
||||
open
|
||||
onclose={dismissDelete}
|
||||
trigger={pendingDelete?.anchor ?? undefined}
|
||||
mode="portal"
|
||||
width={272}
|
||||
sheetOnMobile
|
||||
sheetTitle="Delete {pendingDelete?.att.filename ?? ''}"
|
||||
ariaLabel="Delete {pendingDelete?.att.filename ?? ''}"
|
||||
focusKey={pendingDelete?.att.id}
|
||||
>
|
||||
<AttachmentDeleteConfirm
|
||||
prompt={pendingDelete?.prompt ?? ''}
|
||||
{promptId}
|
||||
oncancel={cancelDelete}
|
||||
onconfirm={confirmDelete}
|
||||
/>
|
||||
</Menu>
|
||||
{/if}
|
||||
|
||||
{#if lightbox}
|
||||
<Lightbox
|
||||
images={lightbox.images}
|
||||
|
||||
@@ -822,17 +822,63 @@ describe('ItemAttachmentStrip', () => {
|
||||
});
|
||||
|
||||
|
||||
// ── 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<HTMLButtonElement>('.att-delete'));
|
||||
}
|
||||
|
||||
/** Portaled to <body> like every other Menu, so queried document-wide. */
|
||||
function confirmPanel(): HTMLElement | null {
|
||||
return document.querySelector<HTMLElement>('[role="menu"]');
|
||||
}
|
||||
|
||||
function confirmRows(): HTMLElement[] {
|
||||
return Array.from(document.querySelectorAll<HTMLElement>('[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
|
||||
// <body> 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 <img> 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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<WorkspaceStorageInfo | null>(null);
|
||||
let attachments = $state<AttachmentListItem[]>([]);
|
||||
/**
|
||||
* 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 @@
|
||||
<button
|
||||
type="button"
|
||||
class="btn btn-small btn-remove"
|
||||
onclick={() => handleDelete(att)}
|
||||
onclick={(e) => requestDelete(att, e.currentTarget)}
|
||||
>
|
||||
Delete
|
||||
</button>
|
||||
@@ -751,6 +812,33 @@
|
||||
{/if}
|
||||
</div>
|
||||
|
||||
<!--
|
||||
The delete confirmation (PLAN-2392 DR-18 / TASK-2425) — the same in-app
|
||||
drill-down the item strip and the options panel show, anchored to the row's
|
||||
own Delete button. Props are read through `?.` because `Menu` places itself
|
||||
in a `tick().then()` that can run after this block is torn down.
|
||||
-->
|
||||
{#if pendingDelete}
|
||||
<Menu
|
||||
open
|
||||
onclose={dismissDelete}
|
||||
trigger={pendingDelete?.anchor ?? undefined}
|
||||
mode="portal"
|
||||
width={272}
|
||||
sheetOnMobile
|
||||
sheetTitle="Delete {pendingDelete?.att.filename ?? ''}"
|
||||
ariaLabel="Delete {pendingDelete?.att.filename ?? ''}"
|
||||
focusKey={pendingDelete?.att.id}
|
||||
>
|
||||
<AttachmentDeleteConfirm
|
||||
prompt={pendingDelete?.prompt ?? ''}
|
||||
{promptId}
|
||||
oncancel={cancelDelete}
|
||||
onconfirm={confirmDelete}
|
||||
/>
|
||||
</Menu>
|
||||
{/if}
|
||||
|
||||
<style>
|
||||
/* ── Local copies of the parent settings page primitives ────────────────
|
||||
* Svelte 5 scopes styles to the component, so a child cannot reach the
|
||||
|
||||
@@ -141,6 +141,27 @@ describe('StorageTab workspace switching', () => {
|
||||
return target.textContent ?? '';
|
||||
}
|
||||
|
||||
/**
|
||||
* The delete confirmation is an in-app drill-down as of TASK-2425
|
||||
* (PLAN-2392 DR-18) — the same one the item strip and the options panel
|
||||
* show — rather than a browser-native `confirm()`. It is portaled to
|
||||
* <body> like every other Menu, so it is queried document-wide.
|
||||
*/
|
||||
function confirmPanel(): HTMLElement | null {
|
||||
return document.querySelector<HTMLElement>('[role="menu"]');
|
||||
}
|
||||
|
||||
/** Click a row's Delete, then the destructive row of the confirmation. */
|
||||
function deleteAndConfirm(index = 0) {
|
||||
Array.from(target.querySelectorAll<HTMLButtonElement>('.btn-remove'))[index].click();
|
||||
flushSync();
|
||||
const confirmRow = Array.from(
|
||||
document.querySelectorAll<HTMLElement>('[role="menu"] [role="menuitem"]')
|
||||
).find((el) => el.querySelector('.mi-label')?.textContent?.trim() === 'Delete file');
|
||||
confirmRow!.click();
|
||||
flushSync();
|
||||
}
|
||||
|
||||
it('loads exactly once on mount', async () => {
|
||||
mountTab();
|
||||
await settle();
|
||||
@@ -277,9 +298,7 @@ describe('StorageTab workspace switching', () => {
|
||||
|
||||
const slowDelete = deferred<void>();
|
||||
deleteMock.mockReturnValueOnce(slowDelete.promise);
|
||||
const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true);
|
||||
target.querySelector<HTMLButtonElement>('.btn-remove')?.click();
|
||||
flushSync();
|
||||
deleteAndConfirm();
|
||||
expect(deleteMock).toHaveBeenCalledWith('ws-a', 'a1');
|
||||
|
||||
listMock.mockResolvedValueOnce(response([att({ id: 'b1', filename: 'from-b.pdf' })]));
|
||||
@@ -299,7 +318,6 @@ describe('StorageTab workspace switching', () => {
|
||||
expect(toastMock).not.toHaveBeenCalled();
|
||||
expect(listMock.mock.calls.length).toBe(listCalls);
|
||||
expect(text()).toContain('from-b.pdf');
|
||||
confirmSpy.mockRestore();
|
||||
});
|
||||
|
||||
it('still suppresses a delete continuation after an A→B→A round trip', async () => {
|
||||
@@ -313,9 +331,7 @@ describe('StorageTab workspace switching', () => {
|
||||
|
||||
const slowDelete = deferred<void>();
|
||||
deleteMock.mockReturnValueOnce(slowDelete.promise);
|
||||
const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true);
|
||||
target.querySelector<HTMLButtonElement>('.btn-remove')?.click();
|
||||
flushSync();
|
||||
deleteAndConfirm();
|
||||
expect(deleteMock).toHaveBeenCalledWith('ws-a', 'a1');
|
||||
|
||||
props.wsSlug = 'ws-b';
|
||||
@@ -339,7 +355,6 @@ describe('StorageTab workspace switching', () => {
|
||||
// against ws-a (Codex round 4).
|
||||
expect(listMock.mock.calls.length).toBe(listCalls + 1);
|
||||
expect(listMock).toHaveBeenLastCalledWith('ws-a', expect.anything());
|
||||
confirmSpy.mockRestore();
|
||||
});
|
||||
|
||||
it('does not toast or refetch for a delete that resolves after unmount', async () => {
|
||||
@@ -349,9 +364,7 @@ describe('StorageTab workspace switching', () => {
|
||||
|
||||
const slowDelete = deferred<void>();
|
||||
deleteMock.mockReturnValueOnce(slowDelete.promise);
|
||||
const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true);
|
||||
target.querySelector<HTMLButtonElement>('.btn-remove')?.click();
|
||||
flushSync();
|
||||
deleteAndConfirm();
|
||||
|
||||
unmount(instance!);
|
||||
instance = undefined;
|
||||
@@ -366,7 +379,6 @@ describe('StorageTab workspace switching', () => {
|
||||
expect(announceMock).toHaveBeenCalledWith('ws-a', 'a1');
|
||||
expect(toastMock).not.toHaveBeenCalled();
|
||||
expect(listMock.mock.calls.length).toBe(listCalls);
|
||||
confirmSpy.mockRestore();
|
||||
});
|
||||
|
||||
it('does not toast for a list or usage request that fails after unmount', async () => {
|
||||
@@ -401,7 +413,6 @@ describe('StorageTab workspace switching', () => {
|
||||
mountTab();
|
||||
await settle();
|
||||
|
||||
const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true);
|
||||
props.wsSlug = 'ws-b';
|
||||
// No flushSync between the switch and the click: that IS the window.
|
||||
target.querySelector<HTMLButtonElement>('.btn-remove')?.click();
|
||||
@@ -409,8 +420,9 @@ describe('StorageTab workspace switching', () => {
|
||||
await settle();
|
||||
|
||||
expect(deleteMock).not.toHaveBeenCalled();
|
||||
expect(confirmSpy).not.toHaveBeenCalled();
|
||||
confirmSpy.mockRestore();
|
||||
// Not even prompted — the row the user aimed at belongs to a workspace
|
||||
// this tab is no longer showing.
|
||||
expect(confirmPanel()).toBeNull();
|
||||
});
|
||||
|
||||
it('ignores a superseded usage response and its error toast', async () => {
|
||||
|
||||
Reference in New Issue
Block a user