Merge pull request #515 from hmcelik/fix/layout-state-leak

fix: scope remembered layout to the input graph instead of module state

Author: rustedgrail

lib/layout.ts

@@ -34,8 +34,13 @@ interface EdgeProxyNodeLabel extends Omit<NodeLabel, 'e'> {
     e: Edge;
 }
 
-let _oldGraph: Graph<GraphLabel, NodeLabel, EdgeLabel> | null = null;
-let _rawOldNodes: NodeCollection = null;
+interface PreviousLayout {
+    graph: Graph<GraphLabel, NodeLabel, EdgeLabel>;
+    rawNodes: NodeCollection;
+}
+
+// Remember layout state per input graph so unrelated callers cannot affect each other.
+const previousLayouts = new WeakMap<Graph<GraphLabel, NodeLabel, EdgeLabel>, PreviousLayout>();
 
 export function layout(g: Graph<GraphLabel, NodeLabel, EdgeLabel>, opts: LayoutOptions = {}): Graph<GraphLabel, NodeLabel, EdgeLabel> {
     recursiveClusterLayout(g, util.notime, opts);
@@ -97,7 +102,7 @@ function recursiveClusterLayout(g: Graph<GraphLabel, NodeLabel, EdgeLabel>, time
             recursiveClusterLayout(subgraph, time, opts);
             // Run the layout pipeline on the subgraph via a proper layout graph
             const subLayoutG = buildLayoutGraph(subgraph);
-            runLayout(subLayoutG, time, opts);
+            runLayout(subLayoutG, time, opts, null);
             updateInputGraph(subgraph, subLayoutG);
             // Compute bounding box for the cluster
             let minX = Infinity, minY = Infinity, maxX = -Infinity, maxY = -Infinity;
@@ -235,7 +240,8 @@ function recursiveClusterLayout(g: Graph<GraphLabel, NodeLabel, EdgeLabel>, time
 
     // --- Step 5: run the main layout for the top-level graph ---
     const layoutG = buildLayoutGraph(g);
-    runLayout(layoutG, time, opts);
+    const result = runLayout(layoutG, time, opts, previousLayouts.get(g) ?? null);
+    previousLayouts.set(g, result);
     updateInputGraph(g, layoutG);
 
     // --- Step 6: remove proxy edges, restore cluster internals, position children ---
@@ -317,15 +323,15 @@ function recursiveClusterLayout(g: Graph<GraphLabel, NodeLabel, EdgeLabel>, time
 function runLayout(
     g: Graph<GraphLabel, NodeLabel, EdgeLabel>,
     time: <T>(name: string, fn: () => T) => T,
-    opts: LayoutOptions
-): void {
-    if (opts?.useDynamic === false) {
-        _oldGraph = null;
-        _rawOldNodes = null;
-    }
+    opts: LayoutOptions,
+    previous: PreviousLayout | null = null
+): PreviousLayout {
+    const dynamic = opts?.useDynamic !== false;
+    const oldGraph = dynamic ? previous?.graph ?? null : null;
+    const rawOldNodes = dynamic ? previous?.rawNodes ?? null : null;
     time("    makeSpaceForEdgeLabels", () => makeSpaceForEdgeLabels(g));
     time("    removeSelfEdges", () => removeSelfEdges(g));
-    time("    acyclic", () => acyclic.run(g, _oldGraph));
+    time("    acyclic", () => acyclic.run(g, oldGraph));
     time("    nestingGraph.run", () => nestingGraph.run(g));
     time("    rank", () => rank(util.asNonCompoundGraph(g)));
     time("    injectEdgeLabelProxies", () => injectEdgeLabelProxies(g));
@@ -337,12 +343,12 @@ function runLayout(
     time("    normalize.run", () => normalize.run(g));
     time("    parentDummyChains", () => parentDummyChains(g));
     time("    addBorderSegments", () => addBorderSegments(g));
-    time("    order", () => order(g, opts, _rawOldNodes));
+    time("    order", () => order(g, opts, rawOldNodes));
     time("    insertSelfEdges", () => insertSelfEdges(g));
     time("    adjustCoordinateSystem", () => coordinateSystem.adjust(g));
     time("    position", () => position(g, opts.corePath));
     time("    positionSelfEdges", () => positionSelfEdges(g));
-    _rawOldNodes = JSON.parse(JSON.stringify((g as unknown as { _nodes: NodeCollection })._nodes));
+    const rawNodes: NodeCollection = JSON.parse(JSON.stringify((g as unknown as { _nodes: NodeCollection })._nodes));
     time("    removeBorderNodes", () => removeBorderNodes(g));
     time("    normalize.undo", () => normalize.undo(g));
     time("    fixupEdgeLabelCoords", () => fixupEdgeLabelCoords(g));
@@ -351,7 +357,8 @@ function runLayout(
     time("    assignNodeIntersects", () => assignNodeIntersects(g));
     time("    reversePoints", () => reversePointsForReversedEdges(g));
     time("    acyclic.undo", () => acyclic.undo(g));
-    _oldGraph = g;
+
+    return {graph: g, rawNodes};
 }
 
 /*

test/layout-test.ts

@@ -296,8 +296,65 @@ describe("layout", () => {
             b: {x: 50 + 200 + 75 / 2, y: 200 / 2}
         });
     });
+
+    describe("dynamic layout state", () => {
+        const graphPairs: Array<[[string, string][], [string, string][]]> = [
+            [
+                [["b", "a"]],
+                [["a", "c"], ["a", "b"], ["c", "b"]]
+            ],
+            [
+                [["a", "c"], ["a", "b"], ["c", "b"]],
+                [["b", "a"]]
+            ]
+        ];
+
+        it.each(graphPairs)("does not share state between unrelated graphs", (firstEdges, secondEdges) => {
+            layout(createGraph(firstEdges));
+            expect(() => layout(createGraph(secondEdges))).not.toThrow();
+        });
+
+        it("produces stable positions when laying out the same graph repeatedly", () => {
+            const graph = createGraph([["a", "c"], ["a", "b"], ["c", "b"]]);
+
+            layout(graph);
+            const firstCoordinates = extractCoordinates(graph);
+            layout(graph);
+            expect(extractCoordinates(graph)).toEqual(firstCoordinates);
+            layout(graph);
+            expect(extractCoordinates(graph)).toEqual(firstCoordinates);
+        });
+
+        it("keeps state isolated when graph layouts are interleaved", () => {
+            const first = createGraph([["b", "a"]]);
+            const second = createGraph([["a", "c"], ["a", "b"], ["c", "b"]]);
+
+            layout(first);
+            layout(second);
+            expect(() => layout(first)).not.toThrow();
+            expect(() => layout(second)).not.toThrow();
+        });
+
+        it("can disable dynamic layout", () => {
+            const graph = createGraph([["a", "c"], ["a", "b"], ["c", "b"]]);
+
+            expect(() => layout(graph, {useDynamic: false})).not.toThrow();
+        });
+    });
 });
 
+function createGraph(edges: [string, string][]): Graph {
+    const graph = new Graph()
+        .setGraph({})
+        .setDefaultEdgeLabel(() => ({}));
+    edges.forEach(([v, w]) => {
+        graph.setNode(v, {width: 100, height: 100});
+        graph.setNode(w, {width: 100, height: 100});
+        graph.setEdge(v, w);
+    });
+    return graph;
+}
+
 function extractCoordinates(g: Graph) {
     const nodes = g.nodes();
     return nodes.reduce((acc: { [key: string]: { x: number, y: number } }, v) => {