From cc194f0b7e97f52cef68d18cb880e20843c76266 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 23 Apr 2026 18:18:44 -0700 Subject: [PATCH] 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