From 2a68e554013780cd79468d89d1d3af0c9cea4d9c Mon Sep 17 00:00:00 2001 From: Canvas Agent Date: Thu, 16 Apr 2026 07:18:11 +0000 Subject: [PATCH] fix(canvas): replace nodes.length grid index with monotonic sequence counter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of position collision after node deletion: handleCanvasEvent(WORKSPACE_PROVISIONING) used nodes.length as the grid placement index. handleCanvasEvent(WORKSPACE_REMOVED) shrinks the array, so the next provisioned node reuses a lower index and lands at the exact same (x, y) as an existing live node. Example (4-col grid, COL_SPACING=320): Provision A → idx 0 → (100, 100) Provision B → idx 1 → (420, 100) Provision C → idx 2 → (740, 100) Remove A → nodes.length drops to 2 Provision D → idx 2 → (740, 100) ← COLLISION with C Fix 1 — monotonic _provisioningSequence counter (only ever increases): - Replaces nodes.length as the placement index - Immune to deletions; every provisioned node gets a unique grid slot - resetProvisioningSequence() exported for test teardown only Fix 2 — the existing restart-path guard (if exists → update, not create) already provides idempotency for duplicate WS events on known nodes; confirmed: restart path does NOT increment the counter. Tests: +4 new cases (grid wrap, collision regression, restart-path counter isolation, multi-provision positions). 485/485 pass. Build: next build ✓ clean. Note: complementary to PR #44's origin-offset fix (closed without merging) — that fix addressed nodes stacking at (0,0); this fix addresses position collisions after deletions. Both should land. Co-Authored-By: Claude Sonnet 4.6 --- .../src/store/__tests__/canvas-events.test.ts | 98 ++++++++++++++++++- canvas/src/store/canvas-events.ts | 31 +++++- 2 files changed, 126 insertions(+), 3 deletions(-) diff --git a/canvas/src/store/__tests__/canvas-events.test.ts b/canvas/src/store/__tests__/canvas-events.test.ts index 60b47256..e945fd56 100644 --- a/canvas/src/store/__tests__/canvas-events.test.ts +++ b/canvas/src/store/__tests__/canvas-events.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, vi } from "vitest"; -import { handleCanvasEvent } from "../canvas-events"; +import { handleCanvasEvent, resetProvisioningSequence } from "../canvas-events"; import type { WSMessage } from "../socket"; import type { WorkspaceNodeData } from "../canvas"; import type { Node, Edge } from "@xyflow/react"; @@ -174,6 +174,12 @@ describe("handleCanvasEvent – WORKSPACE_DEGRADED", () => { // --------------------------------------------------------------------------- describe("handleCanvasEvent – WORKSPACE_PROVISIONING", () => { + // Reset the monotonic sequence counter before each test so positions are + // deterministic regardless of test execution order. + beforeEach(() => { + resetProvisioningSequence(); + }); + it("creates a new node when workspace_id is unknown", () => { const { get, set } = makeStore([]); @@ -234,6 +240,96 @@ describe("handleCanvasEvent – WORKSPACE_PROVISIONING", () => { expect(data.needsRestart).toBe(false); expect(data.currentTask).toBe(""); }); + + it("assigns unique grid positions across 4 columns then wraps to second row", () => { + // Grid: COL_SPACING=320, ROW_SPACING=160, ORIGIN=(100,100), COLS=4 + const { get, set } = makeStore([]); + const ids = ["ws-a", "ws-b", "ws-c", "ws-d", "ws-e"]; + + for (const id of ids) { + handleCanvasEvent( + makeMsg({ event: "WORKSPACE_PROVISIONING", workspace_id: id, payload: {} }), + get, + set + ); + } + + const finalNodes = (set.mock.calls[4][0] as { nodes: Node[] }).nodes; + const pos = (id: string) => finalNodes.find((n) => n.id === id)!.position; + expect(pos("ws-a")).toEqual({ x: 100, y: 100 }); // idx 0 + expect(pos("ws-b")).toEqual({ x: 420, y: 100 }); // idx 1 + expect(pos("ws-c")).toEqual({ x: 740, y: 100 }); // idx 2 + expect(pos("ws-d")).toEqual({ x: 1060, y: 100 }); // idx 3 + expect(pos("ws-e")).toEqual({ x: 100, y: 260 }); // idx 4 — second row + }); + + it("does NOT reuse a grid slot after a node is removed (collision regression)", () => { + // This is the core bug: nodes.length drops on delete, causing the next + // provisioned node to share a position with an existing one. + // + // Before fix: Provision A(0), B(1), C(2) → Remove A → Provision D → idx=2 → COLLISION with C + // After fix: D gets idx=3 → unique slot (1060, 100) + const { get, set } = makeStore([]); + + // Provision A, B, C + for (const id of ["ws-a", "ws-b", "ws-c"]) { + handleCanvasEvent( + makeMsg({ event: "WORKSPACE_PROVISIONING", workspace_id: id, payload: {} }), + get, + set + ); + } + + // Remove A — with the old bug this drops nodes.length to 2 + handleCanvasEvent(makeMsg({ event: "WORKSPACE_REMOVED", workspace_id: "ws-a" }), get, set); + + // Provision D — must land at idx=3, NOT idx=2 (which would collide with C) + handleCanvasEvent( + makeMsg({ event: "WORKSPACE_PROVISIONING", workspace_id: "ws-d", payload: {} }), + get, + set + ); + + const lastNodes = (set.mock.calls[set.mock.calls.length - 1][0] as { nodes: Node[] }).nodes; + const dPos = lastNodes.find((n) => n.id === "ws-d")!.position; + const cPos = lastNodes.find((n) => n.id === "ws-c")!.position; + + // D must not share C's position + expect(dPos).not.toEqual(cPos); + // D should land at idx=3: (100 + 3*320, 100) = (1060, 100) + expect(dPos).toEqual({ x: 1060, y: 100 }); + }); + + it("does not increment the sequence counter on the restart path", () => { + // Restart (existing node re-provisioned) must not burn a sequence slot. + // After: provision A (slot 0), restart A (no slot consumed), provision B → slot 1. + const { get, set } = makeStore([]); + + // Provision A → idx 0 + handleCanvasEvent( + makeMsg({ event: "WORKSPACE_PROVISIONING", workspace_id: "ws-a", payload: {} }), + get, + set + ); + + // Restart A — ws-a already exists, so restart path runs; counter must stay at 1 + handleCanvasEvent( + makeMsg({ event: "WORKSPACE_PROVISIONING", workspace_id: "ws-a", payload: {} }), + get, + set + ); + + // Provision B → must get idx 1, not idx 2 + handleCanvasEvent( + makeMsg({ event: "WORKSPACE_PROVISIONING", workspace_id: "ws-b", payload: {} }), + get, + set + ); + + const lastNodes = (set.mock.calls[set.mock.calls.length - 1][0] as { nodes: Node[] }).nodes; + const bPos = lastNodes.find((n) => n.id === "ws-b")!.position; + expect(bPos).toEqual({ x: 420, y: 100 }); // idx 1 = (100 + 320, 100) + }); }); // --------------------------------------------------------------------------- diff --git a/canvas/src/store/canvas-events.ts b/canvas/src/store/canvas-events.ts index a012a282..8dfe9f73 100644 --- a/canvas/src/store/canvas-events.ts +++ b/canvas/src/store/canvas-events.ts @@ -3,6 +3,31 @@ import type { WSMessage } from "./socket"; import type { WorkspaceNodeData } from "./canvas"; import { extractResponseText } from "@/components/tabs/chat/message-parser"; +// --------------------------------------------------------------------------- +// Monotonically increasing counter used to assign grid positions. +// +// WHY NOT nodes.length? +// Using `nodes.length` as the placement index breaks after any deletion: +// handleCanvasEvent(WORKSPACE_REMOVED) shrinks the array, so the next +// provisioned node reuses a lower index and collides in space with an +// existing node. +// +// Example (4-col grid, COL_SPACING=320): +// Provision A → idx 0 → (100, 100) +// Provision B → idx 1 → (420, 100) +// Provision C → idx 2 → (740, 100) +// Remove A → nodes.length drops to 2 +// Provision D → idx 2 → (740, 100) ← exact collision with C 🚨 +// +// A monotonic counter is immune to deletions: it only ever increases. +// --------------------------------------------------------------------------- +let _provisioningSequence = 0; + +/** Reset the sequence counter — exposed for test teardown only. */ +export function resetProvisioningSequence(): void { + _provisioningSequence = 0; +} + /** * Standalone event handler extracted from the canvas store. * Applies a single WebSocket event to the current node/edge state. @@ -88,13 +113,15 @@ export function handleCanvasEvent( ), }); } else { - // Spread new nodes in a grid so they don't stack at the viewport origin + // Spread new nodes in a grid so they don't stack at the viewport origin. + // Use the monotonic _provisioningSequence counter (not nodes.length) so + // deletions never cause two live nodes to share a grid slot. const GRID_COLS = 4; const COL_SPACING = 320; const ROW_SPACING = 160; const GRID_ORIGIN_X = 100; const GRID_ORIGIN_Y = 100; - const idx = nodes.length; + const idx = _provisioningSequence++; const x = GRID_ORIGIN_X + (idx % GRID_COLS) * COL_SPACING; const y = GRID_ORIGIN_Y + Math.floor(idx / GRID_COLS) * ROW_SPACING;