Fix layout issue when re-entering graph explorer (#792)

aws · Feb 12, 2025 · 3e98ad1 · 3e98ad1
1 parent 13f4f09
commit 3e98ad1
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 72 deletions.
diff --git a/packages/graph-explorer/src/components/Graph/hooks/useRunLayout.ts b/packages/graph-explorer/src/components/Graph/hooks/useRunLayout.ts
@@ -15,6 +15,11 @@ interface UseUpdateLayout {
   mounted: boolean;
 }
 
+/**
+ * Performs a layout on the graph when new nodes or edges are added or removed.
+ *
+ * Existing nodes are locked when the layout is run, and unlocked when the layout is complete.
+ */
 function useUpdateLayout({
   cy,
   layout,
@@ -29,39 +34,51 @@ function useUpdateLayout({
   const previousGraphStructureVersionRef = useRef(graphStructureVersion);
 
   useEffect(() => {
-    if (!cy || !layout || !mounted) {
+    // Ensure Cytoscape is mounted and skip the first graph structure version
+    if (!cy || !layout || !mounted || graphStructureVersion === 0) {
       return;
     }
 
+    // Only lock the previous nodes if the layout is the same, the graph has been updated, and there is at least one node.
+    const shouldLock =
+      previousLayoutRef.current === layout &&
+      previousGraphStructureVersionRef.current !== graphStructureVersion &&
+      previousNodesRef.current.size > 0;
+
+    // Reduce the number of iterations over the node collection
     const nodesInGraph = cy.nodes();
-    if (nodesInGraph.length) {
-      const shouldLock =
-        previousLayoutRef.current === layout &&
-        previousGraphStructureVersionRef.current !== graphStructureVersion;
+    const nodesToLock = shouldLock
+      ? nodesInGraph.filter(cyNode =>
+          previousNodesRef.current.has(cyNode.data().id)
+        )
+      : [];
 
-      if (shouldLock) {
-        nodesInGraph
-          .filter(cyNode => previousNodesRef.current.has(cyNode.data().id))
-          .forEach(node => {
-            node.lock();
-          });
-      }
+    if (shouldLock) {
+      // Lock all the previous nodes
+      cy.batch(() => {
+        nodesToLock.forEach(node => {
+          node.lock();
+        });
+      });
+    }
 
-      runLayout(cy, layout, additionalLayoutsConfig, useAnimation);
-      onLayoutUpdated?.(cy, layout);
+    // Perform the layout for any new nodes
+    runLayout(cy, layout, additionalLayoutsConfig, useAnimation);
+    onLayoutUpdated?.(cy, layout);
 
-      if (shouldLock) {
-        nodesInGraph
-          .filter(cyNode => previousNodesRef.current.has(cyNode.data().id))
-          .forEach(node => {
-            node.unlock();
-          });
-      }
-      previousLayoutRef.current = layout;
+    if (shouldLock) {
+      // Unlock all the previous nodes
+      cy.batch(() => {
+        nodesToLock.forEach(node => {
+          node.unlock();
+        });
+      });
     }
 
-    // Ensure the previousNodesRef is updated on every run
+    // Update the refs for previous state so we can compare the next time the graph is updated
+    previousLayoutRef.current = layout;
     previousNodesRef.current = new Set(nodesInGraph.map(node => node.id()));
+    previousGraphStructureVersionRef.current = graphStructureVersion;
   }, [
     cy,
     layout,

diff --git a/packages/graph-explorer/src/components/Graph/hooks/useUpdateGraphElements.ts b/packages/graph-explorer/src/components/Graph/hooks/useUpdateGraphElements.ts
@@ -1,6 +1,6 @@
 import cytoscape from "cytoscape";
 import cloneDeep from "lodash/cloneDeep";
-import { useCallback, useEffect, useState } from "react";
+import { useEffect, useState } from "react";
 import type { CytoscapeType, GraphEdge, GraphNode } from "../Graph.model";
 
 export interface UseUpdateGraphElementsProps {
@@ -15,21 +15,13 @@ function wereElementsAddedOrRemoved(
   cyElements: cytoscape.EdgeCollection | cytoscape.NodeCollection,
   elements: GraphNode[]
 ) {
-  if (cyElements.length !== elements.length) {
-    return true;
-  }
+  const cyElementsIds = new Set(cyElements.map(e => e.data("id")));
+  const elementsIds = new Set(elements.map(e => e.data.id));
 
-  // see if any element ids are different
-  const set = new Set(elements.map((e: GraphNode) => e.data.id));
-  let result = false;
-  cyElements.forEach((e: cytoscape.NodeSingular | cytoscape.EdgeSingular) => {
-    const id = e.data().id;
-    if (!set.has(id)) {
-      result = true;
-      return false;
-    }
-  });
-  return result;
+  return (
+    cyElementsIds.size !== elementsIds.size ||
+    !cyElementsIds.isSubsetOf(elementsIds)
+  );
 }
 
 const useUpdateGraphElements = ({
@@ -38,37 +30,9 @@ const useUpdateGraphElements = ({
   edges,
   lockedNodesIds,
   disableLockOnChange,
-}: UseUpdateGraphElementsProps): number => {
+}: UseUpdateGraphElementsProps) => {
   const [graphStructureVersion, setGraphStructureVersion] = useState(0);
 
-  const unlockNodes = useCallback(() => {
-    if (!cy) {
-      return;
-    }
-
-    cy.batch(() => {
-      cy.nodes().forEach(node => {
-        const id = node.data("id");
-        if (!lockedNodesIds.has(id)) {
-          node.unlock();
-        }
-      });
-    });
-  }, [cy, lockedNodesIds]);
-
-  useEffect(() => {
-    if (!cy) {
-      return;
-    }
-
-    // When layout is ready, unlock all nodes locked during the addition
-    cy.on("layoutready", unlockNodes);
-
-    return () => {
-      cy.off("layoutready", unlockNodes);
-    };
-  }, [cy, unlockNodes]);
-
   useEffect(() => {
     if (!cy) {
       return;
@@ -82,11 +46,6 @@ const useUpdateGraphElements = ({
       setGraphStructureVersion(v => v + 1);
     }
 
-    if (structureChanged && !disableLockOnChange) {
-      // If change, lock all nodes before add new nodes
-      cy.nodes().lock();
-    }
-
     // Cytoscape edits node and edge objects in-memory, which can screw up any of our earlier logic
     // that expects graph data to be unchanged by effects and stuff. Cloning deeply here ensures that
     // cytoscape operates on its own set of objects. Not the best thing in the world to do, maybe, but