diff --git a/src/App.tsx b/src/App.tsx index 8edd153..de9fd3f 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, @@ -1478,15 +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); + 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/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}`; 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); + }); +});