From 1c7a386bb428b8e70b0fa1ddee4318372d9f21c7 Mon Sep 17 00:00:00 2001 From: Endri Bezati Date: Fri, 4 Sep 2026 16:22:58 +0200 Subject: [PATCH] fix: a saved layout that stacks nodes is not a usable layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Requiring a saved layout to cover every node turned out not to be enough. A layout captured before its nodes were ever placed names all of them and puts several at the same point — the origin, usually, since that is where a node starts life. It passes the coverage check, suppresses the layout run, and is then restored faithfully on every open. So the diagram opens stacked, opens stacked again after a reload, and the only thing that ever fixes it is running a layout by hand — which saves a good layout and hides the cause. Two nodes at identical coordinates is a reliable signature. No layout engine produces it, and nobody drags one node exactly on top of another, so it means the positions were recorded rather than computed. The check is now coverage AND no coincidence. Both are properties of the saved file rather than guesses about where it came from, and both fail toward laying out again, which is the recoverable direction: a needless layout costs one run and is persisted immediately, while a wrongly trusted one persists forever. `countCoincidentNodes` is exported and tested beside the rule it feeds. It counts every extra at a shared point rather than the pairs, because the number is only ever compared against zero and the simpler definition is the one that stays true if that changes. --- .../src/server/source-model-storage.ts | 37 ++++++++++++-- .../test/saved-layout-coverage.test.ts | 50 ++++++++++++++++++- 2 files changed, 83 insertions(+), 4 deletions(-) diff --git a/packages/diagram-server/src/server/source-model-storage.ts b/packages/diagram-server/src/server/source-model-storage.ts index cffd091..3f3149a 100644 --- a/packages/diagram-server/src/server/source-model-storage.ts +++ b/packages/diagram-server/src/server/source-model-storage.ts @@ -98,8 +98,38 @@ const RUN_OUTPUT_FALLBACK_MAX_RESULTS = 50; * a diagram they can read, the result is persisted immediately so it happens * once, and this failure is at least visible where the other was silent. */ -export function savedLayoutIsUsable(hasSavedPositions: boolean, unpositionedNodeCount: number): boolean { - return hasSavedPositions && unpositionedNodeCount === 0; +export function savedLayoutIsUsable( + hasSavedPositions: boolean, + unpositionedNodeCount: number, + coincidentNodeCount = 0 +): boolean { + return hasSavedPositions && unpositionedNodeCount === 0 && coincidentNodeCount === 0; +} + +/** + * How many nodes share a position with another node. + * + * Covering every node turned out not to be enough. A layout captured before its + * nodes were ever placed names all of them and puts several at the same point — + * the origin, usually, since that is where a node starts. It passes a coverage + * check, suppresses the layout run, and is restored faithfully on every open, + * so the diagram opens stacked forever and only a layout by hand fixes it. + * + * Two nodes at the same coordinates is the reliable signature. No layout engine + * ever produces it, and nobody drags one node exactly on top of another, so it + * means the positions were recorded rather than computed. + */ +export function countCoincidentNodes(positions: Iterable<{ x: number; y: number }>): number { + const seen = new Set(); + let coincident = 0; + for (const { x, y } of positions) { + const key = `${x},${y}`; + if (seen.has(key)) { + coincident++; + } + seen.add(key); + } + return coincident; } /** @@ -484,7 +514,8 @@ export class WorkflowSourceModelStorage implements SourceModelStorage { const hasPersistedLayout = savedLayoutIsUsable( loadedLayoutPositions !== undefined, - unpositionedNodeCount + unpositionedNodeCount, + countCoincidentNodes(loadedLayoutPositions?.values() ?? []) ); const hasPersistedEdgeRoutesEffective = hasPersistedLayout && hasPersistedEdgeRoutes; diff --git a/packages/diagram-server/test/saved-layout-coverage.test.ts b/packages/diagram-server/test/saved-layout-coverage.test.ts index 125094a..cbbd4a0 100644 --- a/packages/diagram-server/test/saved-layout-coverage.test.ts +++ b/packages/diagram-server/test/saved-layout-coverage.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { savedLayoutIsUsable } from '../src/server/source-model-storage'; +import { countCoincidentNodes, savedLayoutIsUsable } from '../src/server/source-model-storage'; /** * When a saved layout may stand, and when the diagram must be laid out again. @@ -42,4 +42,52 @@ describe('savedLayoutIsUsable', () => { // by a different runtime names nothing in this graph. expect(savedLayoutIsUsable(true, 9)).toBe(false); }); + + it('is false when the layout stacks nodes on one another', () => { + // Covering every node is not enough. A layout captured before its nodes + // were placed names all of them and puts several at the same point, so + // it passes coverage, suppresses the run, and is restored faithfully + // every time — the diagram opens stacked forever, and only a layout by + // hand ever fixes it. + expect(savedLayoutIsUsable(true, 0, 1)).toBe(false); + expect(savedLayoutIsUsable(true, 0, 0)).toBe(true); + }); +}); + +describe('countCoincidentNodes', () => { + it('counts a node sharing a position with another', () => { + expect( + countCoincidentNodes([ + { x: 0, y: 0 }, + { x: 0, y: 0 }, + { x: 10, y: 4 } + ]) + ).toBe(1); + }); + + it('is zero for a layout that placed everything apart', () => { + expect( + countCoincidentNodes([ + { x: 0, y: 0 }, + { x: 10, y: 0 }, + { x: 0, y: 10 } + ]) + ).toBe(0); + }); + + it('is zero for nothing at all', () => { + // No saved positions is a different case, already handled by the + // coverage check, and must not be mistaken for a stacked layout. + expect(countCoincidentNodes([])).toBe(0); + }); + + it('counts every extra at a shared point, not just the pair', () => { + expect( + countCoincidentNodes([ + { x: 5, y: 5 }, + { x: 5, y: 5 }, + { x: 5, y: 5 } + ]) + ).toBe(2); + }); });