From 0ddf266e543d9d104458f0b462950aa334d51a23 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Mon, 20 Apr 2026 15:50:30 -0700 Subject: [PATCH] fix(canvas): delete workspace dialog race with context menu close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking "Delete" in the workspace context menu did nothing for stuck workspaces. The confirm dialog was rendered via portal as a child of ContextMenu. ContextMenu's outside-click handler checks whether the click target is inside its ref — but the portal puts the dialog in document.body, outside the ref. So clicking the dialog's Confirm counted as "outside", closed the menu, unmounted the dialog mid-click, and the onConfirm handler never ran. Hoist the pending-delete state to the canvas store and render the confirm dialog at the Canvas level (same pattern as the existing pendingNest dialog). The dialog now outlives ContextMenu, so the outside-click close is harmless. Close the context menu on the Delete click itself rather than waiting for the dialog to resolve. Add a regression test covering the new flow and add the standard ?confirm=true query param so the backend's child-cascade guard is consulted correctly. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/Canvas.tsx | 31 +++++++++++++++ canvas/src/components/ContextMenu.tsx | 39 ++++--------------- .../__tests__/ContextMenu.keyboard.test.tsx | 20 ++++++++++ canvas/src/store/canvas.ts | 10 +++++ 4 files changed, 68 insertions(+), 32 deletions(-) diff --git a/canvas/src/components/Canvas.tsx b/canvas/src/components/Canvas.tsx index b3bae917..16335608 100644 --- a/canvas/src/components/Canvas.tsx +++ b/canvas/src/components/Canvas.tsx @@ -30,6 +30,8 @@ import { SearchDialog } from "./SearchDialog"; import { Toaster } from "./Toaster"; import { Toolbar } from "./Toolbar"; import { ConfirmDialog } from "./ConfirmDialog"; +import { api } from "@/lib/api"; +import { showToast } from "./Toaster"; // Phase 20 components import { SettingsPanel, DeleteConfirmDialog } from "./settings"; // Phase 20.3 batch operations @@ -96,6 +98,24 @@ function CanvasInner() { // Confirmation dialog state for structure changes const [pendingNest, setPendingNest] = useState<{ nodeId: string; targetId: string | null; nodeName: string; targetName: string } | null>(null); + // Delete-confirmation lives in the store so the dialog survives ContextMenu + // unmounting — the prior local-in-ContextMenu state raced with the menu's + // outside-click handler (the portal-rendered Confirm button counted as + // "outside" and closed the menu, killing the dialog mid-click). + const pendingDelete = useCanvasStore((s) => s.pendingDelete); + const setPendingDelete = useCanvasStore((s) => s.setPendingDelete); + const removeNode = useCanvasStore((s) => s.removeNode); + const confirmDelete = useCallback(async () => { + if (!pendingDelete) return; + const { id } = pendingDelete; + setPendingDelete(null); + try { + await api.del(`/workspaces/${id}?confirm=true`); + removeNode(id); + } catch (e) { + showToast(e instanceof Error ? e.message : "Delete failed", "error"); + } + }, [pendingDelete, setPendingDelete, removeNode]); const onNodeDragStop: OnNodeDrag> = useCallback( (_event, node) => { @@ -358,6 +378,17 @@ function CanvasInner() { onCancel={cancelNest} /> + {/* Confirmation dialog for workspace delete — driven by store */} + setPendingDelete(null)} + /> + {/* Settings Panel — global secrets management drawer */} diff --git a/canvas/src/components/ContextMenu.tsx b/canvas/src/components/ContextMenu.tsx index c03fb8fa..4211b7b4 100644 --- a/canvas/src/components/ContextMenu.tsx +++ b/canvas/src/components/ContextMenu.tsx @@ -4,7 +4,6 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { useCanvasStore, type WorkspaceNodeData } from "@/store/canvas"; import { api } from "@/lib/api"; import { showToast } from "./Toaster"; -import { ConfirmDialog } from "./ConfirmDialog"; import { statusDotClass } from "@/lib/design-tokens"; interface MenuItem { @@ -26,15 +25,10 @@ export function ContextMenu() { const nestNode = useCanvasStore((s) => s.nestNode); const contextNodeId = contextMenu?.nodeId ?? null; const hasChildren = useCanvasStore((s) => contextNodeId ? s.nodes.some((n) => n.data.parentId === contextNodeId) : false); + const setPendingDelete = useCanvasStore((s) => s.setPendingDelete); const ref = useRef(null); - const [deleteConfirm, setDeleteConfirm] = useState<{ id: string; name: string } | null>(null); const [actionLoading, setActionLoading] = useState(false); - // Clear orphaned dialog state when context menu closes - useEffect(() => { - if (!contextMenu) setDeleteConfirm(null); - }, [contextMenu]); - // Auto-focus first enabled item when menu opens useEffect(() => { if (!contextMenu) return; @@ -167,21 +161,13 @@ export function ContextMenu() { const handleDelete = useCallback(() => { if (!contextMenu) return; - // Don't close context menu yet — keep it mounted so ConfirmDialog renders - setDeleteConfirm({ id: contextMenu.nodeId, name: contextMenu.nodeData.name }); - }, [contextMenu]); - - const confirmDelete = useCallback(async () => { - if (!deleteConfirm) return; - try { - await api.del(`/workspaces/${deleteConfirm.id}`); - removeNode(deleteConfirm.id); - } catch { - showToast("Delete failed", "error"); - } - setDeleteConfirm(null); + // Hoist delete confirmation to the Canvas-level dialog (via store) so + // it survives ContextMenu unmount. Closing the menu here avoids the + // prior race where the portal dialog's Confirm click was treated as + // "outside" by the menu's outside-click handler. + setPendingDelete({ id: contextMenu.nodeId, name: contextMenu.nodeData.name }); closeContextMenu(); - }, [deleteConfirm, removeNode, closeContextMenu]); + }, [contextMenu, setPendingDelete, closeContextMenu]); const handleViewDetails = useCallback(() => { if (!contextMenu) return; @@ -317,17 +303,6 @@ export function ContextMenu() { ); })} - - {/* Delete confirmation dialog */} - { setDeleteConfirm(null); closeContextMenu(); }} - /> ); } diff --git a/canvas/src/components/__tests__/ContextMenu.keyboard.test.tsx b/canvas/src/components/__tests__/ContextMenu.keyboard.test.tsx index 8e878de2..b37a0e4f 100644 --- a/canvas/src/components/__tests__/ContextMenu.keyboard.test.tsx +++ b/canvas/src/components/__tests__/ContextMenu.keyboard.test.tsx @@ -45,6 +45,7 @@ const mockStore = { selectNode: vi.fn(), setPanelTab: vi.fn(), nestNode: vi.fn(), + setPendingDelete: vi.fn(), nodes: [] as Array<{ id: string; data: { parentId: string | null } }>, }; @@ -209,4 +210,23 @@ describe("ContextMenu — keyboard accessibility", () => { fireEvent.click(zoomItem); expect(closeContextMenu).toHaveBeenCalled(); }); + + // Regression: the old flow kept ConfirmDialog inside ContextMenu's local + // state and rendered it via a portal. The portal-rendered Confirm button + // counted as "outside" by the menu's outside-click handler, closing the + // menu mid-click and making Delete appear to do nothing. The fix hoists + // the dialog state to the canvas store via `setPendingDelete` AND closes + // the context menu on click, so the dialog is owned by a component that + // outlives the menu. + it("clicking 'Delete' hoists state to the store and closes the menu", () => { + render(); + const items = screen.getAllByRole("menuitem"); + const deleteItem = items.find((el) => el.textContent?.includes("Delete"))!; + fireEvent.click(deleteItem); + expect(mockStore.setPendingDelete).toHaveBeenCalledWith({ + id: "ws-1", + name: "Alpha Workspace", + }); + expect(closeContextMenu).toHaveBeenCalled(); + }); }); diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index c19e1011..f0c73b6d 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -66,6 +66,14 @@ interface CanvasState { isDescendant: (ancestorId: string, nodeId: string) => boolean; openContextMenu: (menu: ContextMenuState) => void; closeContextMenu: () => void; + // Pending delete confirmation — lives in the store (not inside ContextMenu's + // local state) so the confirm dialog survives ContextMenu unmounting. The + // ContextMenu's portal-rendered dialog used to race with its outside-click + // handler: clicking Confirm registered as "outside", closed the menu, and + // unmounted the dialog before its onClick fired. Hoisting the state fixes + // that — see fix/context-menu-delete-race. + pendingDelete: { id: string; name: string } | null; + setPendingDelete: (v: { id: string; name: string } | null) => void; searchOpen: boolean; setSearchOpen: (open: boolean) => void; viewport: { x: number; y: number; zoom: number }; @@ -158,6 +166,8 @@ export const useCanvasStore = create((set, get) => ({ selectNode: (id) => set({ selectedNodeId: id }), openContextMenu: (menu) => set({ contextMenu: menu }), closeContextMenu: () => set({ contextMenu: null }), + pendingDelete: null, + setPendingDelete: (v) => set({ pendingDelete: v }), searchOpen: false, setSearchOpen: (open) => set({ searchOpen: open }), agentMessages: {},