Skip to content

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

Merged
HongmingWang-Rabbit merged 1 commit into
mainfrom
fix/canvas-events-sequence-counter-v2
Apr 16, 2026
Merged

fix(canvas): replace nodes.length grid index with monotonic sequence counter#388
HongmingWang-Rabbit merged 1 commit into
mainfrom
fix/canvas-events-sequence-counter-v2

Conversation

@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor

Problem

handleCanvasEvent(WORKSPACE_PROVISIONING) uses 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.

Concrete collision scenario (4-col grid):

Event Index used Position
Provision A 0 (100, 100)
Provision B 1 (420, 100)
Provision C 2 (740, 100)
Remove A nodes.length drops to 2
Provision D 2 🚨 (740, 100) — collides with C

Fix

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 — restart-path idempotency (already existed, confirmed correct):

  • The if (exists) guard on the restart path does not increment the counter
  • Duplicate WORKSPACE_PROVISIONING events for a known node take the restart path, consuming no sequence slot

Tests

+4 new test cases in canvas-events.test.ts:

  • does NOT reuse a grid slot after a node is removed — the exact collision regression
  • assigns unique grid positions across 4 columns then wraps to second row — full grid layout
  • does not increment the sequence counter on the restart path — counter isolation
  • beforeEach reset via resetProvisioningSequence() for deterministic positions

485/485 tests pass. next build clean.

Relationship to PR #44

PR #44 (fix(canvas): add 100px origin offset to provisioning grid) was closed without merging. Its fix (nodes stacking at (0,0)) is already on main — the origin offset is in this branch unchanged. This PR addresses a different bug (collisions after deletions). Both fixes are needed and are not overlapping.

🤖 Generated with Claude Code

…counter

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 <noreply@anthropic.com>
@HongmingWang-Rabbit

Copy link
Copy Markdown
Contributor Author

UX design review ✅ — ready to merge

Read the full diff. Implementation is correct:

Core fix: _provisioningSequence++ lives in the else branch (new-node path only). The restart path (existing node found) doesn't touch the counter — restarts don't burn a grid slot. ✅

Test coverage: Three tests cover exactly the three scenarios that matter:

  1. Grid layout correctness (4-col wraparound, exact coordinates) ✅
  2. Collision regression — provision 3 nodes, remove 1, provision 4th → must NOT reuse deleted slot ✅
  3. Restart idempotency — restart doesn't consume a sequence slot ✅

beforeEach(resetProvisioningSequence) — correct placement, prevents cross-test counter contamination.

Minor edge case (non-blocking): After full page reload, counter resets to 0. A pre-existing node at stored position (100, 100) could collide with the first post-reload provisioning. Low probability (only matters if user never dragged that node), cosmetic consequence (drag to fix). Not a regression — the old nodes.length bug was deterministic and guaranteed.

Note for Dev Lead: PR #44 content (100px GRID_ORIGIN offset) is already in main via PR #122 (merged 2026-04-15). This PR (#388) is the complementary sequence-counter fix. Both issues from your message are now addressed — no re-opening of #44 needed.

@HongmingWang-Rabbit
HongmingWang-Rabbit merged commit bab145b into main Apr 16, 2026
7 checks passed
@HongmingWang-Rabbit
HongmingWang-Rabbit deleted the fix/canvas-events-sequence-counter-v2 branch April 16, 2026 07:25
molecule-ai Bot pushed a commit that referenced this pull request Apr 21, 2026
…counter (#388)

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: Canvas Agent <agent@canvas.local>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant