From 1d32c687e36fe123e3e0c0933a7b7a012f55f6cd Mon Sep 17 00:00:00 2001 From: handreyrc Date: Mon, 15 Jun 2026 19:40:33 -0400 Subject: [PATCH 1/6] Fix bad edge routing scenarios Signed-off-by: handreyrc # Conflicts: # packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts # packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts --- .../src/core/graph.ts | 121 +++++++ .../src/react-flow/diagram/autoLayout.ts | 204 +++++++++--- .../src/react-flow/edges/Edges.tsx | 20 +- .../tests/core/graph.test.ts | 315 ++++++++++++++++++ .../diagram/autoLayout.integration.test.ts | 138 ++++++-- .../tests/react-flow/edges/Edges.test.tsx | 18 +- .../edges/__snapshots__/Edges.test.tsx.snap | 2 +- 7 files changed, 736 insertions(+), 82 deletions(-) diff --git a/packages/serverless-workflow-diagram-editor/src/core/graph.ts b/packages/serverless-workflow-diagram-editor/src/core/graph.ts index 98c85b8c..6ded402b 100644 --- a/packages/serverless-workflow-diagram-editor/src/core/graph.ts +++ b/packages/serverless-workflow-diagram-editor/src/core/graph.ts @@ -40,7 +40,62 @@ export function fixNodesConnections(graph: FlatGraph): FlatGraph { } }); + // Build a map of nodeId -> node for quick lookups + const nodeMap = new Map(); + graph.nodes.forEach((node) => { + nodeMap.set(node.id, node); + }); + + // Build a map of parentId -> exitNodeId + const parentToExitNode = new Map(); + exitNodes.forEach((node) => { + if (node.parentId) { + parentToExitNode.set(node.parentId, node.id); + } + }); + + // Helper function to check if target is outside source's parent hierarchy + const isTargetOutsideSourceParent = ( + sourceNode: FlatGraphNode, + targetNode: FlatGraphNode, + ): boolean => { + if (!sourceNode.parentId) { + return false; + } + + // Check if target is the source's parent itself + if (targetNode.id === sourceNode.parentId) { + return false; + } + + // Check if target shares the same parent + if (targetNode.parentId === sourceNode.parentId) { + return false; + } + + // Check if target is inside source's parent hierarchy + let currentParentId: string | undefined = targetNode.parentId; + while (currentParentId) { + if (currentParentId === sourceNode.parentId) { + return false; + } + const parentNode = nodeMap.get(currentParentId); + currentParentId = parentNode?.parentId; + } + + return true; + }; + const graphClone = structuredClone(graph); + const newEdges: typeof graphClone.edges = []; + + // Helper function to check if an edge already exists + const edgeExists = (sourceId: string, targetId: string): boolean => { + return ( + graphClone.edges.some((e) => e.sourceId === sourceId && e.targetId === targetId) || + newEdges.some((e) => e.sourceId === sourceId && e.targetId === targetId) + ); + }; // Single pass over edges to rewrite sourceId/targetId graphClone.edges.forEach((edge) => { @@ -55,7 +110,73 @@ export function fixNodesConnections(graph: FlatGraph): FlatGraph { if (exitParent) { edge.sourceId = exitParent; } + + // Check if source node is inside a parent and points outside that parent + const sourceNode = nodeMap.get(edge.sourceId); + const targetNode = nodeMap.get(edge.targetId); + + if ( + sourceNode && + targetNode && + sourceNode.parentId && + isTargetOutsideSourceParent(sourceNode, targetNode) + ) { + // Check if source is itself a parent node (has children) + const sourceIsParent = graph.nodes.some((n) => n.parentId === sourceNode.id); + + // Find the topmost parent that the target is outside of + let currentNode = sourceNode; + let topmostParentId = sourceNode.parentId; + + // Walk up the parent hierarchy to find the topmost parent that the target is outside of + while (currentNode.parentId) { + const parentNode = nodeMap.get(currentNode.parentId); + if (!parentNode) break; + + // Check if target is outside this parent + if (parentNode.parentId && isTargetOutsideSourceParent(parentNode, targetNode)) { + // Target is also outside this parent's parent, keep going up + topmostParentId = parentNode.parentId; + currentNode = parentNode; + } else { + // Target is not outside this parent's parent (or parent has no parent) + // This means the current parent is the topmost one we need + topmostParentId = currentNode.parentId; + break; + } + } + + // Determine which exit node to use + let exitNodeToUse: string | undefined; + if (sourceIsParent) { + // If source is a parent node, it cannot connect to its own exit node + // Instead, connect to its parent's exit node (same level as the parent) + exitNodeToUse = parentToExitNode.get(sourceNode.parentId); + } else { + // If source is a regular task, use its immediate parent's exit node + exitNodeToUse = parentToExitNode.get(sourceNode.parentId); + } + + if (exitNodeToUse) { + // Redirect the edge to the appropriate exit node + edge.targetId = exitNodeToUse; + + // Create a new edge from the TOPMOST parent to the original target + // Only if an edge with the same source and target doesn't already exist + if (!edgeExists(topmostParentId, targetNode.id)) { + newEdges.push({ + id: `${edge.id}-redirected`, + sourceId: topmostParentId, + targetId: targetNode.id, + label: edge.label || "", + }); + } + } + } }); + // Add the new edges to the graph + graphClone.edges.push(...newEdges); + return graphClone; } diff --git a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts index 9f60a5db..2fb3e601 100644 --- a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts +++ b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts @@ -50,29 +50,45 @@ export type Size = { export type WayPoints = Point[]; export const ROOT_LAYOUT_OPTIONS: LayoutOptions = { - "org.eclipse.elk.algorithm": "org.eclipse.elk.layered", - "org.eclipse.elk.direction": "DOWN", - "org.eclipse.elk.layered.nodePlacement.strategy": "BRANDES_KOEPF", - "org.eclipse.elk.layered.nodePlacement.bk.fixedAlignment": "BALANCED", - "org.eclipse.elk.layered.nodePlacement.bk.edgeStraightening": "IMPROVE_STRAIGHTNESS", - "org.eclipse.elk.layered.nodePlacement.favorStraightEdges": "true", - "org.eclipse.elk.layered.priority.straightness": "10", - "org.eclipse.elk.hierarchyHandling": "INCLUDE_CHILDREN", - "org.eclipse.elk.layered.crossingMinimization.strategy": "LAYER_SWEEP", - "org.eclipse.elk.edgeRouting": "ORTHOGONAL", - "org.eclipse.elk.layered.unnecessaryBendpoints": "true", - "org.eclipse.elk.layered.cycleBreaking.strategy": "GREEDY_MODEL_ORDER", - "org.eclipse.elk.layered.considerModelOrder.crossingCounterNodeInfluence": "0.001", - "org.eclipse.elk.layered.spacing.edgeNode": "24", - "org.eclipse.elk.layered.spacing.edgeNodeBetweenLayers": "40", - "org.eclipse.elk.layered.spacing.nodeNode": "24", - "org.eclipse.elk.layered.spacing.nodeNodeBetweenLayers": "50", - "org.eclipse.elk.layered.spacing.componentComponent": "70", - "org.eclipse.elk.layered.mergeEdges": "true", + // layout algorithm + "elk.algorithm": "org.eclipse.elk.layered", + "elk.direction": "DOWN", + "layered.priority.direction": "MAX_VALUE", + "elk.hierarchyHandling": "INCLUDE_CHILDREN", + "elk.edgeRouting": "ORTHOGONAL", + "layered.layering.strategy": "INTERACTIVE", + // edge routing and crossing minimization + "layered.cycleBreaking.strategy": "DEPTH_FIRST", + "layered.crossingMinimization.greedySwitch.type": "TWO_SIDED", + "layered.crossingMinimization.greedySwitch.activationThreshold": "40", + "layered.crossingMinimization.semiInteractive": "true", + "layered.considerModelOrder.crossingCounterNodeInfluence": "1", + "elk.portConstraints": "FIXED_SIDE", + "layered.northOrSouthPort": "true", + "layered.thoroughness": "15", + "layered.nodePlacement.bk.edgeStraightening": "IMPROVE_STRAIGHTNESS", + "layered.unnecessaryBendpoints": "true", + "layered.mergeEdges": "true", + // node placement + "layered.nodePlacement.strategy": "BRANDES_KOEPF", + "layered.nodePlacement.bk.fixedAlignment": "BALANCED", + "layered.considerModelOrder.strategy": "PREFER_EDGES", + "layered.nodePlacement.favorStraightEdges": "true", + "layered.nodePlacement.bk.iterations": "100", + "layered.nodePlacement.bk.initialTemperature": "1000", + "layered.nodePlacement.bk.coolFactor": "0.005", + "elk.alignment": "TOP", + // spacing + "spacing.edgeNode": "44", + "spacing.edgeEdge": "44", + "spacing.componentComponent": "100", + "spacing.nodeNodeBetweenLayers": "100", + "spacing.edgeNodeBetweenLayers": "50", }; export const PARENT_LAYOUT_OPTIONS: LayoutOptions = { ...ROOT_LAYOUT_OPTIONS, + "org.eclipse.elk.layered.considerModelOrder.strategy": "NONE", "org.eclipse.elk.padding": "[top=60,left=20,bottom=20,right=20]", }; @@ -159,12 +175,42 @@ export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): const reactFlowNodeMap = new Map(reactFlowGraph.nodes.map((node) => [node.id, node])); + // Track which nodes are sources of edges (to add ports) + const nodeOutgoingEdges = new Map(); + reactFlowGraph.edges.forEach((edge) => { + const count = nodeOutgoingEdges.get(edge.source) || 0; + nodeOutgoingEdges.set(edge.source, count + 1); + }); + + // Add ports to parent nodes that have outgoing edges + nodeOutgoingEdges.forEach((count, nodeId) => { + const elkNode = nodeMap.get(nodeId); + const reactFlowNode = reactFlowNodeMap.get(nodeId); + + // Only add port if this is a parent node (has children) + if (elkNode && reactFlowNode && elkNode.children && elkNode.children.length > 0) { + // Add a single output port at the bottom center + elkNode.ports = [ + { + id: `${nodeId}_out`, + layoutOptions: { + "port.side": "SOUTH", + "port.index": "0", + }, + }, + ]; + } + }); + // Nest edges in the appropriate hierarchy level const rootEdges: ElkExtendedEdge[] = []; reactFlowGraph.edges.forEach((edge) => { + const sourceNode = nodeMap.get(edge.source); + const hasPort = sourceNode?.ports && sourceNode.ports.length > 0; + const elkEdge: ElkExtendedEdge = { id: edge.id, - sources: [edge.source], + sources: hasPort ? [`${edge.source}_out`] : [edge.source], targets: [edge.target], }; @@ -227,18 +273,62 @@ function buildElkEdgeMap( return map; } -// Helper function to check if an edge is inside a parent node -function isEdgeInsideParent( - edge: { source: string; target: string }, - nodeMap: Map, -): boolean { - // Edge is inside a parent if the lowest common ancestor is not the root - // This matches the logic used in findCommonAncestor when building the ELK graph - const commonAncestor = findCommonAncestor(edge.source, edge.target, nodeMap); - return commonAncestor !== "root"; +// Helper function to find the parent node containing an edge +function findEdgeParent(elkNode: ElkNode, edgeId: string): ElkNode | null { + if (elkNode.edges?.some((e) => e.id === edgeId)) { + return elkNode; + } + if (elkNode.children) { + for (const child of elkNode.children) { + const parent = findEdgeParent(child, edgeId); + if (parent) { + return parent; + } + } + } + return null; +} + +// Helper function to calculate absolute position of a node +function getAbsolutePosition(nodeId: string, elkNodeMap: Map): Point { + const node = elkNodeMap.get(nodeId); + if (!node || node.x === undefined || node.y === undefined) { + return { x: 0, y: 0 }; + } + + let absoluteX = node.x; + let absoluteY = node.y; + + // Traverse up the hierarchy to accumulate parent positions + let currentNode = node; + while (currentNode) { + const parentId = findParentId(currentNode.id, elkNodeMap); + if (!parentId || parentId === "root") { + break; + } + const parent = elkNodeMap.get(parentId); + if (parent && parent.x !== undefined && parent.y !== undefined) { + absoluteX += parent.x; + absoluteY += parent.y; + currentNode = parent; + } else { + break; + } + } + + return { x: absoluteX, y: absoluteY }; +} + +// Helper function to find parent ID of a node +function findParentId(nodeId: string, elkNodeMap: Map): string | null { + for (const [id, node] of elkNodeMap.entries()) { + if (node.children?.some((child) => child.id === nodeId)) { + return id; + } + } + return null; } -// set export function matchReactFlowGraphWithElkLayoutedGraph( graph: ReactFlowGraph, layoutedGraph: ElkNode, @@ -247,11 +337,6 @@ export function matchReactFlowGraphWithElkLayoutedGraph( const elkNodeMap = buildElkNodeMap(layoutedGraph); const elkEdgeMap = buildElkEdgeMap(layoutedGraph); - // Build node map for O(1) lookups in isEdgeInsideParent - const reactFlowNodeMap = new Map( - graph.nodes.map((node) => [node.id, { id: node.id, parentId: node.parentId }]), - ); - // Map node positions const layoutedNodes = graph.nodes.map((node) => { const elkNode = elkNodeMap.get(node.id); @@ -272,20 +357,49 @@ export function matchReactFlowGraphWithElkLayoutedGraph( if (elkEdge) { // Reconstruct data without old wayPoints to avoid stale routing const { wayPoints: _oldWayPoints, ...restData } = edge.data || {}; - const bendPoints = elkEdge.sections?.flatMap((section) => section.bendPoints || []) || []; - // Always create new data object, only add wayPoints if there are bend points + // Use full ELK section geometry instead of only bend points. + // This avoids mixing React Flow anchor coordinates with ELK bend points, + // which is especially problematic for edges inside parent nodes. + const sectionPoints = + elkEdge.sections?.flatMap((section) => { + const points = []; + if (section.startPoint) { + points.push(section.startPoint); + } + if (section.bendPoints) { + points.push(...section.bendPoints); + } + if (section.endPoint) { + points.push(section.endPoint); + } + return points; + }) || []; + const newData = { ...restData }; - if (bendPoints.length > 0) { - // Drop ELK-provided way points for edges nested inside a parent to avoid React Flow rendering distortion - const isInsideParent = isEdgeInsideParent(edge, reactFlowNodeMap); - if (isInsideParent) { - // There is an incompatibility with the react flow library, the wayPoints are calculated correctly by ELK - // but the way react flow render edges inside parent nodes cause path distortions. - newData.wayPoints = undefined; + if (sectionPoints.length >= 2) { + // Find the parent node containing this edge + const edgeParent = findEdgeParent(layoutedGraph, edge.id); + + // If edge is inside a parent node (not at root level), convert coordinates to absolute + if (edgeParent && edgeParent.id !== "root") { + const parentAbsolutePos = getAbsolutePosition(edgeParent.id, elkNodeMap); + + // Convert all waypoints from parent-relative to absolute coordinates + const absoluteWayPoints = sectionPoints.slice(1, -1).map((point) => ({ + x: point.x + parentAbsolutePos.x, + y: point.y + parentAbsolutePos.y, + })); + + newData.wayPoints = absoluteWayPoints; } else { - newData.wayPoints = bendPoints; + // Edge is at root level, use coordinates as-is + // React Flow already knows the rendered source/target anchors. + // Keep only the intermediate ELK points so the path stays in one coordinate space. + newData.wayPoints = sectionPoints.slice(1, -1); } + } else { + newData.wayPoints = undefined; } return { diff --git a/packages/serverless-workflow-diagram-editor/src/react-flow/edges/Edges.tsx b/packages/serverless-workflow-diagram-editor/src/react-flow/edges/Edges.tsx index 660e4d43..912ce718 100644 --- a/packages/serverless-workflow-diagram-editor/src/react-flow/edges/Edges.tsx +++ b/packages/serverless-workflow-diagram-editor/src/react-flow/edges/Edges.tsx @@ -52,16 +52,24 @@ export function createPathFromWayPoints( targetY: number, wayPoints?: WayPoints, ): string { - if (!wayPoints || wayPoints.length === 0) { + const points = [{ x: sourceX, y: sourceY }, ...(wayPoints || []), { x: targetX, y: targetY }]; + const [firstPoint, ...remainingPoints] = points; + + if (!firstPoint || remainingPoints.length === 0) { return `M ${sourceX},${sourceY} L ${targetX},${targetY}`; } - let path = `M ${sourceX},${sourceY}`; - for (const point of wayPoints) { - path += ` L ${point.x},${point.y}`; - } + let path = `M ${firstPoint.x},${firstPoint.y}`; + let previous = firstPoint; - path += ` L ${targetX},${targetY}`; + for (const current of remainingPoints) { + if (previous.x !== current.x && previous.y !== current.y) { + path += ` L ${current.x},${previous.y}`; + } + + path += ` L ${current.x},${current.y}`; + previous = current; + } return path; } diff --git a/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts b/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts index 0e9d7cdd..e80c731d 100644 --- a/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts +++ b/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts @@ -358,5 +358,320 @@ describe("graph utils", () => { expect(fixedGraph.edges[2]?.sourceId).toBe("task-1"); expect(fixedGraph.edges[2]?.targetId).toBe("task-2"); }); + + it("redirects any child node (including regular tasks) with outgoing edge to outside parent", () => { + // Scenario: parent1 contains taskChild (regular task, not a parent) + // taskChild has an outgoing edge to taskOutside (outside parent1) + // Expected: edge from taskChild should go to parent1's exit node + // and a new edge should be created from parent1 to taskOutside + const parent1: FlatGraphNode = { + id: "parent-1", + type: GraphNodeType.Do, + } as FlatGraphNode; + const exitNode1: FlatGraphNode = { + id: "exit-1", + type: GraphNodeType.Exit, + parentId: "parent-1", + } as FlatGraphNode; + const taskChild: FlatGraphNode = { + id: "task-child", + type: GraphNodeType.Call, + parentId: "parent-1", + } as FlatGraphNode; + const taskOutside: FlatGraphNode = { + id: "task-outside", + type: GraphNodeType.Call, + } as FlatGraphNode; + + const graph = createFlatGraph( + [parent1, exitNode1, taskChild, taskOutside], + [{ id: "edge-1", sourceId: "task-child", targetId: "task-outside", label: "test" }], + ); + + const fixedGraph = fixNodesConnections(graph); + + // The original edge should now point to parent1's exit node + expect(fixedGraph.edges[0]?.sourceId).toBe("task-child"); + expect(fixedGraph.edges[0]?.targetId).toBe("exit-1"); + + // A new edge should be created from parent1 to taskOutside + expect(fixedGraph.edges).toHaveLength(2); + expect(fixedGraph.edges[1]?.sourceId).toBe("parent-1"); + expect(fixedGraph.edges[1]?.targetId).toBe("task-outside"); + expect(fixedGraph.edges[1]?.label).toBe("test"); + }); + + it("redirects nested parent node outgoing edge to parent's exit node and creates new edge from parent to target", () => { + // Scenario: parent1 contains parent2 (which is also a parent node) + // parent2 has an outgoing edge to taskOutside (outside parent1) + // Expected: edge from parent2 should go to parent1's exit node + // and a new edge should be created from parent1 to taskOutside + const parent1: FlatGraphNode = { + id: "parent-1", + type: GraphNodeType.Do, + } as FlatGraphNode; + const parent2: FlatGraphNode = { + id: "parent-2", + type: GraphNodeType.Do, + parentId: "parent-1", + } as FlatGraphNode; + const exitNode1: FlatGraphNode = { + id: "exit-1", + type: GraphNodeType.Exit, + parentId: "parent-1", + } as FlatGraphNode; + const exitNode2: FlatGraphNode = { + id: "exit-2", + type: GraphNodeType.Exit, + parentId: "parent-2", + } as FlatGraphNode; + const childOfParent2: FlatGraphNode = { + id: "child-of-parent-2", + type: GraphNodeType.Call, + parentId: "parent-2", + } as FlatGraphNode; + const taskOutside: FlatGraphNode = { + id: "task-outside", + type: GraphNodeType.Call, + } as FlatGraphNode; + + const graph = createFlatGraph( + [parent1, parent2, exitNode1, exitNode2, childOfParent2, taskOutside], + [{ id: "edge-1", sourceId: "parent-2", targetId: "task-outside", label: "test" }], + ); + + const fixedGraph = fixNodesConnections(graph); + + // The original edge should now point to parent1's exit node (parent2's parent's exit) + // because parent2 is a parent node and cannot connect to its own exit + expect(fixedGraph.edges[0]?.sourceId).toBe("parent-2"); + expect(fixedGraph.edges[0]?.targetId).toBe("exit-1"); + + // A new edge should be created from parent1 (topmost) to taskOutside + expect(fixedGraph.edges).toHaveLength(2); + expect(fixedGraph.edges[1]?.sourceId).toBe("parent-1"); + expect(fixedGraph.edges[1]?.targetId).toBe("task-outside"); + expect(fixedGraph.edges[1]?.label).toBe("test"); + }); + + it("handles multiple levels of nesting recursively", () => { + // Scenario: parent1 contains parent2, parent2 contains parent3 + // parent3 has an outgoing edge to taskOutside (outside all parents) + // Expected: edge redirected through all levels + const parent1: FlatGraphNode = { + id: "parent-1", + type: GraphNodeType.Do, + } as FlatGraphNode; + const parent2: FlatGraphNode = { + id: "parent-2", + type: GraphNodeType.Do, + parentId: "parent-1", + } as FlatGraphNode; + const parent3: FlatGraphNode = { + id: "parent-3", + type: GraphNodeType.Do, + parentId: "parent-2", + } as FlatGraphNode; + const exitNode1: FlatGraphNode = { + id: "exit-1", + type: GraphNodeType.Exit, + parentId: "parent-1", + } as FlatGraphNode; + const exitNode2: FlatGraphNode = { + id: "exit-2", + type: GraphNodeType.Exit, + parentId: "parent-2", + } as FlatGraphNode; + const exitNode3: FlatGraphNode = { + id: "exit-3", + type: GraphNodeType.Exit, + parentId: "parent-3", + } as FlatGraphNode; + const childOfParent3: FlatGraphNode = { + id: "child-of-parent-3", + type: GraphNodeType.Call, + parentId: "parent-3", + } as FlatGraphNode; + const taskOutside: FlatGraphNode = { + id: "task-outside", + type: GraphNodeType.Call, + } as FlatGraphNode; + + const graph = createFlatGraph( + [parent1, parent2, parent3, exitNode1, exitNode2, exitNode3, childOfParent3, taskOutside], + [{ id: "edge-1", sourceId: "parent-3", targetId: "task-outside", label: "test" }], + ); + + const fixedGraph = fixNodesConnections(graph); + + // The original edge should be redirected to parent2's exit node (parent3's parent's exit) + // because parent3 is a parent node and cannot connect to its own exit + expect(fixedGraph.edges[0]?.sourceId).toBe("parent-3"); + expect(fixedGraph.edges[0]?.targetId).toBe("exit-2"); + + // A new edge should be created from parent1 (topmost) to taskOutside + expect(fixedGraph.edges).toHaveLength(2); + expect(fixedGraph.edges[1]?.sourceId).toBe("parent-1"); + expect(fixedGraph.edges[1]?.targetId).toBe("task-outside"); + }); + + it("does not redirect when target is inside the same parent", () => { + // Scenario: parent1 contains taskChild and taskSibling + // taskChild has an outgoing edge to taskSibling (both inside parent1) + // Expected: no redirection should happen + const parent1: FlatGraphNode = { + id: "parent-1", + type: GraphNodeType.Do, + } as FlatGraphNode; + const taskChild: FlatGraphNode = { + id: "task-child", + type: GraphNodeType.Call, + parentId: "parent-1", + } as FlatGraphNode; + const taskSibling: FlatGraphNode = { + id: "task-sibling", + type: GraphNodeType.Call, + parentId: "parent-1", + } as FlatGraphNode; + + const graph = createFlatGraph( + [parent1, taskChild, taskSibling], + [{ id: "edge-1", sourceId: "task-child", targetId: "task-sibling", label: "" }], + ); + + const fixedGraph = fixNodesConnections(graph); + + // The edge should remain unchanged + expect(fixedGraph.edges).toHaveLength(1); + expect(fixedGraph.edges[0]?.sourceId).toBe("task-child"); + expect(fixedGraph.edges[0]?.targetId).toBe("task-sibling"); + }); + + it("does not redirect when target is the parent itself", () => { + // Scenario: parent1 contains taskChild + // taskChild has an outgoing edge to parent1 itself + // Expected: no redirection should happen + const parent1: FlatGraphNode = { + id: "parent-1", + type: GraphNodeType.Do, + } as FlatGraphNode; + const taskChild: FlatGraphNode = { + id: "task-child", + type: GraphNodeType.Call, + parentId: "parent-1", + } as FlatGraphNode; + + const graph = createFlatGraph( + [parent1, taskChild], + [{ id: "edge-1", sourceId: "task-child", targetId: "parent-1", label: "" }], + ); + + const fixedGraph = fixNodesConnections(graph); + + // The edge should remain unchanged + expect(fixedGraph.edges).toHaveLength(1); + expect(fixedGraph.edges[0]?.sourceId).toBe("task-child"); + expect(fixedGraph.edges[0]?.targetId).toBe("parent-1"); + }); + + it("handles catch block scenario with task pointing outside", () => { + // Scenario similar to EV charging stations: try-catch where catch contains a task + // that points outside the catch block + const tryBlock: FlatGraphNode = { + id: "try-block", + type: GraphNodeType.Try, + } as FlatGraphNode; + const catchBlock: FlatGraphNode = { + id: "catch-block", + type: GraphNodeType.Catch, + parentId: "try-block", + } as FlatGraphNode; + const catchExitNode: FlatGraphNode = { + id: "catch-exit", + type: GraphNodeType.Exit, + parentId: "catch-block", + } as FlatGraphNode; + const tryExitNode: FlatGraphNode = { + id: "try-exit", + type: GraphNodeType.Exit, + parentId: "try-block", + } as FlatGraphNode; + const noSlotsAvailable: FlatGraphNode = { + id: "noSlotsAvailable", + type: GraphNodeType.Call, + parentId: "catch-block", + } as FlatGraphNode; + const endNode: FlatGraphNode = { + id: "end", + type: GraphNodeType.End, + } as FlatGraphNode; + + const graph = createFlatGraph( + [tryBlock, catchBlock, catchExitNode, tryExitNode, noSlotsAvailable, endNode], + [{ id: "edge-1", sourceId: "noSlotsAvailable", targetId: "end", label: "" }], + ); + + const fixedGraph = fixNodesConnections(graph); + + // The edge from noSlotsAvailable should be redirected to catch-block's exit (immediate parent) + expect(fixedGraph.edges[0]?.sourceId).toBe("noSlotsAvailable"); + expect(fixedGraph.edges[0]?.targetId).toBe("catch-exit"); + + // A new edge should be created from try-block (topmost parent) to end + expect(fixedGraph.edges).toHaveLength(2); + expect(fixedGraph.edges[1]?.sourceId).toBe("try-block"); + expect(fixedGraph.edges[1]?.targetId).toBe("end"); + }); + + it("does not create duplicate edges when connection already exists", () => { + // Scenario: parent1 contains taskChild1 and taskChild2 + // taskChild1 has an edge to taskOutside + // taskChild2 also has an edge to taskOutside + // Expected: only one edge from parent1 to taskOutside should be created + const parent1: FlatGraphNode = { + id: "parent-1", + type: GraphNodeType.Do, + } as FlatGraphNode; + const exitNode1: FlatGraphNode = { + id: "exit-1", + type: GraphNodeType.Exit, + parentId: "parent-1", + } as FlatGraphNode; + const taskChild1: FlatGraphNode = { + id: "task-child-1", + type: GraphNodeType.Call, + parentId: "parent-1", + } as FlatGraphNode; + const taskChild2: FlatGraphNode = { + id: "task-child-2", + type: GraphNodeType.Call, + parentId: "parent-1", + } as FlatGraphNode; + const taskOutside: FlatGraphNode = { + id: "task-outside", + type: GraphNodeType.Call, + } as FlatGraphNode; + + const graph = createFlatGraph( + [parent1, exitNode1, taskChild1, taskChild2, taskOutside], + [ + { id: "edge-1", sourceId: "task-child-1", targetId: "task-outside", label: "" }, + { id: "edge-2", sourceId: "task-child-2", targetId: "task-outside", label: "" }, + ], + ); + + const fixedGraph = fixNodesConnections(graph); + + // Both original edges should be redirected to exit-1 + expect(fixedGraph.edges[0]?.sourceId).toBe("task-child-1"); + expect(fixedGraph.edges[0]?.targetId).toBe("exit-1"); + expect(fixedGraph.edges[1]?.sourceId).toBe("task-child-2"); + expect(fixedGraph.edges[1]?.targetId).toBe("exit-1"); + + // Only ONE new edge from parent1 to taskOutside should be created (no duplicate) + expect(fixedGraph.edges).toHaveLength(3); + expect(fixedGraph.edges[2]?.sourceId).toBe("parent-1"); + expect(fixedGraph.edges[2]?.targetId).toBe("task-outside"); + }); }); }); diff --git a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts index a577e6f3..6459e46e 100644 --- a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts +++ b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts @@ -294,6 +294,87 @@ describe("autoLayout", () => { expect(elkGraph.children?.[0].children?.[0].edges).toBeUndefined(); }); + it("adds ports to parent nodes with outgoing edges", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "parent", position: { x: 0, y: 0 }, data: {} }, + { id: "child", position: { x: 0, y: 0 }, data: {}, parentId: "parent" }, + { id: "node2", position: { x: 0, y: 0 }, data: {} }, + ] as Node[], + edges: [{ id: "edge1", source: "parent", target: "node2", data: {} }] as Edge[], + }; + + const elkGraph = buildElkGraphFromReactFlowGraph(reactFlowGraph); + + // Parent node should have a port since it has an outgoing edge + expect(elkGraph.children?.[0].ports).toBeDefined(); + expect(elkGraph.children?.[0].ports).toHaveLength(1); + expect(elkGraph.children?.[0].ports?.[0]).toEqual({ + id: "parent_out", + layoutOptions: { + "port.side": "SOUTH", + "port.index": "0", + }, + }); + }); + + it("does not add ports to leaf nodes with outgoing edges", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "node1", position: { x: 0, y: 0 }, data: {} }, + { id: "node2", position: { x: 0, y: 0 }, data: {} }, + ] as Node[], + edges: [{ id: "edge1", source: "node1", target: "node2", data: {} }] as Edge[], + }; + + const elkGraph = buildElkGraphFromReactFlowGraph(reactFlowGraph); + + // Leaf nodes should not have ports + expect(elkGraph.children?.[0].ports).toBeUndefined(); + expect(elkGraph.children?.[1].ports).toBeUndefined(); + }); + + it("uses port ID in edge sources for parent nodes", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "parent", position: { x: 0, y: 0 }, data: {} }, + { id: "child", position: { x: 0, y: 0 }, data: {}, parentId: "parent" }, + { id: "node2", position: { x: 0, y: 0 }, data: {} }, + ] as Node[], + edges: [{ id: "edge1", source: "parent", target: "node2", data: {} }] as Edge[], + }; + + const elkGraph = buildElkGraphFromReactFlowGraph(reactFlowGraph); + + // Edge from parent node should use port ID + expect(elkGraph.edges).toHaveLength(1); + expect(elkGraph.edges?.[0]).toEqual({ + id: "edge1", + sources: ["parent_out"], + targets: ["node2"], + }); + }); + + it("uses node ID in edge sources for leaf nodes", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "node1", position: { x: 0, y: 0 }, data: {} }, + { id: "node2", position: { x: 0, y: 0 }, data: {} }, + ] as Node[], + edges: [{ id: "edge1", source: "node1", target: "node2", data: {} }] as Edge[], + }; + + const elkGraph = buildElkGraphFromReactFlowGraph(reactFlowGraph); + + // Edge from leaf node should use node ID directly + expect(elkGraph.edges).toHaveLength(1); + expect(elkGraph.edges?.[0]).toEqual({ + id: "edge1", + sources: ["node1"], + targets: ["node2"], + }); + }); + it("handles multi-level nesting with edges at different levels", () => { const reactFlowGraph: ReactFlowGraph = { nodes: [ @@ -435,7 +516,8 @@ describe("autoLayout", () => { const result = matchReactFlowGraphWithElkLayoutedGraph(reactFlowGraph, layoutedElkGraph); - expect(result.edges[0].data?.wayPoints).toBeUndefined(); + // When there are no bend points, wayPoints should be an empty array + expect(result.edges[0].data?.wayPoints).toEqual([]); }); it("clears stale wayPoints when ELK edge has no sections", () => { @@ -503,8 +585,9 @@ describe("autoLayout", () => { const result = matchReactFlowGraphWithElkLayoutedGraph(reactFlowGraph, layoutedElkGraph); - expect(result.edges[0].data).toEqual({ label: "Test Edge" }); - expect(result.edges[0].data?.wayPoints).toBeUndefined(); + // When there are no bend points, wayPoints should be an empty array + expect(result.edges[0].data).toEqual({ label: "Test Edge", wayPoints: [] }); + expect(result.edges[0].data?.wayPoints).toEqual([]); }); it("preserves edge data when no ELK edge found", () => { @@ -584,13 +667,17 @@ describe("autoLayout", () => { const result = matchReactFlowGraphWithElkLayoutedGraph(reactFlowGraph, layoutedElkGraph); + // Should include all intermediate points (excluding first startPoint and last endPoint) + // When multiple sections exist, all intermediate points are included (including section boundaries) expect(result.edges[0].data?.wayPoints).toEqual([ - { x: 50, y: 0 }, - { x: 150, y: 100 }, + { x: 50, y: 0 }, // bendPoint from section1 + { x: 100, y: 50 }, // endPoint of section1 + { x: 100, y: 50 }, // startPoint of section2 (duplicate of previous) + { x: 150, y: 100 }, // bendPoint from section2 ]); }); - it("handles edges inside parent nodes - removes wayPoints", () => { + it("handles edges inside parent nodes - converts wayPoints to absolute coordinates", () => { const reactFlowGraph: ReactFlowGraph = { nodes: [ { id: "parent", position: { x: 0, y: 0 }, data: {} }, @@ -605,8 +692,8 @@ describe("autoLayout", () => { children: [ { id: "parent", - x: 0, - y: 0, + x: 100, + y: 200, children: [ { id: "child1", x: 10, y: 10 }, { id: "child2", x: 10, y: 70 }, @@ -633,8 +720,10 @@ describe("autoLayout", () => { const result = matchReactFlowGraphWithElkLayoutedGraph(reactFlowGraph, layoutedElkGraph); - // wayPoints should be undefined for edges inside parent nodes - expect(result.edges[0].data?.wayPoints).toBeUndefined(); + // wayPoints should be converted to absolute coordinates for edges inside parent nodes + // Parent is at (100, 200), bendPoint is at (10, 40) relative to parent + // So absolute position should be (110, 240) + expect(result.edges[0].data?.wayPoints).toEqual([{ x: 110, y: 240 }]); }); it("preserves wayPoints for edges not inside parent nodes", () => { @@ -900,13 +989,22 @@ describe("autoLayout", () => { width: 300, height: 200, children: [{ id: "child1", x: 10, y: 10, width: 100, height: 50 }], + ports: [ + { + id: "parent_out", + layoutOptions: { + "port.side": "SOUTH", + "port.index": "0", + }, + }, + ], }, { id: "node2", x: 350, y: 50, width: 200, height: 60 }, ], edges: [ { id: "edge1", - sources: ["parent"], + sources: ["parent_out"], targets: ["node2"], }, ], @@ -1110,19 +1208,15 @@ describe("autoLayout", () => { describe("ROOT_LAYOUT_OPTIONS", () => { it("contains required ELK layout options", () => { - expect(ROOT_LAYOUT_OPTIONS["org.eclipse.elk.algorithm"]).toBe("org.eclipse.elk.layered"); - expect(ROOT_LAYOUT_OPTIONS["org.eclipse.elk.direction"]).toBe("DOWN"); - expect(ROOT_LAYOUT_OPTIONS["org.eclipse.elk.hierarchyHandling"]).toBe("INCLUDE_CHILDREN"); + expect(ROOT_LAYOUT_OPTIONS["elk.algorithm"]).toBe("org.eclipse.elk.layered"); + expect(ROOT_LAYOUT_OPTIONS["elk.direction"]).toBe("DOWN"); + expect(ROOT_LAYOUT_OPTIONS["elk.hierarchyHandling"]).toBe("INCLUDE_CHILDREN"); }); it("has proper spacing configuration", () => { - expect(ROOT_LAYOUT_OPTIONS["org.eclipse.elk.layered.spacing.edgeNode"]).toBe("24"); - expect(ROOT_LAYOUT_OPTIONS["org.eclipse.elk.layered.spacing.componentComponent"]).toBe( - "70", - ); - expect(ROOT_LAYOUT_OPTIONS["org.eclipse.elk.layered.spacing.nodeNodeBetweenLayers"]).toBe( - "50", - ); + expect(ROOT_LAYOUT_OPTIONS["spacing.edgeNode"]).toBe("44"); + expect(ROOT_LAYOUT_OPTIONS["spacing.componentComponent"]).toBe("100"); + expect(ROOT_LAYOUT_OPTIONS["spacing.nodeNodeBetweenLayers"]).toBe("100"); }); }); @@ -1131,7 +1225,7 @@ describe("autoLayout", () => { expect(PARENT_LAYOUT_OPTIONS["org.eclipse.elk.padding"]).toBe( "[top=60,left=20,bottom=20,right=20]", ); - expect(PARENT_LAYOUT_OPTIONS["org.eclipse.elk.algorithm"]).toBe("org.eclipse.elk.layered"); + expect(PARENT_LAYOUT_OPTIONS["elk.algorithm"]).toBe("org.eclipse.elk.layered"); }); }); }); diff --git a/packages/serverless-workflow-diagram-editor/tests/react-flow/edges/Edges.test.tsx b/packages/serverless-workflow-diagram-editor/tests/react-flow/edges/Edges.test.tsx index 3d12be04..7f13dc80 100644 --- a/packages/serverless-workflow-diagram-editor/tests/react-flow/edges/Edges.test.tsx +++ b/packages/serverless-workflow-diagram-editor/tests/react-flow/edges/Edges.test.tsx @@ -110,7 +110,7 @@ describe("createPathFromWayPoints helper function", () => { targetX: 100, targetY: 100, wayPoints: undefined, - expected: "M 0,0 L 100,100", + expected: "M 0,0 L 100,0 L 100,100", }, { description: "creates simple path with empty waypoints array", @@ -119,7 +119,7 @@ describe("createPathFromWayPoints helper function", () => { targetX: 100, targetY: 100, wayPoints: [], - expected: "M 0,0 L 100,100", + expected: "M 0,0 L 100,0 L 100,100", }, { description: "creates path with single waypoint", @@ -128,7 +128,7 @@ describe("createPathFromWayPoints helper function", () => { targetX: 100, targetY: 100, wayPoints: [{ x: 50, y: 50 }], - expected: "M 0,0 L 50,50 L 100,100", + expected: "M 0,0 L 50,0 L 50,50 L 100,50 L 100,100", }, { description: "creates path with multiple waypoints", @@ -141,7 +141,7 @@ describe("createPathFromWayPoints helper function", () => { { x: 50, y: 50 }, { x: 75, y: 75 }, ], - expected: "M 0,0 L 25,25 L 50,50 L 75,75 L 100,100", + expected: "M 0,0 L 25,0 L 25,25 L 50,25 L 50,50 L 75,50 L 75,75 L 100,75 L 100,100", }, { description: "creates path with negative coordinates", @@ -153,7 +153,7 @@ describe("createPathFromWayPoints helper function", () => { { x: -50, y: -50 }, { x: 50, y: 50 }, ], - expected: "M -100,-100 L -50,-50 L 50,50 L 100,100", + expected: "M -100,-100 L -50,-100 L -50,-50 L 50,-50 L 50,50 L 100,50 L 100,100", }, { description: "creates path with decimal coordinates", @@ -162,7 +162,7 @@ describe("createPathFromWayPoints helper function", () => { targetX: 100.75, targetY: 200.5, wayPoints: [{ x: 50.5, y: 75.25 }], - expected: "M 0.5,10.25 L 50.5,75.25 L 100.75,200.5", + expected: "M 0.5,10.25 L 50.5,10.25 L 50.5,75.25 L 100.75,75.25 L 100.75,200.5", }, { description: "handles complex path with many waypoints", @@ -178,7 +178,8 @@ describe("createPathFromWayPoints helper function", () => { { x: 80, y: 40 }, { x: 90, y: 90 }, ], - expected: "M 0,0 L 10,10 L 20,30 L 40,20 L 60,50 L 80,40 L 90,90 L 100,100", + expected: + "M 0,0 L 10,0 L 10,10 L 20,10 L 20,30 L 40,30 L 40,20 L 60,20 L 60,50 L 80,50 L 80,40 L 90,40 L 90,90 L 100,90 L 100,100", }, { description: "preserves coordinate precision", @@ -190,7 +191,8 @@ describe("createPathFromWayPoints helper function", () => { { x: 33.333333, y: 66.666666 }, { x: 77.777777, y: 88.888888 }, ], - expected: "M 0.1,0.2 L 33.333333,66.666666 L 77.777777,88.888888 L 99.9,99.8", + expected: + "M 0.1,0.2 L 33.333333,0.2 L 33.333333,66.666666 L 77.777777,66.666666 L 77.777777,88.888888 L 99.9,88.888888 L 99.9,99.8", }, ])("$description", ({ sourceX, sourceY, targetX, targetY, wayPoints, expected }) => { const path = createPathFromWayPoints(sourceX, sourceY, targetX, targetY, wayPoints); diff --git a/packages/serverless-workflow-diagram-editor/tests/react-flow/edges/__snapshots__/Edges.test.tsx.snap b/packages/serverless-workflow-diagram-editor/tests/react-flow/edges/__snapshots__/Edges.test.tsx.snap index 0cdecd81..f1771fa6 100644 --- a/packages/serverless-workflow-diagram-editor/tests/react-flow/edges/__snapshots__/Edges.test.tsx.snap +++ b/packages/serverless-workflow-diagram-editor/tests/react-flow/edges/__snapshots__/Edges.test.tsx.snap @@ -3,7 +3,7 @@ exports[`React Flow custom edge types > matches snapshot with waypoints 1`] = ` Date: Tue, 16 Jun 2026 10:29:21 -0400 Subject: [PATCH 2/6] Remove unused import Signed-off-by: handreyrc --- .../tests/side-panel/NodeDetailsView.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/serverless-workflow-diagram-editor/tests/side-panel/NodeDetailsView.test.tsx b/packages/serverless-workflow-diagram-editor/tests/side-panel/NodeDetailsView.test.tsx index 72f61777..13aa17c1 100644 --- a/packages/serverless-workflow-diagram-editor/tests/side-panel/NodeDetailsView.test.tsx +++ b/packages/serverless-workflow-diagram-editor/tests/side-panel/NodeDetailsView.test.tsx @@ -17,7 +17,6 @@ import { describe, it, expect } from "vitest"; import { screen } from "@testing-library/react"; import type * as RF from "@xyflow/react"; -import yaml from "js-yaml"; import { NodeDetailsView } from "../../src/side-panel/NodeDetailsView"; import type { BaseNodeData } from "../../src/react-flow/nodes/Nodes"; import { renderWithProviders } from "../test-utils/render-helpers"; @@ -82,7 +81,8 @@ describe("NodeDetailsView", () => { expect(screen.getByRole("heading", { name: "Source" })).toBeInTheDocument(); expect(container.querySelector(".dec-sidebar-yaml-summary")?.textContent).toBe("View source"); expect(container.querySelector(".dec-sidebar-yaml-pre")?.textContent).toBe( - 'call: http\nwith:\n endpoint: https://api.example.com\n') + "call: http\nwith:\n endpoint: https://api.example.com\n", + ); }); it("renders node details message when the task has no task", () => { From da275fd35f2dc5734b29b3aa2055c8808211334c Mon Sep 17 00:00:00 2001 From: handreyrc Date: Tue, 16 Jun 2026 17:05:06 -0400 Subject: [PATCH 3/6] fix copilot complaints Signed-off-by: handreyrc --- .../src/core/graph.ts | 15 +- .../src/react-flow/diagram/autoLayout.ts | 168 +++++++++--------- .../src/react-flow/edges/Edges.tsx | 13 +- 3 files changed, 91 insertions(+), 105 deletions(-) diff --git a/packages/serverless-workflow-diagram-editor/src/core/graph.ts b/packages/serverless-workflow-diagram-editor/src/core/graph.ts index 6ded402b..39ceb5c8 100644 --- a/packages/serverless-workflow-diagram-editor/src/core/graph.ts +++ b/packages/serverless-workflow-diagram-editor/src/core/graph.ts @@ -121,9 +121,6 @@ export function fixNodesConnections(graph: FlatGraph): FlatGraph { sourceNode.parentId && isTargetOutsideSourceParent(sourceNode, targetNode) ) { - // Check if source is itself a parent node (has children) - const sourceIsParent = graph.nodes.some((n) => n.parentId === sourceNode.id); - // Find the topmost parent that the target is outside of let currentNode = sourceNode; let topmostParentId = sourceNode.parentId; @@ -146,16 +143,8 @@ export function fixNodesConnections(graph: FlatGraph): FlatGraph { } } - // Determine which exit node to use - let exitNodeToUse: string | undefined; - if (sourceIsParent) { - // If source is a parent node, it cannot connect to its own exit node - // Instead, connect to its parent's exit node (same level as the parent) - exitNodeToUse = parentToExitNode.get(sourceNode.parentId); - } else { - // If source is a regular task, use its immediate parent's exit node - exitNodeToUse = parentToExitNode.get(sourceNode.parentId); - } + // Use the immediate parent's exit node + const exitNodeToUse = parentToExitNode.get(sourceNode.parentId); if (exitNodeToUse) { // Redirect the edge to the appropriate exit node diff --git a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts index 2fb3e601..7cb56d89 100644 --- a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts +++ b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts @@ -176,14 +176,13 @@ export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): const reactFlowNodeMap = new Map(reactFlowGraph.nodes.map((node) => [node.id, node])); // Track which nodes are sources of edges (to add ports) - const nodeOutgoingEdges = new Map(); + const nodeOutgoingEdges = new Set(); reactFlowGraph.edges.forEach((edge) => { - const count = nodeOutgoingEdges.get(edge.source) || 0; - nodeOutgoingEdges.set(edge.source, count + 1); + nodeOutgoingEdges.add(edge.source); }); // Add ports to parent nodes that have outgoing edges - nodeOutgoingEdges.forEach((count, nodeId) => { + nodeOutgoingEdges.forEach((nodeId) => { const elkNode = nodeMap.get(nodeId); const reactFlowNode = reactFlowNodeMap.get(nodeId); @@ -241,56 +240,65 @@ export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): }; } -// Helper function to recursively build a flat map of all ELK nodes -function buildElkNodeMap( - elkNode: ElkNode, - map: Map = new Map(), -): Map { - map.set(elkNode.id, elkNode); - if (elkNode.children) { - for (const child of elkNode.children) { - buildElkNodeMap(child, map); - } - } - return map; +/** + * Precomputed maps for O(1) parent lookups + */ +interface ElkMaps { + nodeMap: Map; + edgeMap: Map; + nodeParentMap: Map; // nodeId -> parentNodeId + edgeParentMap: Map; // edgeId -> parentNodeId } -// Helper function to recursively collect all edges from ELK graph -function buildElkEdgeMap( - elkNode: ElkNode, - map: Map = new Map(), -): Map { - if (elkNode.edges) { - for (const edge of elkNode.edges) { - map.set(edge.id, edge); +/** + * Build all maps in a single traversal for O(1) lookups. + * This replaces multiple O(N) scans with a single O(N) traversal. + */ +function buildElkMaps(elkNode: ElkNode, parentId: string | null = null): ElkMaps { + const nodeMap = new Map(); + const edgeMap = new Map(); + const nodeParentMap = new Map(); + const edgeParentMap = new Map(); + + function traverse(node: ElkNode, parentNodeId: string | null) { + // Add node to map + nodeMap.set(node.id, node); + + // Record parent relationship (skip root) + if (parentNodeId !== null) { + nodeParentMap.set(node.id, parentNodeId); } - } - if (elkNode.children) { - for (const child of elkNode.children) { - buildElkEdgeMap(child, map); + + // Process edges at this level + if (node.edges) { + for (const edge of node.edges) { + edgeMap.set(edge.id, edge); + edgeParentMap.set(edge.id, node.id); + } } - } - return map; -} -// Helper function to find the parent node containing an edge -function findEdgeParent(elkNode: ElkNode, edgeId: string): ElkNode | null { - if (elkNode.edges?.some((e) => e.id === edgeId)) { - return elkNode; - } - if (elkNode.children) { - for (const child of elkNode.children) { - const parent = findEdgeParent(child, edgeId); - if (parent) { - return parent; + // Recursively process children + if (node.children) { + for (const child of node.children) { + traverse(child, node.id); } } } - return null; + + traverse(elkNode, parentId); + + return { nodeMap, edgeMap, nodeParentMap, edgeParentMap }; } -// Helper function to calculate absolute position of a node -function getAbsolutePosition(nodeId: string, elkNodeMap: Map): Point { +/** + * Calculate absolute position of a node using precomputed parent map. + * Time complexity: O(depth) instead of O(N * depth) + */ +function getAbsolutePosition( + nodeId: string, + elkNodeMap: Map, + nodeParentMap: Map, +): Point { const node = elkNodeMap.get(nodeId); if (!node || node.x === undefined || node.y === undefined) { return { x: 0, y: 0 }; @@ -299,18 +307,14 @@ function getAbsolutePosition(nodeId: string, elkNodeMap: Map): let absoluteX = node.x; let absoluteY = node.y; - // Traverse up the hierarchy to accumulate parent positions - let currentNode = node; - while (currentNode) { - const parentId = findParentId(currentNode.id, elkNodeMap); - if (!parentId || parentId === "root") { - break; - } - const parent = elkNodeMap.get(parentId); + // Traverse up the hierarchy using precomputed parent map + let currentId: string | undefined = nodeParentMap.get(nodeId); + while (currentId && currentId !== "root") { + const parent = elkNodeMap.get(currentId); if (parent && parent.x !== undefined && parent.y !== undefined) { absoluteX += parent.x; absoluteY += parent.y; - currentNode = parent; + currentId = nodeParentMap.get(currentId); } else { break; } @@ -319,23 +323,17 @@ function getAbsolutePosition(nodeId: string, elkNodeMap: Map): return { x: absoluteX, y: absoluteY }; } -// Helper function to find parent ID of a node -function findParentId(nodeId: string, elkNodeMap: Map): string | null { - for (const [id, node] of elkNodeMap.entries()) { - if (node.children?.some((child) => child.id === nodeId)) { - return id; - } - } - return null; -} - export function matchReactFlowGraphWithElkLayoutedGraph( graph: ReactFlowGraph, layoutedGraph: ElkNode, ): ReactFlowGraph { - // Build flat maps for O(1) lookups - const elkNodeMap = buildElkNodeMap(layoutedGraph); - const elkEdgeMap = buildElkEdgeMap(layoutedGraph); + // Build all maps in a single traversal for O(1) lookups + const { + nodeMap: elkNodeMap, + edgeMap: elkEdgeMap, + nodeParentMap, + edgeParentMap, + } = buildElkMaps(layoutedGraph); // Map node positions const layoutedNodes = graph.nodes.map((node) => { @@ -362,31 +360,33 @@ export function matchReactFlowGraphWithElkLayoutedGraph( // This avoids mixing React Flow anchor coordinates with ELK bend points, // which is especially problematic for edges inside parent nodes. const sectionPoints = - elkEdge.sections?.flatMap((section) => { - const points = []; - if (section.startPoint) { - points.push(section.startPoint); - } - if (section.bendPoints) { - points.push(...section.bendPoints); - } - if (section.endPoint) { - points.push(section.endPoint); - } - return points; - }) || []; + elkEdge.sections?.flatMap( + (section: { startPoint?: Point; bendPoints?: Point[]; endPoint?: Point }) => { + const points: Point[] = []; + if (section.startPoint) { + points.push(section.startPoint); + } + if (section.bendPoints) { + points.push(...section.bendPoints); + } + if (section.endPoint) { + points.push(section.endPoint); + } + return points; + }, + ) || []; const newData = { ...restData }; if (sectionPoints.length >= 2) { - // Find the parent node containing this edge - const edgeParent = findEdgeParent(layoutedGraph, edge.id); + // Use O(1) lookup to find the parent node containing this edge + const edgeParentId = edgeParentMap.get(edge.id); // If edge is inside a parent node (not at root level), convert coordinates to absolute - if (edgeParent && edgeParent.id !== "root") { - const parentAbsolutePos = getAbsolutePosition(edgeParent.id, elkNodeMap); + if (edgeParentId && edgeParentId !== "root") { + const parentAbsolutePos = getAbsolutePosition(edgeParentId, elkNodeMap, nodeParentMap); // Convert all waypoints from parent-relative to absolute coordinates - const absoluteWayPoints = sectionPoints.slice(1, -1).map((point) => ({ + const absoluteWayPoints = sectionPoints.slice(1, -1).map((point: Point) => ({ x: point.x + parentAbsolutePos.x, y: point.y + parentAbsolutePos.y, })); diff --git a/packages/serverless-workflow-diagram-editor/src/react-flow/edges/Edges.tsx b/packages/serverless-workflow-diagram-editor/src/react-flow/edges/Edges.tsx index 912ce718..1bc31ac2 100644 --- a/packages/serverless-workflow-diagram-editor/src/react-flow/edges/Edges.tsx +++ b/packages/serverless-workflow-diagram-editor/src/react-flow/edges/Edges.tsx @@ -53,16 +53,13 @@ export function createPathFromWayPoints( wayPoints?: WayPoints, ): string { const points = [{ x: sourceX, y: sourceY }, ...(wayPoints || []), { x: targetX, y: targetY }]; - const [firstPoint, ...remainingPoints] = points; - if (!firstPoint || remainingPoints.length === 0) { - return `M ${sourceX},${sourceY} L ${targetX},${targetY}`; - } - - let path = `M ${firstPoint.x},${firstPoint.y}`; - let previous = firstPoint; + // points always contains at least source and target, so points[0] is guaranteed + let path = `M ${points[0]!.x},${points[0]!.y}`; + let previous = points[0]!; - for (const current of remainingPoints) { + for (let i = 1; i < points.length; i++) { + const current = points[i]!; if (previous.x !== current.x && previous.y !== current.y) { path += ` L ${current.x},${previous.y}`; } From 5fa2a42b7f7137aab0d5436a30982ea2ce1ad6d3 Mon Sep 17 00:00:00 2001 From: handreyrc Date: Wed, 17 Jun 2026 10:07:50 -0400 Subject: [PATCH 4/6] fix review comments Signed-off-by: handreyrc --- .../src/core/graph.ts | 69 ++-- .../src/react-flow/diagram/autoLayout.ts | 57 +-- .../tests/core/graph.test.ts | 334 +++++++++++++++++- 3 files changed, 397 insertions(+), 63 deletions(-) diff --git a/packages/serverless-workflow-diagram-editor/src/core/graph.ts b/packages/serverless-workflow-diagram-editor/src/core/graph.ts index 39ceb5c8..95c0d125 100644 --- a/packages/serverless-workflow-diagram-editor/src/core/graph.ts +++ b/packages/serverless-workflow-diagram-editor/src/core/graph.ts @@ -20,6 +20,39 @@ export function getNodesByType(graph: FlatGraph, type: GraphNodeType): FlatGraph return graph.nodes.filter((node) => node.type === type); } +// Helper function to check if target is outside source's parent hierarchy +export function isTargetOutsideSourceParent( + sourceNode: FlatGraphNode, + targetNode: FlatGraphNode, + nodeMap: Map, +): boolean { + if (!sourceNode.parentId) { + return false; + } + + // Check if target is the source's parent itself + if (targetNode.id === sourceNode.parentId) { + return false; + } + + // Check if target shares the same parent + if (targetNode.parentId === sourceNode.parentId) { + return false; + } + + // Check if target is inside source's parent hierarchy + let currentParentId: string | undefined = targetNode.parentId; + while (currentParentId) { + if (currentParentId === sourceNode.parentId) { + return false; + } + const parentNode = nodeMap.get(currentParentId); + currentParentId = parentNode?.parentId; + } + + return true; +} + // Inner entry and exit nodes cannot be connected external nodes so connections shall be moved to parent node export function fixNodesConnections(graph: FlatGraph): FlatGraph { const entryNodes = getNodesByType(graph, GraphNodeType.Entry); @@ -54,38 +87,6 @@ export function fixNodesConnections(graph: FlatGraph): FlatGraph { } }); - // Helper function to check if target is outside source's parent hierarchy - const isTargetOutsideSourceParent = ( - sourceNode: FlatGraphNode, - targetNode: FlatGraphNode, - ): boolean => { - if (!sourceNode.parentId) { - return false; - } - - // Check if target is the source's parent itself - if (targetNode.id === sourceNode.parentId) { - return false; - } - - // Check if target shares the same parent - if (targetNode.parentId === sourceNode.parentId) { - return false; - } - - // Check if target is inside source's parent hierarchy - let currentParentId: string | undefined = targetNode.parentId; - while (currentParentId) { - if (currentParentId === sourceNode.parentId) { - return false; - } - const parentNode = nodeMap.get(currentParentId); - currentParentId = parentNode?.parentId; - } - - return true; - }; - const graphClone = structuredClone(graph); const newEdges: typeof graphClone.edges = []; @@ -119,7 +120,7 @@ export function fixNodesConnections(graph: FlatGraph): FlatGraph { sourceNode && targetNode && sourceNode.parentId && - isTargetOutsideSourceParent(sourceNode, targetNode) + isTargetOutsideSourceParent(sourceNode, targetNode, nodeMap) ) { // Find the topmost parent that the target is outside of let currentNode = sourceNode; @@ -131,7 +132,7 @@ export function fixNodesConnections(graph: FlatGraph): FlatGraph { if (!parentNode) break; // Check if target is outside this parent - if (parentNode.parentId && isTargetOutsideSourceParent(parentNode, targetNode)) { + if (parentNode.parentId && isTargetOutsideSourceParent(parentNode, targetNode, nodeMap)) { // Target is also outside this parent's parent, keep going up topmostParentId = parentNode.parentId; currentNode = parentNode; diff --git a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts index 7cb56d89..d2badbc2 100644 --- a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts +++ b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts @@ -254,40 +254,41 @@ interface ElkMaps { * Build all maps in a single traversal for O(1) lookups. * This replaces multiple O(N) scans with a single O(N) traversal. */ -function buildElkMaps(elkNode: ElkNode, parentId: string | null = null): ElkMaps { - const nodeMap = new Map(); - const edgeMap = new Map(); - const nodeParentMap = new Map(); - const edgeParentMap = new Map(); - - function traverse(node: ElkNode, parentNodeId: string | null) { - // Add node to map - nodeMap.set(node.id, node); - - // Record parent relationship (skip root) - if (parentNodeId !== null) { - nodeParentMap.set(node.id, parentNodeId); - } +function buildElkMaps(elkNode: ElkNode, parentId: string | null = null, maps?: ElkMaps): ElkMaps { + // Initialize maps on first call + if (!maps) { + maps = { + nodeMap: new Map(), + edgeMap: new Map(), + nodeParentMap: new Map(), + edgeParentMap: new Map(), + }; + } - // Process edges at this level - if (node.edges) { - for (const edge of node.edges) { - edgeMap.set(edge.id, edge); - edgeParentMap.set(edge.id, node.id); - } - } + // Add node to map + maps.nodeMap.set(elkNode.id, elkNode); - // Recursively process children - if (node.children) { - for (const child of node.children) { - traverse(child, node.id); - } + // Record parent relationship (skip root) + if (parentId !== null) { + maps.nodeParentMap.set(elkNode.id, parentId); + } + + // Process edges at this level + if (elkNode.edges) { + for (const edge of elkNode.edges) { + maps.edgeMap.set(edge.id, edge); + maps.edgeParentMap.set(edge.id, elkNode.id); } } - traverse(elkNode, parentId); + // Recursively process children + if (elkNode.children) { + for (const child of elkNode.children) { + buildElkMaps(child, elkNode.id, maps); + } + } - return { nodeMap, edgeMap, nodeParentMap, edgeParentMap }; + return maps; } /** diff --git a/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts b/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts index e80c731d..9159c2ec 100644 --- a/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts +++ b/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts @@ -16,7 +16,11 @@ import { describe, it, expect } from "vitest"; import { FlatGraphNode, GraphNodeType } from "@serverlessworkflow/sdk"; -import { getNodesByType, fixNodesConnections } from "../../src/core/graph"; +import { + getNodesByType, + fixNodesConnections, + isTargetOutsideSourceParent, +} from "../../src/core/graph"; import { createFlatGraph } from "../test-utils/graph-helpers"; describe("graph utils", () => { @@ -76,6 +80,334 @@ describe("graph utils", () => { }); }); + describe("isTargetOutsideSourceParent", () => { + describe("returns false", () => { + it.each([ + { + description: "when source node has no parent", + setup: () => { + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + } as FlatGraphNode; + const nodeMap = new Map([ + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + { + description: "when target is the source's parent itself", + setup: () => { + const parentNode: FlatGraphNode = { + id: "parent", + type: GraphNodeType.Do, + } as FlatGraphNode; + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "parent", + } as FlatGraphNode; + const nodeMap = new Map([ + ["parent", parentNode], + ["source", sourceNode], + ]); + return { sourceNode, targetNode: parentNode, nodeMap }; + }, + }, + { + description: "when target shares the same parent as source", + setup: () => { + const parentNode: FlatGraphNode = { + id: "parent", + type: GraphNodeType.Do, + } as FlatGraphNode; + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "parent", + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + parentId: "parent", + } as FlatGraphNode; + const nodeMap = new Map([ + ["parent", parentNode], + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + { + description: "when target is inside source's parent hierarchy (direct child)", + setup: () => { + const grandparentNode: FlatGraphNode = { + id: "grandparent", + type: GraphNodeType.Do, + } as FlatGraphNode; + const parentNode: FlatGraphNode = { + id: "parent", + type: GraphNodeType.Do, + parentId: "grandparent", + } as FlatGraphNode; + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "grandparent", + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + parentId: "parent", + } as FlatGraphNode; + const nodeMap = new Map([ + ["grandparent", grandparentNode], + ["parent", parentNode], + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + { + description: "when target is inside source's parent hierarchy (nested multiple levels)", + setup: () => { + const rootNode: FlatGraphNode = { + id: "root", + type: GraphNodeType.Do, + } as FlatGraphNode; + const level1Node: FlatGraphNode = { + id: "level1", + type: GraphNodeType.Do, + parentId: "root", + } as FlatGraphNode; + const level2Node: FlatGraphNode = { + id: "level2", + type: GraphNodeType.Do, + parentId: "level1", + } as FlatGraphNode; + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "root", + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + parentId: "level2", + } as FlatGraphNode; + const nodeMap = new Map([ + ["root", rootNode], + ["level1", level1Node], + ["level2", level2Node], + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + ])("$description", ({ setup }) => { + const { sourceNode, targetNode, nodeMap } = setup(); + const result = isTargetOutsideSourceParent(sourceNode, targetNode, nodeMap); + expect(result).toBe(false); + }); + }); + + describe("returns true", () => { + it.each([ + { + description: "when target is completely outside source's parent hierarchy", + setup: () => { + const parentNode: FlatGraphNode = { + id: "parent", + type: GraphNodeType.Do, + } as FlatGraphNode; + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "parent", + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + } as FlatGraphNode; + const nodeMap = new Map([ + ["parent", parentNode], + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + { + description: "when target is in a different parent hierarchy", + setup: () => { + const parent1Node: FlatGraphNode = { + id: "parent1", + type: GraphNodeType.Do, + } as FlatGraphNode; + const parent2Node: FlatGraphNode = { + id: "parent2", + type: GraphNodeType.Do, + } as FlatGraphNode; + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "parent1", + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + parentId: "parent2", + } as FlatGraphNode; + const nodeMap = new Map([ + ["parent1", parent1Node], + ["parent2", parent2Node], + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + { + description: "when target is at root level and source is nested", + setup: () => { + const parentNode: FlatGraphNode = { + id: "parent", + type: GraphNodeType.Do, + } as FlatGraphNode; + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "parent", + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + } as FlatGraphNode; + const nodeMap = new Map([ + ["parent", parentNode], + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + { + description: "when target is in a sibling branch of parent hierarchy", + setup: () => { + const rootNode: FlatGraphNode = { + id: "root", + type: GraphNodeType.Do, + } as FlatGraphNode; + const branch1Node: FlatGraphNode = { + id: "branch1", + type: GraphNodeType.Do, + parentId: "root", + } as FlatGraphNode; + const branch2Node: FlatGraphNode = { + id: "branch2", + type: GraphNodeType.Do, + parentId: "root", + } as FlatGraphNode; + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "branch1", + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + parentId: "branch2", + } as FlatGraphNode; + const nodeMap = new Map([ + ["root", rootNode], + ["branch1", branch1Node], + ["branch2", branch2Node], + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + { + description: "when parent node is missing in nodeMap", + setup: () => { + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "parent", + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + parentId: "missing-parent", + } as FlatGraphNode; + const nodeMap = new Map([ + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + { + description: "when deeply nested source with target at intermediate level outside", + setup: () => { + const level0Node: FlatGraphNode = { + id: "level0", + type: GraphNodeType.Do, + } as FlatGraphNode; + const level1Node: FlatGraphNode = { + id: "level1", + type: GraphNodeType.Do, + parentId: "level0", + } as FlatGraphNode; + const level2Node: FlatGraphNode = { + id: "level2", + type: GraphNodeType.Do, + parentId: "level1", + } as FlatGraphNode; + const level3Node: FlatGraphNode = { + id: "level3", + type: GraphNodeType.Do, + parentId: "level2", + } as FlatGraphNode; + const sourceNode: FlatGraphNode = { + id: "source", + type: GraphNodeType.Call, + parentId: "level3", + } as FlatGraphNode; + const targetNode: FlatGraphNode = { + id: "target", + type: GraphNodeType.Call, + parentId: "level1", + } as FlatGraphNode; + const nodeMap = new Map([ + ["level0", level0Node], + ["level1", level1Node], + ["level2", level2Node], + ["level3", level3Node], + ["source", sourceNode], + ["target", targetNode], + ]); + return { sourceNode, targetNode, nodeMap }; + }, + }, + ])("$description", ({ setup }) => { + const { sourceNode, targetNode, nodeMap } = setup(); + const result = isTargetOutsideSourceParent(sourceNode, targetNode, nodeMap); + expect(result).toBe(true); + }); + }); + }); + describe("fixNodesConnections", () => { it("moves entry node incoming connections to parent node", () => { const parentNode: FlatGraphNode = { id: "parent-1", type: GraphNodeType.Do } as FlatGraphNode; From 5c2009f5d7182d73db99fb404c98eef26365cac0 Mon Sep 17 00:00:00 2001 From: handreyrc Date: Wed, 17 Jun 2026 10:46:12 -0400 Subject: [PATCH 5/6] fix copilot complaints Signed-off-by: handreyrc --- .changeset/edges-routing.md | 5 ++++ .../src/core/graph.ts | 24 ++++++------------- .../src/react-flow/diagram/autoLayout.ts | 6 ++++- .../tests/core/graph.test.ts | 12 ++++++---- .../diagram/autoLayout.integration.test.ts | 5 ++-- 5 files changed, 28 insertions(+), 24 deletions(-) create mode 100644 .changeset/edges-routing.md diff --git a/.changeset/edges-routing.md b/.changeset/edges-routing.md new file mode 100644 index 00000000..98c51c36 --- /dev/null +++ b/.changeset/edges-routing.md @@ -0,0 +1,5 @@ +--- +"@serverlessworkflow/diagram-editor": minor +--- + +Apply auto-layout calculated waypoints to edges defined into parent nodes. Add explicit north / south fixed ports to nodes defined in ELK graph. diff --git a/packages/serverless-workflow-diagram-editor/src/core/graph.ts b/packages/serverless-workflow-diagram-editor/src/core/graph.ts index 95c0d125..6912f271 100644 --- a/packages/serverless-workflow-diagram-editor/src/core/graph.ts +++ b/packages/serverless-workflow-diagram-editor/src/core/graph.ts @@ -90,14 +90,6 @@ export function fixNodesConnections(graph: FlatGraph): FlatGraph { const graphClone = structuredClone(graph); const newEdges: typeof graphClone.edges = []; - // Helper function to check if an edge already exists - const edgeExists = (sourceId: string, targetId: string): boolean => { - return ( - graphClone.edges.some((e) => e.sourceId === sourceId && e.targetId === targetId) || - newEdges.some((e) => e.sourceId === sourceId && e.targetId === targetId) - ); - }; - // Single pass over edges to rewrite sourceId/targetId graphClone.edges.forEach((edge) => { // Move entry node incoming connections to parent @@ -152,15 +144,13 @@ export function fixNodesConnections(graph: FlatGraph): FlatGraph { edge.targetId = exitNodeToUse; // Create a new edge from the TOPMOST parent to the original target - // Only if an edge with the same source and target doesn't already exist - if (!edgeExists(topmostParentId, targetNode.id)) { - newEdges.push({ - id: `${edge.id}-redirected`, - sourceId: topmostParentId, - targetId: targetNode.id, - label: edge.label || "", - }); - } + // All edges are preserved to maintain complete connection information + newEdges.push({ + id: `${edge.id}-redirected`, + sourceId: topmostParentId, + targetId: targetNode.id, + label: edge.label || "", + }); } } }); diff --git a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts index d2badbc2..8caa43af 100644 --- a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts +++ b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts @@ -378,6 +378,8 @@ export function matchReactFlowGraphWithElkLayoutedGraph( ) || []; const newData = { ...restData }; + // Normalize wayPoints: always use empty array when ELK edge exists but has no intermediate points. + // Reserve undefined only for "no ELK edge found" case (handled by returning edge unchanged at line 411). if (sectionPoints.length >= 2) { // Use O(1) lookup to find the parent node containing this edge const edgeParentId = edgeParentMap.get(edge.id); @@ -400,7 +402,9 @@ export function matchReactFlowGraphWithElkLayoutedGraph( newData.wayPoints = sectionPoints.slice(1, -1); } } else { - newData.wayPoints = undefined; + // ELK edge exists but has fewer than 2 points (no intermediate points) + // Use empty array to indicate "no bend points" rather than undefined + newData.wayPoints = []; } return { diff --git a/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts b/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts index 9159c2ec..b5cd3ec5 100644 --- a/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts +++ b/packages/serverless-workflow-diagram-editor/tests/core/graph.test.ts @@ -955,11 +955,11 @@ describe("graph utils", () => { expect(fixedGraph.edges[1]?.targetId).toBe("end"); }); - it("does not create duplicate edges when connection already exists", () => { + it("creates separate edges for each child connection to preserve all edge information", () => { // Scenario: parent1 contains taskChild1 and taskChild2 // taskChild1 has an edge to taskOutside // taskChild2 also has an edge to taskOutside - // Expected: only one edge from parent1 to taskOutside should be created + // Expected: TWO edges from parent1 to taskOutside (one for each child connection) const parent1: FlatGraphNode = { id: "parent-1", type: GraphNodeType.Do, @@ -1000,10 +1000,14 @@ describe("graph utils", () => { expect(fixedGraph.edges[1]?.sourceId).toBe("task-child-2"); expect(fixedGraph.edges[1]?.targetId).toBe("exit-1"); - // Only ONE new edge from parent1 to taskOutside should be created (no duplicate) - expect(fixedGraph.edges).toHaveLength(3); + // TWO new edges from parent1 to taskOutside should be created (one for each original edge) + expect(fixedGraph.edges).toHaveLength(4); expect(fixedGraph.edges[2]?.sourceId).toBe("parent-1"); expect(fixedGraph.edges[2]?.targetId).toBe("task-outside"); + expect(fixedGraph.edges[2]?.id).toBe("edge-1-redirected"); + expect(fixedGraph.edges[3]?.sourceId).toBe("parent-1"); + expect(fixedGraph.edges[3]?.targetId).toBe("task-outside"); + expect(fixedGraph.edges[3]?.id).toBe("edge-2-redirected"); }); }); }); diff --git a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts index 6459e46e..f06f338b 100644 --- a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts +++ b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts @@ -547,8 +547,9 @@ describe("autoLayout", () => { const result = matchReactFlowGraphWithElkLayoutedGraph(reactFlowGraph, layoutedElkGraph); - expect(result.edges[0].data).toEqual({ label: "Test Edge" }); - expect(result.edges[0].data?.wayPoints).toBeUndefined(); + // When ELK edge exists but has no sections, wayPoints is normalized to empty array + expect(result.edges[0].data).toEqual({ label: "Test Edge", wayPoints: [] }); + expect(result.edges[0].data?.wayPoints).toEqual([]); }); it("clears stale wayPoints when ELK edge sections have no bend points", () => { From 52df137d7ac061dae3a594639e6c7d1ebf6983c4 Mon Sep 17 00:00:00 2001 From: handreyrc Date: Wed, 17 Jun 2026 14:24:35 -0400 Subject: [PATCH 6/6] fix review comments Signed-off-by: handreyrc --- .../src/react-flow/diagram/autoLayout.ts | 10 +++++----- .../react-flow/diagram/autoLayout.integration.test.ts | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts index 8caa43af..5a10cb84 100644 --- a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts +++ b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/autoLayout.ts @@ -79,11 +79,11 @@ export const ROOT_LAYOUT_OPTIONS: LayoutOptions = { "layered.nodePlacement.bk.coolFactor": "0.005", "elk.alignment": "TOP", // spacing - "spacing.edgeNode": "44", - "spacing.edgeEdge": "44", - "spacing.componentComponent": "100", - "spacing.nodeNodeBetweenLayers": "100", - "spacing.edgeNodeBetweenLayers": "50", + "spacing.edgeNode": "24", + "spacing.edgeEdge": "24", + "spacing.componentComponent": "70", + "spacing.nodeNodeBetweenLayers": "70", + "spacing.edgeNodeBetweenLayers": "40", }; export const PARENT_LAYOUT_OPTIONS: LayoutOptions = { diff --git a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts index f06f338b..45205a44 100644 --- a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts +++ b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/autoLayout.integration.test.ts @@ -1215,9 +1215,9 @@ describe("autoLayout", () => { }); it("has proper spacing configuration", () => { - expect(ROOT_LAYOUT_OPTIONS["spacing.edgeNode"]).toBe("44"); - expect(ROOT_LAYOUT_OPTIONS["spacing.componentComponent"]).toBe("100"); - expect(ROOT_LAYOUT_OPTIONS["spacing.nodeNodeBetweenLayers"]).toBe("100"); + expect(ROOT_LAYOUT_OPTIONS["spacing.edgeNode"]).toBe("24"); + expect(ROOT_LAYOUT_OPTIONS["spacing.componentComponent"]).toBe("70"); + expect(ROOT_LAYOUT_OPTIONS["spacing.nodeNodeBetweenLayers"]).toBe("70"); }); });