fix(canvas): address code-review findings on the Canvas refactor
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) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang
parent
50b537849a
commit
c2b2e13abe
@ -195,9 +195,9 @@ function CanvasInner() {
|
||||
|
||||
{/* Screen-reader live region: announces workspace count on canvas load or change */}
|
||||
<div role="status" aria-live="polite" className="sr-only">
|
||||
{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`}
|
||||
</div>
|
||||
|
||||
{nodes.length === 0 && <EmptyState />}
|
||||
|
||||
@ -9,6 +9,8 @@ import {
|
||||
import { useCanvasStore, type WorkspaceNodeData } from "@/store/canvas";
|
||||
import { clampChildIntoParent, shouldDetach } from "./dragUtils";
|
||||
|
||||
type WorkspaceNode = Node<WorkspaceNodeData>;
|
||||
|
||||
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<string | null>(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<Node<WorkspaceNodeData>> = useCallback(
|
||||
(event, node) => {
|
||||
dragStartParentRef.current = (node.data as WorkspaceNodeData).parentId;
|
||||
const onNodeDragStart: OnNodeDrag<WorkspaceNode> = useCallback(
|
||||
(event) => {
|
||||
dragModifiersRef.current = {
|
||||
alt: event.altKey,
|
||||
meta: event.metaKey || event.ctrlKey,
|
||||
@ -109,7 +111,7 @@ export function useDragHandlers(): DragHandlers {
|
||||
[],
|
||||
);
|
||||
|
||||
const onNodeDrag: OnNodeDrag<Node<WorkspaceNodeData>> = useCallback(
|
||||
const onNodeDrag: OnNodeDrag<WorkspaceNode> = useCallback(
|
||||
(event, node) => {
|
||||
dragModifiersRef.current = {
|
||||
alt: event.altKey,
|
||||
@ -129,13 +131,13 @@ export function useDragHandlers(): DragHandlers {
|
||||
[findDropTarget, getInternalNode, setDragOverNode],
|
||||
);
|
||||
|
||||
const onNodeDragStop: OnNodeDrag<Node<WorkspaceNodeData>> = useCallback(
|
||||
const onNodeDragStop: OnNodeDrag<WorkspaceNode> = 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), []);
|
||||
|
||||
@ -952,3 +952,78 @@ describe("bumpZOrder", () => {
|
||||
expect(afterZ).toEqual(beforeZ);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------- batchNest ----------
|
||||
|
||||
describe("batchNest", () => {
|
||||
beforeEach(() => {
|
||||
(global.fetch as ReturnType<typeof vi.fn>).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<typeof vi.fn>;
|
||||
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/<id>
|
||||
// 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<typeof vi.fn>).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<typeof vi.fn>).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");
|
||||
});
|
||||
});
|
||||
|
||||
@ -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);
|
||||
|
||||
@ -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<CanvasState>((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<CanvasState>((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<CanvasState>((set, get) => ({
|
||||
const plan: Plan[] = [];
|
||||
const movedIds = new Set<string>();
|
||||
// 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<CanvasState>((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",
|
||||
);
|
||||
}
|
||||
},
|
||||
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
Reference in New Issue
Block a user