From 489e3aa11036b9aab48b5bfc23727affb51daf86 Mon Sep 17 00:00:00 2001 From: handreyrc Date: Mon, 25 May 2026 18:03:24 -0400 Subject: [PATCH 1/6] add entry and exit node types Signed-off-by: handreyrc --- .../src/react-flow/nodes/Nodes.tsx | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/serverless-workflow-diagram-editor/src/react-flow/nodes/Nodes.tsx b/packages/serverless-workflow-diagram-editor/src/react-flow/nodes/Nodes.tsx index dafeadf..31ae80e 100644 --- a/packages/serverless-workflow-diagram-editor/src/react-flow/nodes/Nodes.tsx +++ b/packages/serverless-workflow-diagram-editor/src/react-flow/nodes/Nodes.tsx @@ -27,6 +27,8 @@ export const CATCH_CONTAINER_NODE_TYPE = "catch-container"; export const ReactFlowNodeTypes: RF.NodeTypes = { [GraphNodeType.Start]: StartNode, [GraphNodeType.End]: EndNode, + [GraphNodeType.Entry]: EntryNode, + [GraphNodeType.Exit]: ExitNode, [GraphNodeType.Call]: CallNode, [GraphNodeType.Do]: DoNode, [GraphNodeType.Emit]: EmitNode, @@ -158,6 +160,20 @@ export function EndNode({ id, data, selected, type }: RF.NodeProps) return ; } +/* entry node */ +export type EntryNodeType = RF.Node; +export function EntryNode({ id, data, selected, type }: RF.NodeProps) { + // TODO: This component is just a placeholder + return ; +} + +/* exit node */ +export type ExitNodeType = RF.Node; +export function ExitNode({ id, data, selected, type }: RF.NodeProps) { + // TODO: This component is just a placeholder + return ; +} + /* call leaf node */ export type CallNodeType = RF.Node, typeof GraphNodeType.Call>; export function CallNode({ id, data, selected, type }: RF.NodeProps) { From a6c70645b3b23c7b7f6acc235d85cff729d1ecf0 Mon Sep 17 00:00:00 2001 From: handreyrc Date: Wed, 27 May 2026 22:19:47 -0400 Subject: [PATCH 2/6] Add containment support integrated with auto-layout Signed-off-by: handreyrc --- .../src/react-flow/diagram/autoLayout.ts | 190 +++++++-- .../src/react-flow/diagram/diagramBuilder.ts | 10 +- .../tests-e2e/diagram-editor.spec.ts | 4 +- .../diagram/autoLayout.integration.test.ts | 327 +++++++++++++++- .../react-flow/diagram/diagramBuilder.test.ts | 365 +++++++++++++++++- 5 files changed, 846 insertions(+), 50 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 ec01f12..194dd62 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 @@ -39,39 +39,41 @@ export type Size = { export type WayPoints = Point[]; export const ROOT_LAYOUT_OPTIONS: LayoutOptions = { - "elk.algorithm": "org.eclipse.elk.layered", - "elk.hierarchyHandling": "INCLUDE_CHILDREN", - "elk.direction": "DOWN", - "org.eclipse.elk.layered.layering.strategy": "INTERACTIVE", - "org.eclipse.elk.edgeRouting": "ORTHOGONAL", - "elk.layered.unnecessaryBendpoints": "true", + "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.cycleBreaking.strategy": "DEPTH_FIRST", - "org.eclipse.elk.insideSelfLoops.activate": "true", - "elk.separateConnectedComponents": "false", + "org.eclipse.elk.layered.nodePlacement.bk.edgeStraightening": "true", "org.eclipse.elk.layered.nodePlacement.favorStraightEdges": "true", - "org.eclipse.elk.layered.considerModelOrder.strategy": "EDGES", + "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", - "elk.layered.crossingMinimization.strategy": "INTERACTIVE", - spacing: "75", - "spacing.componentComponent": "70", - "spacing.nodeNodeBetweenLayers": "80", - "elk.layered.spacing.edgeNodeBetweenLayers": "40", - "org.eclipse.elk.spacing.edgeNode": "24", + "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": "80", + "org.eclipse.elk.layered.spacing.componentComponent": "70", "org.eclipse.elk.layered.mergeEdges": "true", }; +export const PARENT_LAYOUT_OPTIONS: LayoutOptions = { + ...ROOT_LAYOUT_OPTIONS, + "elk.padding": "[top=60,left=20,bottom=20,right=20]", +}; + export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): ElkNode { - // Create a map for easy lookup - const nodeMap = new Map( + // Create a map for easy lookup (without width/height initially) + const nodeMap = new Map( reactFlowGraph.nodes.map((node) => [ node.id, { id: node.id, - width: node.measured?.width ?? DEFAULT_NODE_SIZE.width, - height: node.measured?.height ?? DEFAULT_NODE_SIZE.height, children: [] as ElkNode[], + edges: [] as ElkExtendedEdge[], }, ]), ); @@ -81,24 +83,96 @@ export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): reactFlowGraph.nodes.forEach((node) => { const elkNode = nodeMap.get(node.id)!; if (node.parentId && nodeMap.has(node.parentId)) { - nodeMap.get(node.parentId)!.children.push(elkNode); + const parentNode = nodeMap.get(node.parentId)!; + if (!parentNode.children) { + parentNode.children = []; + } + parentNode.children.push(elkNode); } else { rootChildren.push(elkNode); } }); - // edges - const elkEdges: ElkExtendedEdge[] = reactFlowGraph.edges.map((edge) => ({ - id: edge.id, - sources: [edge.source], - targets: [edge.target], - })); + // Apply layout options and dimensions based on whether node has children + reactFlowGraph.nodes.forEach((node) => { + const elkNode = nodeMap.get(node.id)!; + if (elkNode.children && elkNode.children.length > 0) { + // Nodes with children get layout options but no fixed dimensions + elkNode.layoutOptions = PARENT_LAYOUT_OPTIONS; + } else { + // Leaf nodes get fixed dimensions + elkNode.width = node.measured?.width ?? DEFAULT_NODE_SIZE.width; + elkNode.height = node.measured?.height ?? DEFAULT_NODE_SIZE.height; + } + }); + + // Helper function to find the common ancestor of two nodes + const findCommonAncestor = (sourceId: string, targetId: string): string => { + // Build path from source to root + const sourcePath: string[] = []; + let currentId: string | undefined = sourceId; + while (currentId) { + sourcePath.push(currentId); + const node = reactFlowGraph.nodes.find((n) => n.id === currentId); + currentId = node?.parentId; + } + + // Traverse from target to root and find first common node + currentId = targetId; + while (currentId) { + if (sourcePath.includes(currentId)) { + return currentId; + } + const node = reactFlowGraph.nodes.find((n) => n.id === currentId); + currentId = node?.parentId; + } + + // If no common ancestor found, return "root" + return "root"; + }; + + // Nest edges in the appropriate hierarchy level + const rootEdges: ElkExtendedEdge[] = []; + reactFlowGraph.edges.forEach((edge) => { + const elkEdge: ElkExtendedEdge = { + id: edge.id, + sources: [edge.source], + targets: [edge.target], + }; + + // Find the lowest common ancestor that contains both source and target + const commonAncestor = findCommonAncestor(edge.source, edge.target); + + if (commonAncestor === "root") { + rootEdges.push(elkEdge); + } else { + const ancestorNode = nodeMap.get(commonAncestor); + if (ancestorNode) { + if (!ancestorNode.edges) { + ancestorNode.edges = []; + } + ancestorNode.edges.push(elkEdge); + } + } + }); + + // Clean up empty edges arrays from nodes that don't need them + const cleanupEmptyEdges = (node: ElkNode) => { + if (node.edges && node.edges.length === 0) { + delete node.edges; + } + if (node.children) { + node.children.forEach(cleanupEmptyEdges); + } + }; + + rootChildren.forEach(cleanupEmptyEdges); return { id: "root", layoutOptions: ROOT_LAYOUT_OPTIONS, children: rootChildren, - edges: elkEdges, + edges: rootEdges, }; } @@ -116,6 +190,40 @@ function buildElkNodeMap( return map; } +// 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); + } + } + if (elkNode.children) { + for (const child of elkNode.children) { + buildElkEdgeMap(child, map); + } + } + return map; +} + +// Helper function to check if an edge is inside a parent node +function isEdgeInsideParent( + edge: { source: string; target: string }, + graph: ReactFlowGraph, +): boolean { + const sourceNode = graph.nodes.find((n) => n.id === edge.source); + const targetNode = graph.nodes.find((n) => n.id === edge.target); + + // Edge is inside a parent if both source and target have the same parentId + return !!( + sourceNode?.parentId && + targetNode?.parentId && + sourceNode.parentId === targetNode.parentId + ); +} + // set export function matchReactFlowGraphWithElkLayoutedGraph( graph: ReactFlowGraph, @@ -123,7 +231,7 @@ export function matchReactFlowGraphWithElkLayoutedGraph( ): ReactFlowGraph { // Build flat maps for O(1) lookups const elkNodeMap = buildElkNodeMap(layoutedGraph); - const elkEdgeMap = new Map(layoutedGraph.edges?.map((e) => [e.id, e]) || []); + const elkEdgeMap = buildElkEdgeMap(layoutedGraph); // Map node positions const layoutedNodes = graph.nodes.map((node) => { @@ -143,15 +251,27 @@ export function matchReactFlowGraphWithElkLayoutedGraph( const layoutedEdges = graph.edges.map((edge) => { const elkEdge = elkEdgeMap.get(edge.id); if (elkEdge) { - // Reconstruct data without old wayPoints to avoid stale routing whenever ELK produced this edge. + // 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 + const newData = { ...restData }; + if (bendPoints.length > 0) { + // Check if edge is inside a parent node and apply offset + const isInsideParent = isEdgeInsideParent(edge, graph); + if (isInsideParent) { + // There is an incompatibity with the react flow library, the wayPoints are calculated correctly by ELK + // but the way react flow rendeer edges inside parent nodes cause path distotions. + newData.wayPoints = undefined; + } else { + newData.wayPoints = bendPoints; + } + } + return { ...edge, - data: { - ...restData, - ...(bendPoints.length > 0 && { wayPoints: bendPoints }), - }, + data: newData, }; } return edge; diff --git a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/diagramBuilder.ts b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/diagramBuilder.ts index 2847135..268d7d3 100644 --- a/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/diagramBuilder.ts +++ b/packages/serverless-workflow-diagram-editor/src/react-flow/diagram/diagramBuilder.ts @@ -92,6 +92,7 @@ function buildReactFlowNode( height: DEFAULT_NODE_SIZE.height, width: DEFAULT_NODE_SIZE.width, position: { x: 0, y: 0 }, + ...(graphNode.parentId !== "root" && { parentId: graphNode.parentId, extent: "parent" }), }; } @@ -121,12 +122,9 @@ export function buildDiagramElements(model: sdk.Specification.Workflow | null): const graph = buildFlatGraph(model); const catchContainerIds = getCatchContainerNodeIds(graph); - graph.nodes.forEach((graphNode) => { - // TODO: only nodes on root level are supported for now - if (graphNode.parentId === "root") { - nodes.push(buildReactFlowNode(graphNode, catchContainerIds)); - } - }); + graph.nodes.forEach((graphNode) => + nodes.push(buildReactFlowNode(graphNode, catchContainerIds)), + ); // Precompute node ID set for O(1) membership checks const nodeIdSet = new Set(nodes.map((node) => node.id)); diff --git a/packages/serverless-workflow-diagram-editor/tests-e2e/diagram-editor.spec.ts b/packages/serverless-workflow-diagram-editor/tests-e2e/diagram-editor.spec.ts index 371d080..d0a8fe4 100644 --- a/packages/serverless-workflow-diagram-editor/tests-e2e/diagram-editor.spec.ts +++ b/packages/serverless-workflow-diagram-editor/tests-e2e/diagram-editor.spec.ts @@ -27,9 +27,9 @@ test("diagram editor renders correctly", async ({ page }) => { // Check total nodes const nodes = page.locator('[data-testid^="rf__node-"]'); - await expect(nodes).toHaveCount(6); + await expect(nodes).toHaveCount(9); // Check total edge const edges = page.locator('[data-testid^="rf__edge-"]'); - await expect(edges).toHaveCount(5); + await expect(edges).toHaveCount(7); }); 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 a00b4db..ff1949a 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 @@ -23,6 +23,7 @@ import { applyAutoLayout, DEFAULT_NODE_SIZE, ROOT_LAYOUT_OPTIONS, + PARENT_LAYOUT_OPTIONS, } from "../../../src/react-flow/diagram/autoLayout"; import type { ReactFlowGraph } from "../../../src/react-flow/diagram/diagramBuilder"; import * as core from "../../../src/core"; @@ -188,6 +189,129 @@ describe("autoLayout", () => { expect(elkGraph.children).toHaveLength(1); expect(elkGraph.children?.[0].id).toBe("node1"); }); + + it("applies PARENT_LAYOUT_OPTIONS to nodes with children", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { + id: "parent", + position: { x: 0, y: 0 }, + data: {}, + measured: { width: 300, height: 200 }, + }, + { + id: "child", + position: { x: 10, y: 10 }, + data: {}, + parentId: "parent", + measured: { width: 100, height: 50 }, + }, + ] as Node[], + edges: [], + }; + + const elkGraph = buildElkGraphFromReactFlowGraph(reactFlowGraph); + + // Parent node should have layout options and no fixed dimensions + expect(elkGraph.children?.[0].layoutOptions).toBeDefined(); + expect(elkGraph.children?.[0].layoutOptions?.["elk.padding"]).toBe( + "[top=60,left=20,bottom=20,right=20]", + ); + expect(elkGraph.children?.[0].width).toBeUndefined(); + expect(elkGraph.children?.[0].height).toBeUndefined(); + + // Child node should have fixed dimensions and no layout options + expect(elkGraph.children?.[0].children?.[0].width).toBe(100); + expect(elkGraph.children?.[0].children?.[0].height).toBe(50); + expect(elkGraph.children?.[0].children?.[0].layoutOptions).toBeUndefined(); + }); + + it("places edges at correct hierarchy level - root level", () => { + 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); + + expect(elkGraph.edges).toHaveLength(1); + expect(elkGraph.edges?.[0].id).toBe("edge1"); + }); + + it("places edges at correct hierarchy level - inside parent", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "parent", position: { x: 0, y: 0 }, data: {} }, + { id: "child1", position: { x: 0, y: 0 }, data: {}, parentId: "parent" }, + { id: "child2", position: { x: 0, y: 0 }, data: {}, parentId: "parent" }, + ] as Node[], + edges: [{ id: "edge1", source: "child1", target: "child2", data: {} }] as Edge[], + }; + + const elkGraph = buildElkGraphFromReactFlowGraph(reactFlowGraph); + + // Edge should be inside parent, not at root + expect(elkGraph.edges).toHaveLength(0); + expect(elkGraph.children?.[0].edges).toHaveLength(1); + expect(elkGraph.children?.[0].edges?.[0].id).toBe("edge1"); + }); + + it("places edges at lowest common ancestor", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "parent1", position: { x: 0, y: 0 }, data: {} }, + { id: "child1", position: { x: 0, y: 0 }, data: {}, parentId: "parent1" }, + { id: "parent2", position: { x: 0, y: 0 }, data: {} }, + { id: "child2", position: { x: 0, y: 0 }, data: {}, parentId: "parent2" }, + ] as Node[], + edges: [{ id: "edge1", source: "child1", target: "child2", data: {} }] as Edge[], + }; + + const elkGraph = buildElkGraphFromReactFlowGraph(reactFlowGraph); + + // Edge connects children from different parents, should be at root + expect(elkGraph.edges).toHaveLength(1); + expect(elkGraph.children?.[0].edges).toBeUndefined(); + expect(elkGraph.children?.[1].edges).toBeUndefined(); + }); + + it("cleans up empty edges arrays", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "parent", position: { x: 0, y: 0 }, data: {} }, + { id: "child", position: { x: 0, y: 0 }, data: {}, parentId: "parent" }, + ] as Node[], + edges: [], + }; + + const elkGraph = buildElkGraphFromReactFlowGraph(reactFlowGraph); + + // Nodes should not have empty edges arrays + expect(elkGraph.children?.[0].edges).toBeUndefined(); + expect(elkGraph.children?.[0].children?.[0].edges).toBeUndefined(); + }); + + it("handles multi-level nesting with edges at different levels", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "grandparent", position: { x: 0, y: 0 }, data: {} }, + { id: "parent", position: { x: 0, y: 0 }, data: {}, parentId: "grandparent" }, + { id: "child1", position: { x: 0, y: 0 }, data: {}, parentId: "parent" }, + { id: "child2", position: { x: 0, y: 0 }, data: {}, parentId: "parent" }, + ] as Node[], + edges: [{ id: "edge1", source: "child1", target: "child2", data: {} }] as Edge[], + }; + + const elkGraph = buildElkGraphFromReactFlowGraph(reactFlowGraph); + + // Edge should be at parent level (lowest common ancestor) + expect(elkGraph.edges).toHaveLength(0); + expect(elkGraph.children?.[0].edges).toBeUndefined(); + expect(elkGraph.children?.[0].children?.[0].edges).toHaveLength(1); + }); }); describe("matchReactFlowGraphWithElkLayoutedGraph", () => { @@ -465,6 +589,152 @@ describe("autoLayout", () => { { x: 150, y: 100 }, ]); }); + + it("handles edges inside parent nodes - removes wayPoints", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "parent", position: { x: 0, y: 0 }, data: {} }, + { id: "child1", position: { x: 0, y: 0 }, data: {}, parentId: "parent" }, + { id: "child2", position: { x: 0, y: 0 }, data: {}, parentId: "parent" }, + ] as Node[], + edges: [{ id: "edge1", source: "child1", target: "child2", data: {} }] as Edge[], + }; + + const layoutedElkGraph: ElkNode = { + id: "root", + children: [ + { + id: "parent", + x: 0, + y: 0, + children: [ + { id: "child1", x: 10, y: 10 }, + { id: "child2", x: 10, y: 70 }, + ], + edges: [ + { + id: "edge1", + sources: ["child1"], + targets: ["child2"], + sections: [ + { + id: "section1", + startPoint: { x: 10, y: 10 }, + endPoint: { x: 10, y: 70 }, + bendPoints: [{ x: 10, y: 40 }], + }, + ], + }, + ], + }, + ], + edges: [], + }; + + const result = matchReactFlowGraphWithElkLayoutedGraph(reactFlowGraph, layoutedElkGraph); + + // wayPoints should be undefined for edges inside parent nodes + expect(result.edges[0].data?.wayPoints).toBeUndefined(); + }); + + it("preserves wayPoints for edges not inside parent nodes", () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [ + { id: "parent1", position: { x: 0, y: 0 }, data: {} }, + { id: "child1", position: { x: 0, y: 0 }, data: {}, parentId: "parent1" }, + { id: "parent2", position: { x: 0, y: 0 }, data: {} }, + { id: "child2", position: { x: 0, y: 0 }, data: {}, parentId: "parent2" }, + ] as Node[], + edges: [{ id: "edge1", source: "child1", target: "child2", data: {} }] as Edge[], + }; + + const layoutedElkGraph: ElkNode = { + id: "root", + children: [ + { + id: "parent1", + x: 0, + y: 0, + children: [{ id: "child1", x: 10, y: 10 }], + }, + { + id: "parent2", + x: 200, + y: 0, + children: [{ id: "child2", x: 10, y: 10 }], + }, + ], + edges: [ + { + id: "edge1", + sources: ["child1"], + targets: ["child2"], + sections: [ + { + id: "section1", + startPoint: { x: 10, y: 10 }, + endPoint: { x: 210, y: 10 }, + bendPoints: [{ x: 100, y: 10 }], + }, + ], + }, + ], + }; + + const result = matchReactFlowGraphWithElkLayoutedGraph(reactFlowGraph, layoutedElkGraph); + + // wayPoints should be preserved for edges crossing parent boundaries + expect(result.edges[0].data?.wayPoints).toEqual([{ x: 100, y: 10 }]); + }); + + it("preserves other edge data when updating wayPoints", () => { + 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: { label: "Test", color: "blue", customProp: 123 }, + }, + ] as Edge[], + }; + + const layoutedElkGraph: ElkNode = { + id: "root", + children: [ + { id: "node1", x: 0, y: 0 }, + { id: "node2", x: 200, y: 100 }, + ], + edges: [ + { + id: "edge1", + sources: ["node1"], + targets: ["node2"], + sections: [ + { + id: "section1", + startPoint: { x: 0, y: 0 }, + endPoint: { x: 200, y: 100 }, + bendPoints: [{ x: 100, y: 50 }], + }, + ], + }, + ], + }; + + const result = matchReactFlowGraphWithElkLayoutedGraph(reactFlowGraph, layoutedElkGraph); + + expect(result.edges[0].data).toEqual({ + label: "Test", + color: "blue", + customProp: 123, + wayPoints: [{ x: 100, y: 50 }], + }); + }); }); describe("applyAutoLayout", () => { @@ -651,6 +921,40 @@ describe("autoLayout", () => { expect(result.nodes[2].position).toEqual({ x: 350, y: 50 }); }); + it("passes abort signal to processElkLayout", async () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [{ id: "node1", position: { x: 0, y: 0 }, data: {} }] as Node[], + edges: [], + }; + + const layoutedElkGraph: ElkNode = { + id: "root", + children: [{ id: "node1", x: 50, y: 100 }], + edges: [], + }; + + vi.mocked(core.processElkLayout).mockResolvedValue(layoutedElkGraph); + + const abortController = new AbortController(); + await applyAutoLayout(reactFlowGraph, abortController.signal); + + expect(core.processElkLayout).toHaveBeenCalledWith( + expect.any(Object), + abortController.signal, + ); + }); + + it("handles processElkLayout rejection gracefully", async () => { + const reactFlowGraph: ReactFlowGraph = { + nodes: [{ id: "node1", position: { x: 10, y: 20 }, data: {} }] as Node[], + edges: [], + }; + + vi.mocked(core.processElkLayout).mockRejectedValue(new Error("Layout failed")); + + await expect(applyAutoLayout(reactFlowGraph)).rejects.toThrow("Layout failed"); + }); + describe("buildElkNodeMap helper", () => { it("handles nested nodes correctly in matchReactFlowGraphWithElkLayoutedGraph", () => { const reactFlowGraph: ReactFlowGraph = { @@ -806,15 +1110,26 @@ describe("autoLayout", () => { describe("ROOT_LAYOUT_OPTIONS", () => { it("contains required ELK layout options", () => { - 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"); + 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"); }); it("has proper spacing configuration", () => { - expect(ROOT_LAYOUT_OPTIONS["spacing"]).toBe("75"); - expect(ROOT_LAYOUT_OPTIONS["spacing.componentComponent"]).toBe("70"); - expect(ROOT_LAYOUT_OPTIONS["spacing.nodeNodeBetweenLayers"]).toBe("80"); + 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( + "80", + ); + }); + }); + + describe("PARENT_LAYOUT_OPTIONS", () => { + it("extends ROOT_LAYOUT_OPTIONS with padding", () => { + expect(PARENT_LAYOUT_OPTIONS["elk.padding"]).toBe("[top=60,left=20,bottom=20,right=20]"); + expect(PARENT_LAYOUT_OPTIONS["org.eclipse.elk.algorithm"]).toBe("org.eclipse.elk.layered"); }); }); }); diff --git a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts index 2c7e0d3..ad473b7 100644 --- a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts +++ b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts @@ -16,7 +16,7 @@ import { describe, it, expect, beforeAll } from "vitest"; import * as RF from "@xyflow/react"; -import { FlatGraphNode, GraphNodeType } from "@serverlessworkflow/sdk"; +import { FlatGraphNode, GraphNodeType, Specification } from "@serverlessworkflow/sdk"; import { getEdgeType, edgeSourceAndTargetExist, @@ -24,6 +24,7 @@ import { getCatchContainerNodeIds, } from "../../../src/react-flow/diagram/diagramBuilder"; import { EdgeTypes } from "../../../src/react-flow/edges/Edges"; +import { CATCH_CONTAINER_NODE_TYPE } from "../../../src/react-flow/nodes/Nodes"; import { parseWorkflow } from "../../../src/core"; import { BASIC_VALID_WORKFLOW_JSON, @@ -279,6 +280,15 @@ describe("diagramBuilder", () => { expect(diagram.nodes).toEqual([]); expect(diagram.edges).toEqual([]); }); + + it("returns ReactFlowGraph with correct structure", () => { + const diagram: DiagramElements = buildDiagramFromWorkflow(BASIC_VALID_WORKFLOW_JSON); + + expect(diagram).toHaveProperty("nodes"); + expect(diagram).toHaveProperty("edges"); + expect(Array.isArray(diagram.nodes)).toBe(true); + expect(Array.isArray(diagram.edges)).toBe(true); + }); }); describe("node properties", () => { @@ -355,6 +365,237 @@ describe("diagramBuilder", () => { const uniqueIds = new Set(edgeIds); expect(uniqueIds.size).toBe(edgeIds.length); }); + + it("sets animated property correctly for error edges", () => { + // Create a workflow with a raise node to generate error edges + const workflowWithRaise = JSON.stringify({ + document: { + dsl: "1.0.3", + name: "workflow-with-raise", + version: "1.0.0", + namespace: "default", + }, + do: [ + { + raiseError: { + raise: { + error: { + type: "https://example.com/errors/test", + status: 500, + }, + }, + }, + }, + ], + }); + + const diagram = buildDiagramFromWorkflow(workflowWithRaise); + const errorEdges = diagram.edges.filter((edge) => edge.type === EdgeTypes.Error); + + errorEdges.forEach((edge) => { + expect(edge.animated).toBe(true); + }); + }); + + it("does not create edges for non-existent nodes", () => { + const diagram: DiagramElements = buildDiagramFromWorkflow(BASIC_VALID_WORKFLOW_JSON_TASKS); + const nodeIdSet = new Set(diagram.nodes.map((node) => node.id)); + + // All edges should reference existing nodes + diagram.edges.forEach((edge) => { + expect(nodeIdSet.has(edge.source)).toBe(true); + expect(nodeIdSet.has(edge.target)).toBe(true); + }); + }); + }); + + describe("parent-child relationships", () => { + it("sets parentId for child nodes", () => { + const workflowWithNesting = JSON.stringify({ + document: { + dsl: "1.0.3", + name: "workflow-with-nesting", + version: "1.0.0", + namespace: "default", + }, + do: [ + { + tryBlock: { + try: [ + { + step1: { + set: { + variable: "nested task", + }, + }, + }, + ], + catch: { + errors: { + with: { + type: "https://example.com/errors/test", + }, + }, + do: [ + { + recover: { + set: { + variable: "recovery", + }, + }, + }, + ], + }, + }, + }, + ], + }); + + const diagram = buildDiagramFromWorkflow(workflowWithNesting); + const childNodes = diagram.nodes.filter( + (node) => node.parentId && node.parentId !== "root", + ); + + expect(childNodes.length).toBeGreaterThan(0); + childNodes.forEach((node) => { + expect(node.parentId).toBeDefined(); + expect(node.extent).toBe("parent"); + }); + }); + + it("does not set parentId for root-level nodes", () => { + const diagram: DiagramElements = buildDiagramFromWorkflow(BASIC_VALID_WORKFLOW_JSON_TASKS); + const rootNodes = diagram.nodes.filter( + (node) => !node.parentId || node.parentId === "root", + ); + + expect(rootNodes.length).toBeGreaterThan(0); + rootNodes.forEach((node) => { + expect(node.extent).toBeUndefined(); + }); + }); + }); + + describe("catch container nodes", () => { + it("creates catch container nodes for catch blocks with children", () => { + const workflowWithCatch = JSON.stringify({ + document: { + dsl: "1.0.3", + name: "workflow-with-catch", + version: "1.0.0", + namespace: "default", + }, + do: [ + { + tryBlock: { + try: [ + { + step1: { + set: { + variable: "task", + }, + }, + }, + ], + catch: { + errors: { + with: { + type: "https://example.com/errors/test", + }, + }, + do: [ + { + recover: { + set: { + variable: "recovery", + }, + }, + }, + ], + }, + }, + }, + ], + }); + + const diagram = buildDiagramFromWorkflow(workflowWithCatch); + const catchContainerNodes = diagram.nodes.filter( + (node) => node.type === CATCH_CONTAINER_NODE_TYPE, + ); + + expect(catchContainerNodes.length).toBeGreaterThan(0); + }); + + it("does not create catch container nodes for leaf catch blocks", () => { + const workflowWithLeafCatch = JSON.stringify({ + document: { + dsl: "1.0.3", + name: "workflow-with-leaf-catch", + version: "1.0.0", + namespace: "default", + }, + do: [ + { + tryBlock: { + try: [ + { + step1: { + set: { + variable: "task", + }, + }, + }, + ], + catch: { + errors: { + with: { + type: "https://example.com/errors/test", + }, + }, + }, + }, + }, + ], + }); + + const diagram = buildDiagramFromWorkflow(workflowWithLeafCatch); + const catchNodes = diagram.nodes.filter((node) => node.type === GraphNodeType.Catch); + + // Leaf catch nodes should use GraphNodeType.Catch, not CATCH_CONTAINER_NODE_TYPE + catchNodes.forEach((node) => { + expect(node.type).toBe(GraphNodeType.Catch); + expect(node.type).not.toBe(CATCH_CONTAINER_NODE_TYPE); + }); + }); + }); + + describe("task data preservation", () => { + it("preserves task data in node data", () => { + const diagram: DiagramElements = buildDiagramFromWorkflow(BASIC_VALID_WORKFLOW_JSON_TASKS); + const taskNodes = diagram.nodes.filter((node) => node.data.task !== undefined); + + taskNodes.forEach((node) => { + expect(node.data.task).toBeDefined(); + expect(typeof node.data.task).toBe("object"); + }); + }); + + it("clones task data to prevent mutations", () => { + const parseResult = parseWorkflow(BASIC_VALID_WORKFLOW_JSON_TASKS); + const diagram1 = buildDiagramElements(parseResult.model); + const diagram2 = buildDiagramElements(parseResult.model); + + const taskNode1 = diagram1.nodes.find((node) => node.data.task !== undefined); + const taskNode2 = diagram2.nodes.find((node) => node.id === taskNode1?.id); + + if (taskNode1 && taskNode2) { + // Modify one task + (taskNode1.data.task as Specification.Task).modified = true; + + // The other should not be affected + expect((taskNode2.data.task as Specification.Task).modified).toBeUndefined(); + } + }); }); describe("graph structure", () => { @@ -363,6 +604,41 @@ describe("diagramBuilder", () => { expect(diagram.nodes.length).toBeGreaterThan(0); }); + + it("maintains correct node-edge relationships", () => { + const diagram: DiagramElements = buildDiagramFromWorkflow(BASIC_VALID_WORKFLOW_JSON_TASKS); + const nodeIdSet = new Set(diagram.nodes.map((node) => node.id)); + + // Every edge should connect existing nodes + diagram.edges.forEach((edge) => { + expect(nodeIdSet.has(edge.source)).toBe(true); + expect(nodeIdSet.has(edge.target)).toBe(true); + }); + }); + + it("creates connected graph structure", () => { + const diagram: DiagramElements = buildDiagramFromWorkflow(BASIC_VALID_WORKFLOW_JSON_TASKS); + + // Should have at least one edge connecting nodes + expect(diagram.edges.length).toBeGreaterThan(0); + + // All nodes except end nodes should have outgoing edges + const nodesWithOutgoingEdges = new Set(diagram.edges.map((edge) => edge.source)); + const endNodes = diagram.nodes.filter((node) => node.type === GraphNodeType.End); + + diagram.nodes.forEach((node) => { + const isEndNode = endNodes.some((endNode) => endNode.id === node.id); + if (!isEndNode && node.type !== GraphNodeType.Exit) { + // Most nodes should have outgoing edges (some exceptions for specific node types) + const hasOutgoing = nodesWithOutgoingEdges.has(node.id); + // This is a soft check as some node types may not have outgoing edges + if (!hasOutgoing) { + // Just verify the node exists in the graph + expect(node.id).toBeDefined(); + } + } + }); + }); }); }); @@ -499,5 +775,92 @@ describe("diagramBuilder", () => { expect(result.has("/do/0/TryCatch/catch")).toBe(true); expect(result.has("/do/0/TryCatch")).toBe(false); }); + + it("should handle multiple catch containers at the same level", () => { + const sdkGraph = createFlatGraph( + [ + { + id: "start", + type: GraphNodeType.Start, + } as FlatGraphNode, + { + id: "end", + type: GraphNodeType.End, + } as FlatGraphNode, + { + id: "/do/0/Catch1", + type: GraphNodeType.Catch, + label: "Catch1", + } as FlatGraphNode, + { + id: "/do/0/Catch1/do/0/step1", + type: GraphNodeType.Set, + label: "step1", + parentId: "/do/0/Catch1", + } as FlatGraphNode, + { + id: "/do/0/Catch2", + type: GraphNodeType.Catch, + label: "Catch2", + } as FlatGraphNode, + { + id: "/do/0/Catch2/do/0/step2", + type: GraphNodeType.Set, + label: "step2", + parentId: "/do/0/Catch2", + } as FlatGraphNode, + ], + [], + ); + + const result = getCatchContainerNodeIds(sdkGraph); + + expect(result.has("/do/0/Catch1")).toBe(true); + expect(result.has("/do/0/Catch2")).toBe(true); + expect(result.size).toBe(2); + }); + + it("should return empty set for graph with no nodes", () => { + const sdkGraph = createFlatGraph([], []); + + const result = getCatchContainerNodeIds(sdkGraph); + + expect(result.size).toBe(0); + }); + + it("should handle deeply nested catch containers", () => { + const sdkGraph = createFlatGraph( + [ + { + id: "start", + type: GraphNodeType.Start, + } as FlatGraphNode, + { + id: "/do/0/OuterCatch", + type: GraphNodeType.Catch, + label: "OuterCatch", + } as FlatGraphNode, + { + id: "/do/0/OuterCatch/do/0/InnerCatch", + type: GraphNodeType.Catch, + label: "InnerCatch", + parentId: "/do/0/OuterCatch", + } as FlatGraphNode, + { + id: "/do/0/OuterCatch/do/0/InnerCatch/do/0/step", + type: GraphNodeType.Set, + label: "step", + parentId: "/do/0/OuterCatch/do/0/InnerCatch", + } as FlatGraphNode, + ], + [], + ); + + const result = getCatchContainerNodeIds(sdkGraph); + + expect(result.has("/do/0/OuterCatch")).toBe(true); + expect(result.has("/do/0/OuterCatch/do/0/InnerCatch")).toBe(true); + expect(result.size).toBe(2); + }); }); }); From 1c721ca353a0dd787f462504eb40da5658fa8094 Mon Sep 17 00:00:00 2001 From: handreyrc Date: Wed, 27 May 2026 22:29:26 -0400 Subject: [PATCH 3/6] Add changeset Signed-off-by: handreyrc --- .changeset/containment-auto-layout.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/containment-auto-layout.md diff --git a/.changeset/containment-auto-layout.md b/.changeset/containment-auto-layout.md new file mode 100644 index 0000000..aae2b55 --- /dev/null +++ b/.changeset/containment-auto-layout.md @@ -0,0 +1,5 @@ +--- +"@serverlessworkflow/diagram-editor": minor +--- + +Add containment support integrated with auto-layout. From ed4cea17ecc4a03da491cbb57016fb685d84c5a8 Mon Sep 17 00:00:00 2001 From: handreyrc Date: Thu, 28 May 2026 11:37:35 -0400 Subject: [PATCH 4/6] Fix copilot complaints Signed-off-by: handreyrc --- .../.storybook/vitest.setup.ts | 23 ------------- .../src/react-flow/diagram/autoLayout.ts | 33 +++++++++++-------- .../diagram/autoLayout.integration.test.ts | 6 ++-- .../react-flow/diagram/diagramBuilder.test.ts | 33 +++++++++---------- .../vitest.config.ts | 16 +++++---- 5 files changed, 50 insertions(+), 61 deletions(-) delete mode 100644 packages/serverless-workflow-diagram-editor/.storybook/vitest.setup.ts diff --git a/packages/serverless-workflow-diagram-editor/.storybook/vitest.setup.ts b/packages/serverless-workflow-diagram-editor/.storybook/vitest.setup.ts deleted file mode 100644 index 958ce16..0000000 --- a/packages/serverless-workflow-diagram-editor/.storybook/vitest.setup.ts +++ /dev/null @@ -1,23 +0,0 @@ -/* - * Copyright 2021-Present The Serverless Workflow Specification Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import * as a11yAddonAnnotations from "@storybook/addon-a11y/preview"; -import { setProjectAnnotations } from "@storybook/react-vite"; -import * as projectAnnotations from "./preview"; - -// This is an important step to apply the right configuration when testing your stories. -// More info at: https://storybook.js.org/docs/api/portable-stories/portable-stories-vitest#setprojectannotations -setProjectAnnotations([a11yAddonAnnotations, projectAnnotations]); 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 194dd62..13012b3 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 @@ -62,7 +62,7 @@ export const ROOT_LAYOUT_OPTIONS: LayoutOptions = { export const PARENT_LAYOUT_OPTIONS: LayoutOptions = { ...ROOT_LAYOUT_OPTIONS, - "elk.padding": "[top=60,left=20,bottom=20,right=20]", + "org.eclipse.elk.padding": "[top=60,left=20,bottom=20,right=20]", }; export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): ElkNode { @@ -98,7 +98,7 @@ export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): const elkNode = nodeMap.get(node.id)!; if (elkNode.children && elkNode.children.length > 0) { // Nodes with children get layout options but no fixed dimensions - elkNode.layoutOptions = PARENT_LAYOUT_OPTIONS; + elkNode.layoutOptions = { ...PARENT_LAYOUT_OPTIONS }; } else { // Leaf nodes get fixed dimensions elkNode.width = node.measured?.width ?? DEFAULT_NODE_SIZE.width; @@ -106,24 +106,26 @@ export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): } }); + const reactFlowNodeMap = new Map(reactFlowGraph.nodes.map((node) => [node.id, node])); + // Helper function to find the common ancestor of two nodes const findCommonAncestor = (sourceId: string, targetId: string): string => { // Build path from source to root - const sourcePath: string[] = []; + const sourcePath = new Set(); let currentId: string | undefined = sourceId; while (currentId) { - sourcePath.push(currentId); - const node = reactFlowGraph.nodes.find((n) => n.id === currentId); + sourcePath.add(currentId); + const node = reactFlowNodeMap.get(currentId); currentId = node?.parentId; } // Traverse from target to root and find first common node currentId = targetId; while (currentId) { - if (sourcePath.includes(currentId)) { + if (sourcePath.has(currentId)) { return currentId; } - const node = reactFlowGraph.nodes.find((n) => n.id === currentId); + const node = reactFlowNodeMap.get(currentId); currentId = node?.parentId; } @@ -211,10 +213,10 @@ function buildElkEdgeMap( // Helper function to check if an edge is inside a parent node function isEdgeInsideParent( edge: { source: string; target: string }, - graph: ReactFlowGraph, + nodeMap: Map, ): boolean { - const sourceNode = graph.nodes.find((n) => n.id === edge.source); - const targetNode = graph.nodes.find((n) => n.id === edge.target); + const sourceNode = nodeMap.get(edge.source); + const targetNode = nodeMap.get(edge.target); // Edge is inside a parent if both source and target have the same parentId return !!( @@ -233,6 +235,11 @@ 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); @@ -259,10 +266,10 @@ export function matchReactFlowGraphWithElkLayoutedGraph( const newData = { ...restData }; if (bendPoints.length > 0) { // Check if edge is inside a parent node and apply offset - const isInsideParent = isEdgeInsideParent(edge, graph); + const isInsideParent = isEdgeInsideParent(edge, reactFlowNodeMap); if (isInsideParent) { - // There is an incompatibity with the react flow library, the wayPoints are calculated correctly by ELK - // but the way react flow rendeer edges inside parent nodes cause path distotions. + // 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; } else { newData.wayPoints = bendPoints; 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 ff1949a..5cc1914 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 @@ -214,7 +214,7 @@ describe("autoLayout", () => { // Parent node should have layout options and no fixed dimensions expect(elkGraph.children?.[0].layoutOptions).toBeDefined(); - expect(elkGraph.children?.[0].layoutOptions?.["elk.padding"]).toBe( + expect(elkGraph.children?.[0].layoutOptions?.["org.eclipse.elk.padding"]).toBe( "[top=60,left=20,bottom=20,right=20]", ); expect(elkGraph.children?.[0].width).toBeUndefined(); @@ -1128,7 +1128,9 @@ describe("autoLayout", () => { describe("PARENT_LAYOUT_OPTIONS", () => { it("extends ROOT_LAYOUT_OPTIONS with padding", () => { - expect(PARENT_LAYOUT_OPTIONS["elk.padding"]).toBe("[top=60,left=20,bottom=20,right=20]"); + 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"); }); }); diff --git a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts index ad473b7..51f9e7c 100644 --- a/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts +++ b/packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts @@ -588,13 +588,15 @@ describe("diagramBuilder", () => { const taskNode1 = diagram1.nodes.find((node) => node.data.task !== undefined); const taskNode2 = diagram2.nodes.find((node) => node.id === taskNode1?.id); - if (taskNode1 && taskNode2) { - // Modify one task - (taskNode1.data.task as Specification.Task).modified = true; + // Ensure both nodes exist before testing clone behavior + expect(taskNode1).toBeDefined(); + expect(taskNode2).toBeDefined(); - // The other should not be affected - expect((taskNode2.data.task as Specification.Task).modified).toBeUndefined(); - } + // Modify one task + (taskNode1!.data.task as Specification.Task).modified = true; + + // The other should not be affected + expect((taskNode2!.data.task as Specification.Task).modified).toBeUndefined(); }); }); @@ -622,20 +624,17 @@ describe("diagramBuilder", () => { // Should have at least one edge connecting nodes expect(diagram.edges.length).toBeGreaterThan(0); - // All nodes except end nodes should have outgoing edges + // All non-terminal nodes should have outgoing edges const nodesWithOutgoingEdges = new Set(diagram.edges.map((edge) => edge.source)); - const endNodes = diagram.nodes.filter((node) => node.type === GraphNodeType.End); + const terminalNodeIds = new Set( + diagram.nodes + .filter((node) => node.type === GraphNodeType.End || node.type === GraphNodeType.Exit) + .map((node) => node.id), + ); diagram.nodes.forEach((node) => { - const isEndNode = endNodes.some((endNode) => endNode.id === node.id); - if (!isEndNode && node.type !== GraphNodeType.Exit) { - // Most nodes should have outgoing edges (some exceptions for specific node types) - const hasOutgoing = nodesWithOutgoingEdges.has(node.id); - // This is a soft check as some node types may not have outgoing edges - if (!hasOutgoing) { - // Just verify the node exists in the graph - expect(node.id).toBeDefined(); - } + if (!terminalNodeIds.has(node.id)) { + expect(nodesWithOutgoingEdges.has(node.id)).toBe(true); } }); }); diff --git a/packages/serverless-workflow-diagram-editor/vitest.config.ts b/packages/serverless-workflow-diagram-editor/vitest.config.ts index b5cc3b6..a868fa9 100644 --- a/packages/serverless-workflow-diagram-editor/vitest.config.ts +++ b/packages/serverless-workflow-diagram-editor/vitest.config.ts @@ -32,10 +32,17 @@ export default defineConfig({ test: { globals: true, environment: "jsdom", - setupFiles: ["./tests/setupTests.ts", "./.storybook/vitest.setup.ts"], - css: true, - include: ["tests/**/*.test.ts", "tests/**/*.test.tsx"], + setupFiles: ["./tests/setupTests.ts"], projects: [ + { + extends: true, + test: { + name: "unit", + css: true, + include: ["tests/**/*.test.ts", "tests/**/*.test.tsx"], + exclude: ["tests/**/*.story.test.ts", "tests/**/*.story.test.tsx"], + }, + }, { extends: true, plugins: [ @@ -51,9 +58,6 @@ export default defineConfig({ provider: playwright({}), instances: [{ browser: "chromium" }], }, - globals: true, - environment: "jsdom", - setupFiles: ["./tests/setupTests.ts", "./.storybook/vitest.setup.ts"], }, }, ], From 84ee56711232f9e20662b7d5b8e49de9277f7623 Mon Sep 17 00:00:00 2001 From: handreyrc Date: Fri, 29 May 2026 11:01:49 -0400 Subject: [PATCH 5/6] Fix review issues Signed-off-by: handreyrc --- .../src/react-flow/diagram/autoLayout.ts | 75 ++++++++++--------- 1 file changed, 40 insertions(+), 35 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 13012b3..573beb3 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 @@ -65,6 +65,45 @@ export const PARENT_LAYOUT_OPTIONS: LayoutOptions = { "org.eclipse.elk.padding": "[top=60,left=20,bottom=20,right=20]", }; +// Helper function to clean up empty edges arrays from nodes +function cleanupEmptyEdges(node: ElkNode): void { + if (node.edges && node.edges.length === 0) { + delete node.edges; + } + if (node.children) { + node.children.forEach(cleanupEmptyEdges); + } +} + +// Helper function to find the common ancestor of two nodes +function findCommonAncestor( + sourceId: string, + targetId: string, + reactFlowNodeMap: Map, +): string { + // Build path from source to root + const sourcePath = new Set(); + let currentId: string | undefined = sourceId; + while (currentId) { + sourcePath.add(currentId); + const node = reactFlowNodeMap.get(currentId); + currentId = node?.parentId; + } + + // Traverse from target to root and find first common node + currentId = targetId; + while (currentId) { + if (sourcePath.has(currentId)) { + return currentId; + } + const node = reactFlowNodeMap.get(currentId); + currentId = node?.parentId; + } + + // If no common ancestor found, return "root" + return "root"; +} + export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): ElkNode { // Create a map for easy lookup (without width/height initially) const nodeMap = new Map( @@ -108,31 +147,6 @@ export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): const reactFlowNodeMap = new Map(reactFlowGraph.nodes.map((node) => [node.id, node])); - // Helper function to find the common ancestor of two nodes - const findCommonAncestor = (sourceId: string, targetId: string): string => { - // Build path from source to root - const sourcePath = new Set(); - let currentId: string | undefined = sourceId; - while (currentId) { - sourcePath.add(currentId); - const node = reactFlowNodeMap.get(currentId); - currentId = node?.parentId; - } - - // Traverse from target to root and find first common node - currentId = targetId; - while (currentId) { - if (sourcePath.has(currentId)) { - return currentId; - } - const node = reactFlowNodeMap.get(currentId); - currentId = node?.parentId; - } - - // If no common ancestor found, return "root" - return "root"; - }; - // Nest edges in the appropriate hierarchy level const rootEdges: ElkExtendedEdge[] = []; reactFlowGraph.edges.forEach((edge) => { @@ -143,7 +157,7 @@ export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): }; // Find the lowest common ancestor that contains both source and target - const commonAncestor = findCommonAncestor(edge.source, edge.target); + const commonAncestor = findCommonAncestor(edge.source, edge.target, reactFlowNodeMap); if (commonAncestor === "root") { rootEdges.push(elkEdge); @@ -159,15 +173,6 @@ export function buildElkGraphFromReactFlowGraph(reactFlowGraph: ReactFlowGraph): }); // Clean up empty edges arrays from nodes that don't need them - const cleanupEmptyEdges = (node: ElkNode) => { - if (node.edges && node.edges.length === 0) { - delete node.edges; - } - if (node.children) { - node.children.forEach(cleanupEmptyEdges); - } - }; - rootChildren.forEach(cleanupEmptyEdges); return { From 4e56044878107dee951a64632b7db0d5e1616296 Mon Sep 17 00:00:00 2001 From: handreyrc Date: Fri, 29 May 2026 11:19:38 -0400 Subject: [PATCH 6/6] Fix copilot complaints Signed-off-by: handreyrc --- .../src/react-flow/diagram/autoLayout.ts | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 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 573beb3..d238fe3 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 @@ -43,7 +43,7 @@ export const ROOT_LAYOUT_OPTIONS: LayoutOptions = { "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": "true", + "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", @@ -79,7 +79,7 @@ function cleanupEmptyEdges(node: ElkNode): void { function findCommonAncestor( sourceId: string, targetId: string, - reactFlowNodeMap: Map, + reactFlowNodeMap: Map, ): string { // Build path from source to root const sourcePath = new Set(); @@ -220,15 +220,10 @@ function isEdgeInsideParent( edge: { source: string; target: string }, nodeMap: Map, ): boolean { - const sourceNode = nodeMap.get(edge.source); - const targetNode = nodeMap.get(edge.target); - - // Edge is inside a parent if both source and target have the same parentId - return !!( - sourceNode?.parentId && - targetNode?.parentId && - sourceNode.parentId === targetNode.parentId - ); + // 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"; } // set @@ -270,7 +265,7 @@ export function matchReactFlowGraphWithElkLayoutedGraph( // Always create new data object, only add wayPoints if there are bend points const newData = { ...restData }; if (bendPoints.length > 0) { - // Check if edge is inside a parent node and apply offset + // 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