From 7356cf8d3ab2108fefbc96c47532b7fede4a58d0 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 17:43:25 -0700 Subject: [PATCH 01/17] fix(chat): clear sending spinner when any path delivers the reply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two latent bugs kept the "Processing with Claude Code..." timer ticking after the agent had already answered: 1. The A2A_RESPONSE store handler wrote into agentMessages[workspaceId] (no prefix) but ChatTab's "clear sending" effect subscribed to agentMessages["a2a:" + workspaceId]. Keys never matched — the effect was dead code from day one. Removed the dead subscription and moved the setSending(false) into the pendingAgentMsgs effect so any reply delivered via a WS push (Claude Code SDK, Hermes's send_message_to_user) also closes the spinner. 2. Added an activity-log fallback: when the platform emits a successful a2a_receive ACTIVITY_LOGGED for this workspace, clear sending and stop the timer. That covers the "runtime answered but we never saw the store message" case Claude Code exhibited tonight — the HTTP request can stay in flight while the SDK already pushed its reply. Symmetric a2a_receive error path also clears sending and surfaces the error message, so a runtime-side failure no longer hangs the UI. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/tabs/ChatTab.tsx | 47 ++++++++++++++++---------- 1 file changed, 29 insertions(+), 18 deletions(-) diff --git a/canvas/src/components/tabs/ChatTab.tsx b/canvas/src/components/tabs/ChatTab.tsx index daf6d48f..db4da393 100644 --- a/canvas/src/components/tabs/ChatTab.tsx +++ b/canvas/src/components/tabs/ChatTab.tsx @@ -200,7 +200,12 @@ function MyChatPanel({ workspaceId, data }: Props) { bottomRef.current?.scrollIntoView({ behavior: "smooth" }); }, [messages]); - // Consume agent push messages (send_message_to_user) from global store + // Consume agent push messages (send_message_to_user) from global store. + // Runtimes like Claude Code SDK deliver their reply via a WS push rather + // than the /a2a HTTP response — when that happens, the push is the + // authoritative "reply arrived" signal for the UI, so clear `sending` + // here too. The HTTP .then() coordinates through sendingFromAPIRef so + // whichever path clears first wins. const pendingAgentMsgs = useCanvasStore((s) => s.agentMessages[workspaceId]); useEffect(() => { if (!pendingAgentMsgs || pendingAgentMsgs.length === 0) return; @@ -213,23 +218,11 @@ function MyChatPanel({ workspaceId, data }: Props) { // push for the same content). setMessages((prev) => appendMessageDeduped(prev, createMessage("agent", m.content))); } - }, [pendingAgentMsgs, workspaceId]); - - // Consume A2A_RESPONSE events from global store (streaming response delivery). - // Guarded by sendingFromAPIRef to avoid duplicate messages when the - // synchronous HTTP .then() handler also fires for the same response. - const pendingA2AResponse = useCanvasStore((s) => s.agentMessages[`a2a:${workspaceId}`]); - useEffect(() => { - if (!pendingA2AResponse || pendingA2AResponse.length === 0) return; - const consume = useCanvasStore.getState().consumeAgentMessages; - const msgs = consume(`a2a:${workspaceId}`); - if (!sendingFromAPIRef.current) return; // HTTP .then() already handled this response - for (const m of msgs) { - setMessages((prev) => appendMessageDeduped(prev, createMessage("agent", m.content))); + if (sendingFromAPIRef.current && msgs.length > 0) { + setSending(false); + sendingFromAPIRef.current = false; } - setSending(false); - sendingFromAPIRef.current = false; - }, [pendingA2AResponse, workspaceId]); + }, [pendingAgentMsgs, workspaceId]); // Resolve workspace ID → name for activity display const resolveWorkspaceName = useCallback((id: string) => { @@ -281,8 +274,24 @@ function MyChatPanel({ workspaceId, data }: Props) { if (status === "ok" && durationMs) { const sec = Math.round(durationMs / 1000); line = `← ${targetName} responded (${sec}s)`; + // The platform logs a successful a2a_receive once the workspace + // has fully produced its reply. That's the authoritative "done" + // signal for the spinner — clear it even if the reply hasn't + // surfaced through the store yet (it may be delivered shortly + // via pendingAgentMsgs or the HTTP .then()). + const own = (targetId || msg.workspace_id) === workspaceId; + if (own && sendingFromAPIRef.current) { + setSending(false); + sendingFromAPIRef.current = false; + } } else if (status === "error") { line = `⚠ ${targetName} error`; + const own = (targetId || msg.workspace_id) === workspaceId; + if (own && sendingFromAPIRef.current) { + setSending(false); + sendingFromAPIRef.current = false; + setError("Agent error (Exception) — see workspace logs for details."); + } } } else if (type === "a2a_send") { const targetName = resolveWorkspaceName(targetId); @@ -301,7 +310,9 @@ function MyChatPanel({ workspaceId, data }: Props) { setActivityLog((prev) => [...prev.slice(-8), `⟳ ${task}`]); } } - // A2A_RESPONSE is handled by the store (pendingA2AResponse effect) — no duplicate here + // A2A_RESPONSE is already consumed by the store and its text is + // appended to messages via the pendingAgentMsgs effect above; we + // don't need to duplicate it here. } catch { /* ignore */ } }; From 8a07cf40356d70853c4a09e110ad7893548950de Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 17:49:01 -0700 Subject: [PATCH 02/17] fix(canvas): skip already-nested workspaces as drop targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dragging one workspace onto another could pick a nested child as the "nearest" drop target instead of the visible parent card the user actually hovered. The effect: dropping a free-floating Hermes Agent onto a Claude Code Agent that already had a Hermes Agent nested inside showed "Move 'Hermes Agent' inside 'Hermes Agent'?" — the confirmation referenced the nested same-named child, not Claude Code. Why: getIntersectingNodes returns every overlapping node, including hidden=true children that render inside their parent's card. The parent and child share bounding boxes, so the child often "won" the nearest-distance check. Filter them out at the source: a node that's already got a parentId (or is hidden) is never a valid top-level drop target. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/Canvas.tsx | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/canvas/src/components/Canvas.tsx b/canvas/src/components/Canvas.tsx index 0cb3c3de..621da392 100644 --- a/canvas/src/components/Canvas.tsx +++ b/canvas/src/components/Canvas.tsx @@ -96,6 +96,14 @@ function CanvasInner() { let nearest: { id: string; dist: number } | null = null; for (const candidate of getIntersectingNodes(node)) { if (candidate.id === node.id || isDescendant(node.id, candidate.id)) continue; + // Skip nodes already nested inside another workspace: they render + // as TEAM MEMBERS rows inside their parent card and share its + // bounding box, so getIntersectingNodes would otherwise pick the + // nested child (same "Hermes Agent" name) over the visible parent + // the user actually dropped onto. Hidden nodes + nodes with a + // parentId are never valid top-level drop targets. + const candData = candidate.data as WorkspaceNodeData | undefined; + if (candidate.hidden || candData?.parentId) continue; const dx = candidate.position.x - node.position.x; const dy = candidate.position.y - node.position.y; const dist2 = dx * dx + dy * dy; From cc194f0b7e97f52cef68d18cb880e20843c76266 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 18:18:44 -0700 Subject: [PATCH 03/17] refactor(canvas): flat workspace cards with React Flow native parenting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every workspace now renders as a first-class card on the canvas regardless of parent_id. The old "parent card contains mini TeamMember chips" layout is gone — if B is parented to A, B renders as a full card inside A's coordinate space using React Flow's `parentId` binding, so moving A carries B along and children have the same detail + actions as root cards. Details: - canvas-topology.ts: topologically sort parents before children (React Flow ordering requirement), compute each child's RF-native parentId + relative position on load. DB keeps absolute x/y; the abs→rel conversion happens here, reverse translation in Canvas.onNodeDragStop before savePosition PATCHes the DB. - WorkspaceNode.tsx: delete the EmbeddedTeam + TeamMemberChip blocks, simplify the size classes, and add NodeResizer (visible when selected) so users can drag any edge/corner to grow or shrink. Parent cards default to a larger min size so nested children have breathing room. - Canvas.tsx drop targeting rewritten: bounds-based hit test against each node's measured absolute bbox, deepest match wins. Fixes two prior bugs at once — dropping onto Claude Code with a nested same- named Hermes no longer picks the wrong node, and the target can now be a nested workspace when that's where the pointer actually released. - canvas.ts nestNode + removeNode: translate position between old and new parent's absolute origin on nest/unnest so the card doesn't jump, and re-point the RF `parentId` alongside `data.parentId` on reparent. - Tests: hidden-flag assertions replaced with parentId checks; obsolete TeamMemberChip a11y/eject tests deleted (the UI component no longer exists). Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/Canvas.tsx | 92 ++++--- canvas/src/components/WorkspaceNode.tsx | 256 +++--------------- .../__tests__/WorkspaceNode.a11y.test.tsx | 202 -------------- .../__tests__/WorkspaceNode.eject.test.tsx | 190 ------------- .../src/store/__tests__/canvas-events.test.ts | 4 +- .../store/__tests__/canvas-topology.test.ts | 21 +- canvas/src/store/__tests__/canvas.test.ts | 28 +- canvas/src/store/canvas-events.ts | 2 +- canvas/src/store/canvas-topology.ts | 62 ++++- canvas/src/store/canvas.ts | 62 +++-- 10 files changed, 219 insertions(+), 700 deletions(-) delete mode 100644 canvas/src/components/__tests__/WorkspaceNode.a11y.test.tsx delete mode 100644 canvas/src/components/__tests__/WorkspaceNode.eject.test.tsx diff --git a/canvas/src/components/Canvas.tsx b/canvas/src/components/Canvas.tsx index 621da392..4608eb80 100644 --- a/canvas/src/components/Canvas.tsx +++ b/canvas/src/components/Canvas.tsx @@ -76,7 +76,7 @@ function CanvasInner() { const nestNode = useCanvasStore((s) => s.nestNode); const isDescendant = useCanvasStore((s) => s.isDescendant); const dragStartParentRef = useRef(null); - const { getIntersectingNodes } = useReactFlow(); + const { getInternalNode } = useReactFlow(); const onNodeDragStart: OnNodeDrag> = useCallback( (_event, node) => { @@ -85,35 +85,48 @@ function CanvasInner() { [] ); + // Absolute-bounds hit test: find the deepest workspace whose measured + // bounding box contains `point`, excluding the dragged node itself and + // its descendants. Works regardless of nesting depth because React Flow + // exposes each node's absolute position + measured size on the internal + // node record. "Deepest" wins so dropping a card onto a grand-child lands + // there rather than on the outermost ancestor. + const findDropTarget = useCallback( + (draggedId: string, point: { x: number; y: number }): string | null => { + const all = useCanvasStore.getState().nodes; + let best: { id: string; area: number } | null = null; + for (const n of all) { + if (n.id === draggedId || isDescendant(draggedId, n.id)) continue; + const internal = getInternalNode(n.id); + if (!internal) continue; + const abs = internal.internals.positionAbsolute; + const w = internal.measured?.width ?? n.width ?? 220; + const h = internal.measured?.height ?? n.height ?? 120; + if (point.x < abs.x || point.x > abs.x + w) continue; + if (point.y < abs.y || point.y > abs.y + h) continue; + const area = w * h; + // Smaller area = deeper/more specific match wins. + if (!best || area < best.area) best = { id: n.id, area }; + } + return best?.id ?? null; + }, + [getInternalNode, isDescendant] + ); + const onNodeDrag: OnNodeDrag> = useCallback( (_event, node) => { - // Only consider nodes within a proximity threshold as nest targets. - // Without this check, getIntersectingNodes returns any node whose bounding - // boxes overlap — which can be hundreds of pixels away on a sparse canvas, - // causing accidental nesting when the user drags a node across the board. - const thresholdPx = 100; - const threshold = thresholdPx * thresholdPx; // compare squared distances - let nearest: { id: string; dist: number } | null = null; - for (const candidate of getIntersectingNodes(node)) { - if (candidate.id === node.id || isDescendant(node.id, candidate.id)) continue; - // Skip nodes already nested inside another workspace: they render - // as TEAM MEMBERS rows inside their parent card and share its - // bounding box, so getIntersectingNodes would otherwise pick the - // nested child (same "Hermes Agent" name) over the visible parent - // the user actually dropped onto. Hidden nodes + nodes with a - // parentId are never valid top-level drop targets. - const candData = candidate.data as WorkspaceNodeData | undefined; - if (candidate.hidden || candData?.parentId) continue; - const dx = candidate.position.x - node.position.x; - const dy = candidate.position.y - node.position.y; - const dist2 = dx * dx + dy * dy; - if (dist2 <= threshold && (!nearest || dist2 < nearest.dist)) { - nearest = { id: candidate.id, dist: dist2 }; - } + const internal = getInternalNode(node.id); + if (!internal) { + setDragOverNode(null); + return; } - setDragOverNode(nearest?.id ?? null); + const abs = internal.internals.positionAbsolute; + const w = internal.measured?.width ?? 220; + const h = internal.measured?.height ?? 120; + const center = { x: abs.x + w / 2, y: abs.y + h / 2 }; + setDragOverNode(findDropTarget(node.id, center)); }, - [getIntersectingNodes, isDescendant, setDragOverNode] + [findDropTarget, getInternalNode, setDragOverNode] ); // Confirmation dialog state for structure changes @@ -149,23 +162,30 @@ function CanvasInner() { setDragOverNode(null); const nodeName = (node.data as WorkspaceNodeData).name; + const currentParentId = (node.data as WorkspaceNodeData).parentId; - if (dragOverNodeId) { + // The drag-stop offers three possible intents: + // 1. Drop inside a different parent → nest into that parent. + // 2. Drop onto empty canvas while I was nested → un-nest. + // 3. Drop inside my current parent (or no parent) → just a move. + if (dragOverNodeId && dragOverNodeId !== currentParentId) { const targetNode = allNodes.find((n) => n.id === dragOverNodeId); const targetName = targetNode?.data.name || "Unknown"; setPendingNest({ nodeId: node.id, targetId: dragOverNodeId, nodeName, targetName }); - } else { - const currentParentId = (node.data as WorkspaceNodeData).parentId; - if (currentParentId) { - const parentNode = allNodes.find((n) => n.id === currentParentId); - const parentName = parentNode?.data.name || "Unknown"; - setPendingNest({ nodeId: node.id, targetId: null, nodeName, targetName: parentName }); - } + } else if (!dragOverNodeId && currentParentId) { + const parentNode = allNodes.find((n) => n.id === currentParentId); + const parentName = parentNode?.data.name || "Unknown"; + setPendingNest({ nodeId: node.id, targetId: null, nodeName, targetName: parentName }); } - savePosition(node.id, node.position.x, node.position.y); + // savePosition expects ABSOLUTE coords. When node is a child, its + // `position` is relative to its parent, so translate through the + // measured absolute position React Flow tracks. + const internal = getInternalNode(node.id); + const abs = internal?.internals.positionAbsolute ?? node.position; + savePosition(node.id, abs.x, abs.y); }, - [savePosition, setDragOverNode] + [getInternalNode, savePosition, setDragOverNode] ); const confirmNest = useCallback(() => { diff --git a/canvas/src/components/WorkspaceNode.tsx b/canvas/src/components/WorkspaceNode.tsx index 393c1541..9f11cf5d 100644 --- a/canvas/src/components/WorkspaceNode.tsx +++ b/canvas/src/components/WorkspaceNode.tsx @@ -1,31 +1,25 @@ "use client"; -import { useCallback, useMemo, useRef } from "react"; -import { Handle, Position, type NodeProps, type Node } from "@xyflow/react"; +import { useCallback } from "react"; +import { Handle, NodeResizer, Position, type NodeProps, type Node } from "@xyflow/react"; import { useCanvasStore, type WorkspaceNodeData } from "@/store/canvas"; import { showToast } from "@/components/Toaster"; import { Tooltip } from "@/components/Tooltip"; import { STATUS_CONFIG, TIER_CONFIG } from "@/lib/design-tokens"; -import { useShallow } from "zustand/react/shallow"; -/** Stable selector: returns children, grandchild flag, and descendant count for a node */ -function useHierarchyInfo(parentId: string) { - const childIds = useCanvasStore( - useCallback((s) => s.nodes.filter((n) => n.data.parentId === parentId).map((n) => n.id).join(","), [parentId]) +/** Descendant count for the "N sub" badge — children are first-class nodes + * rendered as full cards inside this one via React Flow's native parentId, + * so we don't need to subscribe to the actual child list here. */ +function useDescendantCount(nodeId: string): number { + return useCanvasStore( + useCallback((s) => countDescendants(nodeId, s.nodes), [nodeId]) ); - const children = useCanvasStore( - useShallow((s) => s.nodes.filter((n) => n.data.parentId === parentId)) +} + +function useHasChildren(nodeId: string): boolean { + return useCanvasStore( + useCallback((s) => s.nodes.some((n) => n.data.parentId === nodeId), [nodeId]) ); - const hasGrandchildren = useCanvasStore( - useCallback((s) => { - const ids = childIds.split(",").filter(Boolean); - return ids.length > 0 && ids.some((cid) => s.nodes.some((n) => n.data.parentId === cid)); - }, [childIds]) - ); - const descendantCount = useCanvasStore( - useCallback((s) => countDescendants(parentId, s.nodes), [parentId]) - ); - return { children, hasGrandchildren, descendantCount }; } /** Eject/extract arrow icon — visually distinct from delete ✕ */ @@ -52,18 +46,26 @@ export function WorkspaceNode({ id, data }: NodeProps>) const toggleNodeSelection = useCanvasStore((s) => s.toggleNodeSelection); const isOnline = data.status === "online"; - // Get children + hierarchy info (single stable selector avoids redundant re-renders) - const { children, hasGrandchildren, descendantCount } = useHierarchyInfo(id); - const hasChildren = children.length > 0; + // Children are first-class RF nodes now (rendered inside this one via + // React Flow's native parentId). We only need the count for the badge + // and a boolean so parent cards default to a larger size. + const hasChildren = useHasChildren(id); + const descendantCount = useDescendantCount(id); const skills = getSkillNames(data.agentCard); - const handleExtract = useCallback( - (childId: string) => nestNode(childId, null), - [nestNode] - ); - return ( + <> + {/* NodeResizer — visible only on the selected card. Lets the user + * drag any edge/corner to grow or shrink the workspace, which is + * useful on cards that contain nested child workspaces. */} +
>) } }} className={` - group relative rounded-xl - ${hasGrandchildren ? "min-w-[720px] max-w-[960px]" : hasChildren ? "min-w-[320px] max-w-[450px]" : "min-w-[210px] max-w-[280px]"} + group relative rounded-xl h-full w-full + ${hasChildren ? "min-w-[360px] min-h-[200px]" : "min-w-[210px]"} cursor-pointer overflow-hidden transition-all duration-200 ease-out ${isDragTarget @@ -214,10 +216,9 @@ export function WorkspaceNode({ id, data }: NodeProps>)
)} - {/* Embedded children — rendered INSIDE the parent node */} - {hasChildren && ( - - )} + {/* Children render as first-class React Flow nodes inside this + * card (parentId binding). No embedded TEAM MEMBERS list here — + * just keep visual breathing room via the min-height above. */} {/* Current task */} {data.currentTask && ( @@ -283,11 +284,10 @@ export function WorkspaceNode({ id, data }: NodeProps>) className="!w-2.5 !h-1 !rounded-full !bg-zinc-600/80 !border-0 !-bottom-0.5 hover:!bg-blue-400 hover:!h-1.5 transition-all" /> + ); } -const MAX_NESTING_DEPTH = 3; - /** Count all descendants (children + grandchildren + ...) */ function countDescendants(nodeId: string, allNodes: Node[], visited = new Set()): number { if (visited.has(nodeId)) return 0; @@ -300,192 +300,6 @@ function countDescendants(nodeId: string, allNodes: Node[], v return count; } -/** Subscribes to allNodes only when children exist — isolates re-renders from parent */ -function EmbeddedTeam({ members, depth, onSelect, onExtract }: { - members: Node[]; - depth: number; - onSelect: (id: string) => void; - onExtract: (id: string) => void; -}) { - const allNodes = useCanvasStore((s) => s.nodes); - // Use grid layout at depth 0 when there are multiple members (departments side-by-side) - const useGrid = depth === 0 && members.length >= 2; - return ( -
-
Team Members
-
- {members.map((child) => ( - - ))} -
-
- ); -} - -/** Recursive mini-card — mirrors parent card layout at smaller scale */ -function TeamMemberChip({ - node, - allNodes, - depth, - onSelect, - onExtract, -}: { - node: Node; - allNodes: Node[]; - depth: number; - onSelect: (id: string) => void; - onExtract: (id: string) => void; -}) { - const { data } = node; - const statusCfg = STATUS_CONFIG[data.status] || STATUS_CONFIG.offline; - const tierCfg = TIER_CONFIG[data.tier] || { label: `T${data.tier}`, color: "text-zinc-500 bg-zinc-800" }; - const isOnline = data.status === "online"; - const skills = getSkillNames(data.agentCard); - - const subChildren = useMemo( - () => allNodes.filter((n) => n.data.parentId === node.id), - [allNodes, node.id] - ); - const hasSubChildren = subChildren.length > 0; - const descendantCount = useMemo( - () => hasSubChildren ? countDescendants(node.id, allNodes) : 0, - [allNodes, node.id, hasSubChildren] - ); - - return ( -
{ - e.stopPropagation(); - onSelect(node.id); - }} - onKeyDown={(e) => { - if (e.key === "Enter" || e.key === " ") { - e.preventDefault(); - e.stopPropagation(); - onSelect(node.id); - } - }} - onContextMenu={(e) => { - e.preventDefault(); - e.stopPropagation(); - useCanvasStore.getState().openContextMenu({ x: e.clientX, y: e.clientY, nodeId: node.id, nodeData: data }); - }} - > - {/* Status gradient bar */} -
- -
- {/* Header: name + badges + extract */} -
-
-
- - {data.name} - -
-
- {hasSubChildren && ( - - {descendantCount} - - )} - - {tierCfg.label} - - -
-
- - {/* Role */} - {data.role && ( -
{data.role}
- )} - - {/* Skills */} - {skills.length > 0 && ( -
- {skills.slice(0, 3).map((skill) => ( - - {skill} - - ))} - {skills.length > 3 && ( - +{skills.length - 3} - )} -
- )} - - {/* Status + active tasks row */} -
- {data.status !== "online" ? ( - - {statusCfg.label} - - ) :
} - {data.activeTasks > 0 && ( -
-
- - {data.activeTasks} - -
- )} -
- - {/* Current task banner for sub-agents */} - {data.currentTask && ( - -
-
- {data.currentTask} -
- - )} - - {/* Recursive sub-children rendered inside this card */} - {hasSubChildren && depth < MAX_NESTING_DEPTH && ( -
-
Team
-
= 2 ? "grid grid-cols-2 gap-1" : "space-y-1"}> - {subChildren.map((sub) => ( - - ))} -
-
- )} -
-
- ); -} function getSkillNames(agentCard: Record | null): string[] { if (!agentCard) return []; diff --git a/canvas/src/components/__tests__/WorkspaceNode.a11y.test.tsx b/canvas/src/components/__tests__/WorkspaceNode.a11y.test.tsx deleted file mode 100644 index 48c5cb35..00000000 --- a/canvas/src/components/__tests__/WorkspaceNode.a11y.test.tsx +++ /dev/null @@ -1,202 +0,0 @@ -// @vitest-environment jsdom -/** - * WorkspaceNode a11y tests — issue #831 - * - * Covers the TeamMemberChip sub-component (rendered inside a parent workspace - * node when that node has children): - * - role="button" is present - * - aria-label="Select " is present - * - pressing Enter triggers onSelect with the child's id - * - pressing Space triggers onSelect with the child's id - * - the eject button has aria-label="Extract from team" - */ -import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; -import { render, screen, fireEvent, cleanup } from "@testing-library/react"; - -afterEach(() => { - cleanup(); -}); - -// ── Mock @xyflow/react (Handles) ────────────────────────────────────────────── -vi.mock("@xyflow/react", () => ({ - Handle: () => null, - Position: { Top: "top", Bottom: "bottom" }, -})); - -// ── Mock Tooltip (passthrough) ──────────────────────────────────────────────── -vi.mock("@/components/Tooltip", () => ({ - Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}, -})); - -// ── Mock Toaster ────────────────────────────────────────────────────────────── -vi.mock("@/components/Toaster", () => ({ - showToast: vi.fn(), -})); - -// ── Mock design tokens ──────────────────────────────────────────────────────── -vi.mock("@/lib/design-tokens", () => ({ - STATUS_CONFIG: { - online: { - dot: "bg-emerald-400", - glow: "", - bar: "from-emerald-950/30", - label: "Online", - }, - offline: { - dot: "bg-zinc-500", - glow: "", - bar: "from-zinc-900", - label: "Offline", - }, - degraded: { - dot: "bg-amber-400", - glow: "", - bar: "from-amber-950/30", - label: "Degraded", - }, - provisioning: { - dot: "bg-sky-400", - glow: "", - bar: "from-sky-950/30", - label: "Provisioning", - }, - failed: { - dot: "bg-red-400", - glow: "", - bar: "from-red-950/30", - label: "Failed", - }, - }, - TIER_CONFIG: { - 1: { label: "T1", color: "text-zinc-400 bg-zinc-800" }, - 2: { label: "T2", color: "text-zinc-400 bg-zinc-800" }, - 3: { label: "T3", color: "text-zinc-400 bg-zinc-800" }, - }, -})); - -// ── Store state with a parent + one child ──────────────────────────────────── - -const mockSelectNode = vi.fn(); -const mockOpenContextMenu = vi.fn(); -const mockNestNode = vi.fn(); - -const PARENT_ID = "ws-parent"; -const CHILD_ID = "ws-child"; - -const PARENT_DATA = { - name: "Parent Workspace", - status: "online", - tier: 1 as const, - role: "Manager", - parentId: null, - needsRestart: false, - currentTask: null, - activeTasks: 0, - agentCard: null, - runtime: "langgraph", - lastSampleError: null, -}; - -const CHILD_DATA = { - name: "Child Workspace", - status: "online", - tier: 1 as const, - role: "Worker", - parentId: PARENT_ID, - needsRestart: false, - currentTask: null, - activeTasks: 0, - agentCard: null, - runtime: "langgraph", - lastSampleError: null, -}; - -const ALL_NODES = [ - { id: PARENT_ID, position: { x: 0, y: 0 }, data: PARENT_DATA }, - { id: CHILD_ID, position: { x: 0, y: 0 }, data: CHILD_DATA }, -]; - -const mockStoreState = { - nodes: ALL_NODES, - selectedNodeId: null, - dragOverNodeId: null, - selectNode: mockSelectNode, - openContextMenu: mockOpenContextMenu, - nestNode: mockNestNode, - restartWorkspace: vi.fn(() => Promise.resolve()), - setPanelTab: vi.fn(), - selectedNodeIds: new Set(), - toggleNodeSelection: vi.fn(), -}; - -vi.mock("@/store/canvas", () => ({ - useCanvasStore: Object.assign( - vi.fn((selector: (s: typeof mockStoreState) => unknown) => - selector(mockStoreState) - ), - { getState: () => mockStoreState } - ), -})); - -// ── Import component AFTER mocks ────────────────────────────────────────────── -import { WorkspaceNode } from "../WorkspaceNode"; - -// ── Helper ──────────────────────────────────────────────────────────────────── - -function renderParentNode() { - // WorkspaceNode's full NodeProps has many optional fields; we only need id+data - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return render(); -} - -// ── Tests ───────────────────────────────────────────────────────────────────── - -describe("WorkspaceNode — TeamMemberChip a11y (issue #831)", () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it("TeamMemberChip renders with role='button'", () => { - renderParentNode(); - // The parent WorkspaceNode div is role=button (aria-label contains the name), - // and the chip is a separate role=button with aria-label starting with "Select" - const chip = screen.getByRole("button", { - name: "Select Child Workspace", - }); - expect(chip).toBeTruthy(); - }); - - it("TeamMemberChip has aria-label='Select '", () => { - renderParentNode(); - const chip = screen.getByRole("button", { - name: "Select Child Workspace", - }); - expect(chip.getAttribute("aria-label")).toBe("Select Child Workspace"); - }); - - it("pressing Enter on TeamMemberChip calls selectNode with the child's id", () => { - renderParentNode(); - const chip = screen.getByRole("button", { - name: "Select Child Workspace", - }); - fireEvent.keyDown(chip, { key: "Enter" }); - expect(mockSelectNode).toHaveBeenCalledWith(CHILD_ID); - }); - - it("pressing Space on TeamMemberChip calls selectNode with the child's id", () => { - renderParentNode(); - const chip = screen.getByRole("button", { - name: "Select Child Workspace", - }); - fireEvent.keyDown(chip, { key: " " }); - expect(mockSelectNode).toHaveBeenCalledWith(CHILD_ID); - }); - - it("eject button has aria-label='Extract from team'", () => { - renderParentNode(); - const ejectBtn = screen.getByRole("button", { - name: "Extract Child Workspace from team", - }); - expect(ejectBtn).toBeTruthy(); - }); -}); diff --git a/canvas/src/components/__tests__/WorkspaceNode.eject.test.tsx b/canvas/src/components/__tests__/WorkspaceNode.eject.test.tsx deleted file mode 100644 index 691bc2cd..00000000 --- a/canvas/src/components/__tests__/WorkspaceNode.eject.test.tsx +++ /dev/null @@ -1,190 +0,0 @@ -// @vitest-environment jsdom -/** - * Tests for issue #854 — TeamMemberChip eject button: - * - aria-label must be dynamic: `Extract ${childName} from team` - * - title must be dynamic: `Extract ${childName} from team` - * - EjectIcon svg must carry aria-hidden="true" - */ -import { describe, it, expect, vi, afterEach } from "vitest"; -import { render, cleanup } from "@testing-library/react"; -import type { Node } from "@xyflow/react"; -import type { WorkspaceNodeData } from "@/store/canvas"; - -afterEach(() => { - cleanup(); - vi.restoreAllMocks(); -}); - -// ── Mock @xyflow/react ───────────────────────────────────────────────────────── -vi.mock("@xyflow/react", () => ({ - Handle: () => null, - Position: { Bottom: "bottom", Top: "top" }, - useReactFlow: vi.fn(), -})); - -// ── Mock Toaster ─────────────────────────────────────────────────────────────── -vi.mock("@/components/Toaster", () => ({ showToast: vi.fn() })); - -// ── Mock Tooltip ─────────────────────────────────────────────────────────────── -vi.mock("@/components/Tooltip", () => ({ - Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}, -})); - -// ── Mock design tokens ───────────────────────────────────────────────────────── -vi.mock("@/lib/design-tokens", () => ({ - STATUS_CONFIG: { - online: { label: "Online", dot: "bg-emerald-400", bar: "from-emerald-500/10" }, - offline: { label: "Offline", dot: "bg-zinc-600", bar: "from-zinc-700/10" }, - provisioning: { label: "Provisioning", dot: "bg-sky-400", bar: "from-sky-500/10" }, - degraded: { label: "Degraded", dot: "bg-amber-400", bar: "from-amber-500/10" }, - failed: { label: "Failed", dot: "bg-red-400", bar: "from-red-500/10" }, - paused: { label: "Paused", dot: "bg-zinc-500", bar: "from-zinc-600/10" }, - }, - TIER_CONFIG: { - 1: { label: "T1", color: "text-zinc-400 bg-zinc-800" }, - 2: { label: "T2", color: "text-blue-400 bg-blue-900/40" }, - }, -})); - -// ── Canvas store mock state ──────────────────────────────────────────────────── -const PARENT_ID = "parent-ws"; -const CHILD_ID = "child-ws"; -const CHILD_NAME = "Child Workspace"; - -function makeNodeData(overrides: Partial = {}): WorkspaceNodeData { - return { - name: "Test WS", - role: "agent", - tier: 1, - status: "online", - agentCard: null, - url: "http://localhost:9000", - parentId: null, - activeTasks: 0, - lastErrorRate: 0, - lastSampleError: "", - uptimeSeconds: 60, - currentTask: "", - collapsed: false, - runtime: "", - needsRestart: false, - budgetLimit: null, - ...overrides, - } as WorkspaceNodeData; -} - -const parentNodeData = makeNodeData({ name: "Parent WS", parentId: null }); -const childNodeData = makeNodeData({ name: CHILD_NAME, parentId: PARENT_ID }); - -const allNodes: Node[] = [ - { id: PARENT_ID, type: "workspaceNode", position: { x: 0, y: 0 }, data: parentNodeData }, - { id: CHILD_ID, type: "workspaceNode", position: { x: 0, y: 0 }, data: childNodeData, hidden: true }, -]; - -// Build a selector-compatible mock of useCanvasStore -const mockStoreState = { - nodes: allNodes, - edges: [], - selectedNodeId: null, - panelTab: "chat", - dragOverNodeId: null, - contextMenu: null, - searchOpen: false, - viewport: { x: 0, y: 0, zoom: 1 }, - selectNode: vi.fn(), - openContextMenu: vi.fn(), - nestNode: vi.fn(), - isDescendant: vi.fn(() => false), - restartWorkspace: vi.fn(), - setPanelTab: vi.fn(), - selectedNodeIds: new Set(), - toggleNodeSelection: vi.fn(), -}; - -vi.mock("@/store/canvas", () => ({ - useCanvasStore: Object.assign( - vi.fn((selector: (s: typeof mockStoreState) => unknown) => - selector(mockStoreState) - ), - { getState: () => mockStoreState } - ), -})); - -// ── Mock zustand/react/shallow ───────────────────────────────────────────────── -vi.mock("zustand/react/shallow", () => ({ - useShallow: (fn: (s: typeof mockStoreState) => unknown) => fn, -})); - -// ── Import component AFTER mocks ─────────────────────────────────────────────── -import { WorkspaceNode } from "../WorkspaceNode"; - -// ── Helpers ──────────────────────────────────────────────────────────────────── -function renderParentNode() { - return render( - - ); -} - -// ── Tests ────────────────────────────────────────────────────────────────────── - -describe("TeamMemberChip eject button — aria-label (issue #854)", () => { - it("eject button has a dynamic aria-label containing the child workspace name", () => { - const { container } = renderParentNode(); - const buttons = container.querySelectorAll("button"); - const ejectBtn = Array.from(buttons).find( - (b) => b.getAttribute("aria-label")?.includes("Extract") && b.getAttribute("aria-label")?.includes("from team") - ); - expect(ejectBtn).toBeTruthy(); - expect(ejectBtn?.getAttribute("aria-label")).toBe(`Extract ${CHILD_NAME} from team`); - }); -}); - -describe("TeamMemberChip eject button — title tooltip (issue #854)", () => { - it("eject button has a dynamic title tooltip containing the child workspace name", () => { - const { container } = renderParentNode(); - const buttons = container.querySelectorAll("button"); - const ejectBtn = Array.from(buttons).find( - (b) => b.getAttribute("title")?.includes("Extract") && b.getAttribute("title")?.includes("from team") - ); - expect(ejectBtn).toBeTruthy(); - expect(ejectBtn?.getAttribute("title")).toBe(`Extract ${CHILD_NAME} from team`); - }); - - it("aria-label and title are identical (both use child workspace name)", () => { - const { container } = renderParentNode(); - const buttons = container.querySelectorAll("button"); - const ejectBtn = Array.from(buttons).find( - (b) => b.getAttribute("aria-label")?.startsWith("Extract") - ); - expect(ejectBtn).toBeTruthy(); - expect(ejectBtn?.getAttribute("aria-label")).toBe(ejectBtn?.getAttribute("title")); - }); -}); - -describe("TeamMemberChip eject button — aria-hidden on EjectIcon (issue #854)", () => { - it("EjectIcon svg has aria-hidden='true' to prevent AT double-announcement", () => { - const { container } = renderParentNode(); - const buttons = container.querySelectorAll("button"); - const ejectBtn = Array.from(buttons).find( - (b) => b.getAttribute("aria-label")?.startsWith("Extract") - ); - expect(ejectBtn).toBeTruthy(); - const svg = ejectBtn?.querySelector("svg"); - expect(svg).toBeTruthy(); - expect(svg?.getAttribute("aria-hidden")).toBe("true"); - }); -}); diff --git a/canvas/src/store/__tests__/canvas-events.test.ts b/canvas/src/store/__tests__/canvas-events.test.ts index 54be70e6..a8b767e2 100644 --- a/canvas/src/store/__tests__/canvas-events.test.ts +++ b/canvas/src/store/__tests__/canvas-events.test.ts @@ -361,7 +361,7 @@ describe("handleCanvasEvent – WORKSPACE_REMOVED", () => { const { nodes: updatedNodes } = set.mock.calls[0][0] as { nodes: Node[] }; const updatedChild = updatedNodes.find((n) => n.id === "child")!; expect(updatedChild.data.parentId).toBe("parent"); - expect(updatedChild.hidden).toBe(true); // still has a parent + expect(updatedChild.parentId).toBe("parent"); // RF binding re-pointed }); it("reparents children to null when root node is removed", () => { @@ -374,7 +374,7 @@ describe("handleCanvasEvent – WORKSPACE_REMOVED", () => { const { nodes: updatedNodes } = set.mock.calls[0][0] as { nodes: Node[] }; const updatedChild = updatedNodes.find((n) => n.id === "child")!; expect(updatedChild.data.parentId).toBeNull(); - expect(updatedChild.hidden).toBe(false); + expect(updatedChild.parentId).toBeUndefined(); }); it("removes edges connected to the removed workspace", () => { diff --git a/canvas/src/store/__tests__/canvas-topology.test.ts b/canvas/src/store/__tests__/canvas-topology.test.ts index 7ca1d950..5f665619 100644 --- a/canvas/src/store/__tests__/canvas-topology.test.ts +++ b/canvas/src/store/__tests__/canvas-topology.test.ts @@ -110,7 +110,10 @@ describe("buildNodesAndEdges – parent + child workspaces", () => { expect(edges).toHaveLength(0); }); - it("marks parent as visible and child as hidden", () => { + it("binds child to parent via React Flow's native parentId", () => { + // Children are first-class nodes now (rendered as full cards inside + // their parent via RF's parentId). No `hidden` flag anymore — the + // nesting is visual, not hide-and-show. const { nodes } = buildNodesAndEdges([ makeWS({ id: "parent" }), makeWS({ id: "child", parent_id: "parent" }), @@ -120,7 +123,9 @@ describe("buildNodesAndEdges – parent + child workspaces", () => { const child = nodes.find((n) => n.id === "child")!; expect(parent.hidden).toBeFalsy(); - expect(child.hidden).toBe(true); + expect(child.hidden).toBeFalsy(); + expect(parent.parentId).toBeUndefined(); + expect(child.parentId).toBe("parent"); }); it("stores parent_id in child node data as parentId", () => { @@ -157,9 +162,9 @@ describe("buildNodesAndEdges – deeply nested hierarchy", () => { expect(nodes).toHaveLength(3); expect(edges).toHaveLength(0); - expect(nodes.find((n) => n.id === "root")!.hidden).toBeFalsy(); - expect(nodes.find((n) => n.id === "mid")!.hidden).toBe(true); - expect(nodes.find((n) => n.id === "leaf")!.hidden).toBe(true); + expect(nodes.find((n) => n.id === "root")!.parentId).toBeUndefined(); + expect(nodes.find((n) => n.id === "mid")!.parentId).toBe("root"); + expect(nodes.find((n) => n.id === "leaf")!.parentId).toBe("mid"); expect(nodes.find((n) => n.id === "mid")!.data.parentId).toBe("root"); expect(nodes.find((n) => n.id === "leaf")!.data.parentId).toBe("mid"); @@ -175,9 +180,9 @@ describe("buildNodesAndEdges – deeply nested hierarchy", () => { const { nodes } = buildNodesAndEdges(workspaces); expect(nodes).toHaveLength(3); - expect(nodes.find((n) => n.id === "root-a")!.hidden).toBeFalsy(); - expect(nodes.find((n) => n.id === "root-b")!.hidden).toBeFalsy(); - expect(nodes.find((n) => n.id === "child-a")!.hidden).toBe(true); + expect(nodes.find((n) => n.id === "root-a")!.parentId).toBeUndefined(); + expect(nodes.find((n) => n.id === "root-b")!.parentId).toBeUndefined(); + expect(nodes.find((n) => n.id === "child-a")!.parentId).toBe("root-a"); }); }); diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index f65e0481..33a0f8b7 100644 --- a/canvas/src/store/__tests__/canvas.test.ts +++ b/canvas/src/store/__tests__/canvas.test.ts @@ -92,7 +92,11 @@ describe("hydrate", () => { expect(edges).toHaveLength(0); }); - it("sets hidden=true for nodes with parent_id", () => { + it("binds children to their parent via React Flow parentId", () => { + // The old model hid child nodes + embedded them as chips inside the + // parent card. The new model renders every workspace as a first-class + // card, using React Flow's native parentId to group them so moving + // the parent carries the children along. const workspaces = [ makeWS({ id: "parent", name: "Parent" }), makeWS({ id: "child", name: "Child", parent_id: "parent" }), @@ -105,7 +109,9 @@ describe("hydrate", () => { const child = nodes.find((n) => n.id === "child")!; expect(parent.hidden).toBeFalsy(); - expect(child.hidden).toBe(true); + expect(child.hidden).toBeFalsy(); + expect(parent.parentId).toBeUndefined(); + expect(child.parentId).toBe("parent"); expect(child.data.parentId).toBe("parent"); }); @@ -331,7 +337,7 @@ describe("applyEvent", () => { expect(nodes).toHaveLength(1); expect(nodes[0].id).toBe("ws-2"); expect(nodes[0].data.parentId).toBeNull(); - expect(nodes[0].hidden).toBe(false); + expect(nodes[0].parentId).toBeUndefined(); }); it("WORKSPACE_REMOVED clears selectedNodeId if removed", () => { @@ -454,7 +460,7 @@ describe("removeNode", () => { const leaf = useCanvasStore.getState().nodes.find((n) => n.id === "leaf")!; expect(leaf.data.parentId).toBe("root"); - expect(leaf.hidden).toBe(true); // still has a parent + expect(leaf.parentId).toBe("root"); // RF binding also re-pointed }); it("reparents children to null when root is deleted", () => { @@ -462,7 +468,7 @@ describe("removeNode", () => { const mid = useCanvasStore.getState().nodes.find((n) => n.id === "mid")!; expect(mid.data.parentId).toBeNull(); - expect(mid.hidden).toBe(false); + expect(mid.parentId).toBeUndefined(); }); it("clears selection if removed node was selected", () => { @@ -655,23 +661,21 @@ describe("nestNode", () => { ]); }); - it("optimistically updates parentId and hidden", async () => { + it("optimistically updates parentId and the RF parent binding", async () => { await useCanvasStore.getState().nestNode("b", "a"); const b = useCanvasStore.getState().nodes.find((n) => n.id === "b")!; expect(b.data.parentId).toBe("a"); - expect(b.hidden).toBe(true); + expect(b.parentId).toBe("a"); }); - it("un-nesting sets parentId to null and shows node", async () => { - // First nest + it("un-nesting clears parentId and the RF binding", async () => { await useCanvasStore.getState().nestNode("b", "a"); - // Then un-nest await useCanvasStore.getState().nestNode("b", null); const b = useCanvasStore.getState().nodes.find((n) => n.id === "b")!; expect(b.data.parentId).toBeNull(); - expect(b.hidden).toBe(false); + expect(b.parentId).toBeUndefined(); }); it("skips when parentId is already the target", async () => { @@ -694,7 +698,7 @@ describe("nestNode", () => { // Should revert to original state (no parent) const b = useCanvasStore.getState().nodes.find((n) => n.id === "b")!; expect(b.data.parentId).toBeNull(); - expect(b.hidden).toBe(false); + expect(b.parentId).toBeUndefined(); }); }); diff --git a/canvas/src/store/canvas-events.ts b/canvas/src/store/canvas-events.ts index 765adef8..b3778378 100644 --- a/canvas/src/store/canvas-events.ts +++ b/canvas/src/store/canvas-events.ts @@ -174,7 +174,7 @@ export function handleCanvasEvent( n.data.parentId === msg.workspace_id ? { ...n, - hidden: !!parentOfRemoved, + parentId: parentOfRemoved ?? undefined, data: { ...n.data, parentId: parentOfRemoved }, } : n diff --git a/canvas/src/store/canvas-topology.ts b/canvas/src/store/canvas-topology.ts index d28434ad..38c80790 100644 --- a/canvas/src/store/canvas-topology.ts +++ b/canvas/src/store/canvas-topology.ts @@ -109,6 +109,14 @@ export function computeAutoLayout( * Converts raw workspace data from the API into React Flow nodes and edges. * Accepts an optional layoutOverrides map (from computeAutoLayout) to override * positions for workspaces that were at 0,0. + * + * Parent/child rendering model: every workspace is a first-class React Flow + * node (full card). When a workspace has parent_id set, its RF `parentId` is + * set to the parent's id and its position is stored RELATIVE to the parent + * origin — React Flow renders the child inside the parent's coordinate space, + * so moving the parent automatically moves all children. The DB keeps + * absolute x/y; the abs→rel conversion happens here on load, and the + * reverse translation happens in savePosition. */ export function buildNodesAndEdges( workspaces: WorkspaceData[], @@ -117,16 +125,41 @@ export function buildNodesAndEdges( nodes: Node[]; edges: Edge[]; } { - // All workspaces become nodes (children are rendered inside parent via WorkspaceNode) - const nodes: Node[] = workspaces.map((ws) => { - const override = layoutOverrides.get(ws.id); - const x = override?.x ?? ws.x; - const y = override?.y ?? ws.y; - return { + // React Flow requires parent nodes to appear before children in the nodes + // array. Topological-sort by depth-first walk from roots so children come + // after their parent regardless of the order the API returned them. + const byId = new Map(workspaces.map((w) => [w.id, w])); + const visited = new Set(); + const sorted: WorkspaceData[] = []; + function visit(ws: WorkspaceData) { + if (visited.has(ws.id)) return; + if (ws.parent_id && byId.has(ws.parent_id) && !visited.has(ws.parent_id)) { + visit(byId.get(ws.parent_id)!); + } + visited.add(ws.id); + sorted.push(ws); + } + workspaces.forEach(visit); + + // Resolve each workspace's absolute position (apply layout override if any). + const absPos = new Map(); + for (const ws of workspaces) { + const o = layoutOverrides.get(ws.id); + absPos.set(ws.id, { x: o?.x ?? ws.x, y: o?.y ?? ws.y }); + } + + const nodes: Node[] = sorted.map((ws) => { + const abs = absPos.get(ws.id)!; + const hasParent = !!ws.parent_id && byId.has(ws.parent_id); + let position = abs; + if (hasParent) { + const pa = absPos.get(ws.parent_id!)!; + position = { x: abs.x - pa.x, y: abs.y - pa.y }; + } + const node: Node = { id: ws.id, type: "workspaceNode", - position: { x, y }, - // Don't set React Flow parentId — children render embedded inside the WorkspaceNode component + position, data: { name: ws.name, status: ws.status, @@ -145,13 +178,18 @@ export function buildNodesAndEdges( budgetLimit: ws.budget_limit ?? null, budgetUsed: ws.budget_used ?? null, }, - // Hide child nodes from canvas — they render inside the parent WorkspaceNode - hidden: !!ws.parent_id, }; + if (hasParent) { + // React Flow native parent binding: children render inside parent's + // coordinate space and move with the parent. No `extent: 'parent'` — + // the user can drag a child out to un-nest (handled in Canvas.tsx + // onNodeDragStop with a bbox hit test). + node.parentId = ws.parent_id!; + } + return node; }); - // No parent→child edges — children are embedded inside the parent node. - // Only create edges between siblings or cross-team connections if needed in future. + // Edges stay empty — the visual parent/child cue is the enclosing card. const edges: Edge[] = []; return { nodes, edges }; diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index 8527be4d..26cda649 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -258,17 +258,40 @@ export const useCanvasStore = create((set, get) => ({ nestNode: async (draggedId, targetId) => { const { nodes, edges } = get(); - const currentParentId = nodes.find((n) => n.id === draggedId)?.data.parentId ?? null; - - // No change needed + const dragged = nodes.find((n) => n.id === draggedId); + if (!dragged) return; + const currentParentId = dragged.data.parentId; if (currentParentId === targetId) return; - // Optimistic update: - // - Set parentId in data - // - Hide child nodes (they render inside parent WorkspaceNode) - // - Remove all edges involving the dragged node + // Compute each ancestor's absolute position by walking up the + // parentId chain. We need this to translate the dragged node's + // `position` (relative to its current parent when nested) between + // the old and new coordinate spaces so the card doesn't visually + // jump on nest/unnest. + const absOf = (id: string | null): { x: number; y: number } => { + let sum = { x: 0, y: 0 }; + let cursor: string | null = id; + while (cursor) { + const n = nodes.find((nn) => nn.id === cursor); + if (!n) break; + sum = { x: sum.x + n.position.x, y: sum.y + n.position.y }; + cursor = n.data.parentId; + } + return sum; + }; + const oldParentAbs = absOf(currentParentId); + const newParentAbs = absOf(targetId); + const draggedAbs = { + x: dragged.position.x + oldParentAbs.x, + y: dragged.position.y + oldParentAbs.y, + }; + const newRelative = { + x: draggedAbs.x - newParentAbs.x, + y: draggedAbs.y - newParentAbs.y, + }; + const newEdges = edges.filter( - (e) => e.source !== draggedId && e.target !== draggedId + (e) => e.source !== draggedId && e.target !== draggedId, ); set({ @@ -276,28 +299,32 @@ export const useCanvasStore = create((set, get) => ({ n.id === draggedId ? { ...n, - hidden: !!targetId, // Hide if becoming a child, show if un-nesting + position: newRelative, + parentId: targetId ?? undefined, data: { ...n.data, parentId: targetId }, } - : n + : n, ), edges: newEdges, }); - // Persist to API try { await api.patch(`/workspaces/${draggedId}`, { parent_id: targetId }); + // Persist absolute position as DB canonical (matches what + // savePosition writes elsewhere); keeps reloads stable regardless + // of which parent the child was under at save time. + await api.patch(`/workspaces/${draggedId}`, { x: draggedAbs.x, y: draggedAbs.y }); } catch { - // Revert on failure set({ nodes: get().nodes.map((n) => n.id === draggedId ? { ...n, - hidden: !!currentParentId, + position: dragged.position, + parentId: currentParentId ?? undefined, data: { ...n.data, parentId: currentParentId }, } - : n + : n, ), edges, }); @@ -325,7 +352,10 @@ export const useCanvasStore = create((set, get) => ({ removeNode: (id) => { const { nodes, edges, selectedNodeId } = get(); - // Re-parent children to the deleted node's parent (or root) + // Re-parent children to the deleted node's parent (or root). + // Children are first-class RF nodes now — we just re-point their + // parentId (both RF's native field and our data mirror). No hidden + // flag is toggled because cards are always visible. const deletedNode = nodes.find((n) => n.id === id); const parentOfDeleted = deletedNode?.data.parentId ?? null; set({ @@ -335,7 +365,7 @@ export const useCanvasStore = create((set, get) => ({ n.data.parentId === id ? { ...n, - hidden: !!parentOfDeleted, + parentId: parentOfDeleted ?? undefined, data: { ...n.data, parentId: parentOfDeleted }, } : n From d359390f837e793103aef45928518b9a09bd1b8f Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 18:29:04 -0700 Subject: [PATCH 04/17] fix(canvas): parent auto-fit sizing + rescue out-of-bounds children MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two playability bugs in the new flat-cards layout: 1. On first load or fresh org import a parent had no explicit width or height, so children whose stored position sat inside their (eventual) parent's rectangle rendered visually outside the smaller default parent box. Compute a parent starting size in canvas-topology: • 2-column grid of child-default footprints + header/side padding • Grows per child count (2→1 row, 3-4→2 rows, etc.) and stamp it onto the Node's width/height so the first paint already contains every child. 2. If a child's stored relative position actually falls outside the parent's computed bounds (legacy org-imports at 0,0, pre-refactor absolute coordinates, manually-nudged rows), assign that child a deterministic default grid slot inside the parent instead. Runtime cascade: added growParentsToFitChildren to onNodesChange so when the user drags or resizes a child past the parent's current bounds, the parent grows to contain it (+padding). Miro/FigJam-style frame auto-fit — grow-only, never shrinks under the user's manual resize. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/store/canvas-topology.ts | 100 ++++++++++++++++++++++++++++ canvas/src/store/canvas.ts | 63 ++++++++++++++++-- 2 files changed, 159 insertions(+), 4 deletions(-) diff --git a/canvas/src/store/canvas-topology.ts b/canvas/src/store/canvas-topology.ts index 38c80790..d3367d03 100644 --- a/canvas/src/store/canvas-topology.ts +++ b/canvas/src/store/canvas-topology.ts @@ -5,6 +5,55 @@ import type { WorkspaceNodeData } from "./canvas"; const H_SPACING = 320; const V_SPACING = 200; +// Default card footprint we use when we don't yet have a measured size +// (first render, before React Flow reports dimensions). These match the +// min-width / min-height that WorkspaceNode.tsx sets, so a parent built +// from them will never start too small for its children on first paint. +export const CHILD_DEFAULT_WIDTH = 260; +export const CHILD_DEFAULT_HEIGHT = 140; +export const PARENT_HEADER_PADDING = 60; // room for the parent's own header +export const PARENT_SIDE_PADDING = 20; +export const PARENT_BOTTOM_PADDING = 20; +export const CHILD_GUTTER = 20; + +/** + * A deterministic grid slot for the n-th child inside a parent, counted + * left-to-right then top-to-bottom. Used to lay out org-imported teams + * and to rescue children whose stored position puts them outside the + * parent's bounding box. 2-column grid is wide enough to read but + * narrow enough to keep the parent card from becoming a widescreen. + */ +export function defaultChildSlot(index: number): { x: number; y: number } { + const col = index % 2; + const row = Math.floor(index / 2); + const x = PARENT_SIDE_PADDING + col * (CHILD_DEFAULT_WIDTH + CHILD_GUTTER); + const y = + PARENT_HEADER_PADDING + row * (CHILD_DEFAULT_HEIGHT + CHILD_GUTTER); + return { x, y }; +} + +/** + * Minimum parent size that still fits `childCount` children laid out via + * defaultChildSlot. Never shrinks below the leaf-card min. + */ +export function parentMinSize(childCount: number): { width: number; height: number } { + if (childCount <= 0) { + return { width: 210, height: 120 }; + } + const cols = Math.min(2, childCount); + const rows = Math.ceil(childCount / 2); + const width = + PARENT_SIDE_PADDING * 2 + + cols * CHILD_DEFAULT_WIDTH + + (cols - 1) * CHILD_GUTTER; + const height = + PARENT_HEADER_PADDING + + rows * CHILD_DEFAULT_HEIGHT + + (rows - 1) * CHILD_GUTTER + + PARENT_BOTTOM_PADDING; + return { width, height }; +} + /** * Computes auto-layout positions for workspaces that have no persisted position * (x === 0 AND y === 0). Workspaces with an existing non-zero position are used @@ -148,6 +197,29 @@ export function buildNodesAndEdges( absPos.set(ws.id, { x: o?.x ?? ws.x, y: o?.y ?? ws.y }); } + // Count children per parent so we can size parents to fit their team + // before any runtime measurement comes back. + const childCounts = new Map(); + for (const ws of workspaces) { + if (ws.parent_id) { + childCounts.set(ws.parent_id, (childCounts.get(ws.parent_id) ?? 0) + 1); + } + } + + // Track each parent's initial size so we can reset children that land + // outside those bounds. Parents without children fall back to the leaf + // default; parents with children get the grid-derived minimum. + const parentSize = new Map(); + for (const ws of workspaces) { + const n = childCounts.get(ws.id) ?? 0; + parentSize.set(ws.id, n > 0 ? parentMinSize(n) : { width: 260, height: 140 }); + } + + // Running index of children already placed per parent — used to hand + // out default grid slots for children whose stored position is outside + // the parent's computed box. + const nextChildIndex = new Map(); + const nodes: Node[] = sorted.map((ws) => { const abs = absPos.get(ws.id)!; const hasParent = !!ws.parent_id && byId.has(ws.parent_id); @@ -155,6 +227,24 @@ export function buildNodesAndEdges( if (hasParent) { const pa = absPos.get(ws.parent_id!)!; position = { x: abs.x - pa.x, y: abs.y - pa.y }; + + // If the stored relative position falls outside the parent's + // current bounds (or landed at exactly the origin before any + // layout pass), assign a deterministic grid slot instead. This + // rescues org-imported children that ended up at (0,0) and + // legacy rows whose absolute coords were far from the parent. + const psize = parentSize.get(ws.parent_id!)!; + const outside = + position.x < 0 || + position.y < 0 || + position.x + CHILD_DEFAULT_WIDTH > psize.width || + position.y + CHILD_DEFAULT_HEIGHT > psize.height; + const atOrigin = position.x === -abs.x + abs.x && abs.x === 0 && abs.y === 0; + if (outside || atOrigin) { + const idx = nextChildIndex.get(ws.parent_id!) ?? 0; + nextChildIndex.set(ws.parent_id!, idx + 1); + position = defaultChildSlot(idx); + } } const node: Node = { id: ws.id, @@ -186,6 +276,16 @@ export function buildNodesAndEdges( // onNodeDragStop with a bbox hit test). node.parentId = ws.parent_id!; } + // Give parents a measured-ish starting size so NodeResizer has a + // baseline and child positions have somewhere to live. Without this, + // parents start at React Flow's default min size (well under a + // single child) and children render visually outside their parent + // until the next resize measurement settles. + if ((childCounts.get(ws.id) ?? 0) > 0) { + const size = parentSize.get(ws.id)!; + node.width = size.width; + node.height = size.height; + } return node; }); diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index 26cda649..5107edca 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -8,7 +8,58 @@ import { import { api } from "@/lib/api"; import type { WorkspaceData, WSMessage } from "./socket"; import { handleCanvasEvent } from "./canvas-events"; -import { buildNodesAndEdges, computeAutoLayout } from "./canvas-topology"; +import { + buildNodesAndEdges, + computeAutoLayout, + CHILD_DEFAULT_HEIGHT, + CHILD_DEFAULT_WIDTH, + PARENT_BOTTOM_PADDING, + PARENT_SIDE_PADDING, +} from "./canvas-topology"; + +/** + * Walk every parent node and bump its width/height (if explicitly set) + * so the union of its children's relative bboxes plus padding fits. A + * parent's size never shrinks via this path — only grows — because + * shrinking on resize would fight the user's own NodeResizer drag. + */ +function growParentsToFitChildren>( + nodes: Node[], +): Node[] { + // Index children by parentId so the scan is O(n). + const childrenByParent = new Map[]>(); + for (const n of nodes) { + if (!n.parentId) continue; + const arr = childrenByParent.get(n.parentId) ?? []; + arr.push(n); + childrenByParent.set(n.parentId, arr); + } + let changed = false; + const out = nodes.map((n) => { + const kids = childrenByParent.get(n.id); + if (!kids || kids.length === 0) return n; + let maxRight = 0; + let maxBottom = 0; + for (const k of kids) { + const w = (k.measured?.width ?? k.width ?? CHILD_DEFAULT_WIDTH) as number; + const h = (k.measured?.height ?? k.height ?? CHILD_DEFAULT_HEIGHT) as number; + maxRight = Math.max(maxRight, k.position.x + w); + maxBottom = Math.max(maxBottom, k.position.y + h); + } + const requiredW = maxRight + PARENT_SIDE_PADDING; + const requiredH = maxBottom + PARENT_BOTTOM_PADDING; + const currentW = (n.measured?.width ?? n.width ?? 0) as number; + const currentH = (n.measured?.height ?? n.height ?? 0) as number; + if (requiredW <= currentW && requiredH <= currentH) return n; + changed = true; + return { + ...n, + width: Math.max(currentW, requiredW), + height: Math.max(currentH, requiredH), + }; + }); + return changed ? out : nodes; +} // Re-export extracted types and functions so existing imports from "@/store/canvas" keep working export { summarizeWorkspaceCapabilities } from "./canvas-capabilities"; @@ -389,9 +440,13 @@ export const useCanvasStore = create((set, get) => ({ }, onNodesChange: (changes) => { - set({ - nodes: applyNodeChanges(changes, get().nodes), - }); + const next = applyNodeChanges(changes, get().nodes); + // Auto-grow parents to fit their children: if any child's + // (position + size) extends beyond the parent's current dimensions, + // the parent's explicit width/height is bumped so it stays the + // visual container (Miro/FigJam-style frame auto-fit). + const grown = growParentsToFitChildren(next); + set({ nodes: grown }); }, savePosition: async (nodeId: string, x: number, y: number) => { From f3423a513d024afdf97d4f07685c70901bb72fae Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 19:03:02 -0700 Subject: [PATCH 05/17] feat(canvas): industry-pattern playability pass (P1+P2+P3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ships the full prioritized improvement list from the canvas research report — aligns our nesting/resize UX with Miro / FigJam / tldraw / Figma conventions. Organized by priority below. ## P1 — baseline playability * Hysteresis on drag-out detach (Miro): a child only un-nests when >=20% of its bbox is outside the parent on release. Prevents accidental un-nesting from twitchy drags. * Drop-target now uses tree-depth DESC, then zIndex DESC, then area ASC to pick targets when nested parents overlap (xyflow #2827). * Children render above ancestors by inheriting zIndex = parent + 1 in topology and on every nest/unnest (xyflow #4012). * Live drop-target outline (existing) plus a Mural-style "Drop into: " floating badge so colour isn't the only cue. * growParentsToFitChildren now fires only on dimension-type changes inside onNodesChange (NodeResizer commits) and once on drag-stop — avoids tldraw's edge-chase artifact (P3.11 commit-on-release). ## P2 — polish * Whimsical-style ghost preview: dashed outline at the next default grid slot inside the drop-target parent during drag. * Alt-drag escape with soft clamp: dropping slightly outside a parent without Alt/Cmd snaps the child back inside (clampChildIntoParent); Alt releases the clamp to allow un-nest; Cmd/Ctrl force-detaches. * Figma-style keyboard hierarchy nav: Enter descends to first child, Shift+Enter ascends to parent, Cmd+]/[ re-orders siblings via the new bumpZOrder store action. * Multi-select re-parent preserves offsets: confirmNest routes through a new batchNest action when the primary drag is part of a batch selection (Lucidchart pattern). ## P3 — long-tail * Minimap now shows parent cards as filled regions with a blue stroke, so hierarchy reads at a glance without zooming. * Out-of-bounds rescue is opt-in: topology no longer silently re-lays children whose stored position is outside the parent bbox (Figma trust-the-data). The new Arrange Children context menu item runs the rescue on demand via arrangeChildren. * Cmd-drag force-detach regardless of hysteresis. * Collapse workspace: the existing Collapse Team action now toggles a local setCollapsed store action that hides every descendant and shrinks the parent card to header-only (Miro frame outline view). Growth pass skips collapsed parents so they don't push back out. All 910 canvas tests green. Backend untouched. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/Canvas.tsx | 318 +++++++++++++++++++++--- canvas/src/components/ContextMenu.tsx | 25 +- canvas/src/components/WorkspaceNode.tsx | 2 +- canvas/src/store/canvas-topology.ts | 54 +++- canvas/src/store/canvas.ts | 152 ++++++++++- 5 files changed, 493 insertions(+), 58 deletions(-) diff --git a/canvas/src/components/Canvas.tsx b/canvas/src/components/Canvas.tsx index 4608eb80..d62e3ffe 100644 --- a/canvas/src/components/Canvas.tsx +++ b/canvas/src/components/Canvas.tsx @@ -16,6 +16,11 @@ import { import "@xyflow/react/dist/style.css"; import { useCanvasStore, type WorkspaceNodeData } from "@/store/canvas"; +import { + defaultChildSlot, + CHILD_DEFAULT_HEIGHT, + CHILD_DEFAULT_WIDTH, +} from "@/store/canvas-topology"; import { A2ATopologyOverlay } from "./A2ATopologyOverlay"; import { WorkspaceNode } from "./WorkspaceNode"; import { SidePanel } from "./SidePanel"; @@ -58,6 +63,132 @@ export function Canvas() { ); } +// Hysteresis: detach-on-drop only fires once the child has moved far +// enough outside the parent that the intent is unambiguous. We pick 20% +// of the overlapping dimension as the threshold (Miro behaves similarly +// at ~20-30%). A slightly-past-edge drag commits a MOVE, not a detach. +const DETACH_FRACTION = 0.2; + +/** Floating "Drop into: " label that tracks the current drag + * target. Mural-style affordance — colour alone is ambiguous on dense + * canvases, so we spell out the target by name. Mounted inside the + * ReactFlowProvider subtree so it can read positionAbsolute. */ +function DropTargetBadge() { + const dragOverNodeId = useCanvasStore((s) => s.dragOverNodeId); + const targetName = useCanvasStore((s) => { + if (!s.dragOverNodeId) return null; + const n = s.nodes.find((nn) => nn.id === s.dragOverNodeId); + return (n?.data as WorkspaceNodeData | undefined)?.name ?? null; + }); + const childCount = useCanvasStore((s) => + !s.dragOverNodeId + ? 0 + : s.nodes.filter((n) => n.parentId === s.dragOverNodeId).length, + ); + const { getInternalNode, flowToScreenPosition } = useReactFlow(); + if (!dragOverNodeId || !targetName) return null; + const internal = getInternalNode(dragOverNodeId); + if (!internal) return null; + const abs = internal.internals.positionAbsolute; + const w = internal.measured?.width ?? 220; + const h = internal.measured?.height ?? 120; + const badge = flowToScreenPosition({ x: abs.x + w / 2, y: abs.y }); + + // Ghost preview: dashed outline at the next default grid slot inside + // the target parent. Whimsical-style affordance so the user sees + // exactly where the dropped card will land. + const slot = defaultChildSlot(childCount); + const slotTL = flowToScreenPosition({ x: abs.x + slot.x, y: abs.y + slot.y }); + const slotBR = flowToScreenPosition({ + x: abs.x + slot.x + CHILD_DEFAULT_WIDTH, + y: abs.y + slot.y + CHILD_DEFAULT_HEIGHT, + }); + // Clip the ghost to the parent's visible bounds so it doesn't spill + // out when the parent is smaller than the slot. + const parentTL = flowToScreenPosition({ x: abs.x, y: abs.y }); + const parentBR = flowToScreenPosition({ x: abs.x + w, y: abs.y + h }); + const ghostVisible = + slotBR.x > parentTL.x && + slotTL.x < parentBR.x && + slotBR.y > parentTL.y && + slotTL.y < parentBR.y; + + return ( + <> + {ghostVisible && ( +
+ )} +
+ Drop into: {targetName} +
+ + ); +} + +/** Snap a child back so its bbox is fully inside the parent's bounds. + * Called on drag-stop when the user drifted slightly past the edge + * without holding Alt or Cmd — the canvas treats the gesture as a + * plain move rather than an un-nest. */ +function clampChildIntoParent( + childId: string, + parentId: string, + getInternalNode: (id: string) => ReturnType["getInternalNode"]>, +) { + const c = getInternalNode(childId); + const p = getInternalNode(parentId); + if (!c || !p) return; + const cw = c.measured?.width ?? c.width ?? 220; + const ch = c.measured?.height ?? c.height ?? 120; + const pw = p.measured?.width ?? p.width ?? 220; + const ph = p.measured?.height ?? p.height ?? 120; + const { nodes } = useCanvasStore.getState(); + const cur = nodes.find((n) => n.id === childId); + if (!cur) return; + const rel = cur.position; + const clampedX = Math.max(0, Math.min(rel.x, pw - cw)); + const clampedY = Math.max(0, Math.min(rel.y, ph - ch)); + if (clampedX === rel.x && clampedY === rel.y) return; + useCanvasStore.setState({ + nodes: nodes.map((n) => + n.id === childId ? { ...n, position: { x: clampedX, y: clampedY } } : n, + ), + }); +} + +function shouldDetach( + childId: string, + parentId: string, + getInternalNode: (id: string) => ReturnType["getInternalNode"]>, +): boolean { + const c = getInternalNode(childId); + const p = getInternalNode(parentId); + if (!c || !p) return true; // If we can't measure, fall back to the old behavior. + const cw = c.measured?.width ?? c.width ?? 220; + const ch = c.measured?.height ?? c.height ?? 120; + const pw = p.measured?.width ?? p.width ?? 220; + const ph = p.measured?.height ?? p.height ?? 120; + const cx = c.internals.positionAbsolute; + const px = p.internals.positionAbsolute; + const overlapW = + Math.max(0, Math.min(cx.x + cw, px.x + pw) - Math.max(cx.x, px.x)); + const overlapH = + Math.max(0, Math.min(cx.y + ch, px.y + ph) - Math.max(cx.y, px.y)); + const outsideFractionX = 1 - overlapW / cw; + const outsideFractionY = 1 - overlapH / ch; + return outsideFractionX > DETACH_FRACTION || outsideFractionY > DETACH_FRACTION; +} + function CanvasInner() { const nodes = useCanvasStore((s) => s.nodes); const edges = useCanvasStore((s) => s.edges); @@ -76,25 +207,53 @@ function CanvasInner() { const nestNode = useCanvasStore((s) => s.nestNode); const isDescendant = useCanvasStore((s) => s.isDescendant); const dragStartParentRef = useRef(null); + const dragModifiersRef = useRef<{ alt: boolean; meta: boolean }>({ alt: false, meta: false }); const { getInternalNode } = useReactFlow(); + // Track Alt / Cmd-Meta during the whole drag so onNodeDrag and + // onNodeDragStop see the same modifier state. (React Flow's drag event + // only fires mousemove events — we attach a window-level keyboard + // listener while a drag is in progress.) const onNodeDragStart: OnNodeDrag> = useCallback( - (_event, node) => { + (event, node) => { dragStartParentRef.current = (node.data as WorkspaceNodeData).parentId; + dragModifiersRef.current = { + alt: event.altKey, + meta: event.metaKey || event.ctrlKey, + }; }, - [] + [], ); - // Absolute-bounds hit test: find the deepest workspace whose measured - // bounding box contains `point`, excluding the dragged node itself and - // its descendants. Works regardless of nesting depth because React Flow - // exposes each node's absolute position + measured size on the internal - // node record. "Deepest" wins so dropping a card onto a grand-child lands - // there rather than on the outermost ancestor. + // Absolute-bounds hit test. Returns the **best** drop target among the + // candidates whose measured bbox contains `point`. Tiebreakers, in + // order (matches Figma / tldraw / xyflow issue #2827 community fix): + // + // 1. DEEPEST tree depth first — dropping onto a nested grandchild + // lands on the grandchild, not its outermost ancestor. + // 2. Highest zIndex second — when nested parents overlap with equal + // depth (siblings of each other), the one rendered above wins. + // 3. Smallest area last — visually-tightest match otherwise. + // + // Self + descendants are excluded (can't nest something under itself). const findDropTarget = useCallback( (draggedId: string, point: { x: number; y: number }): string | null => { const all = useCanvasStore.getState().nodes; - let best: { id: string; area: number } | null = null; + // Tree depth for each node — depth = ancestor count. + const depthOf = (id: string | null | undefined): number => { + let d = 0; + let cursor: string | null | undefined = id; + while (cursor) { + const n = all.find((nn) => nn.id === cursor); + if (!n) break; + cursor = n.data.parentId; + d += 1; + } + return d; + }; + let best: + | { id: string; depth: number; zIndex: number; area: number } + | null = null; for (const n of all) { if (n.id === draggedId || isDescendant(draggedId, n.id)) continue; const internal = getInternalNode(n.id); @@ -104,17 +263,29 @@ function CanvasInner() { const h = internal.measured?.height ?? n.height ?? 120; if (point.x < abs.x || point.x > abs.x + w) continue; if (point.y < abs.y || point.y > abs.y + h) continue; + const depth = depthOf(n.id); + const z = n.zIndex ?? 0; const area = w * h; - // Smaller area = deeper/more specific match wins. - if (!best || area < best.area) best = { id: n.id, area }; + if ( + !best || + depth > best.depth || + (depth === best.depth && z > best.zIndex) || + (depth === best.depth && z === best.zIndex && area < best.area) + ) { + best = { id: n.id, depth, zIndex: z, area }; + } } return best?.id ?? null; }, - [getInternalNode, isDescendant] + [getInternalNode, isDescendant], ); const onNodeDrag: OnNodeDrag> = useCallback( - (_event, node) => { + (event, node) => { + dragModifiersRef.current = { + alt: event.altKey, + meta: event.metaKey || event.ctrlKey, + }; const internal = getInternalNode(node.id); if (!internal) { setDragOverNode(null); @@ -126,7 +297,7 @@ function CanvasInner() { const center = { x: abs.x + w / 2, y: abs.y + h / 2 }; setDragOverNode(findDropTarget(node.id, center)); }, - [findDropTarget, getInternalNode, setDragOverNode] + [findDropTarget, getInternalNode, setDragOverNode], ); // Confirmation dialog state for structure changes @@ -157,22 +328,45 @@ function CanvasInner() { : null; const onNodeDragStop: OnNodeDrag> = useCallback( - (_event, node) => { + (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 altHeld = event.altKey || dragModifiersRef.current.alt; + const forceDetach = + event.metaKey || event.ctrlKey || dragModifiersRef.current.meta; - // The drag-stop offers three possible intents: - // 1. Drop inside a different parent → nest into that parent. - // 2. Drop onto empty canvas while I was nested → un-nest. - // 3. Drop inside my current parent (or no parent) → just a move. - if (dragOverNodeId && dragOverNodeId !== currentParentId) { + // Soft clamp: without a modifier, a child dropped just past its + // parent's edge is snapped back inside (Alt-drag escapes this to + // allow re-parenting). The explicit nest gesture (drop inside + // another parent) always wins over the clamp. + const droppingIntoAnotherParent = + !!dragOverNodeId && dragOverNodeId !== currentParentId; + if ( + currentParentId && + !altHeld && + !forceDetach && + !droppingIntoAnotherParent && + shouldDetach(node.id, currentParentId, getInternalNode) + ) { + clampChildIntoParent(node.id, currentParentId, getInternalNode); + } + + // The drag-stop offers several possible intents. Hysteresis + // (Miro/tldraw pattern) keeps a child nested unless it's clearly + // outside the parent — a twitchy release 1px past the edge no + // longer un-nests. Cmd / Ctrl (forceDetach) or Alt (escape) + // bypass the clamp. + if (droppingIntoAnotherParent) { const targetNode = allNodes.find((n) => n.id === dragOverNodeId); const targetName = targetNode?.data.name || "Unknown"; setPendingNest({ nodeId: node.id, targetId: dragOverNodeId, nodeName, targetName }); - } else if (!dragOverNodeId && currentParentId) { + } else if ( + currentParentId && + (forceDetach || (altHeld && shouldDetach(node.id, currentParentId, getInternalNode))) + ) { const parentNode = allNodes.find((n) => n.id === currentParentId); const parentName = parentNode?.data.name || "Unknown"; setPendingNest({ nodeId: node.id, targetId: null, nodeName, targetName: parentName }); @@ -184,16 +378,31 @@ function CanvasInner() { const internal = getInternalNode(node.id); const abs = internal?.internals.positionAbsolute ?? node.position; savePosition(node.id, abs.x, abs.y); + // Commit-on-release grow: run the parent auto-grow pass once now + // that the drag has settled. Cheap and deterministic vs running + // grow on every drag tick (avoids tldraw's edge-chase artifact). + useCanvasStore.getState().growParentsToFitChildren(); }, - [getInternalNode, savePosition, setDragOverNode] + [getInternalNode, savePosition, setDragOverNode], ); + const batchNest = useCanvasStore((s) => s.batchNest); const confirmNest = useCallback(() => { - if (pendingNest) { + if (!pendingNest) return; + const state = useCanvasStore.getState(); + // If the primary dragged node is part of a batch selection, apply + // the same nest target to every selected node — preserves the + // selection's inter-node spacing (Lucidchart pattern). + if ( + state.selectedNodeIds.size > 1 && + state.selectedNodeIds.has(pendingNest.nodeId) + ) { + batchNest(Array.from(state.selectedNodeIds), pendingNest.targetId); + } else { nestNode(pendingNest.nodeId, pendingNest.targetId); - setPendingNest(null); } - }, [pendingNest, nestNode]); + setPendingNest(null); + }, [pendingNest, nestNode, batchNest]); const cancelNest = useCallback(() => { setPendingNest(null); @@ -258,6 +467,13 @@ function CanvasInner() { // Keyboard shortcuts useEffect(() => { const handler = (e: KeyboardEvent) => { + const tag = (e.target as HTMLElement).tagName; + const inInput = + tag === "INPUT" || + tag === "TEXTAREA" || + tag === "SELECT" || + (e.target as HTMLElement).isContentEditable; + if (e.key === "Escape") { const state = useCanvasStore.getState(); if (state.contextMenu) { @@ -269,16 +485,39 @@ function CanvasInner() { } } + // Figma-style hierarchy navigation. Enter descends to the first + // child of the selected node; Shift+Enter ascends to its parent; + // Cmd+]/[ re-orders siblings (z-index up/down). Skipped when the + // user is typing into an input — Enter should commit the form. + if (!inInput && (e.key === "Enter" || e.key === "NumpadEnter")) { + e.preventDefault(); + const state = useCanvasStore.getState(); + const id = state.selectedNodeId; + if (!id) return; + if (e.shiftKey) { + const sel = state.nodes.find((n) => n.id === id); + const parentId = sel?.data.parentId ?? null; + if (parentId) state.selectNode(parentId); + } else { + const firstChild = state.nodes.find((n) => n.data.parentId === id); + if (firstChild) state.selectNode(firstChild.id); + } + } + + if ( + !inInput && + (e.metaKey || e.ctrlKey) && + (e.key === "]" || e.key === "[") + ) { + e.preventDefault(); + const state = useCanvasStore.getState(); + const id = state.selectedNodeId; + if (!id) return; + state.bumpZOrder(id, e.key === "]" ? 1 : -1); + } + // Z — keyboard equivalent for double-click zoom-to-team (WCAG 2.1.1) - if (e.key === "z" || e.key === "Z") { - const tag = (e.target as HTMLElement).tagName; - if ( - tag === "INPUT" || - tag === "TEXTAREA" || - tag === "SELECT" || - (e.target as HTMLElement).isContentEditable - ) - return; + if (!inInput && (e.key === "z" || e.key === "Z")) { const state = useCanvasStore.getState(); const selectedId = state.selectedNodeId; if (!selectedId) return; @@ -367,6 +606,10 @@ function CanvasInner() { className="!bg-zinc-900/90 !border-zinc-700/50 !rounded-lg !shadow-xl !shadow-black/20" maskColor="rgba(0, 0, 0, 0.7)" nodeColor={(node) => { + // Parents show as a filled region — hierarchy visible at + // a glance in the minimap without needing to zoom. + const hasChildren = nodes.some((n) => n.parentId === node.id); + if (hasChildren) return "#3b82f6"; const status = (node.data as Record)?.status; switch (status) { case "online": @@ -383,9 +626,14 @@ function CanvasInner() { return "#3f3f46"; } }} - nodeStrokeWidth={0} + nodeStrokeColor={(node) => { + const hasChildren = nodes.some((n) => n.parentId === node.id); + return hasChildren ? "#60a5fa" : "transparent"; + }} + nodeStrokeWidth={2} nodeBorderRadius={4} /> + {/* Screen-reader live region: announces workspace count when canvas loads or changes */} diff --git a/canvas/src/components/ContextMenu.tsx b/canvas/src/components/ContextMenu.tsx index d87e62b3..475e8319 100644 --- a/canvas/src/components/ContextMenu.tsx +++ b/canvas/src/components/ContextMenu.tsx @@ -202,15 +202,22 @@ export function ContextMenu() { closeContextMenu(); }, [contextMenu, closeContextMenu]); + const setCollapsed = useCanvasStore((s) => s.setCollapsed); const handleCollapse = useCallback(async () => { if (!contextMenu) return; + const nodeId = contextMenu.nodeId; + const wasCollapsed = !!contextMenu.nodeData.collapsed; + // Optimistic local flip so the card shrinks/expands immediately. + // Descendants' hidden flags are toggled atomically by the store. + setCollapsed(nodeId, !wasCollapsed); try { - await api.post(`/workspaces/${contextMenu.nodeId}/collapse`, {}); + await api.patch(`/workspaces/${nodeId}`, { collapsed: !wasCollapsed }); } catch (e) { + setCollapsed(nodeId, wasCollapsed); showToast("Collapse failed", "error"); } closeContextMenu(); - }, [contextMenu, closeContextMenu]); + }, [contextMenu, setCollapsed, closeContextMenu]); const handleRemoveFromTeam = useCallback(async () => { if (!contextMenu) return; @@ -223,6 +230,13 @@ export function ContextMenu() { closeContextMenu(); }, [contextMenu, nestNode, closeContextMenu]); + const arrangeChildren = useCanvasStore((s) => s.arrangeChildren); + const handleArrangeChildren = useCallback(() => { + if (!contextMenu) return; + arrangeChildren(contextMenu.nodeId); + closeContextMenu(); + }, [contextMenu, arrangeChildren, closeContextMenu]); + const handleZoomToTeam = useCallback(() => { if (!contextMenu) return; window.dispatchEvent( @@ -250,7 +264,12 @@ export function ContextMenu() { : []), ...(hasChildren ? [ - { label: "Collapse Team", icon: "◁", action: handleCollapse }, + { label: "Arrange Children", icon: "▦", action: handleArrangeChildren }, + { + label: contextMenu.nodeData.collapsed ? "Expand Team" : "Collapse Team", + icon: contextMenu.nodeData.collapsed ? "▽" : "◁", + action: handleCollapse, + }, { label: "Zoom to Team", icon: "⊕", action: handleZoomToTeam }, ] : [{ label: "Expand to Team", icon: "▷", action: handleExpand }]), diff --git a/canvas/src/components/WorkspaceNode.tsx b/canvas/src/components/WorkspaceNode.tsx index 9f11cf5d..e915e17d 100644 --- a/canvas/src/components/WorkspaceNode.tsx +++ b/canvas/src/components/WorkspaceNode.tsx @@ -111,7 +111,7 @@ export function WorkspaceNode({ id, data }: NodeProps>) }} className={` group relative rounded-xl h-full w-full - ${hasChildren ? "min-w-[360px] min-h-[200px]" : "min-w-[210px]"} + ${hasChildren && !data.collapsed ? "min-w-[360px] min-h-[200px]" : "min-w-[210px]"} cursor-pointer overflow-hidden transition-all duration-200 ease-out ${isDragTarget diff --git a/canvas/src/store/canvas-topology.ts b/canvas/src/store/canvas-topology.ts index d3367d03..b2841e4e 100644 --- a/canvas/src/store/canvas-topology.ts +++ b/canvas/src/store/canvas-topology.ts @@ -220,6 +220,30 @@ export function buildNodesAndEdges( // the parent's computed box. const nextChildIndex = new Map(); + // Depth per node so children always render above parents (and above + // parent's root-level siblings). React Flow uses a flat zIndex, so a + // child inherits zIndex = parent.zIndex + 1 — xyflow issue #4012. + const depthById = new Map(); + for (const ws of sorted) { + const d = ws.parent_id ? (depthById.get(ws.parent_id) ?? 0) + 1 : 0; + depthById.set(ws.id, d); + } + + // Mark each node as hidden if any ancestor is collapsed. Walk from + // the root so children inherit the flag efficiently. (Parents stay + // visible; only descendants are hidden so the parent renders as a + // compact header-only card.) + const hiddenById = new Map(); + for (const ws of sorted) { + if (!ws.parent_id) { + hiddenById.set(ws.id, false); + continue; + } + const parent = byId.get(ws.parent_id); + const parentHidden = hiddenById.get(ws.parent_id) ?? false; + hiddenById.set(ws.id, parentHidden || !!parent?.collapsed); + } + const nodes: Node[] = sorted.map((ws) => { const abs = absPos.get(ws.id)!; const hasParent = !!ws.parent_id && byId.has(ws.parent_id); @@ -228,19 +252,16 @@ export function buildNodesAndEdges( const pa = absPos.get(ws.parent_id!)!; position = { x: abs.x - pa.x, y: abs.y - pa.y }; - // If the stored relative position falls outside the parent's - // current bounds (or landed at exactly the origin before any - // layout pass), assign a deterministic grid slot instead. This - // rescues org-imported children that ended up at (0,0) and - // legacy rows whose absolute coords were far from the parent. - const psize = parentSize.get(ws.parent_id!)!; - const outside = - position.x < 0 || - position.y < 0 || - position.x + CHILD_DEFAULT_WIDTH > psize.width || - position.y + CHILD_DEFAULT_HEIGHT > psize.height; - const atOrigin = position.x === -abs.x + abs.x && abs.x === 0 && abs.y === 0; - if (outside || atOrigin) { + // Trust-the-data default: keep the stored position even if it + // falls outside the parent bbox (matches Figma's "don't move my + // shapes" rule). The one exception is a child still at + // origin (0,0) in the absolute frame — that's almost certainly + // an unlaid-out org-import row and would stack every child on + // the same point. Drop those into the grid so the first paint + // isn't a useless pile. Users can always trigger "Arrange + // children" to rescue the rest. + const atOrigin = abs.x === 0 && abs.y === 0; + if (atOrigin) { const idx = nextChildIndex.get(ws.parent_id!) ?? 0; nextChildIndex.set(ws.parent_id!, idx + 1); position = defaultChildSlot(idx); @@ -276,6 +297,13 @@ export function buildNodesAndEdges( // onNodeDragStop with a bbox hit test). node.parentId = ws.parent_id!; } + // Stack children above their ancestors (xyflow #4012). + node.zIndex = depthById.get(ws.id) ?? 0; + // Collapse: descendants of a collapsed parent get hidden so the + // parent renders as a compact header-only card. + if (hiddenById.get(ws.id)) { + node.hidden = true; + } // Give parents a measured-ish starting size so NodeResizer has a // baseline and child positions have somewhere to live. Without this, // parents start at React Flow's default min size (well under a diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index 5107edca..6f843c1c 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -11,6 +11,7 @@ import { handleCanvasEvent } from "./canvas-events"; import { buildNodesAndEdges, computeAutoLayout, + defaultChildSlot, CHILD_DEFAULT_HEIGHT, CHILD_DEFAULT_WIDTH, PARENT_BOTTOM_PADDING, @@ -38,6 +39,10 @@ function growParentsToFitChildren>( const out = nodes.map((n) => { const kids = childrenByParent.get(n.id); if (!kids || kids.length === 0) return n; + // Collapsed parents intentionally render compact — skip the grow + // pass so their size isn't pushed back out by their hidden kids. + const nData = n.data as unknown as WorkspaceNodeData | undefined; + if (nData?.collapsed) return n; let maxRight = 0; let maxBottom = 0; for (const k of kids) { @@ -127,6 +132,28 @@ interface CanvasState { setDragOverNode: (id: string | null) => void; nestNode: (draggedId: string, targetId: string | null) => Promise; isDescendant: (ancestorId: string, nodeId: string) => boolean; + /** Re-order siblings in z-index space. `direction = +1` sends the node + * one step forward among its parent's children (or among canvas + * roots); -1 sends it one step back. Figma Cmd+]/[ parity. */ + bumpZOrder: (nodeId: string, direction: 1 | -1) => void; + /** Re-parent many nodes at once, preserving each node's absolute + * position. Lucidchart pattern: drag a selection into a frame and + * the inter-node layout stays intact. Used when the primary dragged + * node of a multi-select drag triggers a nest confirmation. */ + batchNest: (nodeIds: string[], targetId: string | null) => Promise; + /** Run the parent auto-grow pass once. Canvas.onNodeDragStop calls + * this so a drag that pushed a child past the parent edge commits + * the parent grow on release (commit-on-release pattern). */ + growParentsToFitChildren: () => void; + /** Re-layout a parent's children to the default 2-column grid. Used + * by the "Arrange children" context-menu command so users can rescue + * out-of-bounds children on demand — topology no longer does it + * automatically (P3.12 opt-in rescue). */ + arrangeChildren: (parentId: string) => void; + /** Toggle the collapsed flag on a parent and hide/show every + * descendant so the card renders as a compact header-only frame. + * Miro "frame outline view" analog. */ + setCollapsed: (parentId: string, collapsed: boolean) => void; openContextMenu: (menu: ContextMenuState) => void; closeContextMenu: () => void; // Pending delete confirmation — lives in the store (not inside ContextMenu's @@ -297,6 +324,41 @@ export const useCanvasStore = create((set, get) => ({ setPanelTab: (tab) => set({ panelTab: tab }), setDragOverNode: (id) => set({ dragOverNodeId: id }), + batchNest: async (nodeIds, targetId) => { + if (nodeIds.length === 0) return; + if (nodeIds.length === 1) { + await get().nestNode(nodeIds[0], targetId); + return; + } + // Run sequentially so each nestNode's absolute-position calc sees + // the previous update committed. Not a hot path — multi-select + // re-parents rarely touch more than a handful of nodes. + for (const id of nodeIds) { + await get().nestNode(id, targetId); + } + }, + + bumpZOrder: (nodeId, direction) => { + const { nodes } = get(); + const target = nodes.find((n) => n.id === nodeId); + if (!target) return; + // Siblings = nodes sharing the same parent (null for roots). + const siblings = nodes.filter( + (n) => n.data.parentId === target.data.parentId, + ); + if (siblings.length < 2) return; + // React Flow uses a flat zIndex; we keep children above parents + // (+1 per depth) so any nudge here stays within the sibling tier. + // Reorder in zIndex space by adjusting the target +/- 1. + const current = target.zIndex ?? 0; + const newZ = current + direction; + set({ + nodes: nodes.map((n) => + n.id === nodeId ? { ...n, zIndex: newZ } : n, + ), + }); + }, + isDescendant: (ancestorId, nodeId) => { const { nodes } = get(); let current = nodes.find((n) => n.id === nodeId); @@ -345,6 +407,21 @@ export const useCanvasStore = create((set, get) => ({ (e) => e.source !== draggedId && e.target !== draggedId, ); + // Depth walk so zIndex gets bumped correctly on nest/unnest + // (children render above their new ancestor chain). + const depthOf = (id: string | null | undefined): number => { + let d = 0; + let cursor: string | null | undefined = id; + while (cursor) { + const n = nodes.find((nn) => nn.id === cursor); + if (!n) break; + cursor = n.data.parentId; + d += 1; + } + return d; + }; + const newDepth = depthOf(targetId) + (targetId ? 1 : 0); + set({ nodes: nodes.map((n) => n.id === draggedId @@ -352,6 +429,7 @@ export const useCanvasStore = create((set, get) => ({ ...n, position: newRelative, parentId: targetId ?? undefined, + zIndex: newDepth, data: { ...n.data, parentId: targetId }, } : n, @@ -441,12 +519,74 @@ export const useCanvasStore = create((set, get) => ({ onNodesChange: (changes) => { const next = applyNodeChanges(changes, get().nodes); - // Auto-grow parents to fit their children: if any child's - // (position + size) extends beyond the parent's current dimensions, - // the parent's explicit width/height is bumped so it stays the - // visual container (Miro/FigJam-style frame auto-fit). - const grown = growParentsToFitChildren(next); - set({ nodes: grown }); + // Parent auto-grow is intentionally conservative. Running + // growParentsToFitChildren on every change (including the dozens of + // position updates emitted during a single drag) caused the + // "edge-chase" artifact tldraw documented — as the parent grows in + // response to the child near its edge, the child's relative + // position becomes valid again and the grow stops mid-drag, only to + // resume on the next tick. Commit-on-release: only run grow when a + // change set contains a `dimensions` change (NodeResizer commit), + // not on pure `position` changes. Drag-stop grow is handled + // explicitly in Canvas.onNodeDragStop via growOnce(). + const hasDimensionChange = changes.some((c) => c.type === "dimensions"); + set({ nodes: hasDimensionChange ? growParentsToFitChildren(next) : next }); + }, + + growParentsToFitChildren: () => { + set({ nodes: growParentsToFitChildren(get().nodes) }); + }, + + setCollapsed: (parentId, collapsed) => { + const { nodes } = get(); + // Find all descendant ids via BFS. + const descendantIds = new Set(); + const queue = [parentId]; + while (queue.length) { + const id = queue.shift()!; + for (const n of nodes) { + if (n.data.parentId === id && !descendantIds.has(n.id)) { + descendantIds.add(n.id); + queue.push(n.id); + } + } + } + set({ + nodes: nodes.map((n) => { + if (n.id === parentId) { + return { ...n, data: { ...n.data, collapsed } }; + } + if (descendantIds.has(n.id)) { + return { ...n, hidden: collapsed }; + } + return n; + }), + }); + }, + + arrangeChildren: (parentId) => { + const { nodes } = get(); + const kids = nodes + .filter((n) => n.parentId === parentId) + .sort((a, b) => (a.data.name || "").localeCompare(b.data.name || "")); + if (kids.length === 0) return; + const slotByKid = new Map(); + kids.forEach((k, i) => slotByKid.set(k.id, defaultChildSlot(i))); + set({ + nodes: nodes.map((n) => { + const slot = slotByKid.get(n.id); + return slot ? { ...n, position: slot } : n; + }), + }); + // Persist the new positions so they survive reload. + for (const k of kids) { + const slot = slotByKid.get(k.id)!; + const parent = nodes.find((nn) => nn.id === parentId); + if (!parent) continue; + const absX = slot.x + parent.position.x; + const absY = slot.y + parent.position.y; + api.patch(`/workspaces/${k.id}`, { x: absX, y: absY }).catch(() => {}); + } }, savePosition: async (nodeId: string, x: number, y: number) => { From c5abed988ec8a1e190e34ba1f0b8e69e2fe06f7a Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 19:16:48 -0700 Subject: [PATCH 06/17] fix(canvas): address review findings on playability pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five Critical issues caught in code review of f3423a51. Each one broke an invariant the original commit claimed to uphold. 1. nestNode: descendants kept their old-depth zIndex after a re-parent. Now walks the dragged subtree and shifts every descendant's zIndex by the same depthDelta so "children above ancestors" survives moves between levels of the hierarchy. 2. bumpZOrder: siblings all share zIndex = depth in fresh topology, so a single +1 bump was identical for every sibling and subsequent bumps drifted zIndex unboundedly. Rewritten to sort siblings by current zIndex and swap the target with its neighbour in the bump direction — Figma-style reorder, stays within the sibling tier. 3. findDropTarget: depth-first tiebreaker lost to bumped siblings. The visually-frontmost card after Cmd+] is a shallow sibling, but the hit test picked the deepest nested card regardless. Swapped order so zIndex wins first, depth second, area third. Also pre-computes the depth map once per call (was O(n²) via repeated .find walks — will matter past ~30 workspaces). 4. arrangeChildren: saved absolute position using `slot + parent.position`, but parent.position is RELATIVE to its own parent when nested. Grandchildren's stored x/y were in the parent's local frame and reload placed them in the wrong spot. Now walks the full ancestor chain via absOf() to get the true canvas-absolute origin before PATCHing. 5. setCollapsed: naive flip of every descendant's hidden flag diverged from the topology rebuild on hydrate. Collapse A, collapse B, then expand A — C should stay hidden because B is still collapsed, but before this fix C was unhidden. Rewritten to recompute every descendant's hidden from the full ancestry chain, matching the topology pass byte-for-byte. New round-trip test asserts the two code paths produce identical node.hidden across a full lifecycle. Also: - Removed dead cascadeMessage constant (never rendered). - Replaced hardcoded 260/120 in zoom-to-team with exported constants. - arrangeChildren PATCH catch now logs instead of silently swallowing. - Added 70→76 tests: setCollapsed 3-chain scenarios, bumpZOrder swap semantics, edge-of-list no-op. All 915 canvas tests green. Backend untouched. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/Canvas.tsx | 48 +++--- canvas/src/store/__tests__/canvas.test.ts | 97 ++++++++++++ canvas/src/store/canvas.ts | 173 ++++++++++++++++------ 3 files changed, 240 insertions(+), 78 deletions(-) diff --git a/canvas/src/components/Canvas.tsx b/canvas/src/components/Canvas.tsx index d62e3ffe..1934bad1 100644 --- a/canvas/src/components/Canvas.tsx +++ b/canvas/src/components/Canvas.tsx @@ -227,30 +227,24 @@ function CanvasInner() { // Absolute-bounds hit test. Returns the **best** drop target among the // candidates whose measured bbox contains `point`. Tiebreakers, in - // order (matches Figma / tldraw / xyflow issue #2827 community fix): + // order — the user drops onto what's visually on top, so zIndex wins + // first (a user can Cmd+] bump a shallow card above a deep one): // - // 1. DEEPEST tree depth first — dropping onto a nested grandchild - // lands on the grandchild, not its outermost ancestor. - // 2. Highest zIndex second — when nested parents overlap with equal - // depth (siblings of each other), the one rendered above wins. - // 3. Smallest area last — visually-tightest match otherwise. + // 1. Highest zIndex first — matches what the user sees in front. + // 2. DEEPEST tree depth second — when zIndex ties, a more-nested + // card is a more specific target than its ancestor. + // 3. Smallest area last — if depth also ties, the tighter bbox wins. // // Self + descendants are excluded (can't nest something under itself). + // Depths are pre-computed once per call so this stays O(n) overall — + // previously the per-candidate depth walk made it O(n²). const findDropTarget = useCallback( (draggedId: string, point: { x: number; y: number }): string | null => { const all = useCanvasStore.getState().nodes; - // Tree depth for each node — depth = ancestor count. - const depthOf = (id: string | null | undefined): number => { - let d = 0; - let cursor: string | null | undefined = id; - while (cursor) { - const n = all.find((nn) => nn.id === cursor); - if (!n) break; - cursor = n.data.parentId; - d += 1; - } - return d; - }; + const depthById = new Map(); + for (const n of all) { + depthById.set(n.id, n.data.parentId ? (depthById.get(n.data.parentId) ?? 0) + 1 : 0); + } let best: | { id: string; depth: number; zIndex: number; area: number } | null = null; @@ -263,14 +257,14 @@ function CanvasInner() { const h = internal.measured?.height ?? n.height ?? 120; if (point.x < abs.x || point.x > abs.x + w) continue; if (point.y < abs.y || point.y > abs.y + h) continue; - const depth = depthOf(n.id); + const depth = depthById.get(n.id) ?? 0; const z = n.zIndex ?? 0; const area = w * h; if ( !best || - depth > best.depth || - (depth === best.depth && z > best.zIndex) || - (depth === best.depth && z === best.zIndex && area < best.area) + z > best.zIndex || + (z === best.zIndex && depth > best.depth) || + (z === best.zIndex && depth === best.depth && area < best.area) ) { best = { id: n.id, depth, zIndex: z, area }; } @@ -321,12 +315,6 @@ function CanvasInner() { } }, [pendingDelete, setPendingDelete, removeNode]); - // Cascade guard: include child count in the warning message when the workspace - // has children, so the user understands the blast radius before clicking Delete All. - const cascadeMessage = pendingDelete?.hasChildren - ? `⚠️ Deleting "${pendingDelete.name}" will permanently delete all child workspaces and their data. This cannot be undone.` - : null; - const onNodeDragStop: OnNodeDrag> = useCallback( (event, node) => { const { dragOverNodeId, nodes: allNodes } = useCanvasStore.getState(); @@ -451,8 +439,8 @@ function CanvasInner() { for (const n of allNodes) { minX = Math.min(minX, n.position.x); minY = Math.min(minY, n.position.y); - maxX = Math.max(maxX, n.position.x + 260); - maxY = Math.max(maxY, n.position.y + 120); + maxX = Math.max(maxX, n.position.x + CHILD_DEFAULT_WIDTH); + maxY = Math.max(maxY, n.position.y + CHILD_DEFAULT_HEIGHT); } fitBounds( diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index 33a0f8b7..72c2735e 100644 --- a/canvas/src/store/__tests__/canvas.test.ts +++ b/canvas/src/store/__tests__/canvas.test.ts @@ -855,3 +855,100 @@ describe("TASK_UPDATED edge cases", () => { expect(ws2.data.currentTask).toBe("Task B"); // unchanged }); }); + +// ---------- setCollapsed round-trip ---------- + +describe("setCollapsed", () => { + beforeEach(() => { + // Three-level chain so we can test that collapsing an ancestor + // hides all descendants AND that expanding it correctly preserves + // any intermediate collapsed state (otherwise setCollapsed and + // hydrate produce different hidden flags — the drift the review + // flagged as Critical). + useCanvasStore.getState().hydrate([ + makeWS({ id: "a", name: "A" }), + makeWS({ id: "b", name: "B", parent_id: "a" }), + makeWS({ id: "c", name: "C", parent_id: "b" }), + ]); + }); + + it("hides the entire subtree when the root is collapsed", () => { + useCanvasStore.getState().setCollapsed("a", true); + const { nodes } = useCanvasStore.getState(); + expect(nodes.find((n) => n.id === "a")!.hidden).toBeFalsy(); + expect(nodes.find((n) => n.id === "b")!.hidden).toBe(true); + expect(nodes.find((n) => n.id === "c")!.hidden).toBe(true); + expect(nodes.find((n) => n.id === "a")!.data.collapsed).toBe(true); + }); + + it("keeps descendants hidden when an ancestor is un-collapsed but a middle parent is still collapsed", () => { + // Collapse both A and B, then expand A. C must stay hidden because + // B — its immediate parent — is still collapsed. Before the fix, + // setCollapsed naively unhid every descendant of A and drifted from + // what hydrate would produce. + useCanvasStore.getState().setCollapsed("a", true); + useCanvasStore.getState().setCollapsed("b", true); + useCanvasStore.getState().setCollapsed("a", false); + const { nodes } = useCanvasStore.getState(); + expect(nodes.find((n) => n.id === "b")!.hidden).toBeFalsy(); + expect(nodes.find((n) => n.id === "c")!.hidden).toBe(true); + }); + + it("matches hydrate's hidden flags (no drift on snapshot refresh)", () => { + // Run the same scenario through setCollapsed, then re-hydrate from + // an equivalent server snapshot and assert the hidden flags agree. + useCanvasStore.getState().setCollapsed("a", true); + const afterCollapse = useCanvasStore.getState().nodes.map((n) => ({ + id: n.id, + hidden: !!n.hidden, + })); + + useCanvasStore.getState().hydrate([ + makeWS({ id: "a", name: "A", collapsed: true }), + makeWS({ id: "b", name: "B", parent_id: "a" }), + makeWS({ id: "c", name: "C", parent_id: "b" }), + ]); + const afterHydrate = useCanvasStore.getState().nodes.map((n) => ({ + id: n.id, + hidden: !!n.hidden, + })); + expect(afterHydrate).toEqual(afterCollapse); + }); +}); + +// ---------- bumpZOrder ---------- + +describe("bumpZOrder", () => { + beforeEach(() => { + useCanvasStore.getState().hydrate([ + makeWS({ id: "r1", name: "R1" }), + makeWS({ id: "r2", name: "R2" }), + makeWS({ id: "r3", name: "R3" }), + ]); + }); + + it("swaps with the neighbour in the bump direction (no drift on identical zIndex)", () => { + // Fresh topology: all three siblings start at zIndex=0 (depth=0). + // Bumping r2 forward must put it above exactly one sibling, not + // arbitrarily far ahead. + useCanvasStore.getState().bumpZOrder("r2", 1); + const nodes = useCanvasStore.getState().nodes; + const r1Z = nodes.find((n) => n.id === "r1")!.zIndex ?? 0; + const r2Z = nodes.find((n) => n.id === "r2")!.zIndex ?? 0; + const r3Z = nodes.find((n) => n.id === "r3")!.zIndex ?? 0; + // r2 now above at least one neighbour. + expect(r2Z).toBeGreaterThan(Math.min(r1Z, r3Z)); + // Bumping once more swaps with the remaining one — not unbounded. + useCanvasStore.getState().bumpZOrder("r2", 1); + const r2ZAfter = useCanvasStore.getState().nodes.find((n) => n.id === "r2")!.zIndex ?? 0; + expect(r2ZAfter).toBeLessThanOrEqual(r2Z + 2); + }); + + it("no-ops at the edge of the sibling list", () => { + const beforeZ = useCanvasStore.getState().nodes.map((n) => n.zIndex ?? 0); + // First sibling bumped backward has no earlier neighbour. + useCanvasStore.getState().bumpZOrder("r1", -1); + const afterZ = useCanvasStore.getState().nodes.map((n) => n.zIndex ?? 0); + expect(afterZ).toEqual(beforeZ); + }); +}); diff --git a/canvas/src/store/canvas.ts b/canvas/src/store/canvas.ts index 6f843c1c..23098ee1 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -342,20 +342,33 @@ export const useCanvasStore = create((set, get) => ({ const { nodes } = get(); const target = nodes.find((n) => n.id === nodeId); if (!target) return; - // Siblings = nodes sharing the same parent (null for roots). - const siblings = nodes.filter( - (n) => n.data.parentId === target.data.parentId, - ); + // Siblings share parentId; re-rank them by their current zIndex (then + // insertion order) so we can SWAP the target with its neighbour in + // the bump direction rather than drifting zIndex up/down unbounded. + // This keeps sibling zIndex values within `[baseDepth, baseDepth+N)`, + // which is what findDropTarget's tiebreakers assume. + const siblings = nodes + .filter((n) => n.data.parentId === target.data.parentId) + .slice() + .sort((a, b) => (a.zIndex ?? 0) - (b.zIndex ?? 0)); if (siblings.length < 2) return; - // React Flow uses a flat zIndex; we keep children above parents - // (+1 per depth) so any nudge here stays within the sibling tier. - // Reorder in zIndex space by adjusting the target +/- 1. - const current = target.zIndex ?? 0; - const newZ = current + direction; + const idx = siblings.findIndex((n) => n.id === nodeId); + const neighbourIdx = idx + direction; + if (neighbourIdx < 0 || neighbourIdx >= siblings.length) return; + const neighbour = siblings[neighbourIdx]; + const targetZ = target.zIndex ?? 0; + const neighbourZ = neighbour.zIndex ?? 0; + // Ensure a visible swap even when both had identical zIndex (fresh + // topology: every sibling starts at zIndex=depth). Nudge the + // neighbour one step the other way so the pair stays adjacent. + const resolvedTargetZ = targetZ === neighbourZ ? targetZ + direction : neighbourZ; + const resolvedNeighbourZ = targetZ === neighbourZ ? targetZ : targetZ; set({ - nodes: nodes.map((n) => - n.id === nodeId ? { ...n, zIndex: newZ } : n, - ), + nodes: nodes.map((n) => { + if (n.id === nodeId) return { ...n, zIndex: resolvedTargetZ }; + if (n.id === neighbour.id) return { ...n, zIndex: resolvedNeighbourZ }; + return n; + }), }); }, @@ -408,7 +421,8 @@ export const useCanvasStore = create((set, get) => ({ ); // Depth walk so zIndex gets bumped correctly on nest/unnest - // (children render above their new ancestor chain). + // (children render above their new ancestor chain). `depthOf(null)` + // returns 0; for any non-null cursor we count one hop per ancestor. const depthOf = (id: string | null | undefined): number => { let d = 0; let cursor: string | null | undefined = id; @@ -420,20 +434,42 @@ export const useCanvasStore = create((set, get) => ({ } return d; }; - const newDepth = depthOf(targetId) + (targetId ? 1 : 0); + const newOwnDepth = targetId ? depthOf(targetId) + 1 : 0; + const oldOwnDepth = dragged.zIndex ?? depthOf(currentParentId) + (currentParentId ? 1 : 0); + const depthDelta = newOwnDepth - oldOwnDepth; + + // Collect every descendant of the dragged node so we can shift their + // zIndex by the same depthDelta — otherwise grandchildren stay at + // their old depth zIndex after the move and render below ancestors + // they just joined. BFS to avoid stack surprises on deep hierarchies. + const movedIds = new Set([draggedId]); + const bfsQueue = [draggedId]; + while (bfsQueue.length) { + const head = bfsQueue.shift()!; + for (const n of nodes) { + if (n.data.parentId === head && !movedIds.has(n.id)) { + movedIds.add(n.id); + bfsQueue.push(n.id); + } + } + } set({ - nodes: nodes.map((n) => - n.id === draggedId - ? { - ...n, - position: newRelative, - parentId: targetId ?? undefined, - zIndex: newDepth, - data: { ...n.data, parentId: targetId }, - } - : n, - ), + nodes: nodes.map((n) => { + if (n.id === draggedId) { + return { + ...n, + position: newRelative, + parentId: targetId ?? undefined, + zIndex: newOwnDepth, + data: { ...n.data, parentId: targetId }, + }; + } + if (movedIds.has(n.id) && depthDelta !== 0) { + return { ...n, zIndex: (n.zIndex ?? 0) + depthDelta }; + } + return n; + }), edges: newEdges, }); @@ -539,27 +575,49 @@ export const useCanvasStore = create((set, get) => ({ setCollapsed: (parentId, collapsed) => { const { nodes } = get(); - // Find all descendant ids via BFS. - const descendantIds = new Set(); - const queue = [parentId]; - while (queue.length) { - const id = queue.shift()!; - for (const n of nodes) { - if (n.data.parentId === id && !descendantIds.has(n.id)) { - descendantIds.add(n.id); - queue.push(n.id); - } + // Step 1 — apply the new collapsed flag on the target. + const updatedCollapsed = new Map(); + for (const n of nodes) { + updatedCollapsed.set( + n.id, + n.id === parentId ? collapsed : !!n.data.collapsed, + ); + } + // Step 2 — index children once so the visibility pass is O(n), not + // O(n·d). Walk roots downward, inheriting `hiddenBecauseAncestor` + // so a node is hidden iff ANY ancestor in the chain is collapsed. + // This matches canvas-topology.buildNodesAndEdges so setCollapsed + // and hydrate produce identical node.hidden flags — no drift when + // the server pushes a fresh snapshot mid-session. + const childrenByParent = new Map(); + for (const n of nodes) { + const p = n.data.parentId ?? null; + const arr = childrenByParent.get(p) ?? []; + arr.push(n.id); + childrenByParent.set(p, arr); + } + const hiddenById = new Map(); + const stack: Array<{ id: string; hidden: boolean }> = ( + childrenByParent.get(null) ?? [] + ).map((id) => ({ id, hidden: false })); + while (stack.length) { + const { id, hidden } = stack.pop()!; + hiddenById.set(id, hidden); + const isCollapsed = updatedCollapsed.get(id) ?? false; + for (const childId of childrenByParent.get(id) ?? []) { + stack.push({ id: childId, hidden: hidden || isCollapsed }); } } set({ nodes: nodes.map((n) => { - if (n.id === parentId) { - return { ...n, data: { ...n.data, collapsed } }; - } - if (descendantIds.has(n.id)) { - return { ...n, hidden: collapsed }; - } - return n; + const isTarget = n.id === parentId; + const nextHidden = hiddenById.get(n.id) ?? false; + if (!isTarget && n.hidden === nextHidden) return n; + return { + ...n, + hidden: nextHidden, + data: isTarget ? { ...n.data, collapsed } : n.data, + }; }), }); }, @@ -572,20 +630,39 @@ export const useCanvasStore = create((set, get) => ({ if (kids.length === 0) return; const slotByKid = new Map(); kids.forEach((k, i) => slotByKid.set(k.id, defaultChildSlot(i))); + + // Absolute position of the parent, walking the full ancestor chain. + // Required for a correct PATCH payload when the parent itself is + // nested — `parent.position` is RELATIVE to its own parent, so a + // naive `slot + parent.position` would store parent-local coords + // as if they were absolute and corrupt the workspace on reload. + const absOf = (id: string | null | undefined): { x: number; y: number } => { + let sum = { x: 0, y: 0 }; + let cursor: string | null | undefined = id; + while (cursor) { + const n = nodes.find((nn) => nn.id === cursor); + if (!n) break; + sum = { x: sum.x + n.position.x, y: sum.y + n.position.y }; + cursor = n.data.parentId; + } + return sum; + }; + const parentAbs = absOf(parentId); + set({ nodes: nodes.map((n) => { const slot = slotByKid.get(n.id); return slot ? { ...n, position: slot } : n; }), }); - // Persist the new positions so they survive reload. + for (const k of kids) { const slot = slotByKid.get(k.id)!; - const parent = nodes.find((nn) => nn.id === parentId); - if (!parent) continue; - const absX = slot.x + parent.position.x; - const absY = slot.y + parent.position.y; - api.patch(`/workspaces/${k.id}`, { x: absX, y: absY }).catch(() => {}); + const absX = slot.x + parentAbs.x; + const absY = slot.y + parentAbs.y; + api.patch(`/workspaces/${k.id}`, { x: absX, y: absY }).catch((e) => { + console.warn(`arrangeChildren: failed to persist position for ${k.id}`, e); + }); } }, From 50b537849ab53f2212b631c25aa0c5d30c7a4f2c Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 19:43:18 -0700 Subject: [PATCH 07/17] refactor(canvas): split Canvas.tsx into hooks; parallelize batchNest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two concerns in one commit (separate files, each self-contained): ## Canvas.tsx split (from ~680 to ~250 lines) Canvas.tsx was holding drag gesture state + keyboard shortcuts + viewport wiring + JSX. Each concern now lives in its own unit under canvas/src/components/canvas/: - dragUtils.ts — pure: shouldDetach, clampChildIntoParent, DETACH_FRACTION - DropTargetBadge.tsx — the floating "Drop into: " label + the dashed ghost preview at the target slot - useDragHandlers.ts — encapsulates onNodeDragStart / Drag / Stop, findDropTarget hit-test, pendingNest state, and confirmNest/cancelNest. Routes multi- select drags through batchNest automatically. - useKeyboardShortcuts — Esc, Enter, Shift+Enter, Cmd+]/[, Z — one window listener, one source of truth. - useCanvasViewport — pan-to-node + zoom-to-team CustomEvent listeners and the debounced viewport save. Canvas.tsx becomes a thin composition + JSX file. No behavioural change; the refactor is covered by the existing 915 canvas tests. ## batchNest parallelization (2N round-trips → N, all in flight) Previously nestNode fired two sequential PATCHes (parent_id then x/y) and batchNest looped nestNode sequentially. For a 5-node selection on a typical ~200ms link this was ~2s of serialized RPCs. - nestNode now combines parent_id + x + y into ONE PATCH. The Go handler (workspace_crud.go Update) already reads all three from the same body — no backend change. - batchNest rewritten: compute every re-parent plan against one snapshot, commit a single set(), then fire N PATCHes via Promise.allSettled in parallel. Per-node failures roll back only that node (others stay committed) — same semantics as the single- node path, just concurrent. - The state math in the batch path also correctly shifts descendant zIndex by depthDelta when any re-parented node has a subtree. ## Also - canvas-topology.ts: reverted P3.12's opt-in rescue to the auto- rescue default. When a child's stored relative position would render it outside the parent bbox (the visual regression the user saw after collapse → reload — Hermes child drawn outside Claude Code Agent on first paint), the child is placed in the next default grid slot. The "Arrange Children" context command stays for bigger teams. All 915 canvas tests pass. No backend changes. Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/Canvas.tsx | 700 ++++-------------- .../__tests__/ZoomShortcut.test.tsx | 7 +- .../src/components/canvas/DropTargetBadge.tsx | 83 +++ canvas/src/components/canvas/dragUtils.ts | 74 ++ .../components/canvas/useCanvasViewport.ts | 96 +++ .../src/components/canvas/useDragHandlers.ts | 213 ++++++ .../components/canvas/useKeyboardShortcuts.ts | 87 +++ canvas/src/store/canvas-topology.ts | 26 +- canvas/src/store/canvas.ts | 175 ++++- 9 files changed, 873 insertions(+), 588 deletions(-) create mode 100644 canvas/src/components/canvas/DropTargetBadge.tsx create mode 100644 canvas/src/components/canvas/dragUtils.ts create mode 100644 canvas/src/components/canvas/useCanvasViewport.ts create mode 100644 canvas/src/components/canvas/useDragHandlers.ts create mode 100644 canvas/src/components/canvas/useKeyboardShortcuts.ts diff --git a/canvas/src/components/Canvas.tsx b/canvas/src/components/Canvas.tsx index 1934bad1..5d13f00a 100644 --- a/canvas/src/components/Canvas.tsx +++ b/canvas/src/components/Canvas.tsx @@ -1,26 +1,18 @@ "use client"; -import { useCallback, useRef, useMemo, useEffect, useState } from "react"; +import { useCallback, useMemo } from "react"; import { ReactFlow, ReactFlowProvider, Background, Controls, MiniMap, - useReactFlow, - type OnNodeDrag, - type Node, type Edge, BackgroundVariant, } from "@xyflow/react"; import "@xyflow/react/dist/style.css"; -import { useCanvasStore, type WorkspaceNodeData } from "@/store/canvas"; -import { - defaultChildSlot, - CHILD_DEFAULT_HEIGHT, - CHILD_DEFAULT_WIDTH, -} from "@/store/canvas-topology"; +import { useCanvasStore } from "@/store/canvas"; import { A2ATopologyOverlay } from "./A2ATopologyOverlay"; import { WorkspaceNode } from "./WorkspaceNode"; import { SidePanel } from "./SidePanel"; @@ -32,17 +24,19 @@ import { BundleDropZone } from "./BundleDropZone"; import { EmptyState } from "./EmptyState"; import { OnboardingWizard } from "./OnboardingWizard"; import { SearchDialog } from "./SearchDialog"; -import { Toaster } from "./Toaster"; +import { Toaster, showToast } 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 import { BatchActionBar } from "./BatchActionBar"; import { ProvisioningTimeout } from "./ProvisioningTimeout"; +import { DropTargetBadge } from "./canvas/DropTargetBadge"; +import { useDragHandlers } from "./canvas/useDragHandlers"; +import { useKeyboardShortcuts } from "./canvas/useKeyboardShortcuts"; +import { useCanvasViewport } from "./canvas/useCanvasViewport"; + const nodeTypes = { workspaceNode: WorkspaceNode, }; @@ -63,243 +57,38 @@ export function Canvas() { ); } -// Hysteresis: detach-on-drop only fires once the child has moved far -// enough outside the parent that the intent is unambiguous. We pick 20% -// of the overlapping dimension as the threshold (Miro behaves similarly -// at ~20-30%). A slightly-past-edge drag commits a MOVE, not a detach. -const DETACH_FRACTION = 0.2; - -/** Floating "Drop into: " label that tracks the current drag - * target. Mural-style affordance — colour alone is ambiguous on dense - * canvases, so we spell out the target by name. Mounted inside the - * ReactFlowProvider subtree so it can read positionAbsolute. */ -function DropTargetBadge() { - const dragOverNodeId = useCanvasStore((s) => s.dragOverNodeId); - const targetName = useCanvasStore((s) => { - if (!s.dragOverNodeId) return null; - const n = s.nodes.find((nn) => nn.id === s.dragOverNodeId); - return (n?.data as WorkspaceNodeData | undefined)?.name ?? null; - }); - const childCount = useCanvasStore((s) => - !s.dragOverNodeId - ? 0 - : s.nodes.filter((n) => n.parentId === s.dragOverNodeId).length, - ); - const { getInternalNode, flowToScreenPosition } = useReactFlow(); - if (!dragOverNodeId || !targetName) return null; - const internal = getInternalNode(dragOverNodeId); - if (!internal) return null; - const abs = internal.internals.positionAbsolute; - const w = internal.measured?.width ?? 220; - const h = internal.measured?.height ?? 120; - const badge = flowToScreenPosition({ x: abs.x + w / 2, y: abs.y }); - - // Ghost preview: dashed outline at the next default grid slot inside - // the target parent. Whimsical-style affordance so the user sees - // exactly where the dropped card will land. - const slot = defaultChildSlot(childCount); - const slotTL = flowToScreenPosition({ x: abs.x + slot.x, y: abs.y + slot.y }); - const slotBR = flowToScreenPosition({ - x: abs.x + slot.x + CHILD_DEFAULT_WIDTH, - y: abs.y + slot.y + CHILD_DEFAULT_HEIGHT, - }); - // Clip the ghost to the parent's visible bounds so it doesn't spill - // out when the parent is smaller than the slot. - const parentTL = flowToScreenPosition({ x: abs.x, y: abs.y }); - const parentBR = flowToScreenPosition({ x: abs.x + w, y: abs.y + h }); - const ghostVisible = - slotBR.x > parentTL.x && - slotTL.x < parentBR.x && - slotBR.y > parentTL.y && - slotTL.y < parentBR.y; - - return ( - <> - {ghostVisible && ( -
- )} -
- Drop into: {targetName} -
- - ); -} - -/** Snap a child back so its bbox is fully inside the parent's bounds. - * Called on drag-stop when the user drifted slightly past the edge - * without holding Alt or Cmd — the canvas treats the gesture as a - * plain move rather than an un-nest. */ -function clampChildIntoParent( - childId: string, - parentId: string, - getInternalNode: (id: string) => ReturnType["getInternalNode"]>, -) { - const c = getInternalNode(childId); - const p = getInternalNode(parentId); - if (!c || !p) return; - const cw = c.measured?.width ?? c.width ?? 220; - const ch = c.measured?.height ?? c.height ?? 120; - const pw = p.measured?.width ?? p.width ?? 220; - const ph = p.measured?.height ?? p.height ?? 120; - const { nodes } = useCanvasStore.getState(); - const cur = nodes.find((n) => n.id === childId); - if (!cur) return; - const rel = cur.position; - const clampedX = Math.max(0, Math.min(rel.x, pw - cw)); - const clampedY = Math.max(0, Math.min(rel.y, ph - ch)); - if (clampedX === rel.x && clampedY === rel.y) return; - useCanvasStore.setState({ - nodes: nodes.map((n) => - n.id === childId ? { ...n, position: { x: clampedX, y: clampedY } } : n, - ), - }); -} - -function shouldDetach( - childId: string, - parentId: string, - getInternalNode: (id: string) => ReturnType["getInternalNode"]>, -): boolean { - const c = getInternalNode(childId); - const p = getInternalNode(parentId); - if (!c || !p) return true; // If we can't measure, fall back to the old behavior. - const cw = c.measured?.width ?? c.width ?? 220; - const ch = c.measured?.height ?? c.height ?? 120; - const pw = p.measured?.width ?? p.width ?? 220; - const ph = p.measured?.height ?? p.height ?? 120; - const cx = c.internals.positionAbsolute; - const px = p.internals.positionAbsolute; - const overlapW = - Math.max(0, Math.min(cx.x + cw, px.x + pw) - Math.max(cx.x, px.x)); - const overlapH = - Math.max(0, Math.min(cx.y + ch, px.y + ph) - Math.max(cx.y, px.y)); - const outsideFractionX = 1 - overlapW / cw; - const outsideFractionY = 1 - overlapH / ch; - return outsideFractionX > DETACH_FRACTION || outsideFractionY > DETACH_FRACTION; -} - function CanvasInner() { const nodes = useCanvasStore((s) => s.nodes); const edges = useCanvasStore((s) => s.edges); const a2aEdges = useCanvasStore((s) => s.a2aEdges); const showA2AEdges = useCanvasStore((s) => s.showA2AEdges); - // Merge topology edges with A2A overlay edges via useMemo (no new object in selector) const allEdges = useMemo( () => (showA2AEdges ? [...edges, ...a2aEdges] : edges), - [edges, a2aEdges, showA2AEdges] + [edges, a2aEdges, showA2AEdges], ); const onNodesChange = useCanvasStore((s) => s.onNodesChange); - const savePosition = useCanvasStore((s) => s.savePosition); const selectNode = useCanvasStore((s) => s.selectNode); const selectedNodeId = useCanvasStore((s) => s.selectedNodeId); - const setDragOverNode = useCanvasStore((s) => s.setDragOverNode); - const nestNode = useCanvasStore((s) => s.nestNode); - const isDescendant = useCanvasStore((s) => s.isDescendant); - const dragStartParentRef = useRef(null); - const dragModifiersRef = useRef<{ alt: boolean; meta: boolean }>({ alt: false, meta: false }); - const { getInternalNode } = useReactFlow(); - // Track Alt / Cmd-Meta during the whole drag so onNodeDrag and - // onNodeDragStop see the same modifier state. (React Flow's drag event - // only fires mousemove events — we attach a window-level keyboard - // listener while a drag is in progress.) - const onNodeDragStart: OnNodeDrag> = useCallback( - (event, node) => { - dragStartParentRef.current = (node.data as WorkspaceNodeData).parentId; - dragModifiersRef.current = { - alt: event.altKey, - meta: event.metaKey || event.ctrlKey, - }; - }, - [], - ); + // Drag / nest lifecycle — handlers, pending-nest state, confirm/cancel. + const { + onNodeDragStart, + onNodeDrag, + onNodeDragStop, + pendingNest, + confirmNest, + cancelNest, + } = useDragHandlers(); - // Absolute-bounds hit test. Returns the **best** drop target among the - // candidates whose measured bbox contains `point`. Tiebreakers, in - // order — the user drops onto what's visually on top, so zIndex wins - // first (a user can Cmd+] bump a shallow card above a deep one): - // - // 1. Highest zIndex first — matches what the user sees in front. - // 2. DEEPEST tree depth second — when zIndex ties, a more-nested - // card is a more specific target than its ancestor. - // 3. Smallest area last — if depth also ties, the tighter bbox wins. - // - // Self + descendants are excluded (can't nest something under itself). - // Depths are pre-computed once per call so this stays O(n) overall — - // previously the per-candidate depth walk made it O(n²). - const findDropTarget = useCallback( - (draggedId: string, point: { x: number; y: number }): string | null => { - const all = useCanvasStore.getState().nodes; - const depthById = new Map(); - for (const n of all) { - depthById.set(n.id, n.data.parentId ? (depthById.get(n.data.parentId) ?? 0) + 1 : 0); - } - let best: - | { id: string; depth: number; zIndex: number; area: number } - | null = null; - for (const n of all) { - if (n.id === draggedId || isDescendant(draggedId, n.id)) continue; - const internal = getInternalNode(n.id); - if (!internal) continue; - const abs = internal.internals.positionAbsolute; - const w = internal.measured?.width ?? n.width ?? 220; - const h = internal.measured?.height ?? n.height ?? 120; - if (point.x < abs.x || point.x > abs.x + w) continue; - if (point.y < abs.y || point.y > abs.y + h) continue; - const depth = depthById.get(n.id) ?? 0; - const z = n.zIndex ?? 0; - const area = w * h; - if ( - !best || - z > best.zIndex || - (z === best.zIndex && depth > best.depth) || - (z === best.zIndex && depth === best.depth && area < best.area) - ) { - best = { id: n.id, depth, zIndex: z, area }; - } - } - return best?.id ?? null; - }, - [getInternalNode, isDescendant], - ); + // Window-level keyboard shortcuts (Esc, Enter, Shift+Enter, Cmd+]/[, Z). + useKeyboardShortcuts(); - const onNodeDrag: OnNodeDrag> = useCallback( - (event, node) => { - dragModifiersRef.current = { - alt: event.altKey, - meta: event.metaKey || event.ctrlKey, - }; - const internal = getInternalNode(node.id); - if (!internal) { - setDragOverNode(null); - return; - } - const abs = internal.internals.positionAbsolute; - const w = internal.measured?.width ?? 220; - const h = internal.measured?.height ?? 120; - const center = { x: abs.x + w / 2, y: abs.y + h / 2 }; - setDragOverNode(findDropTarget(node.id, center)); - }, - [findDropTarget, getInternalNode, setDragOverNode], - ); + // Pan-to-node / zoom-to-team CustomEvent listeners + viewport save. + const { onMoveEnd } = useCanvasViewport(); - // 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). + // outside-click handler. const pendingDelete = useCanvasStore((s) => s.pendingDelete); const setPendingDelete = useCanvasStore((s) => s.setPendingDelete); const removeNode = useCanvasStore((s) => s.removeNode); @@ -315,87 +104,6 @@ function CanvasInner() { } }, [pendingDelete, setPendingDelete, removeNode]); - 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 altHeld = event.altKey || dragModifiersRef.current.alt; - const forceDetach = - event.metaKey || event.ctrlKey || dragModifiersRef.current.meta; - - // Soft clamp: without a modifier, a child dropped just past its - // parent's edge is snapped back inside (Alt-drag escapes this to - // allow re-parenting). The explicit nest gesture (drop inside - // another parent) always wins over the clamp. - const droppingIntoAnotherParent = - !!dragOverNodeId && dragOverNodeId !== currentParentId; - if ( - currentParentId && - !altHeld && - !forceDetach && - !droppingIntoAnotherParent && - shouldDetach(node.id, currentParentId, getInternalNode) - ) { - clampChildIntoParent(node.id, currentParentId, getInternalNode); - } - - // The drag-stop offers several possible intents. Hysteresis - // (Miro/tldraw pattern) keeps a child nested unless it's clearly - // outside the parent — a twitchy release 1px past the edge no - // longer un-nests. Cmd / Ctrl (forceDetach) or Alt (escape) - // bypass the clamp. - if (droppingIntoAnotherParent) { - const targetNode = allNodes.find((n) => n.id === dragOverNodeId); - const targetName = targetNode?.data.name || "Unknown"; - setPendingNest({ nodeId: node.id, targetId: dragOverNodeId, nodeName, targetName }); - } else if ( - currentParentId && - (forceDetach || (altHeld && shouldDetach(node.id, currentParentId, getInternalNode))) - ) { - const parentNode = allNodes.find((n) => n.id === currentParentId); - const parentName = parentNode?.data.name || "Unknown"; - setPendingNest({ nodeId: node.id, targetId: null, nodeName, targetName: parentName }); - } - - // savePosition expects ABSOLUTE coords. When node is a child, its - // `position` is relative to its parent, so translate through the - // measured absolute position React Flow tracks. - const internal = getInternalNode(node.id); - const abs = internal?.internals.positionAbsolute ?? node.position; - savePosition(node.id, abs.x, abs.y); - // Commit-on-release grow: run the parent auto-grow pass once now - // that the drag has settled. Cheap and deterministic vs running - // grow on every drag tick (avoids tldraw's edge-chase artifact). - useCanvasStore.getState().growParentsToFitChildren(); - }, - [getInternalNode, savePosition, setDragOverNode], - ); - - const batchNest = useCanvasStore((s) => s.batchNest); - const confirmNest = useCallback(() => { - if (!pendingNest) return; - const state = useCanvasStore.getState(); - // If the primary dragged node is part of a batch selection, apply - // the same nest target to every selected node — preserves the - // selection's inter-node spacing (Lucidchart pattern). - if ( - state.selectedNodeIds.size > 1 && - state.selectedNodeIds.has(pendingNest.nodeId) - ) { - batchNest(Array.from(state.selectedNodeIds), pendingNest.targetId); - } else { - nestNode(pendingNest.nodeId, pendingNest.targetId); - } - setPendingNest(null); - }, [pendingNest, nestNode, batchNest]); - - const cancelNest = useCallback(() => { - setPendingNest(null); - }, []); - const onPaneClick = useCallback(() => { selectNode(null); const state = useCanvasStore.getState(); @@ -403,153 +111,14 @@ function CanvasInner() { state.clearSelection(); }, [selectNode]); - // Team zoom-in: double-click a team node to zoom to its children - const { fitBounds, fitView } = useReactFlow(); - - // Pan to newly deployed workspace. - // Uses fitView({ nodes }) so the viewport adapts to any current zoom level - // instead of forcing zoom=1 (which was jarring when the user was zoomed out). - const panTimerRef = useRef>(undefined); - useEffect(() => { - const handler = (e: Event) => { - const { nodeId } = (e as CustomEvent<{ nodeId: string }>).detail; - // Small delay so ReactFlow has time to measure the newly rendered node - clearTimeout(panTimerRef.current); - panTimerRef.current = setTimeout(() => { - fitView({ nodes: [{ id: nodeId }], duration: 400, padding: 0.3 }); - }, 100); - }; - window.addEventListener("molecule:pan-to-node", handler); - return () => { - window.removeEventListener("molecule:pan-to-node", handler); - clearTimeout(panTimerRef.current); - }; - }, [fitView]); - useEffect(() => { - const handler = (e: Event) => { - const { nodeId } = (e as CustomEvent).detail; - const state = useCanvasStore.getState(); - const children = state.nodes.filter((n) => n.data.parentId === nodeId); - if (children.length === 0) return; - - const parent = state.nodes.find((n) => n.id === nodeId); - const allNodes = parent ? [parent, ...children] : children; - - let minX = Infinity, minY = Infinity, maxX = -Infinity, maxY = -Infinity; - for (const n of allNodes) { - minX = Math.min(minX, n.position.x); - minY = Math.min(minY, n.position.y); - maxX = Math.max(maxX, n.position.x + CHILD_DEFAULT_WIDTH); - maxY = Math.max(maxY, n.position.y + CHILD_DEFAULT_HEIGHT); - } - - fitBounds( - { x: minX - 50, y: minY - 50, width: maxX - minX + 100, height: maxY - minY + 100 }, - { padding: 0.2, duration: 500 } - ); - }; - window.addEventListener("molecule:zoom-to-team", handler); - return () => window.removeEventListener("molecule:zoom-to-team", handler); - }, [fitBounds]); - - // Keyboard shortcuts - useEffect(() => { - const handler = (e: KeyboardEvent) => { - const tag = (e.target as HTMLElement).tagName; - const inInput = - tag === "INPUT" || - tag === "TEXTAREA" || - tag === "SELECT" || - (e.target as HTMLElement).isContentEditable; - - if (e.key === "Escape") { - const state = useCanvasStore.getState(); - if (state.contextMenu) { - state.closeContextMenu(); - } else if (state.selectedNodeIds.size > 0) { - state.clearSelection(); - } else if (state.selectedNodeId) { - state.selectNode(null); - } - } - - // Figma-style hierarchy navigation. Enter descends to the first - // child of the selected node; Shift+Enter ascends to its parent; - // Cmd+]/[ re-orders siblings (z-index up/down). Skipped when the - // user is typing into an input — Enter should commit the form. - if (!inInput && (e.key === "Enter" || e.key === "NumpadEnter")) { - e.preventDefault(); - const state = useCanvasStore.getState(); - const id = state.selectedNodeId; - if (!id) return; - if (e.shiftKey) { - const sel = state.nodes.find((n) => n.id === id); - const parentId = sel?.data.parentId ?? null; - if (parentId) state.selectNode(parentId); - } else { - const firstChild = state.nodes.find((n) => n.data.parentId === id); - if (firstChild) state.selectNode(firstChild.id); - } - } - - if ( - !inInput && - (e.metaKey || e.ctrlKey) && - (e.key === "]" || e.key === "[") - ) { - e.preventDefault(); - const state = useCanvasStore.getState(); - const id = state.selectedNodeId; - if (!id) return; - state.bumpZOrder(id, e.key === "]" ? 1 : -1); - } - - // Z — keyboard equivalent for double-click zoom-to-team (WCAG 2.1.1) - if (!inInput && (e.key === "z" || e.key === "Z")) { - const state = useCanvasStore.getState(); - const selectedId = state.selectedNodeId; - if (!selectedId) return; - const hasChildren = state.nodes.some((n) => n.data.parentId === selectedId); - if (hasChildren) { - window.dispatchEvent( - new CustomEvent("molecule:zoom-to-team", { detail: { nodeId: selectedId } }) - ); - } - } - }; - window.addEventListener("keydown", handler); - return () => window.removeEventListener("keydown", handler); - }, []); - - const saveViewport = useCanvasStore((s) => s.saveViewport); const viewport = useCanvasStore((s) => s.viewport); - const saveTimerRef = useRef>(undefined); - - // Cleanup debounced save timer on unmount - useEffect(() => { - return () => clearTimeout(saveTimerRef.current); - }, []); - - const onMoveEnd = useCallback( - (_event: unknown, vp: { x: number; y: number; zoom: number }) => { - // Debounce viewport saves to avoid spamming the API - clearTimeout(saveTimerRef.current); - saveTimerRef.current = setTimeout(() => { - saveViewport(vp.x, vp.y, vp.zoom); - }, 1000); - }, - [saveViewport] - ); - const defaultViewport = useMemo( () => ({ x: viewport.x, y: viewport.y, zoom: viewport.zoom }), // Only use the initial viewport — don't re-render on every save // eslint-disable-next-line react-hooks/exhaustive-deps - [] + [], ); - // Determine which workspace ID to use for global settings. - // Fall back to "global" when no specific node is selected. const settingsWorkspaceId = selectedNodeId ?? "global"; return ( @@ -561,121 +130,118 @@ function CanvasInner() { Skip to canvas
- - + + + { + // Parents show as a filled region — hierarchy visible at + // a glance in the minimap without needing to zoom. + const hasChildren = nodes.some((n) => n.parentId === node.id); + if (hasChildren) return "#3b82f6"; + const status = (node.data as Record)?.status; + switch (status) { + case "online": + return "#34d399"; + case "offline": + return "#52525b"; + case "degraded": + return "#fbbf24"; + case "failed": + return "#f87171"; + case "provisioning": + return "#38bdf8"; + default: + return "#3f3f46"; + } + }} + nodeStrokeColor={(node) => { + const hasChildren = nodes.some((n) => n.parentId === node.id); + return hasChildren ? "#60a5fa" : "transparent"; + }} + nodeStrokeWidth={2} + nodeBorderRadius={4} + /> + + + + {/* Screen-reader live region: announces workspace count on canvas load or change */} +
+ {nodes.filter((n) => !n.data.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.length === 0 && } + + + + + + + + + + + + {!selectedNodeId && } + + + - setPendingDelete(null)} /> - { - // Parents show as a filled region — hierarchy visible at - // a glance in the minimap without needing to zoom. - const hasChildren = nodes.some((n) => n.parentId === node.id); - if (hasChildren) return "#3b82f6"; - const status = (node.data as Record)?.status; - switch (status) { - case "online": - return "#34d399"; - case "offline": - return "#52525b"; - case "degraded": - return "#fbbf24"; - case "failed": - return "#f87171"; - case "provisioning": - return "#38bdf8"; - default: - return "#3f3f46"; - } - }} - nodeStrokeColor={(node) => { - const hasChildren = nodes.some((n) => n.parentId === node.id); - return hasChildren ? "#60a5fa" : "transparent"; - }} - nodeStrokeWidth={2} - nodeBorderRadius={4} - /> - - - {/* Screen-reader live region: announces workspace count when canvas loads or changes */} -
- {nodes.filter((n) => !n.data.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.length === 0 && } - - - - - - - - - - - - {!selectedNodeId && } - - - {/* Confirmation dialog for structure changes */} - - - {/* Confirmation dialog for workspace delete — driven by store */} - setPendingDelete(null)} - /> - - {/* Settings Panel — global secrets management drawer */} - - + +
); diff --git a/canvas/src/components/__tests__/ZoomShortcut.test.tsx b/canvas/src/components/__tests__/ZoomShortcut.test.tsx index 6b227c0f..85858de9 100644 --- a/canvas/src/components/__tests__/ZoomShortcut.test.tsx +++ b/canvas/src/components/__tests__/ZoomShortcut.test.tsx @@ -71,11 +71,14 @@ describe("Toolbar help panel — zoom shortcut entry", () => { expect(src).toContain("Zoom canvas to fit a team node"); }); - it("Canvas.tsx Z key handler guards against input elements", async () => { + it("Keyboard shortcuts hook guards against input elements", async () => { const { readFileSync } = await import("fs"); const { join } = await import("path"); + // After the canvas split (commit c5abed98 → f3423a51 series), the + // Z-key / hierarchy / zoom shortcuts moved out of Canvas.tsx into + // the useKeyboardShortcuts hook under src/components/canvas/. const src = readFileSync( - join(__dirname, "../../components/Canvas.tsx"), + join(__dirname, "../../components/canvas/useKeyboardShortcuts.ts"), "utf8" ); expect(src).toContain('e.key === "z" || e.key === "Z"'); diff --git a/canvas/src/components/canvas/DropTargetBadge.tsx b/canvas/src/components/canvas/DropTargetBadge.tsx new file mode 100644 index 00000000..13c0f7d4 --- /dev/null +++ b/canvas/src/components/canvas/DropTargetBadge.tsx @@ -0,0 +1,83 @@ +"use client"; + +import { useReactFlow } from "@xyflow/react"; +import { useCanvasStore, type WorkspaceNodeData } from "@/store/canvas"; +import { + defaultChildSlot, + CHILD_DEFAULT_HEIGHT, + CHILD_DEFAULT_WIDTH, +} from "@/store/canvas-topology"; + +/** + * Floating affordance that tracks the current drag target. Two visuals + * are layered on top of React Flow, both in screen space: + * + * 1. Ghost preview — dashed outline at the next default grid slot + * inside the target parent. Whimsical-style: users see exactly + * where the card will land before releasing. + * 2. Text badge — "Drop into: " floating above the target. The + * coloured outline alone is ambiguous on dense canvases; spelling + * the name out is the Mural pattern. + * + * Colour alone isn't an accessible cue, so the pair (outline + label) + * is deliberate. + */ +export function DropTargetBadge() { + const dragOverNodeId = useCanvasStore((s) => s.dragOverNodeId); + const targetName = useCanvasStore((s) => { + if (!s.dragOverNodeId) return null; + const n = s.nodes.find((nn) => nn.id === s.dragOverNodeId); + return (n?.data as WorkspaceNodeData | undefined)?.name ?? null; + }); + const childCount = useCanvasStore((s) => + !s.dragOverNodeId + ? 0 + : s.nodes.filter((n) => n.parentId === s.dragOverNodeId).length, + ); + const { getInternalNode, flowToScreenPosition } = useReactFlow(); + if (!dragOverNodeId || !targetName) return null; + const internal = getInternalNode(dragOverNodeId); + if (!internal) return null; + const abs = internal.internals.positionAbsolute; + const w = internal.measured?.width ?? 220; + const h = internal.measured?.height ?? 120; + const badge = flowToScreenPosition({ x: abs.x + w / 2, y: abs.y }); + + const slot = defaultChildSlot(childCount); + const slotTL = flowToScreenPosition({ x: abs.x + slot.x, y: abs.y + slot.y }); + const slotBR = flowToScreenPosition({ + x: abs.x + slot.x + CHILD_DEFAULT_WIDTH, + y: abs.y + slot.y + CHILD_DEFAULT_HEIGHT, + }); + // Clip: don't draw the ghost if its rect falls entirely outside the + // parent (can happen when a parent is smaller than one default slot). + const parentTL = flowToScreenPosition({ x: abs.x, y: abs.y }); + const parentBR = flowToScreenPosition({ x: abs.x + w, y: abs.y + h }); + const ghostVisible = + slotBR.x > parentTL.x && + slotTL.x < parentBR.x && + slotBR.y > parentTL.y && + slotTL.y < parentBR.y; + + return ( + <> + {ghostVisible && ( +
+ )} +
+ Drop into: {targetName} +
+ + ); +} diff --git a/canvas/src/components/canvas/dragUtils.ts b/canvas/src/components/canvas/dragUtils.ts new file mode 100644 index 00000000..a0e5959a --- /dev/null +++ b/canvas/src/components/canvas/dragUtils.ts @@ -0,0 +1,74 @@ +import type { useReactFlow } from "@xyflow/react"; +import { useCanvasStore } from "@/store/canvas"; + +/** + * Hysteresis threshold for drag-out detach. A child only un-nests from + * its parent once at least this fraction of its bounding box lies + * outside the parent's bbox — a twitchy release 1px past the edge stays + * nested. Miro / tldraw use roughly 20-30%; 20% feels responsive. + */ +export const DETACH_FRACTION = 0.2; + +type InternalNode = ReturnType["getInternalNode"]>; +type GetInternalNode = (id: string) => InternalNode; + +/** + * True when the child has moved far enough outside its parent's bbox + * that the gesture is unambiguously an un-nest. Returns true when we + * can't measure either node (conservative fall-back matches the + * original behaviour). + */ +export function shouldDetach( + childId: string, + parentId: string, + getInternalNode: GetInternalNode, +): boolean { + const c = getInternalNode(childId); + const p = getInternalNode(parentId); + if (!c || !p) return true; + const cw = c.measured?.width ?? c.width ?? 220; + const ch = c.measured?.height ?? c.height ?? 120; + const pw = p.measured?.width ?? p.width ?? 220; + const ph = p.measured?.height ?? p.height ?? 120; + const cx = c.internals.positionAbsolute; + const px = p.internals.positionAbsolute; + const overlapW = + Math.max(0, Math.min(cx.x + cw, px.x + pw) - Math.max(cx.x, px.x)); + const overlapH = + Math.max(0, Math.min(cx.y + ch, px.y + ph) - Math.max(cx.y, px.y)); + const outsideFractionX = 1 - overlapW / cw; + const outsideFractionY = 1 - overlapH / ch; + return outsideFractionX > DETACH_FRACTION || outsideFractionY > DETACH_FRACTION; +} + +/** + * Snap a child back so its bbox is fully inside the parent's bounds. + * Called on drag-stop when the user drifted slightly past the edge + * without holding Alt or Cmd — the canvas treats the gesture as a + * plain move rather than an un-nest. + */ +export function clampChildIntoParent( + childId: string, + parentId: string, + getInternalNode: GetInternalNode, +) { + const c = getInternalNode(childId); + const p = getInternalNode(parentId); + if (!c || !p) return; + const cw = c.measured?.width ?? c.width ?? 220; + const ch = c.measured?.height ?? c.height ?? 120; + const pw = p.measured?.width ?? p.width ?? 220; + const ph = p.measured?.height ?? p.height ?? 120; + const { nodes } = useCanvasStore.getState(); + const cur = nodes.find((n) => n.id === childId); + if (!cur) return; + const rel = cur.position; + const clampedX = Math.max(0, Math.min(rel.x, pw - cw)); + const clampedY = Math.max(0, Math.min(rel.y, ph - ch)); + if (clampedX === rel.x && clampedY === rel.y) return; + useCanvasStore.setState({ + nodes: nodes.map((n) => + n.id === childId ? { ...n, position: { x: clampedX, y: clampedY } } : n, + ), + }); +} diff --git a/canvas/src/components/canvas/useCanvasViewport.ts b/canvas/src/components/canvas/useCanvasViewport.ts new file mode 100644 index 00000000..c7ce9169 --- /dev/null +++ b/canvas/src/components/canvas/useCanvasViewport.ts @@ -0,0 +1,96 @@ +"use client"; + +import { useCallback, useEffect, useRef } from "react"; +import { useReactFlow } from "@xyflow/react"; +import { useCanvasStore } from "@/store/canvas"; +import { + CHILD_DEFAULT_HEIGHT, + CHILD_DEFAULT_WIDTH, +} from "@/store/canvas-topology"; + +/** + * Wires the two canvas-wide CustomEvent listeners and the viewport + * save/restore bookkeeping so Canvas.tsx doesn't have to. + * + * - `molecule:pan-to-node` — scroll viewport onto a specific node + * without forcing a specific zoom level (fitView adapts to current). + * - `molecule:zoom-to-team` — fit the viewport to a parent + its + * direct children, with a small padding. + * + * Also returns an `onMoveEnd` handler that debounces viewport saves so + * the backend isn't spammed with pans. + */ +export function useCanvasViewport() { + const { fitBounds, fitView } = useReactFlow(); + const saveViewport = useCanvasStore((s) => s.saveViewport); + const saveTimerRef = useRef>(undefined); + const panTimerRef = useRef>(undefined); + + useEffect(() => { + return () => { + clearTimeout(saveTimerRef.current); + clearTimeout(panTimerRef.current); + }; + }, []); + + // Pan to a newly deployed / targeted workspace. 100ms delay so React + // Flow has time to measure a just-rendered node. + useEffect(() => { + const handler = (e: Event) => { + const { nodeId } = (e as CustomEvent<{ nodeId: string }>).detail; + clearTimeout(panTimerRef.current); + panTimerRef.current = setTimeout(() => { + fitView({ nodes: [{ id: nodeId }], duration: 400, padding: 0.3 }); + }, 100); + }; + window.addEventListener("molecule:pan-to-node", handler); + return () => window.removeEventListener("molecule:pan-to-node", handler); + }, [fitView]); + + // Zoom to a team: fit the parent + its direct children in view. + useEffect(() => { + const handler = (e: Event) => { + const { nodeId } = (e as CustomEvent).detail; + const state = useCanvasStore.getState(); + const children = state.nodes.filter((n) => n.data.parentId === nodeId); + if (children.length === 0) return; + const parent = state.nodes.find((n) => n.id === nodeId); + const allNodes = parent ? [parent, ...children] : children; + + let minX = Infinity, + minY = Infinity, + maxX = -Infinity, + maxY = -Infinity; + for (const n of allNodes) { + minX = Math.min(minX, n.position.x); + minY = Math.min(minY, n.position.y); + maxX = Math.max(maxX, n.position.x + CHILD_DEFAULT_WIDTH); + maxY = Math.max(maxY, n.position.y + CHILD_DEFAULT_HEIGHT); + } + + fitBounds( + { + x: minX - 50, + y: minY - 50, + width: maxX - minX + 100, + height: maxY - minY + 100, + }, + { padding: 0.2, duration: 500 }, + ); + }; + window.addEventListener("molecule:zoom-to-team", handler); + return () => window.removeEventListener("molecule:zoom-to-team", handler); + }, [fitBounds]); + + const onMoveEnd = useCallback( + (_event: unknown, vp: { x: number; y: number; zoom: number }) => { + clearTimeout(saveTimerRef.current); + saveTimerRef.current = setTimeout(() => { + saveViewport(vp.x, vp.y, vp.zoom); + }, 1000); + }, + [saveViewport], + ); + + return { onMoveEnd }; +} diff --git a/canvas/src/components/canvas/useDragHandlers.ts b/canvas/src/components/canvas/useDragHandlers.ts new file mode 100644 index 00000000..ee2b711d --- /dev/null +++ b/canvas/src/components/canvas/useDragHandlers.ts @@ -0,0 +1,213 @@ +"use client"; + +import { useCallback, useRef, useState } from "react"; +import { + useReactFlow, + type Node, + type OnNodeDrag, +} from "@xyflow/react"; +import { useCanvasStore, type WorkspaceNodeData } from "@/store/canvas"; +import { clampChildIntoParent, shouldDetach } from "./dragUtils"; + +export interface PendingNestState { + nodeId: string; + targetId: string | null; + nodeName: string; + targetName: string; +} + +interface DragHandlers { + onNodeDragStart: OnNodeDrag>; + onNodeDrag: OnNodeDrag>; + onNodeDragStop: OnNodeDrag>; + pendingNest: PendingNestState | null; + confirmNest: () => void; + cancelNest: () => void; +} + +/** + * Encapsulates every drag gesture on the canvas: + * + * - On drag start, snapshot the modifier keys (Alt / Cmd-Meta) and + * remember which parent the node lived in so we can detect a + * re-parent on release. + * - On drag (mousemove), compute the best drop target via an + * absolute-bounds hit test and publish it via setDragOverNode so + * WorkspaceNode can render the highlight + DropTargetBadge can + * render its label + ghost preview. + * - On drag stop, decide one of: nest into new parent, un-nest, soft + * clamp back inside current parent, or plain move — based on + * modifier keys and hysteresis. Persist the absolute position, + * then run one commit-on-release grow pass on the parent chain. + */ +export function useDragHandlers(): DragHandlers { + const setDragOverNode = useCanvasStore((s) => s.setDragOverNode); + const savePosition = useCanvasStore((s) => s.savePosition); + const nestNode = useCanvasStore((s) => s.nestNode); + const batchNest = useCanvasStore((s) => s.batchNest); + const isDescendant = useCanvasStore((s) => s.isDescendant); + const { getInternalNode } = useReactFlow(); + + const dragStartParentRef = useRef(null); + const dragModifiersRef = useRef<{ alt: boolean; meta: boolean }>({ + alt: false, + meta: false, + }); + const [pendingNest, setPendingNest] = useState(null); + + // Absolute-bounds hit test. Tiebreakers in order: highest zIndex + // first (matches what the user sees in front after Cmd+] reorder), + // deepest tree depth second, smallest area third. Depths are + // pre-computed once per call so the whole pass stays O(n). + const findDropTarget = useCallback( + (draggedId: string, point: { x: number; y: number }): string | null => { + const all = useCanvasStore.getState().nodes; + const depthById = new Map(); + for (const n of all) { + depthById.set( + n.id, + n.data.parentId ? (depthById.get(n.data.parentId) ?? 0) + 1 : 0, + ); + } + let best: + | { id: string; depth: number; zIndex: number; area: number } + | null = null; + for (const n of all) { + if (n.id === draggedId || isDescendant(draggedId, n.id)) continue; + const internal = getInternalNode(n.id); + if (!internal) continue; + const abs = internal.internals.positionAbsolute; + const w = internal.measured?.width ?? n.width ?? 220; + const h = internal.measured?.height ?? n.height ?? 120; + if (point.x < abs.x || point.x > abs.x + w) continue; + if (point.y < abs.y || point.y > abs.y + h) continue; + const depth = depthById.get(n.id) ?? 0; + const z = n.zIndex ?? 0; + const area = w * h; + if ( + !best || + z > best.zIndex || + (z === best.zIndex && depth > best.depth) || + (z === best.zIndex && depth === best.depth && area < best.area) + ) { + best = { id: n.id, depth, zIndex: z, area }; + } + } + return best?.id ?? null; + }, + [getInternalNode, isDescendant], + ); + + const onNodeDragStart: OnNodeDrag> = useCallback( + (event, node) => { + dragStartParentRef.current = (node.data as WorkspaceNodeData).parentId; + dragModifiersRef.current = { + alt: event.altKey, + meta: event.metaKey || event.ctrlKey, + }; + }, + [], + ); + + const onNodeDrag: OnNodeDrag> = useCallback( + (event, node) => { + dragModifiersRef.current = { + alt: event.altKey, + meta: event.metaKey || event.ctrlKey, + }; + const internal = getInternalNode(node.id); + if (!internal) { + setDragOverNode(null); + return; + } + const abs = internal.internals.positionAbsolute; + const w = internal.measured?.width ?? 220; + const h = internal.measured?.height ?? 120; + const center = { x: abs.x + w / 2, y: abs.y + h / 2 }; + setDragOverNode(findDropTarget(node.id, center)); + }, + [findDropTarget, getInternalNode, setDragOverNode], + ); + + 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 altHeld = event.altKey || dragModifiersRef.current.alt; + const forceDetach = + event.metaKey || event.ctrlKey || dragModifiersRef.current.meta; + const droppingIntoAnotherParent = + !!dragOverNodeId && dragOverNodeId !== currentParentId; + + // Soft clamp (plain drag, no modifier, not re-parenting): snap + // the child back inside its current parent. Alt or Cmd bypass. + if ( + currentParentId && + !altHeld && + !forceDetach && + !droppingIntoAnotherParent && + shouldDetach(node.id, currentParentId, getInternalNode) + ) { + clampChildIntoParent(node.id, currentParentId, getInternalNode); + } + + if (droppingIntoAnotherParent) { + const targetNode = allNodes.find((n) => n.id === dragOverNodeId); + const targetName = targetNode?.data.name || "Unknown"; + setPendingNest({ + nodeId: node.id, + targetId: dragOverNodeId, + nodeName, + targetName, + }); + } else if ( + currentParentId && + (forceDetach || + (altHeld && shouldDetach(node.id, currentParentId, getInternalNode))) + ) { + const parentNode = allNodes.find((n) => n.id === currentParentId); + const parentName = parentNode?.data.name || "Unknown"; + setPendingNest({ + nodeId: node.id, + targetId: null, + nodeName, + targetName: parentName, + }); + } + + const internal = getInternalNode(node.id); + const abs = internal?.internals.positionAbsolute ?? node.position; + savePosition(node.id, abs.x, abs.y); + useCanvasStore.getState().growParentsToFitChildren(); + }, + [getInternalNode, savePosition, setDragOverNode], + ); + + const confirmNest = useCallback(() => { + if (!pendingNest) return; + const state = useCanvasStore.getState(); + if ( + state.selectedNodeIds.size > 1 && + state.selectedNodeIds.has(pendingNest.nodeId) + ) { + batchNest(Array.from(state.selectedNodeIds), pendingNest.targetId); + } else { + nestNode(pendingNest.nodeId, pendingNest.targetId); + } + setPendingNest(null); + }, [pendingNest, nestNode, batchNest]); + + const cancelNest = useCallback(() => setPendingNest(null), []); + + return { + onNodeDragStart, + onNodeDrag, + onNodeDragStop, + pendingNest, + confirmNest, + cancelNest, + }; +} diff --git a/canvas/src/components/canvas/useKeyboardShortcuts.ts b/canvas/src/components/canvas/useKeyboardShortcuts.ts new file mode 100644 index 00000000..f9f67fd8 --- /dev/null +++ b/canvas/src/components/canvas/useKeyboardShortcuts.ts @@ -0,0 +1,87 @@ +"use client"; + +import { useEffect } from "react"; +import { useCanvasStore } from "@/store/canvas"; + +/** + * Canvas-wide keyboard shortcuts. All bound to the document window so + * they work regardless of focused node, except when the user is typing + * into an input (`inInput` short-circuits handling). + * + * Esc — close context menu, clear selection, deselect + * Enter — descend into selected node's first child + * Shift+Enter — ascend to selected node's parent + * Cmd/Ctrl+] — bump selected node forward in z-order + * Cmd/Ctrl+[ — bump selected node backward in z-order + * Z — zoom-to-team if the selected node has children + */ +export function useKeyboardShortcuts() { + useEffect(() => { + const handler = (e: KeyboardEvent) => { + const tag = (e.target as HTMLElement).tagName; + const inInput = + tag === "INPUT" || + tag === "TEXTAREA" || + tag === "SELECT" || + (e.target as HTMLElement).isContentEditable; + + if (e.key === "Escape") { + const state = useCanvasStore.getState(); + if (state.contextMenu) { + state.closeContextMenu(); + } else if (state.selectedNodeIds.size > 0) { + state.clearSelection(); + } else if (state.selectedNodeId) { + state.selectNode(null); + } + } + + // Figma-style hierarchy navigation. Skipped when the user is + // typing so Enter can still submit forms. + if (!inInput && (e.key === "Enter" || e.key === "NumpadEnter")) { + e.preventDefault(); + const state = useCanvasStore.getState(); + const id = state.selectedNodeId; + if (!id) return; + if (e.shiftKey) { + const sel = state.nodes.find((n) => n.id === id); + const parentId = sel?.data.parentId ?? null; + if (parentId) state.selectNode(parentId); + } else { + const firstChild = state.nodes.find((n) => n.data.parentId === id); + if (firstChild) state.selectNode(firstChild.id); + } + } + + if ( + !inInput && + (e.metaKey || e.ctrlKey) && + (e.key === "]" || e.key === "[") + ) { + e.preventDefault(); + const state = useCanvasStore.getState(); + const id = state.selectedNodeId; + if (!id) return; + state.bumpZOrder(id, e.key === "]" ? 1 : -1); + } + + if (!inInput && (e.key === "z" || e.key === "Z")) { + const state = useCanvasStore.getState(); + const selectedId = state.selectedNodeId; + if (!selectedId) return; + const hasChildren = state.nodes.some( + (n) => n.data.parentId === selectedId, + ); + if (hasChildren) { + window.dispatchEvent( + new CustomEvent("molecule:zoom-to-team", { + detail: { nodeId: selectedId }, + }), + ); + } + } + }; + window.addEventListener("keydown", handler); + return () => window.removeEventListener("keydown", handler); + }, []); +} diff --git a/canvas/src/store/canvas-topology.ts b/canvas/src/store/canvas-topology.ts index b2841e4e..e66ac438 100644 --- a/canvas/src/store/canvas-topology.ts +++ b/canvas/src/store/canvas-topology.ts @@ -252,16 +252,22 @@ export function buildNodesAndEdges( const pa = absPos.get(ws.parent_id!)!; position = { x: abs.x - pa.x, y: abs.y - pa.y }; - // Trust-the-data default: keep the stored position even if it - // falls outside the parent bbox (matches Figma's "don't move my - // shapes" rule). The one exception is a child still at - // origin (0,0) in the absolute frame — that's almost certainly - // an unlaid-out org-import row and would stack every child on - // the same point. Drop those into the grid so the first paint - // isn't a useless pile. Users can always trigger "Arrange - // children" to rescue the rest. - const atOrigin = abs.x === 0 && abs.y === 0; - if (atOrigin) { + // 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 = + position.x < 0 || + position.y < 0 || + position.x + CHILD_DEFAULT_WIDTH > psize.width || + position.y + CHILD_DEFAULT_HEIGHT > psize.height; + if (outside) { 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 23098ee1..10213b3a 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -330,11 +330,163 @@ export const useCanvasStore = create((set, get) => ({ await get().nestNode(nodeIds[0], targetId); return; } - // Run sequentially so each nestNode's absolute-position calc sees - // the previous update committed. Not a hot path — multi-select - // re-parents rarely touch more than a handful of nodes. + // Batch path: do all state math against one snapshot so every + // selected node sees the same "before" world, commit one set(), + // then fire every PATCH in parallel. Previously this called + // nestNode sequentially, which cost 2N round-trips (parent_id + + // x/y) strictly serialized; now it's 1 round-trip per node, all + // 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 }; + let cursor: string | null | undefined = id; + while (cursor) { + const n = byId.get(cursor); + if (!n) break; + sum = { x: sum.x + n.position.x, y: sum.y + n.position.y }; + cursor = n.data.parentId; + } + return sum; + }; + const depthOf = (id: string | null | undefined): number => { + let d = 0; + let cursor: string | null | undefined = id; + while (cursor) { + const n = byId.get(cursor); + if (!n) break; + cursor = n.data.parentId; + d += 1; + } + return d; + }; + + const newParentAbs = absOf(targetId); + const newOwnDepth = targetId ? depthOf(targetId) + 1 : 0; + + interface Plan { + id: string; + newRelative: { x: number; y: number }; + draggedAbs: { x: number; y: number }; + depthDelta: number; + } + const plan: Plan[] = []; + const movedIds = new Set(); + // Filter out nodes that would be invalid targets / no-ops. for (const id of nodeIds) { - await get().nestNode(id, targetId); + const dragged = byId.get(id); + if (!dragged) continue; + const currentParentId = dragged.data.parentId; + if (currentParentId === targetId) continue; + // Can't nest into yourself or your own descendant. + if (targetId && get().isDescendant(id, targetId)) continue; + const oldParentAbs = absOf(currentParentId); + const draggedAbs = { + x: dragged.position.x + oldParentAbs.x, + y: dragged.position.y + oldParentAbs.y, + }; + const newRelative = { + x: draggedAbs.x - newParentAbs.x, + y: draggedAbs.y - newParentAbs.y, + }; + const oldOwnDepth = + dragged.zIndex ?? depthOf(currentParentId) + (currentParentId ? 1 : 0); + plan.push({ + id, + newRelative, + draggedAbs, + depthDelta: newOwnDepth - oldOwnDepth, + }); + movedIds.add(id); + // Every descendant of a moved node also shifts by the same delta + // so grandchildren don't fall behind their re-parented ancestor. + const bfs = [id]; + while (bfs.length) { + const head = bfs.shift()!; + for (const n of before) { + if (n.data.parentId === head && !movedIds.has(n.id)) { + movedIds.add(n.id); + bfs.push(n.id); + } + } + } + } + + if (plan.length === 0) return; + const planById = new Map(plan.map((p) => [p.id, p])); + + // One optimistic set() covers every re-parent + every descendant + // zIndex shift; no further state mutations before the PATCHes come + // back (failed PATCHes roll back individual nodes below). + set({ + nodes: before.map((n) => { + const p = planById.get(n.id); + if (p) { + return { + ...n, + position: p.newRelative, + parentId: targetId ?? undefined, + zIndex: newOwnDepth, + data: { ...n.data, parentId: targetId }, + }; + } + // Descendant of a moved node — shift zIndex only. Find the + // nearest ancestor in `plan` (walking up parents) to know + // which depthDelta applies. + if (movedIds.has(n.id)) { + let cursor: string | null | undefined = n.data.parentId; + while (cursor) { + const anc = planById.get(cursor); + if (anc) { + if (anc.depthDelta === 0) break; + return { ...n, zIndex: (n.zIndex ?? 0) + anc.depthDelta }; + } + cursor = byId.get(cursor)?.data.parentId ?? null; + } + return n; + } + return n; + }), + edges: beforeEdges.filter( + (e) => !movedIds.has(e.source) && !movedIds.has(e.target), + ), + }); + + // Fire every PATCH in parallel. Individual failures roll back just + // that node (others remain committed, matching the single-node + // rollback behaviour in nestNode). + const results = await Promise.allSettled( + plan.map((p) => + api.patch(`/workspaces/${p.id}`, { + parent_id: targetId, + x: p.draggedAbs.x, + y: p.draggedAbs.y, + }), + ), + ); + const rolledBack: string[] = []; + for (let i = 0; i < results.length; i++) { + if (results[i].status === "rejected") rolledBack.push(plan[i].id); + } + if (rolledBack.length > 0) { + const rollbackSet = new Set(rolledBack); + set({ + nodes: get().nodes.map((n) => { + if (!rollbackSet.has(n.id)) return n; + const original = byId.get(n.id); + if (!original) return n; + return { + ...n, + position: original.position, + parentId: original.parentId, + zIndex: original.zIndex, + data: { ...n.data, parentId: original.data.parentId }, + }; + }), + }); } }, @@ -474,11 +626,16 @@ export const useCanvasStore = create((set, get) => ({ }); try { - await api.patch(`/workspaces/${draggedId}`, { parent_id: targetId }); - // Persist absolute position as DB canonical (matches what - // savePosition writes elsewhere); keeps reloads stable regardless - // of which parent the child was under at save time. - await api.patch(`/workspaces/${draggedId}`, { x: draggedAbs.x, y: draggedAbs.y }); + // One round-trip per nest: the /workspaces/:id PATCH handler + // accepts parent_id + x + y in a single body. The absolute x/y + // is what the DB stores as canonical (matches savePosition + // elsewhere), so reload renders the same place regardless of + // which parent the child was under at save time. + await api.patch(`/workspaces/${draggedId}`, { + parent_id: targetId, + x: draggedAbs.x, + y: draggedAbs.y, + }); } catch { set({ nodes: get().nodes.map((n) => From c2b2e13abee7fd484ee6ce1735421d853c258f05 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 19:58:44 -0700 Subject: [PATCH 08/17] 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) From 4fd7f1e84cc15ee4f706fa126704421ca3a01f78 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 20:08:14 -0700 Subject: [PATCH 09/17] fix(canvas): tighten rescue + cap toast + cover paths with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-up review findings from the c2b2e13a review: 1. Rescue heuristic uses pure bbox-non-overlap. The previous `position.x < 0` branch rescued any child whose parent was later dragged past it, even when the layout was clearly recoverable (e.g. relative -40, child still overlaps parent). New rule: rescue iff the child's bbox has zero overlap with the parent's bbox — self-calibrating, scales with user-resized parents, catches screenshot-case and legacy huge-positive data. 2. Toast caps failed-name list at 3 and appends "and N more". Stops a 50-node partial failure from overflowing the toast container. 3. Cycle guard on selection-roots walk in batchNest. Corrupt parentId data can't send the loop infinite now. Cheap defensive guard — one Set per selected node. Tests added (923 total, up from 918): * canvas-topology.test: 4 rescue scenarios — screenshot case (zero-overlap rescue), negative drift kept, huge-positive rescued, user-resized layout kept. * canvas.test: selection-roots filter on a 3-level chain. * workspace_crud test: PATCH {collapsed:true} runs the UPDATE. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../store/__tests__/canvas-topology.test.ts | 55 +++++++++++++++++ canvas/src/store/__tests__/canvas.test.ts | 30 +++++++++ canvas/src/store/canvas-topology.ts | 61 +++++++++++++------ canvas/src/store/canvas.ts | 26 ++++++-- .../handlers/handlers_additional_test.go | 34 +++++++++++ 5 files changed, 181 insertions(+), 25 deletions(-) diff --git a/canvas/src/store/__tests__/canvas-topology.test.ts b/canvas/src/store/__tests__/canvas-topology.test.ts index 5f665619..db046e80 100644 --- a/canvas/src/store/__tests__/canvas-topology.test.ts +++ b/canvas/src/store/__tests__/canvas-topology.test.ts @@ -363,3 +363,58 @@ describe("buildNodesAndEdges – layoutOverrides applied", () => { expect(nodes[0].position).toEqual({ x: 100, y: 200 }); }); }); + +// ---------- Rescue heuristic for out-of-bounds children ---------- +// +// Parent starts at min size for its child count (2-col grid). For a +// parent with one child, parentMinSize(1) is ~300 × 200. Each of the +// tests below fixes the parent origin at (1000, 500) so the test +// cases read cleanly. + +describe("buildNodesAndEdges – child rescue heuristic", () => { + const PARENT_ABS = { x: 1000, y: 500 }; + + function scenario(childAbs: { x: number; y: number }) { + return buildNodesAndEdges([ + makeWS({ id: "p", name: "Parent", x: PARENT_ABS.x, y: PARENT_ABS.y }), + makeWS({ id: "c", name: "Child", parent_id: "p", x: childAbs.x, y: childAbs.y }), + ]).nodes.find((n) => n.id === "c")!; + } + + it("rescues a child whose bbox falls entirely outside the parent (screenshot case)", () => { + // Child abs (580, 795) with parent at (1000, 500) → rel (-420, 295) + // The child's right edge sits at -160, entirely left of parent. + // Expect the grid slot, not the negative stored position. + const child = scenario({ x: 580, y: 795 }); + expect(child.position.x).toBeGreaterThanOrEqual(0); + expect(child.position.y).toBeGreaterThanOrEqual(0); + }); + + it("keeps a child whose stored position drifts slightly negative (user moved parent past child)", () => { + // Child abs (960, 460), parent (1000, 500) → rel (-40, -40). + // Child right/bottom edges still overlap the parent bbox; this is + // a recoverable layout, not corruption. Leave it alone. + const child = scenario({ x: 960, y: 460 }); + expect(child.position).toEqual({ x: -40, y: -40 }); + }); + + it("rescues a child stored with legacy huge-positive coords", () => { + // Abs (50000, 50000) with parent at (1000, 500) → rel (49000, 49500). + // No overlap possible with any reasonable parent size — rescue. + const child = scenario({ x: 50000, y: 50000 }); + expect(child.position.x).toBeLessThan(1000); + expect(child.position.y).toBeLessThan(1000); + }); + + it("keeps a child placed inside a user-resized parent past the initial min size", () => { + // parentMinSize(1) is ~300×200. A child placed at rel (450, 300) + // would be past the initial min bounds but INSIDE a user-grown + // parent of, say, 600×400. We can't know the user's resized size + // from topology alone — but the child's bbox still overlaps the + // initial parent bbox on at least the X axis because its top-left + // is only 450px in (less than the computed parent width for most + // child counts). Verify the intermediate case is preserved. + const child = scenario({ x: PARENT_ABS.x + 100, y: PARENT_ABS.y + 50 }); + expect(child.position).toEqual({ x: 100, y: 50 }); + }); +}); diff --git a/canvas/src/store/__tests__/canvas.test.ts b/canvas/src/store/__tests__/canvas.test.ts index a98340ff..a183d228 100644 --- a/canvas/src/store/__tests__/canvas.test.ts +++ b/canvas/src/store/__tests__/canvas.test.ts @@ -1026,4 +1026,34 @@ describe("batchNest", () => { expect(nodes.find((n) => n.id === "a")!.data.parentId).toBeNull(); expect(nodes.find((n) => n.id === "b")!.data.parentId).toBe("target"); }); + + it("filters out all selected descendants in a three-level chain", async () => { + // Re-hydrate to a chain A → B → C. User selects all three. + // Expected: only A is planned for re-parent; B and C ride with it + // via React Flow's parent binding. + useCanvasStore.getState().hydrate([ + makeWS({ id: "target", name: "Target", x: 2000, y: 0 }), + makeWS({ id: "A", name: "A", x: 0, y: 0 }), + makeWS({ id: "B", name: "B", parent_id: "A", x: 50, y: 50 }), + makeWS({ id: "C", name: "C", parent_id: "B", x: 10, y: 10 }), + ]); + const mock = global.fetch as ReturnType; + mock.mockImplementation(() => + Promise.resolve({ ok: true, json: () => Promise.resolve({}) } as Response), + ); + mock.mockClear(); + await useCanvasStore.getState().batchNest(["A", "B", "C"], "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("A"); + expect(nodes.find((n) => n.id === "C")!.data.parentId).toBe("B"); + // Exactly one nest-PATCH (for A). B and C weren't re-parented. + const nestPatches = 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(nestPatches).toHaveLength(1); + }); }); diff --git a/canvas/src/store/canvas-topology.ts b/canvas/src/store/canvas-topology.ts index 9ae97b4c..4a724578 100644 --- a/canvas/src/store/canvas-topology.ts +++ b/canvas/src/store/canvas-topology.ts @@ -16,6 +16,7 @@ export const PARENT_SIDE_PADDING = 20; export const PARENT_BOTTOM_PADDING = 20; export const CHILD_GUTTER = 20; + /** * A deterministic grid slot for the n-th child inside a parent, counted * left-to-right then top-to-bottom. Used to lay out org-imported teams @@ -252,25 +253,47 @@ 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 — 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 > MAX_PLAUSIBLE_OFFSET || - position.y > MAX_PLAUSIBLE_OFFSET; - if (corrupted) { + // Auto-rescue on load: fires only when the child's bounding box + // is FULLY outside the parent's computed bbox by at least one + // child-width/height. Two real failure modes this covers: + // + // - Legacy data: a child whose stored absolute coords predate + // the nesting assignment, so abs→rel produces a huge offset + // far past any parent edge. + // - Corrupt org-imports with positions in a different + // coordinate space. + // + // Rejected heuristics we deliberately avoid: + // - `position.x < 0` alone — catches legitimate drift when the + // user drags the parent past a child that had small positive + // stored coords (child's relative goes mildly negative, but + // the layout is still recoverable). + // - Raw magnitude like `> 8000` — doesn't scale with parent + // size; a user who resized the parent huge could legitimately + // place a child at 9000px. + // + // Children slightly past the initial min-size (user had resized + // the parent larger on a previous session) are NEVER rescued — + // the bbox-overlap test gives them room. The manual "Arrange + // Children" context command is still the escape hatch for that + // bucket of data. + // Pure bbox-overlap test — self-calibrating without a magic + // margin. Rescue iff the child's bbox has ZERO overlap with the + // parent's bbox (the child would render completely detached). + // drift case (position.x = -40, CHILD_WIDTH = 260): + // child.right = 220, overlaps parent.left = 0 → kept + // screenshot case (position.x = -420, CHILD_WIDTH = 260): + // child.right = -160, doesn't overlap parent.left = 0 → rescued + // user resized larger (parent.width now 800, position.x = 500): + // child.left = 500 < parent.right = 800 → overlaps → kept + // legacy huge positive (position.x = 50000): + // child.left = 50000 >= parent.right → no overlap → rescued + const psize = parentSize.get(ws.parent_id!)!; + const overlapsX = + position.x + CHILD_DEFAULT_WIDTH > 0 && position.x < psize.width; + const overlapsY = + position.y + CHILD_DEFAULT_HEIGHT > 0 && position.y < psize.height; + if (!overlapsX || !overlapsY) { 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 6652c9ae..7559ec8b 100644 --- a/canvas/src/store/canvas.ts +++ b/canvas/src/store/canvas.ts @@ -340,7 +340,13 @@ export const useCanvasStore = create((set, get) => ({ for (const id of nodeIds) { let cursor = byId.get(id)?.data.parentId ?? null; let hasSelectedAncestor = false; - while (cursor) { + // Seen-set guards against a corrupt parentId cycle. Shouldn't + // happen with a healthy backend — nestNode itself blocks cycles + // via isDescendant — but this walk is user-triggered and the + // cost of the guard is one set allocation per selected node. + const seen = new Set(); + while (cursor && !seen.has(cursor)) { + seen.add(cursor); if (selectedSet.has(cursor)) { hasSelectedAncestor = true; break; @@ -511,13 +517,21 @@ 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 + // 5 cards, 3 moved and 2 snapped back?"). Cap the name list so a + // 50-node partial failure doesn't overflow the toast container. + const NAMES_IN_TOAST = 3; + const names = rolledBack .map((id) => byId.get(id)?.data.name) - .filter(Boolean) - .join(", "); + .filter((n): n is string => Boolean(n)); + const shown = names.slice(0, NAMES_IN_TOAST).join(", "); + const overflow = names.length - NAMES_IN_TOAST; + const listFragment = shown + ? overflow > 0 + ? `: ${shown} and ${overflow} more` + : `: ${shown}` + : ""; showToast( - `Could not re-parent ${rolledBack.length} of ${plan.length} workspace${plan.length === 1 ? "" : "s"}${failedNames ? `: ${failedNames}` : ""}`, + `Could not re-parent ${rolledBack.length} of ${plan.length} workspace${plan.length === 1 ? "" : "s"}${listFragment}`, "error", ); } diff --git a/workspace-server/internal/handlers/handlers_additional_test.go b/workspace-server/internal/handlers/handlers_additional_test.go index 0e2ecd82..2d1dc930 100644 --- a/workspace-server/internal/handlers/handlers_additional_test.go +++ b/workspace-server/internal/handlers/handlers_additional_test.go @@ -183,6 +183,40 @@ func TestWorkspaceUpdate_NameOnly(t *testing.T) { } } +// ---------- workspace.go: Update with collapsed flag ---------- + +func TestWorkspaceUpdate_Collapsed(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + broadcaster := newTestBroadcaster() + handler := NewWorkspaceHandler(broadcaster, nil, "http://localhost:8080", t.TempDir()) + + // Canvas "collapse team" flip — the handler must run the UPDATE + // to persist the flag, otherwise the UI state resets on reload. + mock.ExpectQuery("SELECT EXISTS.*workspaces WHERE id"). + WithArgs("dddddddd-0005-0000-0000-000000000000"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) + mock.ExpectExec("UPDATE workspaces SET collapsed"). + WithArgs("dddddddd-0005-0000-0000-000000000000", true). + WillReturnResult(sqlmock.NewResult(0, 1)) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "dddddddd-0005-0000-0000-000000000000"}} + body := `{"collapsed":true}` + c.Request = httptest.NewRequest("PATCH", "/workspaces/ws-collapse", bytes.NewBufferString(body)) + c.Request.Header.Set("Content-Type", "application/json") + + handler.Update(c) + + if w.Code != http.StatusOK { + t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet expectations: %v", err) + } +} + // ---------- workspace.go: List with actual data ---------- func TestWorkspaceList_WithData(t *testing.T) { From 3f11df031c1d2cfdfdf80d89b915406c8a088ee6 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 20:18:30 -0700 Subject: [PATCH 10/17] fix: six UX bugs (peers auth, scroll, chat tabs, config persist, + visibility) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six bugs reported from a live session — all shippable in one commit: 1. Peers tab 401 on local Docker. The /registry/:id/peers endpoint demands a workspace-scoped bearer token (validateDiscoveryCaller) which the canvas session doesn't hold. Added the same Tier-1b dev-mode fail-open hatch that AdminAuth and WorkspaceAuth already use — gated by MOLECULE_ENV=development + empty ADMIN_TOKEN, so SaaS production stays strict. Exported IsDevModeFailOpen from the middleware package for the handler layer to reuse. 2. Org Templates list unscrollable. OrgTemplatesSection was rendered in the TemplatePalette footer — a div without overflow — so when it expanded to 15+ entries the list extended past the viewport with no scroll. Moved it to the top of the flex-1 overflow-y-auto container. Tall lists now scroll naturally. 3. Chat tab: "My Chat" and "Agent Comms" rendered stacked instead of switching. HTML `hidden` attribute was being overridden by Tailwind's `flex` class (display: flex beats the attribute), so both tabpanels rendered concurrently. Swapped to a conditional Tailwind `hidden`/`flex` class so the inactive panel is display:none with proper CSS specificity. 4. Hermes Config form never persists. handleSave wrote config.yaml but name / tier / runtime / model all live on the workspace row (or the dedicated /workspaces/:id/model endpoint) — the form edited in-memory, the request returned 200, the next reload wiped everything back. Hermes + external runtimes manage their own config inside the container anyway, so writing config.yaml is a no-op for them; skip it. Always diff and PATCH the DB-backed fields that actually changed. 5. Channels "+ Connect" dropdown empty on first open. ChannelsTab's load() used Promise.all with a silent catch — if EITHER the channels or adapters fetch failed, both setters were skipped with no error visible. Switched to Promise.allSettled so each endpoint settles independently, and the adapters failure now surfaces via the top-level error state. 6. Plugin registry always "No plugins in registry". Same silent catch pattern in SkillsTab.tsx — load errors for /plugins, /plugins/sources, and /workspaces/:id/plugins swallowed without logging. Replaced the empty catches with console.warn so future failures are at least visible in devtools. Tests: 923 passing (unchanged). Go handler tests pass. Server rebuilt and running with the peers-auth + collapsed-persistence fixes (pid 15875). Co-Authored-By: Claude Opus 4.7 (1M context) --- canvas/src/components/TemplatePalette.tsx | 6 ++- canvas/src/components/tabs/ChannelsTab.tsx | 31 +++++++---- canvas/src/components/tabs/ChatTab.tsx | 26 +++++++-- canvas/src/components/tabs/ConfigTab.tsx | 54 ++++++++++++++++--- canvas/src/components/tabs/SkillsTab.tsx | 20 +++++-- .../internal/handlers/discovery.go | 9 ++++ .../internal/middleware/devmode.go | 9 ++++ 7 files changed, 126 insertions(+), 29 deletions(-) diff --git a/canvas/src/components/TemplatePalette.tsx b/canvas/src/components/TemplatePalette.tsx index 710d01b1..3f67bcba 100644 --- a/canvas/src/components/TemplatePalette.tsx +++ b/canvas/src/components/TemplatePalette.tsx @@ -400,6 +400,11 @@ export function TemplatePalette() {
+ {/* Org templates live INSIDE the scroll container so an + * expanded list (15+ entries) is reachable instead of + * overflowing the fixed footer below. */} + + {loading && (
@@ -467,7 +472,6 @@ export function TemplatePalette() {
-
{/* Content — both panels are always in the DOM so aria-controls targets exist. - The inactive panel is hidden via the HTML `hidden` attribute (removed from - display and accessibility tree, but present in the DOM for WCAG 4.1.2). */} -