From c2b2e13abee7fd484ee6ce1735421d853c258f05 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 19:58:44 -0700 Subject: [PATCH] fix(canvas): address code-review findings on the Canvas refactor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five issues surfaced in the review of 50b53784. Each was either a real bug waiting to hit users or a silent failure mode. 1. Topology rescue no longer teleports user-resized children. Rescue was comparing against parentMinSize(childCount), so any child the user had placed in space the parent was resized into got snapped to the default grid on reload — undoing the layout. Now rescue fires only on obviously corrupt data: negative relative coords (legacy pre-nesting absolute positions that landed above/left of their assigned parent) or values past an MAX_PLAUSIBLE_OFFSET threshold. Children just-past the initial minimum are left alone. 2. batchNest now filters to selection-roots before planning. Previously selecting both A and A's descendant B and dragging into T yanked B out of A to become a sibling under T. Users reasonably expect the A subtree to move intact. The new pass drops any selected node whose ancestor is also selected — those follow their ancestor via React Flow's parent binding. 3. batchNest surfaces partial failure via showToast. Previously silent: 2 of 5 PATCHes fail, user sees 3 cards re-parented + 2 snapped back with no explanation. Now names the failed cards. 4. confirmNest closes the nest dialog BEFORE dispatching the async store action, so a second drag can't kick off a competing batch while the first is still in flight. 5. collapsed is now persisted. The Go workspace_crud.go Update handler ignored the `collapsed` field, so user-initiated collapse round-tripped to an expanded state on next hydrate. Added the PATCH branch (`UPDATE workspaces SET collapsed = ...`) so the state survives reload. Nits cleaned: * Removed dead dragStartParentRef in useDragHandlers. * Swapped redundant `node.data as WorkspaceNodeData` casts for a named WorkspaceNode type alias. * Canvas.tsx SR-live region now reads n.parentId (matches MiniMap + RF's native field) instead of the mirror n.data.parentId. Tests added (918 total, up from 915): * batchNest happy path — 2-root selection fires 2 combined PATCHes carrying parent_id + x + y, not 2×N sequential round-trips. * batchNest ancestor+descendant selection — subtree stays intact. * batchNest partial failure rollback — only the rejected nodes revert; successful ones stay committed. Backend change is single-line (collapsed PATCH branch); all workspace_crud Go tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/Canvas.tsx | 4 +- .../src/components/canvas/useDragHandlers.ts | 31 +++++--- canvas/src/store/__tests__/canvas.test.ts | 75 +++++++++++++++++++ canvas/src/store/canvas-topology.ts | 31 ++++---- canvas/src/store/canvas.ts | 43 +++++++++-- .../internal/handlers/workspace_crud.go | 8 ++ 6 files changed, 159 insertions(+), 33 deletions(-) diff --git a/canvas/src/components/Canvas.tsx b/canvas/src/components/Canvas.tsx index 5d13f00a..16c299cb 100644 --- a/canvas/src/components/Canvas.tsx +++ b/canvas/src/components/Canvas.tsx @@ -195,9 +195,9 @@ function CanvasInner() { {/* Screen-reader live region: announces workspace count on canvas load or change */}
- {nodes.filter((n) => !n.data.parentId).length === 0 + {nodes.filter((n) => !n.parentId).length === 0 ? "No workspaces on canvas" - : `${nodes.filter((n) => !n.data.parentId).length} workspace${nodes.filter((n) => !n.data.parentId).length !== 1 ? "s" : ""} on canvas`} + : `${nodes.filter((n) => !n.parentId).length} workspace${nodes.filter((n) => !n.parentId).length !== 1 ? "s" : ""} on canvas`}
{nodes.length === 0 && } diff --git a/canvas/src/components/canvas/useDragHandlers.ts b/canvas/src/components/canvas/useDragHandlers.ts index ee2b711d..9f7e8f26 100644 --- a/canvas/src/components/canvas/useDragHandlers.ts +++ b/canvas/src/components/canvas/useDragHandlers.ts @@ -9,6 +9,8 @@ import { import { useCanvasStore, type WorkspaceNodeData } from "@/store/canvas"; import { clampChildIntoParent, shouldDetach } from "./dragUtils"; +type WorkspaceNode = Node; + export interface PendingNestState { nodeId: string; targetId: string | null; @@ -25,6 +27,8 @@ interface DragHandlers { cancelNest: () => void; } + + /** * Encapsulates every drag gesture on the canvas: * @@ -48,7 +52,6 @@ export function useDragHandlers(): DragHandlers { const isDescendant = useCanvasStore((s) => s.isDescendant); const { getInternalNode } = useReactFlow(); - const dragStartParentRef = useRef(null); const dragModifiersRef = useRef<{ alt: boolean; meta: boolean }>({ alt: false, meta: false, @@ -98,9 +101,8 @@ export function useDragHandlers(): DragHandlers { [getInternalNode, isDescendant], ); - const onNodeDragStart: OnNodeDrag> = useCallback( - (event, node) => { - dragStartParentRef.current = (node.data as WorkspaceNodeData).parentId; + const onNodeDragStart: OnNodeDrag = useCallback( + (event) => { dragModifiersRef.current = { alt: event.altKey, meta: event.metaKey || event.ctrlKey, @@ -109,7 +111,7 @@ export function useDragHandlers(): DragHandlers { [], ); - const onNodeDrag: OnNodeDrag> = useCallback( + const onNodeDrag: OnNodeDrag = useCallback( (event, node) => { dragModifiersRef.current = { alt: event.altKey, @@ -129,13 +131,13 @@ export function useDragHandlers(): DragHandlers { [findDropTarget, getInternalNode, setDragOverNode], ); - const onNodeDragStop: OnNodeDrag> = useCallback( + const onNodeDragStop: OnNodeDrag = useCallback( (event, node) => { const { dragOverNodeId, nodes: allNodes } = useCanvasStore.getState(); setDragOverNode(null); - const nodeName = (node.data as WorkspaceNodeData).name; - const currentParentId = (node.data as WorkspaceNodeData).parentId; + const nodeName = node.data.name; + const currentParentId = node.data.parentId; const altHeld = event.altKey || dragModifiersRef.current.alt; const forceDetach = event.metaKey || event.ctrlKey || dragModifiersRef.current.meta; @@ -188,16 +190,21 @@ export function useDragHandlers(): DragHandlers { const confirmNest = useCallback(() => { if (!pendingNest) return; + // Close the dialog before dispatching the async store action so a + // second drag can't kick off a competing batch while this one is + // still mid-flight. The store actions surface their own errors via + // showToast, so `void` is the right pattern here. + const pending = pendingNest; + setPendingNest(null); const state = useCanvasStore.getState(); if ( state.selectedNodeIds.size > 1 && - state.selectedNodeIds.has(pendingNest.nodeId) + state.selectedNodeIds.has(pending.nodeId) ) { - batchNest(Array.from(state.selectedNodeIds), pendingNest.targetId); + void batchNest(Array.from(state.selectedNodeIds), pending.targetId); } else { - nestNode(pendingNest.nodeId, pendingNest.targetId); + void nestNode(pending.nodeId, pending.targetId); } - setPendingNest(null); }, [pendingNest, nestNode, batchNest]); const cancelNest = useCallback(() => setPendingNest(null), []); diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index 72c2735e..a98340ff 100644 --- a/canvas/src/store/__tests__/canvas.test.ts +++ b/canvas/src/store/__tests__/canvas.test.ts @@ -952,3 +952,78 @@ describe("bumpZOrder", () => { expect(afterZ).toEqual(beforeZ); }); }); + +// ---------- batchNest ---------- + +describe("batchNest", () => { + beforeEach(() => { + (global.fetch as ReturnType).mockClear(); + // Scenario: two root nodes (a, b) and one nested under a (a-child). + // Tests below re-parent various subsets into `target`. + useCanvasStore.getState().hydrate([ + makeWS({ id: "target", name: "Target", x: 1000, y: 0 }), + makeWS({ id: "a", name: "A", x: 0, y: 0 }), + makeWS({ id: "b", name: "B", x: 200, y: 0 }), + makeWS({ id: "a-child", name: "A/Child", parent_id: "a", x: 50, y: 50 }), + ]); + }); + + it("re-parents every selected root into the target via one PATCH each", async () => { + const mock = global.fetch as ReturnType; + mock.mockImplementation(() => + Promise.resolve({ ok: true, json: () => Promise.resolve({}) } as Response), + ); + // Clear any PATCHes that hydrate's computeAutoLayout may have fired + // (auto-positioned workspaces trigger a savePosition → PATCH). + mock.mockClear(); + await useCanvasStore.getState().batchNest(["a", "b"], "target"); + const nodes = useCanvasStore.getState().nodes; + expect(nodes.find((n) => n.id === "a")!.data.parentId).toBe("target"); + expect(nodes.find((n) => n.id === "b")!.data.parentId).toBe("target"); + // Every PATCH fired by batchNest should target /workspaces/ + // and carry `parent_id: "target"` plus absolute x,y. One per root. + const nestPatchCalls = mock.mock.calls.filter((c) => { + const init = c[1] as RequestInit | undefined; + if (init?.method !== "PATCH") return false; + const body = init.body ? JSON.parse(init.body as string) : {}; + return body.parent_id === "target"; + }); + expect(nestPatchCalls).toHaveLength(2); + for (const call of nestPatchCalls) { + const body = JSON.parse((call[1] as RequestInit).body as string); + expect(body.x).toBeTypeOf("number"); + expect(body.y).toBeTypeOf("number"); + } + }); + + it("filters out selected descendants so a subtree moves intact", async () => { + // User selects both A AND its child A/Child, then drags into target. + // Intent: move the A subtree — A/Child stays under A, not target. + (global.fetch as ReturnType).mockImplementation(() => + Promise.resolve({ ok: true, json: () => Promise.resolve({}) } as Response), + ); + await useCanvasStore.getState().batchNest(["a", "a-child"], "target"); + const nodes = useCanvasStore.getState().nodes; + expect(nodes.find((n) => n.id === "a")!.data.parentId).toBe("target"); + // The descendant is NOT independently re-parented; its parent is still A. + expect(nodes.find((n) => n.id === "a-child")!.data.parentId).toBe("a"); + }); + + it("rolls back only the nodes whose PATCH rejected", async () => { + // Reject the PATCH for `a`, accept the one for `b`. + (global.fetch as ReturnType).mockImplementation((url: string) => { + if (typeof url === "string" && url.endsWith("/workspaces/a")) { + return Promise.reject(new Error("network")); + } + return Promise.resolve({ + ok: true, + json: () => Promise.resolve({}), + } as Response); + }); + await useCanvasStore.getState().batchNest(["a", "b"], "target"); + const nodes = useCanvasStore.getState().nodes; + // `a` rolled back to its original parent (null), `b` stayed committed. + expect(nodes.find((n) => n.id === "a")!.data.parentId).toBeNull(); + expect(nodes.find((n) => n.id === "b")!.data.parentId).toBe("target"); + }); +}); diff --git a/canvas/src/store/canvas-topology.ts b/canvas/src/store/canvas-topology.ts index e66ac438..9ae97b4c 100644 --- a/canvas/src/store/canvas-topology.ts +++ b/canvas/src/store/canvas-topology.ts @@ -252,22 +252,25 @@ export function buildNodesAndEdges( const pa = absPos.get(ws.parent_id!)!; position = { x: abs.x - pa.x, y: abs.y - pa.y }; - // Auto-rescue on load: if the child's stored relative position - // would render it outside the parent's current bounding box, drop - // it into the next default grid slot. This fixes three real - // failure modes at once: (1) legacy rows written before nesting - // existed, whose absolute coords have no relation to the parent; - // (2) org-imports at (0, 0); (3) a child whose parent was later - // resized smaller. Dragging a child past the edge after load is - // still the way to un-nest — that's handled separately in - // Canvas.onNodeDragStop with the hysteresis check. - const psize = parentSize.get(ws.parent_id!)!; - const outside = + // Auto-rescue on load — only when the stored relative position + // is clearly impossible for an intentionally-placed child: + // negative coords (child would render to the LEFT or ABOVE its + // parent) or absurdly distant (child would render further than + // MAX_PLAUSIBLE_OFFSET from the parent's origin). Both signal + // legacy data written before nesting existed, where the child's + // absolute canvas coords have no relation to the parent that was + // later assigned to it. Children past the right/bottom edge by + // a small margin are left alone — the user may have resized the + // parent larger on a previous session, and silent teleportation + // on every reload would undo their layout. The manual "Arrange + // Children" context command stays available for cleanup. + const MAX_PLAUSIBLE_OFFSET = 8000; + const corrupted = position.x < 0 || position.y < 0 || - position.x + CHILD_DEFAULT_WIDTH > psize.width || - position.y + CHILD_DEFAULT_HEIGHT > psize.height; - if (outside) { + position.x > MAX_PLAUSIBLE_OFFSET || + position.y > MAX_PLAUSIBLE_OFFSET; + if (corrupted) { const idx = nextChildIndex.get(ws.parent_id!) ?? 0; nextChildIndex.set(ws.parent_id!, idx + 1); position = defaultChildSlot(idx); diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index 10213b3a..6652c9ae 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -6,6 +6,7 @@ import { type NodeChange, } from "@xyflow/react"; import { api } from "@/lib/api"; +import { showToast } from "@/components/Toaster"; import type { WorkspaceData, WSMessage } from "./socket"; import { handleCanvasEvent } from "./canvas-events"; import { @@ -326,8 +327,31 @@ export const useCanvasStore = create((set, get) => ({ batchNest: async (nodeIds, targetId) => { if (nodeIds.length === 0) return; - if (nodeIds.length === 1) { - await get().nestNode(nodeIds[0], targetId); + // Selection-roots filter: if the user selected both A and A's + // descendant B and dragged the pair into T, the intent is "move + // the subtree" — B should stay under A, not become a sibling of + // A under T. Drop every selected node whose ancestor is also + // selected; those will follow their ancestor via React Flow's + // parent-of binding automatically. + const selectedSet = new Set(nodeIds); + const { nodes: before, edges: beforeEdges } = get(); + const byId = new Map(before.map((n) => [n.id, n])); + const rootsOnly: string[] = []; + for (const id of nodeIds) { + let cursor = byId.get(id)?.data.parentId ?? null; + let hasSelectedAncestor = false; + while (cursor) { + if (selectedSet.has(cursor)) { + hasSelectedAncestor = true; + break; + } + cursor = byId.get(cursor)?.data.parentId ?? null; + } + if (!hasSelectedAncestor) rootsOnly.push(id); + } + if (rootsOnly.length === 0) return; + if (rootsOnly.length === 1) { + await get().nestNode(rootsOnly[0], targetId); return; } // Batch path: do all state math against one snapshot so every @@ -338,8 +362,6 @@ export const useCanvasStore = create((set, get) => ({ // in flight at once. For a typical 3-5 node selection on a // ~200ms link this drops the perceived re-parent latency from // ~2s to ~200ms. - const { nodes: before, edges: beforeEdges } = get(); - const byId = new Map(before.map((n) => [n.id, n])); const absOf = (id: string | null | undefined): { x: number; y: number } => { let sum = { x: 0, y: 0 }; @@ -376,7 +398,7 @@ export const useCanvasStore = create((set, get) => ({ const plan: Plan[] = []; const movedIds = new Set(); // Filter out nodes that would be invalid targets / no-ops. - for (const id of nodeIds) { + for (const id of rootsOnly) { const dragged = byId.get(id); if (!dragged) continue; const currentParentId = dragged.data.parentId; @@ -487,6 +509,17 @@ export const useCanvasStore = create((set, get) => ({ }; }), }); + // Surface the partial failure — silent rollback would otherwise + // leave the canvas in a state the user can't explain ("I dragged + // 5 cards, 3 moved and 2 snapped back?"). + const failedNames = rolledBack + .map((id) => byId.get(id)?.data.name) + .filter(Boolean) + .join(", "); + showToast( + `Could not re-parent ${rolledBack.length} of ${plan.length} workspace${plan.length === 1 ? "" : "s"}${failedNames ? `: ${failedNames}` : ""}`, + "error", + ); } }, diff --git a/workspace-server/internal/handlers/workspace_crud.go b/workspace-server/internal/handlers/workspace_crud.go index c1c87556..b991cb2f 100644 --- a/workspace-server/internal/handlers/workspace_crud.go +++ b/workspace-server/internal/handlers/workspace_crud.go @@ -188,6 +188,14 @@ func (h *WorkspaceHandler) Update(c *gin.Context) { log.Printf("Update parent_id error for %s: %v", id, err) } } + if collapsed, ok := body["collapsed"]; ok { + // `collapsed` is the canvas UI-only flag that hides descendants + // in the tree view (WorkspaceNode renders the parent as header- + // only). Persisting it here so the state survives reload. + if _, err := db.DB.ExecContext(ctx, `UPDATE workspaces SET collapsed = $2, updated_at = now() WHERE id = $1`, id, collapsed); err != nil { + log.Printf("Update collapsed error for %s: %v", id, err) + } + } if runtime, ok := body["runtime"]; ok { if _, err := db.DB.ExecContext(ctx, `UPDATE workspaces SET runtime = $2, updated_at = now() WHERE id = $1`, id, runtime); err != nil { log.Printf("Update runtime error for %s: %v", id, err)