From 042bd7e4771aba6136dd2cbe05688548b271fabd Mon Sep 17 00:00:00 2001 From: xarmian Date: Tue, 4 Aug 2026 02:05:43 +0000 Subject: [PATCH] fix(attachments): close four review findings across the wave-A surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the orchestrator's fresh-angle Codex pass. All four are the same shape — something read after an await, or captured once and never refreshed — on a component tree built around a no-{#key} item switch. - The delete descriptor snapshotted identity AFTER its confirmation, so an async in-app confirm (which is what DR-18 asks for) left a window where the user could switch items and delete the attachment they were no longer looking at. Snapshot first, re-check the gate and the identity on the way out. - MenuItem's anchor rows had no Space activation. Native anchors take Enter only, and role=menuitem does not add it, so Space would do nothing on Download and Open while working on every button row beside them. - The image NodeView had no destroyed flag, so a HEAD probe in flight at teardown could latch a placeholder onto detached DOM. The chip NodeView has always had one. - CommentEditor configures its extensions once in onMount, but the composer is deliberately reused across an item switch, so its chips kept emitting events addressed to the PREVIOUS item — which the host then correctly ignored, i.e. a tap that silently did nothing. Push the addressing onto the live options, the same way capabilities are pushed. The first two fixes are mutation-tested: reverting either fails the new test. --- web/src/lib/attachments/actions.test.ts | 44 +++++++++++++++++++ web/src/lib/attachments/actions.ts | 22 ++++++++-- web/src/lib/components/CommentEditor.svelte | 31 +++++++++++++ web/src/lib/components/common/MenuItem.svelte | 15 +++++++ .../components/common/MenuItem.svelte.test.ts | 21 +++++++++ .../lib/components/editor/attachment-image.ts | 12 ++++- 6 files changed, 139 insertions(+), 6 deletions(-) diff --git a/web/src/lib/attachments/actions.test.ts b/web/src/lib/attachments/actions.test.ts index 013e807d..40988193 100644 --- a/web/src/lib/attachments/actions.test.ts +++ b/web/src/lib/attachments/actions.test.ts @@ -220,6 +220,50 @@ describe('attachment action descriptors', () => { expect(announceMock).not.toHaveBeenCalled(); }); + it('deletes the attachment the user confirmed, not whatever the context holds later', async () => { + // An in-app confirmation is a whole UI interaction, so the surface can + // switch items underneath it — the pane this renders in is built around + // a no-{#key} A→B switch. The descriptor must act on what was confirmed. + const live: Ctx = ctx(); + const del = action('delete'); + if (del.element !== 'button') throw new Error('unreachable'); + + await del.run({ + ...live, + confirmDelete: async () => { + live.attachment = { id: 'att-OTHER', filename: 'other.pdf', mime_type: 'application/pdf' }; + return true; + }, + get attachment() { + return live.attachment; + }, + } as Ctx); + + // Identity moved while the confirmation was open, so the delete is + // abandoned rather than aimed at the newly-shown attachment. + expect(deleteMock).not.toHaveBeenCalled(); + expect(announceMock).not.toHaveBeenCalled(); + }); + + it('abandons the delete if mutation rights are lost while the confirmation is open', async () => { + const live: Ctx = ctx(); + const del = action('delete'); + if (del.element !== 'button') throw new Error('unreachable'); + + await del.run({ + ...live, + confirmDelete: async () => { + live.mutationsEnabled = false; + return true; + }, + get mutationsEnabled() { + return live.mutationsEnabled; + }, + } as Ctx); + + expect(deleteMock).not.toHaveBeenCalled(); + }); + it('treats a 404 as authoritative: broadcasts and does not throw', async () => { deleteMock.mockRejectedValue(Object.assign(new Error('gone'), { code: 'not_found' })); const onDeleted = vi.fn(); diff --git a/web/src/lib/attachments/actions.ts b/web/src/lib/attachments/actions.ts index eb45ccea..c0c3a7b5 100644 --- a/web/src/lib/attachments/actions.ts +++ b/web/src/lib/attachments/actions.ts @@ -208,13 +208,27 @@ export const ATTACHMENT_ACTIONS: readonly AttachmentAction[] = [ // Belt and braces: a renderer that draws a disabled row can still // be asked to run it by a stray keyboard activation. if (!ctx.mutationsEnabled || !addressable(ctx)) return; - if (ctx.confirmDelete && !(await ctx.confirmDelete(ctx.attachment))) return; - // Capture identity before the await: the surface may switch views - // mid-flight, and the broadcast + metadata-cache key must name the - // workspace the DELETE actually targeted. + // Snapshot identity BEFORE the confirmation, not after. `ctx` may be + // a live object owned by a surface that survives an item switch (the + // no-{#key} pane is built around exactly that), and `confirmDelete` + // is allowed to be async — an in-app confirmation is a whole UI + // interaction, so the user has all the time in the world to switch + // items or lose their mutation rights while it is up. Reading + // `ctx.attachment.id` after the await could name a DIFFERENT + // attachment than the one the user was shown. const ws = ctx.workspaceSlug; const id = ctx.attachment.id; + const subject = { ...ctx.attachment }; + + if (ctx.confirmDelete && !(await ctx.confirmDelete(subject))) return; + + // Re-check the gate on the way out of the confirmation: permission + // can be revoked while it is open (a pane being peeked closes the + // mutation gate), and identity must still agree with what was + // confirmed. + if (!ctx.mutationsEnabled) return; + if (ctx.workspaceSlug !== ws || ctx.attachment.id !== id) return; try { await api.attachments.delete(ws, id); } catch (err) { diff --git a/web/src/lib/components/CommentEditor.svelte b/web/src/lib/components/CommentEditor.svelte index 411c9037..360d0116 100644 --- a/web/src/lib/components/CommentEditor.svelte +++ b/web/src/lib/components/CommentEditor.svelte @@ -206,6 +206,37 @@ empty = editor.isEmpty; }); + /** + * Keep the attachment NodeViews' addressing current (PLAN-2392 DR-8). + * + * The extensions are configured once, inside `onMount`, which captures + * whatever `itemId` / `hostToken` were at that moment. That is fine for the + * body editor — it is remounted per item behind a `{#key}` — but this + * composer is deliberately REUSED across a no-{#key} item switch (see + * `doSubmit` above, which exists for the same reason). Left alone, a chip + * in the composer would keep emitting events addressed to the PREVIOUS + * item, and the host, which matches on both fields, would correctly ignore + * them — a tap that silently does nothing. + * + * Mutating the live extension options is the established shape here; + * `Editor.svelte` pushes `supportedFormats` the same way once server + * capabilities resolve. The NodeViews read these at emit time, so a write + * is enough — nothing needs to re-render. + */ + $effect(() => { + const nextItemId = itemId ?? ''; + const nextToken = hostToken; + if (!editor || editor.isDestroyed) return; + for (const name of ['attachmentChip', 'attachmentImage']) { + const ext = editor.extensionManager.extensions.find( + (e: { name: string }) => e.name === name + ); + if (!ext) continue; + ext.options.itemId = nextItemId; + ext.options.hostToken = nextToken; + } + }); + onDestroy(() => { editor?.destroy(); }); diff --git a/web/src/lib/components/common/MenuItem.svelte b/web/src/lib/components/common/MenuItem.svelte index a794db6b..f3979223 100644 --- a/web/src/lib/components/common/MenuItem.svelte +++ b/web/src/lib/components/common/MenuItem.svelte @@ -60,6 +60,20 @@ // skips rows with `[role^="menuitem"]:not(:disabled)` (Menu.svelte:130), // which no anchor can ever match. const asAnchor = $derived(href !== undefined && !disabled); + + /** + * A native anchor activates on Enter but NOT on Space, while every other + * row in this menu is a