From 5ceca0abcc6c1d9113c8627bf7e978779582a38e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 3 Sep 2026 01:52:16 +0000 Subject: [PATCH 1/2] fix: palette-add mints unique node ids against live topology MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit makeNode uses a module counter that resets on page load, causing duplicate ids when adding nodes of the same kind. handleAddNode now overrides the id using freshId (exported from clipboard.ts), which scans the live node id set — matching the pattern freshAnnId already uses for annotations. Closes xevrion/breakscale#46 --- src/App.tsx | 4 +++- src/clipboard.test.ts | 56 +++++++++++++++++++++++++++++++++++++++++++ src/clipboard.ts | 2 +- 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index 8edd153..c85c50f 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -39,7 +39,7 @@ import { Palette } from './components/Palette'; import { Glossary } from './components/Glossary'; import { Shortcuts } from './components/Shortcuts'; import { Examples } from './components/Examples'; -import { cloneSubgraph, isTopology, selectionSubgraph } from './clipboard'; +import { cloneSubgraph, freshId, isTopology, selectionSubgraph } from './clipboard'; import type { ClipboardSubgraph } from './clipboard'; import { NOTE_DEFAULT_WIDTH, @@ -1479,6 +1479,8 @@ export default function App() { const handleAddNode = useCallback( (kind: NodeKind, x: number, y: number) => { const node = makeNode(kind, x, y); + const used = new Set(topoLiveRef.current.nodes.map((n) => n.id)); + node.id = freshId(kind, used); history.commit('add', snapRef.current); applyTopology({ ...topology, diff --git a/src/clipboard.test.ts b/src/clipboard.test.ts index 8999a86..38ce405 100644 --- a/src/clipboard.test.ts +++ b/src/clipboard.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'; import { buildClipboardText, cloneSubgraph, + freshId, isTopology, parseClipboardText, selectionSubgraph, @@ -194,3 +195,58 @@ describe('cloneSubgraph', () => { expect(clones.nodes.map((n) => n.x)).toEqual(sub.nodes.map((n) => n.x)); }); }); + +describe('freshId (palette-add dedup, issue #46)', () => { + it('two mints of the same kind against an existing topology produce unique ids', () => { + const existing: Topology = { + nodes: [ + { ...node('client', 0, 0), kind: 'client', id: 'client' }, + { ...node('service-1'), kind: 'service', id: 'service-1' }, + { ...node('db', 480, 0), kind: 'db', id: 'db' }, + ], + edges: [], + }; + + const used = new Set(existing.nodes.map((n) => n.id)); + const id1 = freshId('cache', used); + const id2 = freshId('cache', used); + + expect(id1).not.toBe(id2); + expect(new Set([...existing.nodes.map((n) => n.id), id1, id2]).size).toBe(5); + }); + + it('skips ids already present in the topology', () => { + const existing: Topology = { + nodes: [node('cache-1', 0, 0)], + edges: [], + }; + // Simulate: topology already has cache-1 (from an earlier add). + // A second palette-add of cache must NOT mint cache-1 again. + const used = new Set(existing.nodes.map((n) => n.id)); + const id = freshId('cache', used); + expect(id).toBe('cache-2'); + }); + + it('result passes isTopology (no duplicate ids)', () => { + const base: Topology = { + nodes: [ + { ...node('client', 0, 0), kind: 'client', id: 'client' }, + { ...node('service-1'), kind: 'service', id: 'service-1' }, + { ...node('db', 480, 0), kind: 'db', id: 'db' }, + ], + edges: [], + }; + const used = new Set(base.nodes.map((n) => n.id)); + const id1 = freshId('cache', used); + const id2 = freshId('cache', used); + const after: Topology = { + nodes: [ + ...base.nodes, + { ...node(id1, 240, 100), kind: 'cache', id: id1 }, + { ...node(id2, 240, 200), kind: 'cache', id: id2 }, + ], + edges: [], + }; + expect(isTopology(after)).toBe(true); + }); +}); diff --git a/src/clipboard.ts b/src/clipboard.ts index a214bbb..1c78a64 100644 --- a/src/clipboard.ts +++ b/src/clipboard.ts @@ -186,7 +186,7 @@ export function parseClipboardText(text: string): ClipboardSubgraph | null { * the clipboard may come from another tab whose counter this session never * saw, and a collision would silently merge two different nodes. */ -function freshId(kind: NodeKind, used: Set): string { +export function freshId(kind: NodeKind, used: Set): string { let n = 1; while (used.has(`${kind}-${n}`)) n += 1; const id = `${kind}-${n}`; From 0a76206025d65d6e6584e56c468f45d6834c48b5 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 3 Sep 2026 02:00:21 +0000 Subject: [PATCH 2/2] fix: read live topology mirror in handleAddNode, add wiring + behavioral tests handleAddNode now reads topoLiveRef.current instead of the React topology closure, preventing two rapid adds from dropping the first node. topology removed from the dependency array. New test file handleAddNode.wiring.test.ts: - Source assertion: handleAddNode spreads topoLiveRef.current, not the React topology binding, and freshId is called. - Behavioral test: two sequential cache adds against the default preset produce unique ids, both nodes survive, isTopology accepts. --- src/App.tsx | 11 +-- src/handleAddNode.wiring.test.ts | 128 +++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 7 deletions(-) create mode 100644 src/handleAddNode.wiring.test.ts diff --git a/src/App.tsx b/src/App.tsx index c85c50f..de9fd3f 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -1478,17 +1478,14 @@ export default function App() { const handleAddNode = useCallback( (kind: NodeKind, x: number, y: number) => { + const t = topoLiveRef.current; const node = makeNode(kind, x, y); - const used = new Set(topoLiveRef.current.nodes.map((n) => n.id)); - node.id = freshId(kind, used); + node.id = freshId(kind, new Set(t.nodes.map((n) => n.id))); history.commit('add', snapRef.current); - applyTopology({ - ...topology, - nodes: [...topology.nodes, node], - }); + applyTopology({ ...t, nodes: [...t.nodes, node] }); setSelectedIds(new Set([node.id])); }, - [applyTopology, topology, history], + [applyTopology, history], ); /** diff --git a/src/handleAddNode.wiring.test.ts b/src/handleAddNode.wiring.test.ts new file mode 100644 index 0000000..31f4d04 --- /dev/null +++ b/src/handleAddNode.wiring.test.ts @@ -0,0 +1,128 @@ +import { describe, expect, it } from 'vitest'; +import { freshId, isTopology } from './clipboard'; +import { makeNode } from './sim/presets'; +import type { Topology } from './sim/types'; + +/** + * Guards the wiring between handleAddNode and the live topology mirror. + * + * Issue #46: `makeNode` uses a module counter that resets on load, so two + * palette-adds of the same kind produce duplicate ids. The fix mints ids + * via `freshId` against `topoLiveRef.current`. A stale-closure bug (reading + * the React `topology` binding instead of the live ref) causes two rapid + * adds to drop the first node, because the second add spreads the same + * pre-add snapshot. + * + * The source assertion catches regressions that the behavioral test cannot: + * a refactor could reintroduce the stale closure while keeping the id mint, + * and the behavioral test (which cannot simulate React scheduling) would + * still pass. + */ + +const SOURCES = import.meta.glob('./App.tsx', { + eager: true, + query: '?raw', + import: 'default', +}) as Record; + +const APP_SRC = Object.values(SOURCES)[0] ?? ''; + +/** + * Extract the handleAddNode callback body from App.tsx source. + * + * Finds `const handleAddNode = useCallback(` and captures everything up to + * the matching dep array closing paren. + */ +function extractHandleAddNode(src: string): string { + const marker = 'const handleAddNode = useCallback('; + const start = src.indexOf(marker); + if (start === -1) return ''; + let depth = 0; + let i = start + marker.length; + for (; i < src.length; i++) { + if (src[i] === '(') depth++; + if (src[i] === ')') { + if (depth === 0) break; + depth--; + } + } + return src.slice(start, i + 1); +} + +describe('handleAddNode wiring (issue #46)', () => { + const body = extractHandleAddNode(APP_SRC); + + it('finds handleAddNode in App.tsx', () => { + expect(body.length).toBeGreaterThan(0); + }); + + it('builds the next topology from topoLiveRef.current, not the React topology binding', () => { + expect(body).toContain('topoLiveRef.current'); + expect(body).not.toMatch(/\.\.\.\s*topology\b/); + expect(body).not.toMatch(/topology\.nodes/); + }); + + it('mints the id via freshId', () => { + expect(body).toContain('freshId'); + }); + + it('does not list topology in its dependency array', () => { + const depsMatch = body.match(/\},\s*\[([^\]]*)\]/); + expect(depsMatch).not.toBeNull(); + const deps = depsMatch![1]!; + expect(deps).not.toMatch(/\btopology\b/); + }); +}); + +/** + * Behavioral test: two sequential palette-adds against the default preset + * (ids: client, api, db) with no intervening React render. Simulates the + * actual #46 path as a pure function. + */ + +function simulateAddNode( + topo: Topology, + kind: Parameters[0], + x: number, + y: number, +): { topology: Topology; nodeId: string } { + const node = makeNode(kind, x, y); + node.id = freshId(kind, new Set(topo.nodes.map((n) => n.id))); + const next: Topology = { ...topo, nodes: [...topo.nodes, node] }; + return { topology: next, nodeId: node.id }; +} + +describe('two sequential palette-adds (issue #46 behavioral)', () => { + const DEFAULT_PRESET: Topology = { + nodes: [ + makeNode('client', 0, 0), + makeNode('service', 240, 0), + makeNode('db', 480, 0), + ], + edges: [ + { id: 'client-1->service-2', from: 'client-1', to: 'service-2', weight: 1 }, + { id: 'service-2->db-3', from: 'service-2', to: 'db-3', weight: 1 }, + ], + }; + + it('both nodes survive, ids are unique, isTopology accepts the result', () => { + const after1 = simulateAddNode(DEFAULT_PRESET, 'cache', 240, 240); + const after2 = simulateAddNode(after1.topology, 'cache', 240, 360); + + expect(after2.topology.nodes).toHaveLength(5); + expect(after1.nodeId).not.toBe(after2.nodeId); + + const allIds = after2.topology.nodes.map((n) => n.id); + expect(new Set(allIds).size).toBe(allIds.length); + + expect(isTopology(after2.topology)).toBe(true); + }); + + it('second add does not drop the first (stale-closure regression)', () => { + const after1 = simulateAddNode(DEFAULT_PRESET, 'cache', 240, 240); + const after2 = simulateAddNode(after1.topology, 'cache', 240, 360); + + expect(after2.topology.nodes.some((n) => n.id === after1.nodeId)).toBe(true); + expect(after2.topology.nodes.some((n) => n.id === after2.nodeId)).toBe(true); + }); +});