fix(canvas): replace nodes.length grid index with monotonic sequence counter

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<WorkspaceNodeData>[] }).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<WorkspaceNodeData>[] }).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<WorkspaceNodeData>[] }).nodes;
+    const bPos = lastNodes.find((n) => n.id === "ws-b")!.position;
+    expect(bPos).toEqual({ x: 420, y: 100 }); // idx 1 = (100 + 320, 100)
+  });
 });
 
 // ---------------------------------------------------------------------------

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;