fix(canvas): delete workspace dialog race with context menu close
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) <noreply@anthropic.com>
This commit is contained in:
parent
0e70871cfa
commit
0ddf266e54
@ -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} />
|
||||
|
||||
@ -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>
|
||||
);
|
||||
}
|
||||
|
||||
@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
@ -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: {},
|
||||
|
||||
Loading…
Reference in New Issue
Block a user