Bläddra i källkod

Improve workflow auto-layout with dagre library

Replace naive BFS-based layout with proper hierarchical graph layout using dagre:
- Use dagre library for directed graph layout with edge crossing minimization
- Nodes at the same level are properly aligned horizontally
- Loop body nodes positioned as sub-graph below their parent loop
- Downstream nodes from loop "done" output offset to avoid overlapping
- Improved edge styling with consistent color (#64748b)
- Grid snapping preserved for clean alignment

This produces much cleaner layouts compared to the previous algorithm which
resulted in chaotic node positioning and messy connections.
fszontagh 6 månader sedan
förälder
incheckning
d7f352d66f
3 ändrade filer med 215 tillägg och 116 borttagningar
  1. 33 0
      webui/package-lock.json
  2. 2 0
      webui/package.json
  3. 180 116
      webui/src/pages/WorkflowEditorPage.tsx

+ 33 - 0
webui/package-lock.json

@@ -10,8 +10,10 @@
       "dependencies": {
         "@monaco-editor/react": "^4.7.0",
         "@tanstack/react-query": "^5.8.0",
+        "@types/dagre": "^0.7.53",
         "axios": "^1.6.2",
         "clsx": "^2.0.0",
+        "dagre": "^0.8.5",
         "date-fns": "^2.30.0",
         "lucide-react": "^0.294.0",
         "react": "^18.2.0",
@@ -1804,6 +1806,12 @@
         "@types/d3-selection": "*"
       }
     },
+    "node_modules/@types/dagre": {
+      "version": "0.7.53",
+      "resolved": "https://registry.npmjs.org/@types/dagre/-/dagre-0.7.53.tgz",
+      "integrity": "sha512-f4gkWqzPZvYmKhOsDnhq/R8mO4UMcKdxZo+i5SCkOU1wvGeHJeUXGIHeE9pnwGyPMDof1Vx5ZQo4nxpeg2TTVQ==",
+      "license": "MIT"
+    },
     "node_modules/@types/estree": {
       "version": "1.0.8",
       "resolved": "https://registry.npmjs.org/@types/estree/-/estree-1.0.8.tgz",
@@ -2669,6 +2677,16 @@
         "node": ">=12"
       }
     },
+    "node_modules/dagre": {
+      "version": "0.8.5",
+      "resolved": "https://registry.npmjs.org/dagre/-/dagre-0.8.5.tgz",
+      "integrity": "sha512-/aTqmnRta7x7MCCpExk7HQL2O4owCT2h8NT//9I1OQ9vt29Pa0BzSAkR5lwFUcQ7491yVi/3CXU9jQ5o0Mn2Sw==",
+      "license": "MIT",
+      "dependencies": {
+        "graphlib": "^2.1.8",
+        "lodash": "^4.17.15"
+      }
+    },
     "node_modules/date-fns": {
       "version": "2.30.0",
       "resolved": "https://registry.npmjs.org/date-fns/-/date-fns-2.30.0.tgz",
@@ -3454,6 +3472,15 @@
       "dev": true,
       "license": "MIT"
     },
+    "node_modules/graphlib": {
+      "version": "2.1.8",
+      "resolved": "https://registry.npmjs.org/graphlib/-/graphlib-2.1.8.tgz",
+      "integrity": "sha512-jcLLfkpoVGmH7/InMC/1hIvOPSUh38oJtGhvrOFGzioE1DZ+0YW16RgmOJhHiuWTvGiJQ9Z1Ik43JvkRPRvE+A==",
+      "license": "MIT",
+      "dependencies": {
+        "lodash": "^4.17.15"
+      }
+    },
     "node_modules/has-flag": {
       "version": "4.0.0",
       "resolved": "https://registry.npmjs.org/has-flag/-/has-flag-4.0.0.tgz",
@@ -3775,6 +3802,12 @@
         "url": "https://github.com/sponsors/sindresorhus"
       }
     },
+    "node_modules/lodash": {
+      "version": "4.17.23",
+      "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.23.tgz",
+      "integrity": "sha512-LgVTMpQtIopCi79SJeDiP0TfWi5CNEc/L/aRdTh3yIvmZXTnheWpKjSZhnvMl8iXbC1tFg9gdHHDMLoV7CnG+w==",
+      "license": "MIT"
+    },
     "node_modules/lodash.merge": {
       "version": "4.6.2",
       "resolved": "https://registry.npmjs.org/lodash.merge/-/lodash.merge-4.6.2.tgz",

+ 2 - 0
webui/package.json

@@ -12,8 +12,10 @@
   "dependencies": {
     "@monaco-editor/react": "^4.7.0",
     "@tanstack/react-query": "^5.8.0",
+    "@types/dagre": "^0.7.53",
     "axios": "^1.6.2",
     "clsx": "^2.0.0",
+    "dagre": "^0.8.5",
     "date-fns": "^2.30.0",
     "lucide-react": "^0.294.0",
     "react": "^18.2.0",

+ 180 - 116
webui/src/pages/WorkflowEditorPage.tsx

@@ -1,5 +1,6 @@
 import { useParams, useNavigate } from 'react-router-dom'
 import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'
+import dagre from 'dagre'
 import { workflowsApi, nodesApi, Workflow, NodeDefinition, NodeOutput } from '../api/workflows'
 import ReactFlow, {
   Background,
@@ -1185,12 +1186,23 @@ function WorkflowEditorInner() {
     setShowDeleteConfirm(null)
   }, [setNodes, setEdges])
 
-  // Auto-layout nodes in a vertical top-to-bottom arrangement
-  // Special handling for loop nodes: body nodes are positioned below the loop
+  // Auto-layout nodes using dagre for clean hierarchical layout
+  // Special handling for loop nodes: body nodes positioned below their parent loop
   const autoLayout = useCallback(() => {
     if (nodes.length === 0) return
 
-    // Build adjacency maps with edge info
+    const GRID_SIZE = 20
+    const NODE_WIDTH = 200
+    const NODE_HEIGHT = 80
+    const HORIZONTAL_SPACING = 80
+    const VERTICAL_SPACING = 100
+    const LOOP_BODY_OFFSET_Y = 60
+    const LOOP_BODY_INDENT_X = 40
+
+    // Helper to snap to grid
+    const snapToGrid = (value: number) => Math.round(value / GRID_SIZE) * GRID_SIZE
+
+    // Build edge maps
     const outgoingEdges = new Map<string, Array<{ target: string; handle: string }>>()
     const incomingEdges = new Map<string, Array<{ source: string; handle: string }>>()
     nodes.forEach(n => {
@@ -1204,8 +1216,8 @@ function WorkflowEditorInner() {
 
     // Identify loop nodes and their body nodes
     const loopNodes = new Set<string>()
-    const loopBodyNodes = new Map<string, Set<string>>() // loopId -> set of body node IDs
-    const nodeToLoop = new Map<string, string>() // nodeId -> loopId (which loop this node belongs to)
+    const loopBodyNodes = new Map<string, Set<string>>()
+    const nodeToLoop = new Map<string, string>()
 
     nodes.forEach(n => {
       if (n.data.type === 'loop') {
@@ -1214,7 +1226,7 @@ function WorkflowEditorInner() {
       }
     })
 
-    // Find all body nodes for each loop (nodes connected via 'loop' output)
+    // Find body nodes for each loop (connected via 'loop' output handle)
     const findLoopBodyNodes = (loopId: string, nodeId: string, visited: Set<string>) => {
       if (visited.has(nodeId)) return
       visited.add(nodeId)
@@ -1225,18 +1237,14 @@ function WorkflowEditorInner() {
         nodeToLoop.set(nodeId, loopId)
       }
 
-      // Follow connections from this node, but stop at 'done' outputs from the loop
       const outEdges = outgoingEdges.get(nodeId) || []
       for (const edge of outEdges) {
-        // Don't follow 'done' connections from the loop itself
         if (nodeId === loopId && edge.handle === 'done') continue
-        // Don't enter other loops
         if (loopNodes.has(edge.target) && edge.target !== loopId) continue
         findLoopBodyNodes(loopId, edge.target, visited)
       }
     }
 
-    // Populate loop body nodes
     loopNodes.forEach(loopId => {
       const outEdges = outgoingEdges.get(loopId) || []
       const loopEdges = outEdges.filter(e => e.handle === 'loop')
@@ -1246,138 +1254,193 @@ function WorkflowEditorInner() {
       })
     })
 
-    // Find root nodes (triggers or nodes with no incoming edges that aren't loop body nodes)
-    const roots = nodes.filter(n =>
-      (n.data.isTrigger || (incomingEdges.get(n.id)?.length === 0)) && !nodeToLoop.has(n.id)
-    )
+    // Create dagre graph for main flow (excluding loop body nodes)
+    const g = new dagre.graphlib.Graph()
+    g.setGraph({
+      rankdir: 'TB',
+      nodesep: HORIZONTAL_SPACING,
+      ranksep: VERTICAL_SPACING,
+      marginx: 50,
+      marginy: 50,
+    })
+    g.setDefaultEdgeLabel(() => ({}))
 
-    // BFS to assign levels, skipping loop body nodes (they get positioned separately)
-    const levels = new Map<string, number>()
-    const queue: { id: string; level: number }[] = roots.map(n => ({ id: n.id, level: 0 }))
-    const visited = new Set<string>()
+    // Add main flow nodes (not loop body nodes)
+    nodes.forEach(n => {
+      if (!nodeToLoop.has(n.id)) {
+        g.setNode(n.id, { width: NODE_WIDTH, height: NODE_HEIGHT })
+      }
+    })
 
-    while (queue.length > 0) {
-      const { id, level } = queue.shift()!
-      if (visited.has(id)) {
-        if (level > (levels.get(id) || 0)) {
-          levels.set(id, level)
-        }
-        continue
+    // Add main flow edges (skip edges to/from loop body nodes, and skip loop->body edges)
+    edges.forEach(e => {
+      const sourceInBody = nodeToLoop.has(e.source)
+      const targetInBody = nodeToLoop.has(e.target)
+      const isLoopBodyEdge = loopNodes.has(e.source) && e.sourceHandle === 'loop'
+
+      if (!sourceInBody && !targetInBody && !isLoopBodyEdge) {
+        g.setEdge(e.source, e.target)
       }
-      visited.add(id)
-      levels.set(id, Math.max(levels.get(id) || 0, level))
-
-      const outEdges = outgoingEdges.get(id) || []
-
-      // For loop nodes, calculate how many levels the body takes
-      let loopBodyLevels = 0
-      if (loopNodes.has(id)) {
-        const bodyNodes = loopBodyNodes.get(id)!
-        if (bodyNodes.size > 0) {
-          // Layout body nodes internally and find max depth
-          loopBodyLevels = Math.max(1, Math.ceil(bodyNodes.size / 2))
-        }
+    })
+
+    // Run dagre layout
+    dagre.layout(g)
+
+    // Calculate loop body heights to offset 'done' targets
+    const loopBodyHeights = new Map<string, number>()
+    loopNodes.forEach(loopId => {
+      const bodyNodes = loopBodyNodes.get(loopId)!
+      if (bodyNodes.size === 0) {
+        loopBodyHeights.set(loopId, 0)
+        return
       }
 
-      for (const edge of outEdges) {
-        // Skip nodes that are loop body nodes (unless they're this loop's done targets)
-        if (nodeToLoop.has(edge.target) && nodeToLoop.get(edge.target) !== id) continue
+      // Create a sub-graph for the loop body
+      const bodyGraph = new dagre.graphlib.Graph()
+      bodyGraph.setGraph({
+        rankdir: 'TB',
+        nodesep: HORIZONTAL_SPACING * 0.8,
+        ranksep: VERTICAL_SPACING * 0.8,
+      })
+      bodyGraph.setDefaultEdgeLabel(() => ({}))
 
-        // For 'done' outputs from loops, add extra levels for the body
-        const nextLevel = loopNodes.has(id) && edge.handle === 'done'
-          ? level + 1 + loopBodyLevels
-          : level + 1
+      bodyNodes.forEach(nodeId => {
+        bodyGraph.setNode(nodeId, { width: NODE_WIDTH, height: NODE_HEIGHT })
+      })
 
-        queue.push({ id: edge.target, level: nextLevel })
-      }
-    }
+      // Add internal edges within the loop body
+      edges.forEach(e => {
+        if (bodyNodes.has(e.source) && bodyNodes.has(e.target)) {
+          bodyGraph.setEdge(e.source, e.target)
+        }
+        // Also add edge from loop node to first body node
+        if (e.source === loopId && e.sourceHandle === 'loop' && bodyNodes.has(e.target)) {
+          bodyGraph.setNode(loopId + '_entry', { width: 0, height: 0 })
+          bodyGraph.setEdge(loopId + '_entry', e.target)
+        }
+      })
 
-    // Handle unvisited non-body nodes
-    nodes.forEach(n => {
-      if (!visited.has(n.id) && !nodeToLoop.has(n.id)) {
-        levels.set(n.id, 0)
-      }
-    })
+      dagre.layout(bodyGraph)
 
-    // Group nodes by level (excluding loop body nodes)
-    const nodesByLevel = new Map<number, Node[]>()
-    nodes.forEach(n => {
-      if (nodeToLoop.has(n.id)) return // Skip body nodes
-      const level = levels.get(n.id) || 0
-      if (!nodesByLevel.has(level)) {
-        nodesByLevel.set(level, [])
-      }
-      nodesByLevel.get(level)!.push(n)
+      // Find the bounding box of the body
+      let maxY = 0
+      bodyNodes.forEach(nodeId => {
+        const nodePos = bodyGraph.node(nodeId)
+        if (nodePos) {
+          maxY = Math.max(maxY, nodePos.y + NODE_HEIGHT / 2)
+        }
+      })
+      loopBodyHeights.set(loopId, maxY + LOOP_BODY_OFFSET_Y)
     })
 
-    const GRID_SIZE = 20
-    const VERTICAL_SPACING = 140
-    const HORIZONTAL_SPACING = 260 // Divisible by 20
-    const LOOP_BODY_INDENT = 60 // Indent for loop body nodes
+    // Get positions from dagre and apply adjustments
+    const newPositions = new Map<string, { x: number; y: number }>()
 
-    // Helper to snap to grid
-    const snapToGrid = (value: number) => Math.round(value / GRID_SIZE) * GRID_SIZE
+    // Calculate vertical offsets for nodes that follow loop 'done' outputs
+    // We need to push them down to account for loop body height
+    const nodeYOffset = new Map<string, number>()
 
-    const newPositions = new Map<string, { x: number; y: number }>()
-    const maxLevel = Math.max(0, ...levels.values())
-
-    // Position main flow nodes
-    for (let level = 0; level <= maxLevel; level++) {
-      const nodesAtLevel = nodesByLevel.get(level) || []
-      const levelWidth = nodesAtLevel.length * HORIZONTAL_SPACING
-      const startX = snapToGrid((400 - levelWidth / 2) + HORIZONTAL_SPACING / 2)
-
-      nodesAtLevel.forEach((node, idx) => {
-        newPositions.set(node.id, {
-          x: snapToGrid(startX + idx * HORIZONTAL_SPACING),
-          y: snapToGrid(100 + level * VERTICAL_SPACING),
-        })
+    // BFS to propagate offsets from loops
+    const visited = new Set<string>()
+    const queue: string[] = []
+
+    // Start from all loop nodes
+    loopNodes.forEach(loopId => {
+      const bodyHeight = loopBodyHeights.get(loopId) || 0
+      const outEdges = outgoingEdges.get(loopId) || []
+      outEdges.forEach(edge => {
+        if (edge.handle === 'done' && !nodeToLoop.has(edge.target)) {
+          nodeYOffset.set(edge.target, (nodeYOffset.get(edge.target) || 0) + bodyHeight)
+          queue.push(edge.target)
+        }
+      })
+    })
+
+    // Propagate offsets downstream
+    while (queue.length > 0) {
+      const nodeId = queue.shift()!
+      if (visited.has(nodeId)) continue
+      visited.add(nodeId)
+
+      const offset = nodeYOffset.get(nodeId) || 0
+      const outEdges = outgoingEdges.get(nodeId) || []
+      outEdges.forEach(edge => {
+        if (!nodeToLoop.has(edge.target)) {
+          const existingOffset = nodeYOffset.get(edge.target) || 0
+          nodeYOffset.set(edge.target, Math.max(existingOffset, offset))
+          queue.push(edge.target)
+        }
       })
     }
 
-    // Position loop body nodes below their loop
+    // Apply positions for main flow nodes
+    nodes.forEach(n => {
+      if (!nodeToLoop.has(n.id)) {
+        const dagreNode = g.node(n.id)
+        if (dagreNode) {
+          const yOffset = nodeYOffset.get(n.id) || 0
+          newPositions.set(n.id, {
+            x: snapToGrid(dagreNode.x - NODE_WIDTH / 2),
+            y: snapToGrid(dagreNode.y - NODE_HEIGHT / 2 + yOffset),
+          })
+        }
+      }
+    })
+
+    // Position loop body nodes below their parent loop
     loopNodes.forEach(loopId => {
       const loopPos = newPositions.get(loopId)
       if (!loopPos) return
 
-      const bodyNodes = Array.from(loopBodyNodes.get(loopId) || [])
-      if (bodyNodes.length === 0) return
+      const bodyNodeIds = Array.from(loopBodyNodes.get(loopId) || [])
+      if (bodyNodeIds.length === 0) return
 
-      // Sort body nodes by their internal dependencies
-      const bodySorted: string[] = []
-      const bodyVisited = new Set<string>()
+      // Create and layout sub-graph for body
+      const bodyGraph = new dagre.graphlib.Graph()
+      bodyGraph.setGraph({
+        rankdir: 'TB',
+        nodesep: HORIZONTAL_SPACING * 0.8,
+        ranksep: VERTICAL_SPACING * 0.8,
+      })
+      bodyGraph.setDefaultEdgeLabel(() => ({}))
 
-      const sortBody = (nodeId: string) => {
-        if (bodyVisited.has(nodeId) || !loopBodyNodes.get(loopId)!.has(nodeId)) return
-        bodyVisited.add(nodeId)
+      bodyNodeIds.forEach(nodeId => {
+        bodyGraph.setNode(nodeId, { width: NODE_WIDTH, height: NODE_HEIGHT })
+      })
 
-        // Visit dependencies first
-        const inEdges = incomingEdges.get(nodeId) || []
-        for (const edge of inEdges) {
-          if (loopBodyNodes.get(loopId)!.has(edge.source)) {
-            sortBody(edge.source)
-          }
+      edges.forEach(e => {
+        const sourceInBody = loopBodyNodes.get(loopId)!.has(e.source)
+        const targetInBody = loopBodyNodes.get(loopId)!.has(e.target)
+        if (sourceInBody && targetInBody) {
+          bodyGraph.setEdge(e.source, e.target)
         }
-        bodySorted.push(nodeId)
-      }
-
-      bodyNodes.forEach(nodeId => sortBody(nodeId))
+      })
 
-      // Position body nodes in rows below the loop
-      const BODY_HORIZONTAL_SPACING = 220
-      const BODY_VERTICAL_SPACING = 120
-      const nodesPerRow = 2
+      dagre.layout(bodyGraph)
 
-      bodySorted.forEach((nodeId, idx) => {
-        const row = Math.floor(idx / nodesPerRow)
-        const col = idx % nodesPerRow
-        const rowWidth = Math.min(nodesPerRow, bodySorted.length - row * nodesPerRow) * BODY_HORIZONTAL_SPACING
-        const rowStartX = loopPos.x - rowWidth / 2 + BODY_HORIZONTAL_SPACING / 2 + LOOP_BODY_INDENT
+      // Find bounds to center the body below the loop
+      let minX = Infinity, maxX = -Infinity
+      bodyNodeIds.forEach(nodeId => {
+        const pos = bodyGraph.node(nodeId)
+        if (pos) {
+          minX = Math.min(minX, pos.x - NODE_WIDTH / 2)
+          maxX = Math.max(maxX, pos.x + NODE_WIDTH / 2)
+        }
+      })
 
-        newPositions.set(nodeId, {
-          x: snapToGrid(rowStartX + col * BODY_HORIZONTAL_SPACING),
-          y: snapToGrid(loopPos.y + VERTICAL_SPACING + row * BODY_VERTICAL_SPACING),
-        })
+      const bodyWidth = maxX - minX
+      const bodyCenterX = loopPos.x + NODE_WIDTH / 2 + LOOP_BODY_INDENT_X
+      const bodyStartX = bodyCenterX - bodyWidth / 2
+      const bodyStartY = loopPos.y + NODE_HEIGHT + LOOP_BODY_OFFSET_Y
+
+      bodyNodeIds.forEach(nodeId => {
+        const pos = bodyGraph.node(nodeId)
+        if (pos) {
+          newPositions.set(nodeId, {
+            x: snapToGrid(bodyStartX + (pos.x - minX)),
+            y: snapToGrid(bodyStartY + pos.y - NODE_HEIGHT / 2),
+          })
+        }
       })
     })
 
@@ -1864,7 +1927,8 @@ function WorkflowEditorInner() {
             nodeTypes={nodeTypes}
             defaultEdgeOptions={{
               type: 'smoothstep',
-              style: { strokeWidth: 2 },
+              style: { strokeWidth: 2, stroke: '#64748b' },
+              animated: false,
             }}
             connectionLineType={ConnectionLineType.SmoothStep}
             snapToGrid={!isViewingExecution}