Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions packages/diagram-server/src/server/source-model-storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>();
let coincident = 0;
for (const { x, y } of positions) {
const key = `${x},${y}`;
if (seen.has(key)) {
coincident++;
}
seen.add(key);
}
return coincident;
}

/**
Expand Down Expand Up @@ -484,7 +514,8 @@ export class WorkflowSourceModelStorage implements SourceModelStorage {

const hasPersistedLayout = savedLayoutIsUsable(
loadedLayoutPositions !== undefined,
unpositionedNodeCount
unpositionedNodeCount,
countCoincidentNodes(loadedLayoutPositions?.values() ?? [])
);
const hasPersistedEdgeRoutesEffective = hasPersistedLayout && hasPersistedEdgeRoutes;

Expand Down
50 changes: 49 additions & 1 deletion packages/diagram-server/test/saved-layout-coverage.test.ts
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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);
});
});
Loading