fix: improve auto-layout peer alignment and edge direction

- Exclude peer-to-peer edges from Dagre to keep same-type nodes at same rank - Snap peer group Y to average, re-space X preserving user's original order - Reverse upward edges (sourceHandle=top) in Dagre so child nodes always render below parent - Add layout.test.ts covering peer alignment, X ordering, and upward edge cases
2026-03-11 14:29:29 +01:00
parent 92d505f78c
commit 8affcda09d
2 changed files with 239 additions and 14 deletions
@@ -0,0 +1,122 @@
+import { describe, it, expect } from 'vitest'
+import { applyDagreLayout } from '../layout'
+import type { Node, Edge } from '@xyflow/react'
+import type { NodeData, EdgeData } from '@/types'
+
+function makeNode(id: string, type: string, parentId?: string): Node<NodeData> {
+  return {
+    id,
+    type,
+    position: { x: 0, y: 0 },
+    data: { type, label: id } as unknown as NodeData,
+    ...(parentId ? { parentId } : {}),
+  }
+}
+
+function makeEdge(source: string, target: string, sourceHandle?: string): Edge<EdgeData> {
+  return { id: `${source}-${target}`, source, target, sourceHandle, data: {} as EdgeData }
+}
+
+describe('applyDagreLayout', () => {
+  it('places two proxmox nodes connected to each other at the same Y', () => {
+    const nodes = [
+      makeNode('router', 'router'),
+      makeNode('pve1', 'proxmox'),
+      makeNode('pve2', 'proxmox'),
+    ]
+    const edges = [
+      makeEdge('router', 'pve1'),
+      makeEdge('router', 'pve2'),
+      makeEdge('pve1', 'pve2'),
+    ]
+
+    const result = applyDagreLayout(nodes, edges)
+    const pve1 = result.find((n) => n.id === 'pve1')!
+    const pve2 = result.find((n) => n.id === 'pve2')!
+
+    expect(pve1.position.y).toBe(pve2.position.y)
+  })
+
+  it('orders peer nodes left-to-right by chain: endpoint first, middle last', () => {
+    // pve-left -- pve-center -- pve-right  (chain)
+    // router connects to all three
+    const nodes = [
+      makeNode('router', 'router'),
+      makeNode('pve-left', 'proxmox'),
+      makeNode('pve-center', 'proxmox'),
+      makeNode('pve-right', 'proxmox'),
+    ]
+    const edges = [
+      makeEdge('router', 'pve-left'),
+      makeEdge('router', 'pve-center'),
+      makeEdge('router', 'pve-right'),
+      makeEdge('pve-left', 'pve-center'),
+      makeEdge('pve-center', 'pve-right'),
+    ]
+
+    const result = applyDagreLayout(nodes, edges)
+    const left = result.find((n) => n.id === 'pve-left')!
+    const center = result.find((n) => n.id === 'pve-center')!
+    const right = result.find((n) => n.id === 'pve-right')!
+
+    // All at same Y
+    expect(left.position.y).toBe(center.position.y)
+    expect(center.position.y).toBe(right.position.y)
+
+    // X order: endpoint (left or right) < center (middle has 2 peer connections)
+    // The BFS starts from an endpoint, so we just verify the middle is not at the extremes
+    const xs = [left.position.x, center.position.x, right.position.x].sort((a, b) => a - b)
+    expect(center.position.x).toBe(xs[1]) // pve-center must be in the middle
+  })
+
+  it('keeps child nodes (parentId set) in place', () => {
+    const nodes = [
+      makeNode('router', 'router'),
+      makeNode('pve1', 'proxmox'),
+      makeNode('vm1', 'vm', 'pve1'),
+    ]
+    const edges = [makeEdge('router', 'pve1')]
+
+    const result = applyDagreLayout(nodes, edges)
+    const vm1 = result.find((n) => n.id === 'vm1')!
+    expect(vm1.position).toEqual({ x: 0, y: 0 })
+  })
+
+  it('places a node below its parent when the edge exits from the top handle (upward edge)', () => {
+    // Frigate connects UP to Proxmox via its top handle (source=Frigate, sourceHandle='top')
+    // Dagre must place Frigate BELOW Proxmox, not above.
+    const nodes = [
+      makeNode('router', 'router'),
+      makeNode('proxmox', 'proxmox'),
+      makeNode('frigate', 'server'),
+    ]
+    const edges = [
+      makeEdge('router', 'proxmox'),
+      makeEdge('frigate', 'proxmox', 'top'), // upward edge: frigate → proxmox via top handle
+    ]
+
+    const result = applyDagreLayout(nodes, edges)
+    const proxmox = result.find((n) => n.id === 'proxmox')!
+    const frigate = result.find((n) => n.id === 'frigate')!
+
+    expect(frigate.position.y).toBeGreaterThan(proxmox.position.y)
+  })
+
+  it('places two switch nodes connected to each other at the same Y', () => {
+    const nodes = [
+      makeNode('router', 'router'),
+      makeNode('sw1', 'switch'),
+      makeNode('sw2', 'switch'),
+    ]
+    const edges = [
+      makeEdge('router', 'sw1'),
+      makeEdge('router', 'sw2'),
+      makeEdge('sw1', 'sw2'),
+    ]
+
+    const result = applyDagreLayout(nodes, edges)
+    const sw1 = result.find((n) => n.id === 'sw1')!
+    const sw2 = result.find((n) => n.id === 'sw2')!
+    expect(sw1.position.y).toBe(sw2.position.y)
+  })
+})
@@ -5,46 +5,149 @@ import type { NodeData, EdgeData } from '@/types'
 const NODE_WIDTH = 180
 const NODE_HEIGHT = 52

+const PEER_TYPES = new Set(['proxmox', 'switch'])
+
+/**
+ * Find groups of peer nodes (same type, directly connected to each other)
+ * using union-find. Returns a map: nodeId → groupId (the minimum nodeId in the group).
+ */
+function buildPeerGroups(
+  topLevel: Node<NodeData>[],
+  edges: Edge<EdgeData>[],
+): Map<string, string> {
+  const parent = new Map<string, string>(topLevel.map((n) => [n.id, n.id]))
+
+  function find(id: string): string {
+    if (parent.get(id) !== id) parent.set(id, find(parent.get(id)!))
+    return parent.get(id)!
+  }
+  function union(a: string, b: string) {
+    const ra = find(a), rb = find(b)
+    if (ra !== rb) parent.set(ra, rb)
+  }
+
+  const topLevelIds = new Set(topLevel.map((n) => n.id))
+  const peerIds = new Set(topLevel.filter((n) => PEER_TYPES.has(n.type ?? '')).map((n) => n.id))
+
+  for (const edge of edges) {
+    const { source: s, target: t } = edge
+    if (topLevelIds.has(s) && topLevelIds.has(t) && peerIds.has(s) && peerIds.has(t)) {
+      // Only merge if both nodes share the same type (proxmox↔proxmox, switch↔switch)
+      const srcNode = topLevel.find((n) => n.id === s)!
+      const tgtNode = topLevel.find((n) => n.id === t)!
+      if (srcNode.type === tgtNode.type) union(s, t)
+    }
+  }
+
+  // Resolve all to canonical group ids
+  const result = new Map<string, string>()
+  for (const n of topLevel) result.set(n.id, find(n.id))
+  return result
+}
+
 /**
 * Apply Dagre hierarchical (top-to-bottom) layout to nodes and edges.
 * Child nodes (parentId set) keep their relative position inside the parent — only
 * top-level nodes are repositioned by Dagre.
+ *
+ * Post-pass: peer nodes of the same type (proxmox, switch) connected to each other
+ * are snapped to the same Y rank so they appear on the same horizontal level.
 */
 export function applyDagreLayout(
  nodes: Node<NodeData>[],
  edges: Edge<EdgeData>[],
 ): Node<NodeData>[] {
+  const topLevel = nodes.filter((n) => !n.parentId)
+  const topLevelIds = new Set(topLevel.map((n) => n.id))
+
+  // Capture original X positions before Dagre — used to preserve left-to-right
+  // ordering of peer groups as the user set them.
+  const originalX = new Map<string, number>(topLevel.map((n) => [n.id, n.position.x]))
+
+  // Identify peer groups before running Dagre so we can exclude peer edges
+  const peerGroups = buildPeerGroups(topLevel, edges)
+  const isPeerEdge = (e: Edge<EdgeData>) => {
+    const sg = peerGroups.get(e.source)
+    const tg = peerGroups.get(e.target)
+    return sg !== undefined && tg !== undefined && sg === tg
+  }
+
  const g = new dagre.graphlib.Graph()
  g.setDefaultEdgeLabel(() => ({}))
  g.setGraph({ rankdir: 'TB', nodesep: 60, ranksep: 80 })

-  const topLevel = nodes.filter((n) => !n.parentId)
-
  for (const node of topLevel) {
    const w = node.type === 'proxmox' ? (node.width ?? 300) : NODE_WIDTH
    const h = node.type === 'proxmox' ? (node.height ?? 200) : NODE_HEIGHT
    g.setNode(node.id, { width: w, height: h })
  }
  for (const edge of edges) {
-    // Only add edges between top-level nodes
-    const srcTop = topLevel.some((n) => n.id === edge.source)
-    const tgtTop = topLevel.some((n) => n.id === edge.target)
-    if (srcTop && tgtTop) g.setEdge(edge.source, edge.target)
+    const srcTop = topLevelIds.has(edge.source)
+    const tgtTop = topLevelIds.has(edge.target)
+    // Exclude peer-to-peer edges — they confuse Dagre's rank assignment
+    if (!srcTop || !tgtTop || isPeerEdge(edge)) continue
+    // If the edge exits from the TOP handle of the source, the connection goes
+    // upward — meaning the source node is visually below the target. Reverse
+    // the edge direction for Dagre so it places the source below the target.
+    const upward = (edge as { sourceHandle?: string | null }).sourceHandle === 'top'
+    if (upward) {
+      g.setEdge(edge.target, edge.source)
+    } else {
+      g.setEdge(edge.source, edge.target)
+    }
  }

  dagre.layout(g)

-  return nodes.map((node) => {
-    if (node.parentId) return node // keep children in place
+  // Build initial positions from Dagre
+  const positions = new Map<string, { x: number; y: number; w: number; h: number }>()
+  for (const node of topLevel) {
    const pos = g.node(node.id)
    const w = node.type === 'proxmox' ? (node.width ?? 300) : NODE_WIDTH
    const h = node.type === 'proxmox' ? (node.height ?? 200) : NODE_HEIGHT
-    return {
-      ...node,
-      position: {
-        x: pos.x - w / 2,
-        y: pos.y - h / 2,
-      },
+    positions.set(node.id, { x: pos.x - w / 2, y: pos.y - h / 2, w, h })
+  }
+
+  // Post-pass: fix peer groups (same-type nodes directly connected to each other)
+  // Collect members per group
+  const groupMembers = new Map<string, string[]>()
+  for (const [id, groupId] of peerGroups) {
+    if (!groupMembers.has(groupId)) groupMembers.set(groupId, [])
+    groupMembers.get(groupId)!.push(id)
+  }
+
+  for (const [, members] of groupMembers) {
+    if (members.length < 2) continue
+
+    // --- Y: snap all to average Y of the group ---
+    const avgY = members.reduce((sum, id) => sum + positions.get(id)!.y, 0) / members.length
+    for (const id of members) positions.set(id, { ...positions.get(id)!, y: avgY })
+
+    // --- X: sort by original (pre-layout) X to preserve the user's intended
+    //         left-to-right order. Fall back to Dagre X if all nodes share the
+    //         same original X (e.g. freshly created canvas with no positions yet). ---
+    const GAP = 60
+    const origXs = members.map((id) => originalX.get(id) ?? 0)
+    const allSameOrigX = origXs.every((x) => x === origXs[0])
+    const ordered = members.slice().sort((a, b) =>
+      allSameOrigX
+        ? positions.get(a)!.x - positions.get(b)!.x   // fall back to Dagre X
+        : (originalX.get(a) ?? 0) - (originalX.get(b) ?? 0), // preserve user order
+    )
+    const totalWidth = ordered.reduce((sum, id) => sum + positions.get(id)!.w, 0) + GAP * (ordered.length - 1)
+    const centerX = members.reduce((sum, id) => sum + positions.get(id)!.x + positions.get(id)!.w / 2, 0) / members.length
+
+    let curX = centerX - totalWidth / 2
+    for (const id of ordered) {
+      const p = positions.get(id)!
+      positions.set(id, { ...p, x: curX })
+      curX += p.w + GAP
    }
+  }
+
+  return nodes.map((node) => {
+    if (node.parentId) return node
+    const p = positions.get(node.id)!
+    return { ...node, position: { x: p.x, y: p.y } }
  })
 }