From 5282f9c14c83d115a157d7d58ed81e2be4470f0b Mon Sep 17 00:00:00 2001 From: Yeabsira Gashaw Date: Mon, 7 Sep 2026 12:29:16 +0000 Subject: [PATCH 1/2] fix: never mint a node id that is already on the canvas makeNode's counter restarts at zero on every page load, but the ids it minted earlier come back through localStorage, share links and design files. Adding a component after a reload could therefore reuse an id already on the canvas, and two nodes then shared one id: selecting one selected both, the inspector read "2 components", and edges and config changes applied to both. The add path now skips ids the topology already holds, the way the paste path always has. --- src/App.tsx | 10 +++++++++- src/sim/makeNode.ids.test.ts | 25 +++++++++++++++++++++++++ src/sim/presets.ts | 21 +++++++++++++++++++-- 3 files changed, 53 insertions(+), 3 deletions(-) create mode 100644 src/sim/makeNode.ids.test.ts diff --git a/src/App.tsx b/src/App.tsx index 8edd153..6f8ce09 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -1478,7 +1478,15 @@ export default function App() { const handleAddNode = useCallback( (kind: NodeKind, x: number, y: number) => { - const node = makeNode(kind, x, y); + // Pass the ids already on the canvas so a fresh page load cannot + // mint an id a restored design is already using (see makeNode). + const node = makeNode( + kind, + x, + y, + undefined, + new Set(topology.nodes.map((n) => n.id)), + ); history.commit('add', snapRef.current); applyTopology({ ...topology, diff --git a/src/sim/makeNode.ids.test.ts b/src/sim/makeNode.ids.test.ts new file mode 100644 index 0000000..fb10bc4 --- /dev/null +++ b/src/sim/makeNode.ids.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, it } from 'vitest'; +import { makeNode } from './presets'; + +describe('makeNode', () => { + it('never reuses an id that is already on the canvas', () => { + // A design restored after a reload carries ids minted by an earlier + // session, while the module counter has restarted from zero. + const taken = new Set(Array.from({ length: 10 }, (_, i) => `service-${i + 1}`)); + const added = makeNode('service', 0, 0, undefined, taken); + expect(taken.has(added.id)).toBe(false); + expect(added.id).toMatch(/^service-\d+$/); + }); + + it('keeps minting distinct ids across repeated adds', () => { + const taken = new Set(['cache-1', 'cache-2', 'cache-3']); + const seen = new Set(); + for (let i = 0; i < 5; i += 1) { + const n = makeNode('cache', 0, 0, undefined, taken); + expect(taken.has(n.id)).toBe(false); + expect(seen.has(n.id)).toBe(false); + seen.add(n.id); + taken.add(n.id); + } + }); +}); diff --git a/src/sim/presets.ts b/src/sim/presets.ts index 8764b7c..fd7b22d 100644 --- a/src/sim/presets.ts +++ b/src/sim/presets.ts @@ -652,15 +652,32 @@ const DEFAULT_LABEL: Record = { let nodeCounter = 0; +/** + * Mint a node for the canvas. + * + * `taken` is the set of ids already on the canvas. It has to be consulted: + * the counter above restarts at zero on every page load, while the ids it + * minted in earlier sessions come back through localStorage, share links + * and design files. Without the check the eleventh service added after a + * reload is `service-1` again, and two nodes then share one id: selecting + * either selects both, the inspector reads "2 components", edges drawn to + * one land on both, and a config change applies to both. The paste path + * already dedupes this way (clipboard.ts freshId); the add path did not. + */ export function makeNode( kind: NodeKind, x: number, y: number, label?: string, + taken?: ReadonlySet, ): SimNode { - nodeCounter += 1; + let id: string; + do { + nodeCounter += 1; + id = `${kind}-${nodeCounter}`; + } while (taken !== undefined && taken.has(id)); return { - id: `${kind}-${nodeCounter}`, + id, kind, label: label ?? DEFAULT_LABEL[kind], x, From 4561bfccd34efcb2cfedbc2d54d2d5343bb218c9 Mon Sep 17 00:00:00 2001 From: Yeabsira Gashaw Date: Tue, 8 Sep 2026 19:56:20 +0000 Subject: [PATCH 2/2] fix: read the live topology mirror when adding a node Review feedback: the React topology binding is stale inside the add callback, so two quick adds both saw the pre-add snapshot and the second dropped the first node. Read topoLiveRef.current instead, which applyTopology writes synchronously, and drop topology from the deps. --- src/App.tsx | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/App.tsx b/src/App.tsx index 6f8ce09..2bbe871 100644 --- a/src/App.tsx +++ b/src/App.tsx @@ -1478,23 +1478,22 @@ export default function App() { const handleAddNode = useCallback( (kind: NodeKind, x: number, y: number) => { - // Pass the ids already on the canvas so a fresh page load cannot - // mint an id a restored design is already using (see makeNode). - const node = makeNode( - kind, - x, - y, - undefined, - new Set(topology.nodes.map((n) => n.id)), - ); + // Read the live mirror, not the React binding: two adds in quick + // succession both run against the closure's pre-add snapshot, so the + // second would build on a topology missing the first node and drop + // it (see topoLiveRef, and #47 which found the stale closure first). + // The mirror also supplies the ids already on the canvas, so a fresh + // page load cannot mint an id a restored design is using (makeNode). + const t = topoLiveRef.current; + const node = makeNode(kind, x, y, undefined, new Set(t.nodes.map((n) => n.id))); history.commit('add', snapRef.current); applyTopology({ - ...topology, - nodes: [...topology.nodes, node], + ...t, + nodes: [...t.nodes, node], }); setSelectedIds(new Set([node.id])); }, - [applyTopology, topology, history], + [applyTopology, history], ); /**