fix(canvas): address review findings on playability pass
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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang
parent
f3423a513d
commit
c5abed988e
@ -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<string, number>();
|
||||
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<Node<WorkspaceNodeData>> = 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(
|
||||
|
||||
@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@ -342,20 +342,33 @@ export const useCanvasStore = create<CanvasState>((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<CanvasState>((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<CanvasState>((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<string>([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<CanvasState>((set, get) => ({
|
||||
|
||||
setCollapsed: (parentId, collapsed) => {
|
||||
const { nodes } = get();
|
||||
// Find all descendant ids via BFS.
|
||||
const descendantIds = new Set<string>();
|
||||
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<string, boolean>();
|
||||
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<string | null, string[]>();
|
||||
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<string, boolean>();
|
||||
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<CanvasState>((set, get) => ({
|
||||
if (kids.length === 0) return;
|
||||
const slotByKid = new Map<string, { x: number; y: number }>();
|
||||
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);
|
||||
});
|
||||
}
|
||||
},
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user