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); + }); } },