From 21db85d691e7a616a4364ac5c99367ed76b6ea56 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Fri, 24 Apr 2026 20:51:09 -0700 Subject: [PATCH] fix(canvas): cascade delete locally so children disappear without WS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a parent on a wedged WS used to leave the child cards on the canvas as orphaned roots until the user manually refreshed. Why: Canvas.tsx and DetailsTab.tsx both called `removeNode(parentId)` after `DELETE /workspaces/:id?confirm=true` returned 200. `removeNode` deliberately re-parents children rather than cascading — it relies on the per-descendant WORKSPACE_REMOVED WS events the platform emits as part of the cascade to drop each child individually. When the WS is unhealthy those events never arrive, so the local store keeps the children alive (now re-parented to root since their actual parent is gone). Fix: new `removeSubtree(rootId)` action on the canvas store mirrors the server-side cascade — drops the root + every descendant + every incident edge in one atomic set(). Both delete call sites now use it. The WS events still arrive when WS is healthy and become idempotent no-ops because the nodes are already gone. Why a new action instead of changing removeNode: removeNode's re-parenting behavior is correct for non-cascading flows (drag-out, manual node detach in the future). Adding a sibling action keeps both call shapes available rather than forcing every caller to opt out of cascade. 6 new unit tests cover root cascade, mid-level cascade, leaf no-op-cascade, selection clearing across the subtree, selection preservation outside the subtree, and edge cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/Canvas.tsx | 17 +++--- canvas/src/components/tabs/DetailsTab.tsx | 8 ++- canvas/src/store/__tests__/canvas.test.ts | 64 +++++++++++++++++++++++ canvas/src/store/canvas.ts | 47 +++++++++++++++++ 4 files changed, 126 insertions(+), 10 deletions(-) diff --git a/canvas/src/components/Canvas.tsx b/canvas/src/components/Canvas.tsx index 19c6dc41..0d793df5 100644 --- a/canvas/src/components/Canvas.tsx +++ b/canvas/src/components/Canvas.tsx @@ -183,7 +183,7 @@ function CanvasInner() { // outside-click handler. const pendingDelete = useCanvasStore((s) => s.pendingDelete); const setPendingDelete = useCanvasStore((s) => s.setPendingDelete); - const removeNode = useCanvasStore((s) => s.removeNode); + const removeSubtree = useCanvasStore((s) => s.removeSubtree); const confirmDelete = useCallback(async () => { if (!pendingDelete) return; const { id } = pendingDelete; @@ -207,12 +207,13 @@ function CanvasInner() { state.beginDelete(subtree); try { await api.del(`/workspaces/${id}?confirm=true`); - removeNode(id); - // Server-side cascade will emit WORKSPACE_REMOVED per node; - // handleCanvasEvent drops each from the store. Clear the - // deleting set in one shot once the DELETE resolves so any - // node that lags the WS (or is preserved locally, e.g. an - // external workspace) doesn't stay dimmed forever. + // Mirror the server-side cascade locally — drop the parent AND + // every descendant in one atomic update. The per-descendant + // WORKSPACE_REMOVED WS events still arrive (and are no-ops + // because the nodes are already gone), but we no longer depend + // on them: a wedged WS used to leave orphan child cards on the + // canvas until the user refreshed the page. + removeSubtree(id); state.endDelete(subtree); } catch (e) { // Network or server error — restore the subtree to normal @@ -220,7 +221,7 @@ function CanvasInner() { state.endDelete(subtree); showToast(e instanceof Error ? e.message : "Delete failed", "error"); } - }, [pendingDelete, setPendingDelete, removeNode]); + }, [pendingDelete, setPendingDelete, removeSubtree]); const onPaneClick = useCallback(() => { selectNode(null); diff --git a/canvas/src/components/tabs/DetailsTab.tsx b/canvas/src/components/tabs/DetailsTab.tsx index 8bd0d69a..211b1637 100644 --- a/canvas/src/components/tabs/DetailsTab.tsx +++ b/canvas/src/components/tabs/DetailsTab.tsx @@ -36,7 +36,7 @@ export function DetailsTab({ workspaceId, data }: Props) { const [restartError, setRestartError] = useState(null); const [consoleOpen, setConsoleOpen] = useState(false); const updateNodeData = useCanvasStore((s) => s.updateNodeData); - const removeNode = useCanvasStore((s) => s.removeNode); + const removeSubtree = useCanvasStore((s) => s.removeSubtree); const selectNode = useCanvasStore((s) => s.selectNode); // Ref for the "Delete Workspace" trigger — Cancel returns focus here const deleteButtonRef = useRef(null); @@ -94,7 +94,11 @@ export function DetailsTab({ workspaceId, data }: Props) { setDeleteError(null); try { await api.del(`/workspaces/${workspaceId}?confirm=true`); - removeNode(workspaceId); + // Mirror the server-side cascade — drop the row + every + // descendant locally so the canvas reflects the deletion + // immediately, even when the WS is dead and the per-descendant + // WORKSPACE_REMOVED events never arrive. + removeSubtree(workspaceId); selectNode(null); } catch (e) { setDeleteError(e instanceof Error ? e.message : "Failed to delete"); diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index df0460f6..790833f0 100644 --- a/canvas/src/store/__tests__/canvas.test.ts +++ b/canvas/src/store/__tests__/canvas.test.ts @@ -484,6 +484,70 @@ describe("removeNode", () => { }); }); +// ---------- removeSubtree ---------- + +describe("removeSubtree", () => { + beforeEach(() => { + useCanvasStore.getState().hydrate([ + makeWS({ id: "root" }), + makeWS({ id: "mid", parent_id: "root" }), + makeWS({ id: "leaf", parent_id: "mid" }), + makeWS({ id: "sibling", parent_id: "root" }), + makeWS({ id: "unrelated" }), // separate root + ]); + }); + + it("removes the root and every descendant in one shot", () => { + useCanvasStore.getState().removeSubtree("root"); + const ids = useCanvasStore + .getState() + .nodes.map((n) => n.id) + .sort(); + expect(ids).toEqual(["unrelated"]); + }); + + it("removes a mid-level node and its descendants but leaves siblings + ancestors", () => { + useCanvasStore.getState().removeSubtree("mid"); + const ids = useCanvasStore + .getState() + .nodes.map((n) => n.id) + .sort(); + expect(ids).toEqual(["root", "sibling", "unrelated"]); + }); + + it("removing a leaf is a no-op cascade (just drops the leaf)", () => { + useCanvasStore.getState().removeSubtree("leaf"); + const ids = useCanvasStore + .getState() + .nodes.map((n) => n.id) + .sort(); + expect(ids).toEqual(["mid", "root", "sibling", "unrelated"]); + }); + + it("clears selection when the selected node is anywhere in the removed subtree", () => { + useCanvasStore.getState().selectNode("leaf"); + useCanvasStore.getState().removeSubtree("root"); + expect(useCanvasStore.getState().selectedNodeId).toBeNull(); + }); + + it("preserves selection when the selected node is outside the removed subtree", () => { + useCanvasStore.getState().selectNode("unrelated"); + useCanvasStore.getState().removeSubtree("root"); + expect(useCanvasStore.getState().selectedNodeId).toBe("unrelated"); + }); + + it("drops edges incident to any removed node", () => { + // The hydrate-built edges connect parent → child. After removing + // `root`, no edge involving root/mid/leaf/sibling should remain. + useCanvasStore.getState().removeSubtree("root"); + const remaining = useCanvasStore.getState().edges; + for (const e of remaining) { + expect(["root", "mid", "leaf", "sibling"]).not.toContain(e.source); + expect(["root", "mid", "leaf", "sibling"]).not.toContain(e.target); + } + }); +}); + // ---------- isDescendant ---------- describe("isDescendant", () => { diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index 2971de39..f9565e47 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -132,6 +132,16 @@ interface CanvasState { updateNodeData: (id: string, data: Partial) => void; restartWorkspace: (id: string) => Promise; removeNode: (id: string) => void; + /** Remove a node AND every descendant in one atomic update. Mirrors + * the server-side cascade — `DELETE /workspaces/:id?confirm=true` + * drops the row plus every descendant in one transaction. The + * caller (Canvas / DetailsTab delete handlers) used to call + * `removeNode(rootId)` and rely on per-descendant WORKSPACE_REMOVED + * WS events to clear the rest. When the WS is unhealthy those + * events never arrive and the children orphan to the root until a + * manual page refresh — `removeSubtree` makes the cascade + * WS-independent. */ + removeSubtree: (rootId: string) => void; setDragOverNode: (id: string | null) => void; nestNode: (draggedId: string, targetId: string | null) => Promise; isDescendant: (ancestorId: string, nodeId: string) => boolean; @@ -789,6 +799,43 @@ export const useCanvasStore = create((set, get) => ({ }); }, + removeSubtree: (rootId) => { + const { nodes, edges, selectedNodeId } = get(); + // Build a parentId → childIds index once so the descent is O(N), + // not O(N · depth). The store typically holds <500 nodes; even + // doing a linear scan per parent would be fine, but the index + // keeps the cost predictable as orgs grow. + const childrenByParent = new Map(); + for (const n of nodes) { + const p = n.data.parentId ?? null; + if (p === null) continue; + const arr = childrenByParent.get(p); + if (arr) arr.push(n.id); + else childrenByParent.set(p, [n.id]); + } + const removed = new Set([rootId]); + const stack = [rootId]; + while (stack.length) { + const cur = stack.pop()!; + const kids = childrenByParent.get(cur); + if (!kids) continue; + for (const k of kids) { + if (!removed.has(k)) { + removed.add(k); + stack.push(k); + } + } + } + set({ + nodes: nodes.filter((n) => !removed.has(n.id)), + edges: edges.filter((e) => !removed.has(e.source) && !removed.has(e.target)), + selectedNodeId: + selectedNodeId !== null && removed.has(selectedNodeId) + ? null + : selectedNodeId, + }); + }, + hydrate: (workspaces: WorkspaceData[]) => { const layoutOverrides = computeAutoLayout(workspaces); // Carry the live measured/grown parent sizes from the existing