Merge pull request #1133 from Molecule-AI/fix/context-menu-delete-race

fix(canvas): delete workspace dialog race with context menu close

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<Node<WorkspaceNodeData>> = useCallback(
     (_event, node) => {
@@ -358,6 +378,17 @@ function CanvasInner() {
         onCancel={cancelNest}
       />
 
+      {/* Confirmation dialog for workspace delete — driven by store */}
+      <ConfirmDialog
+        open={!!pendingDelete}
+        title="Delete Workspace"
+        message={`Permanently delete "${pendingDelete?.name}"? This will stop the container and remove all configuration. This action cannot be undone.`}
+        confirmLabel="Delete"
+        confirmVariant="danger"
+        onConfirm={confirmDelete}
+        onCancel={() => setPendingDelete(null)}
+      />
+
       {/* Settings Panel — global secrets management drawer */}
       <SettingsPanel workspaceId={settingsWorkspaceId} />
       <DeleteConfirmDialog workspaceId={settingsWorkspaceId} />

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<HTMLDivElement>(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() {
           </button>
         );
       })}
-
-      {/* Delete confirmation dialog */}
-      <ConfirmDialog
-        open={!!deleteConfirm}
-        title="Delete Workspace"
-        message={`Permanently delete "${deleteConfirm?.name}"? This will stop the container and remove all configuration. This action cannot be undone.`}
-        confirmLabel="Delete"
-        confirmVariant="danger"
-        onConfirm={confirmDelete}
-        onCancel={() => { setDeleteConfirm(null); closeContextMenu(); }}
-      />
     </div>
   );
 }

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(<ContextMenu />);
+    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();
+  });
 });

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<CanvasState>((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: {},