feat: Add custom catch container node#145
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a ReactFlow-specific “catch container” node type so that Catch nodes with nested content can render differently from leaf Catch nodes, and refactors test helpers to reuse a shared createFlatGraph utility.
Changes:
- Added detection of
Catchnodes that have child nodes and map them to a custom ReactFlow node type (catch-container). - Registered a new ReactFlow node type/component for catch containers.
- Extracted a shared
createFlatGraphhelper intotests/test-utilsand reused it across tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/serverless-workflow-diagram-editor/src/react-flow/diagram/diagramBuilder.ts | Detect catch container nodes and resolve their ReactFlow node type during diagram build. |
| packages/serverless-workflow-diagram-editor/src/react-flow/nodes/Nodes.tsx | Register catch-container ReactFlow node type and add a placeholder component for it. |
| packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts | Add unit tests for getCatchContainerNodeIds (with some test-name typos to fix). |
| packages/serverless-workflow-diagram-editor/tests/test-utils/graph-helpers.ts | Introduce shared createFlatGraph test helper. |
| packages/serverless-workflow-diagram-editor/tests/test-utils/index.ts | Re-export createFlatGraph from the test-utils barrel. |
Comments suppressed due to low confidence (1)
packages/serverless-workflow-diagram-editor/tests/react-flow/diagram/diagramBuilder.test.ts:465
- There are spelling mistakes in this test name (e.g., "inclyde", "insode"). Please correct them to keep test output readable and searchable.
it("should inclyde nested catch containers insode a parent container", () => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: lornakelly <lornakelly88@gmail.com>
ad0d9d6 to
7105f1e
Compare
fantonangeli
left a comment
There was a problem hiding this comment.
LGTM, I tried to improve getCatchContainerNodeIds() but please check if it does what expected as I only tried with pnpm test
| export function getCatchContainerNodeIds(graph: sdk.FlatGraph): Set<string> { | ||
| const parentIds = new Set<string>(); | ||
| for (const node of graph.nodes) { | ||
| if (node.parentId) { | ||
| parentIds.add(node.parentId); | ||
| } | ||
| } | ||
|
|
||
| const containerIds = new Set<string>(); | ||
| for (const node of graph.nodes) { | ||
| if (node.type === sdk.GraphNodeType.Catch && parentIds.has(node.id)) { | ||
| containerIds.add(node.id); | ||
| } | ||
| } | ||
|
|
||
| return containerIds; | ||
| } |
There was a problem hiding this comment.
| export function getCatchContainerNodeIds(graph: sdk.FlatGraph): Set<string> { | |
| const parentIds = new Set<string>(); | |
| for (const node of graph.nodes) { | |
| if (node.parentId) { | |
| parentIds.add(node.parentId); | |
| } | |
| } | |
| const containerIds = new Set<string>(); | |
| for (const node of graph.nodes) { | |
| if (node.type === sdk.GraphNodeType.Catch && parentIds.has(node.id)) { | |
| containerIds.add(node.id); | |
| } | |
| } | |
| return containerIds; | |
| } | |
| export function getCatchContainerNodeIds(graph: sdk.FlatGraph): Set<string> { | |
| const parentIds = new Set(graph.nodes.filter(n => n.parentId).map(n => n.parentId!)); | |
| return new Set( | |
| graph.nodes | |
| .filter(n => n.type === sdk.GraphNodeType.Catch && parentIds.has(n.id)) | |
| .map(n => n.id) | |
| ); | |
| } |
| expect(result.size).toBe(0); | ||
| }); | ||
|
|
||
| it("should inclyde nested catch containers insode a parent container", () => { |
There was a problem hiding this comment.
| it("should inclyde nested catch containers insode a parent container", () => { | |
| it("should include nested catch containers inside a parent container", () => { |
Closes #141
Summary
Resolve catch leaf vs catch container nodes at ReactFlow boundary as this is a rendering concern
Changes