fix(canvas): add droppable special child placeholders with insert-at-start capability#2997
Merged
lordrip merged 2 commits intoKaotoIO:mainfrom Mar 11, 2026
Merged
fix(canvas): add droppable special child placeholders with insert-at-start capability#2997lordrip merged 2 commits intoKaotoIO:mainfrom
lordrip merged 2 commits intoKaotoIO:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds explicit placeholder viz nodes across mappers, enhances placeholder UI and drop handling (including special-child insertion and forward-drop refactor), introduces insertAtStart for step insertion propagated through hooks/models, updates placeholder sizing/styling, and expands unit and Cypress tests to reflect new placeholder behavior. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Placeholder as PlaceholderNode
participant CustomUtils as CustomNodeUtils
participant InsertHook as useInsertStep
participant BaseEntity as BaseVisualEntity
participant NodeMapper as NodeMapper
User->>Placeholder: click or drop on placeholder
Placeholder->>Placeholder: determine special / special-child
Placeholder->>InsertHook: onInsertStep(predefinedComponent?, insertAtStart?)
InsertHook->>BaseEntity: addStep / pasteStep (insertAtStart)
BaseEntity->>BaseEntity: insertAtStart ? unshift(step) : push(step)
alt Drop a dragged container/node
User->>CustomUtils: drop dragged node
CustomUtils->>CustomUtils: validate compatibility with placeholder
CustomUtils->>BaseEntity: perform forward-drop or insert as special child
CustomUtils->>BaseEntity: remove original dragged node if moved
end
Note over NodeMapper: Mappers emit explicit placeholders via getPlaceHolderNodeForProcessor(path)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2997 +/- ##
========================================
Coverage 89.69% 89.70%
========================================
Files 561 562 +1
Lines 20913 20959 +46
Branches 4889 4752 -137
========================================
+ Hits 18758 18801 +43
- Misses 2028 2156 +128
+ Partials 127 2 -125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts (1)
61-71:⚠️ Potential issue | 🟡 MinorTest name is inconsistent with actual behavior.
The test name "should not return an
onFallbacknode if not defined" contradicts the assertions: the test now verifies that anonFallbacknode IS returned (as a placeholder). Consider renaming to accurately reflect the new behavior.Suggested fix
- it('should not return an `onFallback` node if not defined', () => { + it('should return an `onFallback` placeholder node when onFallback is not defined', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts` around lines 61 - 71, The test name is misleading: update the 'it' description in circuit-breaker-node-mapper.test.ts to reflect that a placeholder onFallback node is returned when onFallback is undefined (e.g., rename "should not return an `onFallback` node if not defined" to "should return a placeholder `onFallback` node if not defined"), and keep the existing assertions that call mapper.getVizNodeFromProcessor and inspect routeDefinition.from.steps[0].circuitBreaker!.onFallback and vizNode.getChildren().packages/ui/src/components/Visualization/Custom/customComponentUtils.ts (1)
55-60:⚠️ Potential issue | 🟡 MinorGuard
canDragGroupagainst missing/empty paths before returningtrue.Right now, nodes with an unset/empty
data.pathcan be treated as draggable due to the unconditional fallback return.Suggested fix
const canDragGroup = (groupVizNode?: IVisualizationNode): boolean => { if (groupVizNode === undefined) { return false; } + const path = groupVizNode.data.path; + if (!path) { + return false; + } + // Do not allow dragging top-level groups like Route - if (groupVizNode.data.path?.split('.').length === 1) { + if (path.split('.').length === 1) { return false; } return true; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/customComponentUtils.ts` around lines 55 - 60, The canDragGroup function currently returns true unconditionally if the path check doesn't match, allowing nodes with missing or empty groupVizNode.data.path to be draggable; update canDragGroup to first guard against a missing/null/empty path (e.g., check that groupVizNode.data.path is a non-empty string) and return false in that case, then perform the existing split('.') length === 1 check on the validated path and return accordingly so only nodes with valid, multi-segment paths are draggable.packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx (1)
124-126:⚠️ Potential issue | 🟠 MajorMove the
vizNodenull-guard outside this component to prevent conditional hook calls.The early return at lines 124-126 causes
useReplaceStep(line 127) anduseInsertStep(line 138) to be conditionally called based onvizNodeavailability, violating the React Hooks rule that hooks must be called in the same order on every render. Move the guard to the outerPlaceholderNodewrapper and makevizNodea required prop toPlaceholderNodeInner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx` around lines 124 - 126, Move the vizNode null-guard out of PlaceholderNodeInner so hooks run unconditionally: make PlaceholderNode responsible for returning null when vizNode is falsy and change PlaceholderNodeInner to require a non-null vizNode prop; then remove the early return inside PlaceholderNodeInner and keep calls to useReplaceStep and useInsertStep (and any other hooks) at the top of PlaceholderNodeInner so they are always invoked in the same order. Update any callers to pass vizNode only when non-null into PlaceholderNodeInner via PlaceholderNode.
🧹 Nitpick comments (7)
packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsx (1)
56-56: Consider adding a guard or assertion for the node lookup.The change to ID-based selection is a good improvement for test stability. However, the non-null assertion
!could mask issues if the node structure changes—find()returningundefinedwould cause a cryptic runtime error downstream rather than a clear failure at the selection point.Consider adding an explicit assertion to provide a clearer failure message:
🔧 Suggested improvement
- selectedNode = nodes.find((node) => node.id === 'test|route.from.steps.1.choice')!; // choice + const foundNode = nodes.find((node) => node.id === 'test|route.from.steps.1.choice'); + expect(foundNode).toBeDefined(); // Fail fast with clear message if node structure changes + selectedNode = foundNode!; // choice🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsx` at line 56, Replace the non-null assertion on the test node lookup to fail fast with a clear message: instead of using the `!` after `nodes.find(...)` for the ID 'test|route.from.steps.1.choice', check the result of `nodes.find` assigned to `selectedNode` and throw or use an explicit test assertion (e.g., expect/throw) if it's undefined so the test fails with a descriptive message mentioning `selectedNode` and the target id; update any subsequent uses of `selectedNode` to rely on this validated variable.packages/ui/src/models/visualization/flows/camel-route-visual-entity.test.ts (1)
354-354: Prefer path-based child lookup over fixed indices in tests.Using
[1]/[2]tightly couples tests to ordering. A path/label-based lookup will be more stable as placeholder logic evolves.♻️ Optional refactor example
+const findChildByPath = (node: IVisualizationNode, path: string) => + node.getChildren()?.find((child) => child.data.path === path); -const whenNode = choiceNode.getChildren()?.[1]; +const whenNode = findChildByPath(choiceNode, 'route.from.steps.1.choice.when.0'); -const otherwiseNode = choiceNode.getChildren()?.[2]; +const otherwiseNode = findChildByPath(choiceNode, 'route.from.steps.1.choice.otherwise');Also applies to: 419-419, 441-441
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/camel-route-visual-entity.test.ts` at line 354, Tests rely on positional indexing whenNode = choiceNode.getChildren()?.[1] which is brittle; replace fixed-index child access with a path/label/type-based lookup: add or use a helper to traverse children (e.g., choiceNode.getChildren()?.find(c => c.label === 'when' || c.type === 'WhenNode' || c.meta?.role === 'when')) and use that result for whenNode (apply same change for other occurrences referenced in the comment), ensuring you null-check and assert the found node exists before proceeding.packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts (1)
55-59: Consider adding assertion for non-placeholder state.For completeness, consider verifying that
isPlaceholderis falsy whenonFallbackis defined. This would complement the assertion at line 70 and ensure the placeholder logic is correctly distinguished.Suggested enhancement
it('should return an `onFallback` node if defined', () => { const vizNode = mapper.getVizNodeFromProcessor(path, { processorName: 'circuitBreaker' }, routeDefinition); expect(vizNode.getChildren()?.[2].data.path).toBe('from.steps.0.circuitBreaker.onFallback'); + expect(vizNode.getChildren()?.[2].data.isPlaceholder).toBeFalsy(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts` around lines 55 - 59, The test currently asserts that an `onFallback` child node path exists but doesn't verify its placeholder state; update the test for mapper.getVizNodeFromProcessor (used in the 'should return an `onFallback` node if defined' spec) to also assert that vizNode.getChildren()?.[2].data.isPlaceholder is falsy so the onFallback node is confirmed to be a non-placeholder instance (keep the existing path assertion and add the isPlaceholder falsy assertion).packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.ts (1)
66-77: Rename test case to match actual placeholder behavior.The test name says “should not return an
otherwisenode”, but assertions now expect anotherwiseplaceholder node. A clearer name will avoid confusion.✏️ Suggested rename
- it('should not return an `otherwise` node if not defined', () => { + it('should return an `otherwise` placeholder if not defined', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.ts` around lines 66 - 77, The test name is misleading: it asserts that an `otherwise` placeholder node is returned but is named "should not return an `otherwise` node"; rename the test to reflect expected behavior. Update the it(...) description in choice-node-mapper.test.ts to something like "should return an `otherwise` placeholder node when not defined" so it matches the assertions that call mapper.getVizNodeFromProcessor and inspect routeDefinition.from.steps[0].choice.otherwise and vizNode.getChildren().packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts (1)
208-212: Expectation update looks consistent with the simplified drag rule.This test now correctly reflects the current
canDragGroupbehavior for non-top-level groups.
Consider adding one more test for an empty/undefinedpathcase if you apply the guard incanDragGroup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts` around lines 208 - 212, Add a test to cover an empty or undefined path case for canDragGroup: create one or two cases using getMockGroupVizNode with path set to '' and undefined and assert canDragGroup(...) returns false (or the guarded behavior you implement) so the test suite verifies the new guard in canDragGroup for missing/empty paths.packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.ts (1)
114-119: Avoid mutating shared mock nodes in-test.Lines 116-117 modify module-scoped fixtures, which can leak state into later tests. Prefer local nodes in this test case.
Suggested refactor
it('should return true for compatible nodes in case of placeholder node', () => { const mockValidate = jest.fn().mockReturnValue(true); - vizNode1.data.name = 'log'; - placeholderNode.data.name = 'placeholder'; - const result = checkNodeDropCompatibility(vizNode1, placeholderNode, mockValidate); + const draggedNode = getMockVizNode('route.from.steps.0.log'); + const droppedPlaceholderNode = getMockVizNode('route.from.steps.3.placeholder'); + draggedNode.data.name = 'log'; + droppedPlaceholderNode.data.isPlaceholder = true; + droppedPlaceholderNode.data.name = 'placeholder'; + const result = checkNodeDropCompatibility(draggedNode, droppedPlaceholderNode, mockValidate); expect(result).toBe(true); expect(mockValidate).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.ts` around lines 114 - 119, This test mutates module-scoped fixtures (vizNode1 and placeholderNode); instead create local copies inside the test (e.g., clone vizNode1 and placeholderNode) and modify the copies' data.name values before calling checkNodeDropCompatibility with mockValidate so shared state isn't mutated and other tests won't be affected.packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsx (1)
126-171: Please add tests for the new options path.Current updates assert the 4th argument exists, but there’s still no direct coverage that:
predefinedComponentbypassesgetNewComponent, andinsertAtStart: trueis forwarded toaddBaseEntityStep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsx` around lines 126 - 171, Add tests exercising the "options" path of useInsertStep/onInsertStep: write one test that calls useInsertStep with an options object containing predefinedComponent set to a mock component and ensure that onInsertStep uses that predefinedComponent and does NOT call mockCatalogModalContext.getNewComponent; and write another test (or extend the same) that passes options with insertAtStart: true and assert mockVizNode.addBaseEntityStep was called with the insertAtStart flag forwarded (fourth argument true) to addBaseEntityStep. Reference the hook/useInsertStep, the handler onInsertStep, the modal method getNewComponent, and the node method addBaseEntityStep to locate where to assert behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsx`:
- Around line 47-52: The guard that clears the graph for special-child
insertions is too broad: instead of checking only property.type ===
'array-clause' in the stepsProperties.some(...) call, narrow the condition to
also match the specific property/component being inserted (compare the
property's name/identifier to the insertion target) so only the relevant
array-clause triggers controller.fromModel(...). Locate the
AddStepMode.InsertSpecialChildStep branch, the call to
CamelComponentSchemaService.getProcessorStepsProperties, and the
stepsProperties.some(...) predicate, and add a check that property.name (or the
schema's property identifier) equals the inserted target name (from the current
insertion context) before invoking controller.fromModel.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts`:
- Around line 121-123: Update the stale comment above the undefined check in
base-node-mapper.ts to reflect that when getValue(entityDefinition, path) is
undefined we now return a placeholder node; specifically, change the comment
that currently says "we don't create a IVisualizationNode for it" to indicate we
create and return a placeholder via this.getPlaceHolderNodeForProcessor(path),
so the comment matches the behavior of the undefined check and the
getPlaceHolderNodeForProcessor call.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.ts`:
- Line 44: The forEach callback currently returns the expect(...) value
(children?.forEach((c) => expect(c.data.isPlaceholder).toBe(true))); change the
arrow callback to a block so it returns void: replace the concise arrow with a
block arrow (children?.forEach((c) => { expect(c.data.isPlaceholder).toBe(true);
})), keeping the same variable name c and the same assertion on
c.data.isPlaceholder to satisfy ESLint's no-array-callback-return rule.
---
Outside diff comments:
In `@packages/ui/src/components/Visualization/Custom/customComponentUtils.ts`:
- Around line 55-60: The canDragGroup function currently returns true
unconditionally if the path check doesn't match, allowing nodes with missing or
empty groupVizNode.data.path to be draggable; update canDragGroup to first guard
against a missing/null/empty path (e.g., check that groupVizNode.data.path is a
non-empty string) and return false in that case, then perform the existing
split('.') length === 1 check on the validated path and return accordingly so
only nodes with valid, multi-segment paths are draggable.
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`:
- Around line 124-126: Move the vizNode null-guard out of PlaceholderNodeInner
so hooks run unconditionally: make PlaceholderNode responsible for returning
null when vizNode is falsy and change PlaceholderNodeInner to require a non-null
vizNode prop; then remove the early return inside PlaceholderNodeInner and keep
calls to useReplaceStep and useInsertStep (and any other hooks) at the top of
PlaceholderNodeInner so they are always invoked in the same order. Update any
callers to pass vizNode only when non-null into PlaceholderNodeInner via
PlaceholderNode.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts`:
- Around line 61-71: The test name is misleading: update the 'it' description in
circuit-breaker-node-mapper.test.ts to reflect that a placeholder onFallback
node is returned when onFallback is undefined (e.g., rename "should not return
an `onFallback` node if not defined" to "should return a placeholder
`onFallback` node if not defined"), and keep the existing assertions that call
mapper.getVizNodeFromProcessor and inspect
routeDefinition.from.steps[0].circuitBreaker!.onFallback and
vizNode.getChildren().
---
Nitpick comments:
In `@packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsx`:
- Line 56: Replace the non-null assertion on the test node lookup to fail fast
with a clear message: instead of using the `!` after `nodes.find(...)` for the
ID 'test|route.from.steps.1.choice', check the result of `nodes.find` assigned
to `selectedNode` and throw or use an explicit test assertion (e.g.,
expect/throw) if it's undefined so the test fails with a descriptive message
mentioning `selectedNode` and the target id; update any subsequent uses of
`selectedNode` to rely on this validated variable.
In
`@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts`:
- Around line 208-212: Add a test to cover an empty or undefined path case for
canDragGroup: create one or two cases using getMockGroupVizNode with path set to
'' and undefined and assert canDragGroup(...) returns false (or the guarded
behavior you implement) so the test suite verifies the new guard in canDragGroup
for missing/empty paths.
In
`@packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsx`:
- Around line 126-171: Add tests exercising the "options" path of
useInsertStep/onInsertStep: write one test that calls useInsertStep with an
options object containing predefinedComponent set to a mock component and ensure
that onInsertStep uses that predefinedComponent and does NOT call
mockCatalogModalContext.getNewComponent; and write another test (or extend the
same) that passes options with insertAtStart: true and assert
mockVizNode.addBaseEntityStep was called with the insertAtStart flag forwarded
(fourth argument true) to addBaseEntityStep. Reference the hook/useInsertStep,
the handler onInsertStep, the modal method getNewComponent, and the node method
addBaseEntityStep to locate where to assert behavior.
In
`@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.ts`:
- Around line 114-119: This test mutates module-scoped fixtures (vizNode1 and
placeholderNode); instead create local copies inside the test (e.g., clone
vizNode1 and placeholderNode) and modify the copies' data.name values before
calling checkNodeDropCompatibility with mockValidate so shared state isn't
mutated and other tests won't be affected.
In
`@packages/ui/src/models/visualization/flows/camel-route-visual-entity.test.ts`:
- Line 354: Tests rely on positional indexing whenNode =
choiceNode.getChildren()?.[1] which is brittle; replace fixed-index child access
with a path/label/type-based lookup: add or use a helper to traverse children
(e.g., choiceNode.getChildren()?.find(c => c.label === 'when' || c.type ===
'WhenNode' || c.meta?.role === 'when')) and use that result for whenNode (apply
same change for other occurrences referenced in the comment), ensuring you
null-check and assert the found node exists before proceeding.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.ts`:
- Around line 66-77: The test name is misleading: it asserts that an `otherwise`
placeholder node is returned but is named "should not return an `otherwise`
node"; rename the test to reflect expected behavior. Update the it(...)
description in choice-node-mapper.test.ts to something like "should return an
`otherwise` placeholder node when not defined" so it matches the assertions that
call mapper.getVizNodeFromProcessor and inspect
routeDefinition.from.steps[0].choice.otherwise and vizNode.getChildren().
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts`:
- Around line 55-59: The test currently asserts that an `onFallback` child node
path exists but doesn't verify its placeholder state; update the test for
mapper.getVizNodeFromProcessor (used in the 'should return an `onFallback` node
if defined' spec) to also assert that
vizNode.getChildren()?.[2].data.isPlaceholder is falsy so the onFallback node is
confirmed to be a non-placeholder instance (keep the existing path assertion and
add the isPlaceholder falsy assertion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e716ec06-c357-435b-97e2-a8c8e8ddaea9
⛔ Files ignored due to path filters (4)
packages/ui/src/components/Visualization/Canvas/Form/__snapshots__/CanvasForm.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Custom/Node/__snapshots__/PlaceholderNode.test.tsx.snapis excluded by!**/*.snappackages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (27)
packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsxpackages/ui/src/components/Visualization/Canvas/canvas.defaults.tspackages/ui/src/components/Visualization/Canvas/flow.service.tspackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.tspackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.tspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.scsspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsxpackages/ui/src/components/Visualization/Custom/customComponentUtils.test.tspackages/ui/src/components/Visualization/Custom/customComponentUtils.tspackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsxpackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsxpackages/ui/src/models/visualization/base-visual-entity.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-visual-entity.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-verbs-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.tspackages/ui/src/models/visualization/visualization-node.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx (1)
124-138:⚠️ Potential issue | 🟠 MajorFix conditional hook ordering violation.
useReplaceStepanduseInsertStepare called after the early return at line 124, which violates React's Rules of Hooks. IfvizNodeflips between undefined and defined across renders, the hook call count and order will change, causing React errors or unpredictable behavior.Extract the code after the
vizNodecheck into a separate component that guarantees these hooks are always called at the top level:Suggested refactor
const PlaceholderNodeInner: FunctionComponent<PlaceholderNodeInnerProps> = observer(({ element }) => { if (!isNode(element)) { throw new Error('PlaceholderNodeInner must be used only on Node elements'); } const vizNode: IVisualizationNode | undefined = element.getData()?.vizNode; const settingsAdapter = useContext(SettingsContext); const entitiesContext = useEntityContext(); const catalogModalContext = useContext(CatalogModalContext); const label = vizNode?.getNodeLabel(settingsAdapter.getSettings().nodeLabel); const updatedLabel = label === 'placeholder' ? 'Add step' : `Add ${label}`; const boxRef = useRef<Rect | null>(null); const boxXRef = useRef<number | null>(null); const boxYRef = useRef<number | null>(null); useAnchor((element: Node) => { return new TargetAnchor(element); }, AnchorEnd.both); - if (!vizNode) { - return null; - } - const { onReplaceNode } = useReplaceStep(vizNode); - const { onInsertStep } = useInsertStep(insertStepTargetNode, AddStepMode.InsertSpecialChildStep, insertStepOptions); + if (!vizNode) return null; + return <PlaceholderNodeContent element={element} vizNode={vizNode} />; }); + +const PlaceholderNodeContent: FunctionComponent<{ + element: GraphElement<ElementModel, CanvasNode['data']>; + vizNode: IVisualizationNode; +}> = observer(({ element, vizNode }) => { + const { onReplaceNode } = useReplaceStep(vizNode); + const isSpecialPlaceholder = vizNode.data.name !== 'placeholder' && vizNode.data.name !== 'placeholder-special-child'; + const parentVizNode = vizNode.getParentNode(); + const insertStepTargetNode = isSpecialPlaceholder ? (parentVizNode ?? vizNode) : vizNode; + const insertStepOptions = isSpecialPlaceholder + ? { + predefinedComponent: { name: vizNode.data.name, type: CatalogKind.Processor }, + insertAtStart: true, + } + : undefined; + const { onInsertStep } = useInsertStep(insertStepTargetNode, AddStepMode.InsertSpecialChildStep, insertStepOptions); // ... rest of component +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx` around lines 124 - 138, The hooks useReplaceStep and useInsertStep are being called conditionally after the early return when vizNode is falsy; extract the logic that uses these hooks into a new child component (e.g., PlaceholderNodeInner) that accepts vizNode (and any needed props) and call useReplaceStep and useInsertStep at the top level inside that new component so hooks are always invoked in a stable order; in PlaceholderNode render return null when vizNode is falsy, otherwise render <PlaceholderNodeInner vizNode={vizNode} /> and move all code that references vizNode, parentVizNode, isSpecialPlaceholder, insertStepOptions, onReplaceNode, and onInsertStep into PlaceholderNodeInner.
♻️ Duplicate comments (1)
packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsx (1)
51-51:⚠️ Potential issue | 🟠 MajorGraph-reset predicate is still too broad for special-child insertions.
Line 51 currently clears the graph when any array-clause exists. This can reset the model for unrelated insertions; scope it to the inserted component/property.
Suggested fix
- if (stepsProperties.some((property) => property.type === 'array-clause' && isDefined(vizNode.getChildren()))) { + if ( + stepsProperties.some( + (property) => + property.type === 'array-clause' && + property.name === definedComponent.name && + isDefined(vizNode.getChildren()), + ) + ) { controller.fromModel({ nodes: [], edges: [], }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsx` at line 51, The graph-reset predicate is too broad: replace the existing check that uses stepsProperties.some((property) => property.type === 'array-clause' && isDefined(vizNode.getChildren())) with a scoped check that only matches array-clause entries tied to the specific insertion target (the vizNode/property being inserted). Concretely, update the predicate to verify the array-clause's target identifier or parent (e.g., property.targetId, property.parentId, or property.propertyName) matches the current insertion target (e.g., vizNode.id or the insertedProperty identifier) in addition to type==='array-clause' and isDefined(vizNode.getChildren()), so the graph is cleared only when the array-clause is for the component/property being inserted.
🧹 Nitpick comments (3)
packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts (1)
61-71: Test name is now misleading after behavior change.The test is named "should not return an
onFallbacknode if not defined", but the updated assertions now verify that anonFallbackplaceholder node is returned (at index 2 withisPlaceholder: true). Consider renaming this test to accurately reflect the new expected behavior.✏️ Suggested rename
- it('should not return an `onFallback` node if not defined', () => { + it('should return an `onFallback` placeholder node if onFallback is not defined', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts` around lines 61 - 71, The test "should not return an `onFallback` node if not defined" is now misleading because the assertions for mapper.getVizNodeFromProcessor expect an onFallback placeholder (vizNode.getChildren() index 2 with data.path 'from.steps.0.circuitBreaker.onFallback' and data.isPlaceholder true); rename the test to reflect the new behavior (for example "should return an `onFallback` placeholder node when onFallback is undefined") and update the test title string accordingly so it matches the assertions that reference routeDefinition.from.steps[0].circuitBreaker.onFallback and vizNode.getChildren.packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts (1)
5-5: Refactor tests to remove stale schema-service coupling.
canDragGroupno longer usesCamelComponentSchemaService, but this test suite still spies/mocks it. Dropping that setup will make these tests less brittle and more behavior-focused.♻️ Proposed cleanup
-import { CamelComponentSchemaService } from '../../../models/visualization/flows/support/camel-component-schema.service'; import { CamelRouteVisualEntityData } from '../../../models/visualization/flows/support/camel-component-types'; @@ - beforeEach(() => { - jest.spyOn(CamelComponentSchemaService, 'getProcessorStepsProperties').mockReturnValue([]); - }); - - afterEach(() => { - jest.restoreAllMocks(); - }); - it('should return false when groupVizNode is undefined', () => { expect(canDragGroup()).toBe(false); - expect(CamelComponentSchemaService.getProcessorStepsProperties).not.toHaveBeenCalled(); }); @@ it('should return false when path is top-level (single segment)', () => { const groupVizNode = getMockGroupVizNode('Route', 'route'); expect(canDragGroup(groupVizNode)).toBe(false); - expect(CamelComponentSchemaService.getProcessorStepsProperties).not.toHaveBeenCalled(); }); @@ - it('should return true when group does not match single-clause property', () => { - (CamelComponentSchemaService.getProcessorStepsProperties as jest.Mock).mockReturnValue([ - { name: 'otherwise', type: 'single-clause' }, - { name: 'when', type: 'array-clause' }, - ]); + it('should return true for non-top-level groups', () => { const groupVizNode = getMockGroupVizNode('route.from.steps.0.choice.when', 'when', 'choice'); expect(canDragGroup(groupVizNode)).toBe(true); });Also applies to: 188-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts` at line 5, The tests are still importing and spying on CamelComponentSchemaService even though canDragGroup no longer depends on it; remove the import of CamelComponentSchemaService and any setup/teardown spy/mock code that references it (including the spyOn/mockRestore calls and fixture data in the customComponentUtils.test.ts suite) and update the tests that referenced that mock so they assert behavior of canDragGroup directly (call canDragGroup with appropriate inputs and expect the same outputs) rather than relying on the schema service mock.packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts (1)
149-156: Minor: Remove redundant template literals.The template literals on lines 150 and 154 are unnecessary since
pathis already a string.Proposed simplification
protected getPlaceHolderNodeForProcessor(path: string): IVisualizationNode { - return createVisualizationNode(`${path}`, { + return createVisualizationNode(path, { catalogKind: CatalogKind.Entity, name: path.split('.').pop() as keyof ProcessorDefinition, isPlaceholder: true, - path: `${path}`, + path, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts` around lines 149 - 156, In getPlaceHolderNodeForProcessor (base-node-mapper.ts) remove the redundant template literals around path passed to createVisualizationNode and the path property—use the plain path string instead (i.e., pass path and set path: path) while keeping name: path.split('.').pop() as keyof ProcessorDefinition and isPlaceholder: true so createVisualizationNode and the returned IVisualizationNode behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts`:
- Around line 82-84: The unconditional call to draggedVizNode.removeChild()
bypasses the existing removal guards; update the block that calls
parentVizNode.pasteBaseEntityStep(...) so that draggedVizNode.removeChild() is
executed only when the node is allowed to be removed—i.e., check the same
removal predicates used elsewhere (draggedVizNode.canRemoveStep() and/or
draggedVizNode.canRemoveFlow()) before calling removeChild(); ensure the
conditional matches the other code paths' logic so special-placeholder cleanup
honors interaction constraints while still calling
parentVizNode.pasteBaseEntityStep(...).
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.ts`:
- Line 63: The test currently asserts only vizNode.getChildren()?.[3].data.path,
which doesn't ensure the `otherwise` branch is a real node; update the assertion
to also check that the node is not a placeholder by asserting
vizNode.getChildren()?.[3].data.isPlaceholder is false (alongside the existing
path assertion) so the test verifies a defined `otherwise` branch rather than a
placeholder.
- Around line 66-77: The test name "should not return an `otherwise` node if not
defined" is misleading because the assertions on mapper.getVizNodeFromProcessor
and vizNode.getChildren confirm an `otherwise` placeholder is returned when
routeDefinition.from.steps[0].choice.otherwise is undefined; rename the test to
reflect that behavior (e.g., "should return an `otherwise` placeholder when
otherwise is not defined") so it matches the assertions referencing
getVizNodeFromProcessor, vizNode.getChildren, and the choice.otherwise path
checks.
---
Outside diff comments:
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`:
- Around line 124-138: The hooks useReplaceStep and useInsertStep are being
called conditionally after the early return when vizNode is falsy; extract the
logic that uses these hooks into a new child component (e.g.,
PlaceholderNodeInner) that accepts vizNode (and any needed props) and call
useReplaceStep and useInsertStep at the top level inside that new component so
hooks are always invoked in a stable order; in PlaceholderNode render return
null when vizNode is falsy, otherwise render <PlaceholderNodeInner
vizNode={vizNode} /> and move all code that references vizNode, parentVizNode,
isSpecialPlaceholder, insertStepOptions, onReplaceNode, and onInsertStep into
PlaceholderNodeInner.
---
Duplicate comments:
In `@packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsx`:
- Line 51: The graph-reset predicate is too broad: replace the existing check
that uses stepsProperties.some((property) => property.type === 'array-clause' &&
isDefined(vizNode.getChildren())) with a scoped check that only matches
array-clause entries tied to the specific insertion target (the vizNode/property
being inserted). Concretely, update the predicate to verify the array-clause's
target identifier or parent (e.g., property.targetId, property.parentId, or
property.propertyName) matches the current insertion target (e.g., vizNode.id or
the insertedProperty identifier) in addition to type==='array-clause' and
isDefined(vizNode.getChildren()), so the graph is cleared only when the
array-clause is for the component/property being inserted.
---
Nitpick comments:
In
`@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts`:
- Line 5: The tests are still importing and spying on
CamelComponentSchemaService even though canDragGroup no longer depends on it;
remove the import of CamelComponentSchemaService and any setup/teardown spy/mock
code that references it (including the spyOn/mockRestore calls and fixture data
in the customComponentUtils.test.ts suite) and update the tests that referenced
that mock so they assert behavior of canDragGroup directly (call canDragGroup
with appropriate inputs and expect the same outputs) rather than relying on the
schema service mock.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts`:
- Around line 149-156: In getPlaceHolderNodeForProcessor (base-node-mapper.ts)
remove the redundant template literals around path passed to
createVisualizationNode and the path property—use the plain path string instead
(i.e., pass path and set path: path) while keeping name: path.split('.').pop()
as keyof ProcessorDefinition and isPlaceholder: true so createVisualizationNode
and the returned IVisualizationNode behavior is unchanged.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts`:
- Around line 61-71: The test "should not return an `onFallback` node if not
defined" is now misleading because the assertions for
mapper.getVizNodeFromProcessor expect an onFallback placeholder
(vizNode.getChildren() index 2 with data.path
'from.steps.0.circuitBreaker.onFallback' and data.isPlaceholder true); rename
the test to reflect the new behavior (for example "should return an `onFallback`
placeholder node when onFallback is undefined") and update the test title string
accordingly so it matches the assertions that reference
routeDefinition.from.steps[0].circuitBreaker.onFallback and vizNode.getChildren.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bf19ced-9a9b-41d1-ad5f-c7f32fa983f9
⛔ Files ignored due to path filters (4)
packages/ui/src/components/Visualization/Canvas/Form/__snapshots__/CanvasForm.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Custom/Node/__snapshots__/PlaceholderNode.test.tsx.snapis excluded by!**/*.snappackages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (27)
packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsxpackages/ui/src/components/Visualization/Canvas/canvas.defaults.tspackages/ui/src/components/Visualization/Canvas/flow.service.tspackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.tspackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.tspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.scsspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsxpackages/ui/src/components/Visualization/Custom/customComponentUtils.test.tspackages/ui/src/components/Visualization/Custom/customComponentUtils.tspackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsxpackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsxpackages/ui/src/models/visualization/base-visual-entity.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-visual-entity.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-verbs-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.tspackages/ui/src/models/visualization/visualization-node.ts
shivamG640
force-pushed
the
fix_2934
branch
2 times, most recently
from
5bdc3a5 to
d10479d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx (1)
124-138:⚠️ Potential issue | 🔴 CriticalHooks called conditionally - violates Rules of Hooks.
The
useInsertStephook at line 138 is called after the early return at line 125. This violates React's Rules of Hooks, which require hooks to be called in the same order on every render. WhenvizNodeis undefined, the early return preventsuseInsertStepfrom being called, causing inconsistent hook call order.The static analysis tool (Biome) also flags this issue.
🐛 Proposed fix: Move hooks before early return
const label = vizNode?.getNodeLabel(settingsAdapter.getSettings().nodeLabel); const updatedLabel = label === 'placeholder' ? 'Add step' : `Add ${label}`; const boxRef = useRef<Rect | null>(null); const boxXRef = useRef<number | null>(null); const boxYRef = useRef<number | null>(null); useAnchor((element: Node) => { return new TargetAnchor(element); }, AnchorEnd.both); + const isSpecialPlaceholder = vizNode?.data.name !== 'placeholder' && vizNode?.data.name !== 'placeholder-special-child'; + const isSpecialChildPlaceholder = vizNode?.data.name === 'placeholder-special-child'; + const parentVizNode = vizNode?.getParentNode(); + const insertStepTargetNode = isSpecialPlaceholder ? (parentVizNode ?? vizNode) : vizNode; + const insertStepOptions = isSpecialPlaceholder + ? { + predefinedComponent: { name: vizNode?.data.name ?? '', type: CatalogKind.Processor }, + insertAtStart: true, + } + : undefined; + const { onInsertStep } = useInsertStep(insertStepTargetNode!, AddStepMode.InsertSpecialChildStep, insertStepOptions); + const { onReplaceNode } = useReplaceStep(vizNode!); + if (!vizNode) { return null; } - const { onReplaceNode } = useReplaceStep(vizNode); - const isSpecialPlaceholder = vizNode.data.name !== 'placeholder' && vizNode.data.name !== 'placeholder-special-child'; - const isSpecialChildPlaceholder = vizNode.data.name === 'placeholder-special-child'; - const parentVizNode = vizNode.getParentNode(); - const insertStepTargetNode = isSpecialPlaceholder ? (parentVizNode ?? vizNode) : vizNode; - const insertStepOptions = isSpecialPlaceholder - ? { - predefinedComponent: { name: vizNode.data.name, type: CatalogKind.Processor }, - insertAtStart: true, - } - : undefined; - const { onInsertStep } = useInsertStep(insertStepTargetNode, AddStepMode.InsertSpecialChildStep, insertStepOptions); const tooltipContent =Note: The fix requires adding optional chaining (
?.) forvizNodeaccesses before the guard, and non-null assertions (!) when passing to hooks since they'll only be used after the guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx` around lines 124 - 138, The hooks are being called conditionally; move the calls to useReplaceStep and useInsertStep above the early return so hooks run unconditionally, computing their inputs with safe accessors (e.g., vizNode?.data?.name and vizNode?.getParentNode()) and build insertStepTargetNode/insertStepOptions using optional chaining; after the early return (where vizNode is guaranteed), pass vizNode! (non-null assertion) or the already-computed values into useReplaceStep/useInsertStep usages so the hook order stays stable while preserving the original behavior.
♻️ Duplicate comments (1)
packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts (1)
73-84:⚠️ Potential issue | 🟠 MajorAlign special-placeholder cleanup with removal constraints.
At Line 83,
draggedVizNode.removeChild()is called unconditionally, while theperformForwardDrophelper (lines 162-167) properly checkscanRemoveStep/canRemoveFlowbefore removing. This inconsistency could lead to unexpected behavior if the dragged node's interaction constraints don't permit removal.🔧 Proposed fix
if (isSpecialChildPlaceholder) { const parentVizNode = droppedVizNode.getParentNode(); if (!parentVizNode) return; parentVizNode.pasteBaseEntityStep(draggedNodeContent, AddStepMode.InsertSpecialChildStep, true); - draggedVizNode.removeChild(); + const draggedVizNodeInteraction = draggedVizNode.getNodeInteraction(); + if (draggedVizNodeInteraction.canRemoveStep) { + draggedVizNode.removeChild(); + } else if (draggedVizNodeInteraction.canRemoveFlow) { + const flowId = draggedVizNode?.getId(); + entitiesContext.camelResource.removeEntity(flowId ? [flowId] : undefined); + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts` around lines 73 - 84, The code unconditionally calls draggedVizNode.removeChild() after parentVizNode.pasteBaseEntityStep(...) which can violate removal constraints; update the special-placeholder branch to mirror performForwardDrop by checking the dragged node's interaction (e.g., draggedVizNode.getNodeInteraction().canRemoveStep and/or .canRemoveFlow) before calling draggedVizNode.removeChild(), and only remove when those checks pass (leave the paste call as-is and do not change parentVizNode.pasteBaseEntityStep or AddStepMode.InsertSpecialChildStep).
🧹 Nitpick comments (7)
packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts (1)
61-70: Rename this test to match the placeholder behavior.Line 61 now reads opposite to what Lines 69-70 assert: the mapper does return an
onFallbackchild here, but as a placeholder. Renaming it to something likeshould return an \onFallback` placeholder if not defined` would make failures much easier to interpret.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts` around lines 61 - 70, The test name is misleading: it currently asserts that an onFallback child is returned as a placeholder but reads "should not return an `onFallback` node if not defined"; rename the test to reflect the actual behavior, e.g., change the it(...) description to "should return an `onFallback` placeholder if not defined", keeping the existing assertions that use mapper.getVizNodeFromProcessor, routeDefinition.from.steps[0].circuitBreaker!.onFallback, and checks on vizNode.getChildren() indices 0–2.packages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.ts (1)
587-600: Make this regression test independent of sibling order.
routeNode.getChildren()?.[1]hard-codes the current layout order, which is exactly what this PR is changing. It would be safer to find thechoicenode by identity and assert that its children are placeholders, not just that there are two of them.🧪 Suggested hardening
- it('should keep isGroup flag when a group has no children since it has placeholders', () => { + it('should keep isGroup when a group has no real children because placeholder children are present', () => { const routeEntity = new CamelRouteVisualEntity({ route: { id: 'route-1234', from: { uri: 'timer:clock', steps: [{ choice: {} }] }, }, }); const routeNode = routeEntity.toVizNode(); - const choiceNode = routeNode.getChildren()?.[1]; + const choiceNode = routeNode.getChildren()?.find((child) => child.data.processorName === 'choice'); + const choiceChildren = choiceNode?.getChildren() ?? []; - // Choice has when and otherwise placeholders as children, so it remains a group (isGroup true) expect(choiceNode?.data.isGroup).toBe(true); - expect(choiceNode?.getChildren()?.length).toEqual(2); + expect(choiceChildren).toHaveLength(2); + expect(choiceChildren.every((child) => child.data.isPlaceholder)).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.ts` around lines 587 - 600, The test currently assumes a fixed sibling order by accessing the choice node via routeNode.getChildren()?.[1]; instead locate the choice node by identity/type from routeNode.getChildren() (e.g., find child where node.data.type === 'choice' or node.data.node?.choice exists) after creating the CamelRouteVisualEntity and calling toVizNode; then assert that that found choice node has data.isGroup === true and that its children are the expected placeholder nodes (verify children exist and that each child is a placeholder via their data.placeholder flag or node type) instead of relying on a specific index.packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts (2)
214-222: Remove or simplify this test - mock is never exercised.The mock setup at line 215-218 for
getProcessorStepsPropertiesis never called by the implementation since that logic was removed. This test now only verifies that a non-top-level path returnstrue, which is the same assertion as the test above it.Consider either removing this test (it's redundant with the test at lines 208-212) or repurposing it to test a distinct scenario.
♻️ Suggested change
- it('should return true when group does not match single-clause property', () => { - (CamelComponentSchemaService.getProcessorStepsProperties as jest.Mock).mockReturnValue([ - { name: 'otherwise', type: 'single-clause' }, - { name: 'when', type: 'array-clause' }, - ]); - const groupVizNode = getMockGroupVizNode('route.from.steps.0.choice.when', 'when', 'choice'); - - expect(canDragGroup(groupVizNode)).toBe(true); - }); + it('should return true for deeply nested group paths', () => { + const groupVizNode = getMockGroupVizNode('route.from.steps.0.choice.when.0.steps'); + + expect(canDragGroup(groupVizNode)).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts` around lines 214 - 222, The test in customComponentUtils.test.ts is redundant because the mocked CamelComponentSchemaService.getProcessorStepsProperties is no longer used by canDragGroup; remove or repurpose this test: either delete the it('should return true when group does not match single-clause property') case entirely, or change it to exercise a distinct behavior of canDragGroup (e.g., a top-level path or a scenario where getProcessorStepsProperties is actually consulted) by using getMockGroupVizNode and invoking canDragGroup accordingly; ensure you also remove the unused mock setup for CamelComponentSchemaService.getProcessorStepsProperties if you delete the test.
188-194: Remove dead mock setup forgetProcessorStepsProperties.Since the implementation of
canDragGroupno longer callsCamelComponentSchemaService.getProcessorStepsProperties, this mock setup inbeforeEach/afterEachis dead code. The assertions at lines 198 and 205 (checking it was not called) are now trivially true and test nothing meaningful.Consider removing this mock setup and updating the test structure to reflect the simplified implementation.
♻️ Proposed cleanup
-import { CamelComponentSchemaService } from '../../../models/visualization/flows/support/camel-component-schema.service'; -import { CamelRouteVisualEntityData } from '../../../models/visualization/flows/support/camel-component-types';describe('canDragGroup', () => { - const getMockGroupVizNode = (path: string, name: string, parentProcessorName?: string): IVisualizationNode => { - const parentData = parentProcessorName - ? ({ processorName: parentProcessorName } as CamelRouteVisualEntityData) - : undefined; + const getMockGroupVizNode = (path: string): IVisualizationNode => { return { id: 'group-1', - data: { path, name }, - getParentNode: jest.fn().mockReturnValue(parentData ? { data: parentData } : undefined), + data: { path }, } as unknown as IVisualizationNode; }; - beforeEach(() => { - jest.spyOn(CamelComponentSchemaService, 'getProcessorStepsProperties').mockReturnValue([]); - }); - - afterEach(() => { - jest.restoreAllMocks(); - }); - it('should return false when groupVizNode is undefined', () => { expect(canDragGroup()).toBe(false); - expect(CamelComponentSchemaService.getProcessorStepsProperties).not.toHaveBeenCalled(); }); it('should return false when path is top-level (single segment)', () => { - const groupVizNode = getMockGroupVizNode('Route', 'route'); + const groupVizNode = getMockGroupVizNode('Route'); expect(canDragGroup(groupVizNode)).toBe(false); - expect(CamelComponentSchemaService.getProcessorStepsProperties).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts` around lines 188 - 194, Remove the dead mock setup for CamelComponentSchemaService.getProcessorStepsProperties in the test file: delete the beforeEach that spies and returns [] and the afterEach restore that only pertains to that spy; update assertions referencing getProcessorStepsProperties (e.g., the expects at lines around where canDragGroup is tested) so they are removed or replaced with meaningful checks for canDragGroup behavior; ensure remaining setup only mocks functions actually used by canDragGroup and restore those mocks in afterEach if needed.packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts (1)
119-138: Complex placeholder validation logic may benefit from comments.The nested conditionals for placeholder drop validation cover several edge cases (same-name placeholders, generic placeholders, circular drops). Consider adding brief inline comments to clarify the intent of each branch for future maintainers.
💡 Suggested documentation
// validation for placeholder nodes if (droppedVizNode.data.isPlaceholder) { + // Same-type placeholder: allow only if from different parent (cross-choice drop) if (droppedVizNode.data.name === draggedVizNode?.data.name) { const draggedParent = draggedVizNode.getParentNode(); const droppedParent = droppedVizNode.getParentNode(); return !(draggedParent?.id === droppedParent?.id && draggedParent?.getId() === droppedParent?.getId()); } + // Generic placeholder: validate as ReplaceStep unless it's a circular drop if (droppedVizNode.data.name === 'placeholder' && droppedVizNode.getPreviousNode() !== draggedVizNode) { if ( droppedVizNode.data.path?.includes(draggedVizNode.data.path ?? '') && droppedVizNode.getId() === draggedVizNode.getId() ) return false; return validate(AddStepMode.ReplaceStep, droppedVizNode, droppedVizNodeContent.name); } + // All other placeholder scenarios are invalid return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts` around lines 119 - 138, The placeholder validation block around if (droppedVizNode.data.isPlaceholder) is dense; add brief inline comments explaining each branch: annotate the first branch that checks droppedVizNode.data.name === draggedVizNode?.data.name to explain it prevents no-op moves within the same parent by comparing draggedParent/getParentNode()/id; annotate the second branch for droppedVizNode.data.name === 'placeholder' that checks getPreviousNode(), path inclusion (droppedVizNode.data.path and draggedVizNode.data.path), and identical node id (getId()) to prevent circular or redundant drops before calling validate(AddStepMode.ReplaceStep, droppedVizNode, droppedVizNodeContent.name); and annotate the final return false to state it disallows other placeholder drops. Ensure comments reference droppedVizNode, draggedVizNode, getPreviousNode, getParentNode, getId, data.path, validate, and AddStepMode.ReplaceStep so future maintainers can quickly understand each edge case.packages/ui/src/models/visualization/flows/camel-route-visual-entity.test.ts (1)
351-353: Avoid hard-coding branch indices around placeholders.These assertions now couple the tests to the current placeholder insertion order (
[1]forwhen,[2]forotherwise). Looking up the child bydata.pathor label would keep the tests stable if the mapper changes the placeholder ordering without changing the graph semantics.Example refactor
- const whenNode = choiceNode.getChildren()?.[1]; + const whenNode = choiceNode.getChildren()?.find( + (child) => child.data.path === 'route.from.steps.1.choice.when.0', + ); - const otherwiseNode = choiceNode.getChildren()?.[2]; + const otherwiseNode = choiceNode.getChildren()?.find( + (child) => child.data.path === 'route.from.steps.1.choice.otherwise', + );Also applies to: 417-418, 439-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/camel-route-visual-entity.test.ts` around lines 351 - 353, The test currently assumes fixed child indices on choiceNode (using choiceNode.getChildren()?.[1] and similar) which couples it to placeholder ordering; update the assertions to locate branches by their identity instead of index—e.g., iterate choiceNode.getChildren() and find the child whose data.path or label matches the expected "when" or "otherwise" branch (use the child.data.path, child.getLabel(), or similar properties exposed by the node API) and then assert against that found node; replace all hard-coded index usages around choiceNode.getChildren() (including occurrences around lines 351-353, 417-418, 439-440) with this lookup approach so tests remain stable if placeholder ordering changes.packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsx (1)
197-215: Assert the special-child insertion contract, not just that an insert callback fired.These cases only prove that some
onInsertStephandler ran. They will still pass ifwhen/otherwiseplaceholders get wired to the wronguseInsertStepmode or options. Please also inspect the mockeduseInsertStepcall arguments, or verify the resulting inserted branch shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsx` around lines 197 - 215, The tests for clicking 'when'/'otherwise' placeholders only assert that mockOnInsertStep was called, not that it was called with the correct insertion mode/options; update the two specs (which use setupWithVizNode and target the placeholder-node__test-placeholder) to also assert the mocked useInsertStep arguments or the resulting inserted branch shape so the special-child contract is verified: e.g. inspect mockOnInsertStep.mock.calls[0] (or the mock for useInsertStep) and assert it contains the expected mode/flags for 'when' vs 'otherwise', and keep the existing asserts that mockOnReplaceNode was not called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts`:
- Around line 28-32: The test currently calls cy.checkNodeExist('otherwise', 2)
before and after cy.removeNodeByName('otherwise'), which doesn't prove the real
branch was removed; update the post-delete check to distinguish real branch
nodes from placeholders by either asserting the node count decreased (e.g.,
expecting 1 instead of 2), asserting absence with cy.checkNodeExist('otherwise',
0) or a negative assertion, or—preferably—querying the model/YAML after
cy.removeNodeByName('otherwise') (use whatever helper exists, e.g., a
getWorkflowYAML/getModel helper) and assert the actual branch entry for
'otherwise' is gone; keep references to cy.checkNodeExist and
cy.removeNodeByName so the change is localized around those calls.
In
`@packages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsx`:
- Around line 142-145: The drop handler in CustomGroupExpanded (the drop:
(_item, monitor, _props) => { ... }) only returns the group itself when
monitor.isOver({ shallow: true }), which causes drops onto nested special
PlaceholderNode to fall through; fix by resolving and returning a compatible
nested placeholder when the pointer is over a descendant: inside that drop
handler, when shallow is false (or when monitor.isOver returns true but not
shallow), inspect the group’s descendant nodes to find a compatible special
placeholder (using the same match logic as PlaceholderNode) and return that
placeholder element as the drop result; alternatively, ensure
PlaceholderNode.tsx's own drop handler returns itself from its drop method—pick
one approach and implement it so handleValidNodeDrop receives a valid dropResult
for nested placeholder drops.
---
Outside diff comments:
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`:
- Around line 124-138: The hooks are being called conditionally; move the calls
to useReplaceStep and useInsertStep above the early return so hooks run
unconditionally, computing their inputs with safe accessors (e.g.,
vizNode?.data?.name and vizNode?.getParentNode()) and build
insertStepTargetNode/insertStepOptions using optional chaining; after the early
return (where vizNode is guaranteed), pass vizNode! (non-null assertion) or the
already-computed values into useReplaceStep/useInsertStep usages so the hook
order stays stable while preserving the original behavior.
---
Duplicate comments:
In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts`:
- Around line 73-84: The code unconditionally calls draggedVizNode.removeChild()
after parentVizNode.pasteBaseEntityStep(...) which can violate removal
constraints; update the special-placeholder branch to mirror performForwardDrop
by checking the dragged node's interaction (e.g.,
draggedVizNode.getNodeInteraction().canRemoveStep and/or .canRemoveFlow) before
calling draggedVizNode.removeChild(), and only remove when those checks pass
(leave the paste call as-is and do not change parentVizNode.pasteBaseEntityStep
or AddStepMode.InsertSpecialChildStep).
---
Nitpick comments:
In
`@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts`:
- Around line 214-222: The test in customComponentUtils.test.ts is redundant
because the mocked CamelComponentSchemaService.getProcessorStepsProperties is no
longer used by canDragGroup; remove or repurpose this test: either delete the
it('should return true when group does not match single-clause property') case
entirely, or change it to exercise a distinct behavior of canDragGroup (e.g., a
top-level path or a scenario where getProcessorStepsProperties is actually
consulted) by using getMockGroupVizNode and invoking canDragGroup accordingly;
ensure you also remove the unused mock setup for
CamelComponentSchemaService.getProcessorStepsProperties if you delete the test.
- Around line 188-194: Remove the dead mock setup for
CamelComponentSchemaService.getProcessorStepsProperties in the test file: delete
the beforeEach that spies and returns [] and the afterEach restore that only
pertains to that spy; update assertions referencing getProcessorStepsProperties
(e.g., the expects at lines around where canDragGroup is tested) so they are
removed or replaced with meaningful checks for canDragGroup behavior; ensure
remaining setup only mocks functions actually used by canDragGroup and restore
those mocks in afterEach if needed.
In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts`:
- Around line 119-138: The placeholder validation block around if
(droppedVizNode.data.isPlaceholder) is dense; add brief inline comments
explaining each branch: annotate the first branch that checks
droppedVizNode.data.name === draggedVizNode?.data.name to explain it prevents
no-op moves within the same parent by comparing
draggedParent/getParentNode()/id; annotate the second branch for
droppedVizNode.data.name === 'placeholder' that checks getPreviousNode(), path
inclusion (droppedVizNode.data.path and draggedVizNode.data.path), and identical
node id (getId()) to prevent circular or redundant drops before calling
validate(AddStepMode.ReplaceStep, droppedVizNode, droppedVizNodeContent.name);
and annotate the final return false to state it disallows other placeholder
drops. Ensure comments reference droppedVizNode, draggedVizNode,
getPreviousNode, getParentNode, getId, data.path, validate, and
AddStepMode.ReplaceStep so future maintainers can quickly understand each edge
case.
In
`@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsx`:
- Around line 197-215: The tests for clicking 'when'/'otherwise' placeholders
only assert that mockOnInsertStep was called, not that it was called with the
correct insertion mode/options; update the two specs (which use setupWithVizNode
and target the placeholder-node__test-placeholder) to also assert the mocked
useInsertStep arguments or the resulting inserted branch shape so the
special-child contract is verified: e.g. inspect mockOnInsertStep.mock.calls[0]
(or the mock for useInsertStep) and assert it contains the expected mode/flags
for 'when' vs 'otherwise', and keep the existing asserts that mockOnReplaceNode
was not called.
In
`@packages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.ts`:
- Around line 587-600: The test currently assumes a fixed sibling order by
accessing the choice node via routeNode.getChildren()?.[1]; instead locate the
choice node by identity/type from routeNode.getChildren() (e.g., find child
where node.data.type === 'choice' or node.data.node?.choice exists) after
creating the CamelRouteVisualEntity and calling toVizNode; then assert that that
found choice node has data.isGroup === true and that its children are the
expected placeholder nodes (verify children exist and that each child is a
placeholder via their data.placeholder flag or node type) instead of relying on
a specific index.
In
`@packages/ui/src/models/visualization/flows/camel-route-visual-entity.test.ts`:
- Around line 351-353: The test currently assumes fixed child indices on
choiceNode (using choiceNode.getChildren()?.[1] and similar) which couples it to
placeholder ordering; update the assertions to locate branches by their identity
instead of index—e.g., iterate choiceNode.getChildren() and find the child whose
data.path or label matches the expected "when" or "otherwise" branch (use the
child.data.path, child.getLabel(), or similar properties exposed by the node
API) and then assert against that found node; replace all hard-coded index
usages around choiceNode.getChildren() (including occurrences around lines
351-353, 417-418, 439-440) with this lookup approach so tests remain stable if
placeholder ordering changes.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts`:
- Around line 61-70: The test name is misleading: it currently asserts that an
onFallback child is returned as a placeholder but reads "should not return an
`onFallback` node if not defined"; rename the test to reflect the actual
behavior, e.g., change the it(...) description to "should return an `onFallback`
placeholder if not defined", keeping the existing assertions that use
mapper.getVizNodeFromProcessor,
routeDefinition.from.steps[0].circuitBreaker!.onFallback, and checks on
vizNode.getChildren() indices 0–2.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99489a13-1f95-453c-823b-3141ab4a3703
⛔ Files ignored due to path filters (4)
packages/ui/src/components/Visualization/Canvas/Form/__snapshots__/CanvasForm.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Custom/Node/__snapshots__/PlaceholderNode.test.tsx.snapis excluded by!**/*.snappackages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (34)
packages/ui-tests/cypress/e2e/designer/basicNodeActions/hoverToolbarActions.cy.tspackages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.tspackages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.tspackages/ui-tests/cypress/support/cypress.d.tspackages/ui-tests/cypress/support/next-commands/default.tspackages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsxpackages/ui/src/components/Visualization/Canvas/canvas.defaults.tspackages/ui/src/components/Visualization/Canvas/flow.service.tspackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.tspackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.tspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.scsspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsxpackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsxpackages/ui/src/components/Visualization/Custom/customComponentUtils.test.tspackages/ui/src/components/Visualization/Custom/customComponentUtils.tspackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsxpackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsxpackages/ui/src/models/visualization/base-visual-entity.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-visual-entity.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-verbs-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.tspackages/ui/src/models/visualization/visualization-node.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/ui/src/components/Visualization/Canvas/canvas.defaults.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.ts
- packages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/camel-rest-visual-entity.ts
- packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsx
- packages/ui/src/models/visualization/flows/abstract-camel-visual-entity.ts
- packages/ui/src/models/visualization/base-visual-entity.ts
Member
|
When there's no step in |
igarashitm
reviewed
Mar 9, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ui/src/components/Visualization/Custom/customComponentUtils.ts (1)
49-60:⚠️ Potential issue | 🟠 MajorRestore the single-clause drag guard here.
This now makes every non-top-level group draggable, including special groups like
otherwise.CustomGroupExpanded.tsxusescanDragGroup()as the drag gate, so this widens the DnD surface beyond the placeholder fix in this PR and drops the only clause-type check that used to protect single-clause groups.If the goal is only to allow dropping into empty containers, keep the prior parent-property filter here and let the new placeholder logic handle insertion. Otherwise, please add matching drop-path validation/tests for single-clause groups before broadening drag support.
Suggested fix
import { Edge, EdgeModel } from '@patternfly/react-topology'; import { CatalogModalContextValue } from '../../../dynamic-catalog/catalog-modal.provider'; import { AddStepMode, IVisualizationNode } from '../../../models/visualization/base-visual-entity'; +import { CamelRouteVisualEntityData } from '../../../models/visualization/flows/support/camel-component-types'; +import { CamelComponentSchemaService } from '../../../models/visualization/flows/support/camel-component-schema.service'; import { EntitiesContextResult } from '../../../providers'; @@ const canDragGroup = (groupVizNode?: IVisualizationNode): boolean => { if (groupVizNode === undefined) { return false; } // Do not allow dragging top-level groups like Route if (groupVizNode.data.path?.split('.').length === 1) { return false; } - return true; + const parentProcessorName = (groupVizNode.getParentNode?.()?.data as CamelRouteVisualEntityData | undefined) + ?.processorName; + if (!parentProcessorName) { + return true; + } + + const matchingProperty = CamelComponentSchemaService.getProcessorStepsProperties(parentProcessorName).find( + (property) => property.name === groupVizNode.data.name, + ); + + return matchingProperty?.type !== 'single-clause'; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/customComponentUtils.ts` around lines 49 - 60, The current canDragGroup function now allows dragging special single-clause groups (like "otherwise"); restore the prior single-clause guard by returning false when the group's parent is a single-clause container — specifically, in canDragGroup check both the top-level path condition (groupVizNode.data.path?.split('.').length === 1) and the parent clause/property condition (e.g., if groupVizNode.data.parent?.property is undefined or indicates a single-clause parent) and return false in that case; keep CustomGroupExpanded.tsx using canDragGroup as the drag gate and let the new placeholder logic handle insertion into empty containers.packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx (1)
124-138:⚠️ Potential issue | 🟠 MajorConditional hook execution violates React Hooks Rules in
PlaceholderNodeInner.The
useReplaceStepanduseInsertStephooks (lines 125, 138) are called only whenvizNodeis truthy. IfvizNodetransitions from falsy to truthy between renders, the hook execution order changes, violating the Rules of Hooks. Either move the guard condition before rendering this component, or restructure the hooks to be called unconditionally by making them accept nullable values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx` around lines 124 - 138, PlaceholderNodeInner currently calls useReplaceStep and useInsertStep only when vizNode is truthy which breaks the Rules of Hooks; fix by ensuring hooks are invoked unconditionally: either move the early return guard out of this component so PlaceholderNodeInner always receives a non-null vizNode, or update useReplaceStep and useInsertStep to accept a nullable vizNode (e.g., vizNode | null) and always call them at top of the component, then compute isSpecialPlaceholder/isSpecialChild/insertStepTargetNode/insertStepOptions after validating vizNode; reference symbols: PlaceholderNodeInner, vizNode, useReplaceStep, useInsertStep, insertStepTargetNode, insertStepOptions.
♻️ Duplicate comments (2)
packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts (1)
24-32:⚠️ Potential issue | 🟠 MajorPost-delete
otherwisecheck still doesn't prove the branch was removed.Line 28 and Line 32 both expect
cy.checkNodeExist('otherwise', 2), so this scenario can still pass when only placeholder nodes remain. Please assert on something that distinguishes the realotherwisebranch from its placeholder counterpart, ideally via the YAML/model aftercy.removeNodeByName('otherwise').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts` around lines 24 - 32, The two checks using cy.checkNodeExist('otherwise', 2) after cy.removeNodeByName('otherwise') can pass with placeholder nodes; update the test so after calling cy.removeNodeByName('otherwise') you also read the flow model/YAML (via the test helper the repo exposes—e.g., the flow model/YAML getter or editor) and assert that the actual "otherwise" branch object is removed (e.g., no "otherwise" key, branch length decreased, or node id gone) instead of only relying on cy.checkNodeExist; keep the initial cy.checkNodeExist/cy.removeNodeByName steps but replace or augment the final cy.checkNodeExist('otherwise', 2) with a concrete assertion against the model/YAML.packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts (1)
78-97:⚠️ Potential issue | 🟠 MajorSpecial-placeholder and backward moves still bypass the guarded cleanup path.
performForwardDrop()now respectscanRemoveStep/canRemoveFlow, but Lines 82-95 still callremoveChild()directly. Dragging a whole flow through either branch pastes the copy and leaves the source entity behind instead of removing it viacamelResource.removeEntity().Suggested fix
+const cleanupDraggedNode = (draggedVizNode: IVisualizationNode, entitiesContext: EntitiesContextResult) => { + const interaction = draggedVizNode.getNodeInteraction(); + if (interaction.canRemoveStep) { + draggedVizNode.removeChild(); + } else if (interaction.canRemoveFlow) { + const flowId = draggedVizNode.getId(); + entitiesContext.camelResource.removeEntity(flowId ? [flowId] : undefined); + } +}; + if (isSpecialChildPlaceholder) { const parentVizNode = droppedVizNode.getParentNode(); if (!parentVizNode) return; parentVizNode.pasteBaseEntityStep(draggedNodeContent, AddStepMode.InsertSpecialChildStep, true); - draggedVizNode.removeChild(); + cleanupDraggedNode(draggedVizNode, entitiesContext); } else { switch (getNodeDragAndDropDirection(draggedVizNode, droppedVizNode, droppedIntoEdge)) { case 'forward': { performForwardDrop(droppedVizNode, draggedVizNode, draggedNodeContent, droppedIntoEdge, entitiesContext); break; } case 'backward': - draggedVizNode.removeChild(); + cleanupDraggedNode(draggedVizNode, entitiesContext); droppedVizNode.pasteBaseEntityStep(draggedNodeContent, AddStepMode.PrependStep); break; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts` around lines 78 - 97, The code directly calls draggedVizNode.removeChild() in the special-placeholder branch and the 'backward' case, bypassing the guarded removal logic that checks canRemoveStep / canRemoveFlow and invokes camelResource.removeEntity(); change those direct removeChild() calls to use the same guarded cleanup path used by performForwardDrop (or extract a small helper that performs the guard checks and calls camelResource.removeEntity() when allowed) so that both special-placeholder and backward moves remove the source entity via camelResource.removeEntity() rather than leaving a stale copy behind.
🧹 Nitpick comments (2)
packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts (1)
61-70: Rename this test to match the new placeholder behavior.Lines 69-70 now assert that an
onFallbackplaceholder is still present whenonFallbackis undefined, so the current test name says the opposite and will make failures harder to interpret.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts` around lines 61 - 70, The test description for the it(...) block in circuit-breaker-node-mapper.test.ts is misleading: update the test name (the string passed to the it function) to reflect that an onFallback placeholder is expected when circuitBreaker.onFallback is undefined; use a name like "should include an `onFallback` placeholder when `onFallback` is undefined" to match the assertions that check vizNode.getChildren() indices [1] and [2] for placeholder behavior.packages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.ts (1)
155-171: Consider simplifying the chained assertion.The assertion at line 170 is fragile due to the long optional chain. If any intermediate value is undefined, the test passes vacuously rather than failing clearly.
♻️ Proposed refactor for clearer assertion
- expect(children?.find((c) => !c.data.isPlaceholder)?.getChildren()?.[0].data.isPlaceholder).toBe(true); + const actualNode = children?.find((c) => !c.data.isPlaceholder); + expect(actualNode).toBeDefined(); + const actualNodeChildren = actualNode?.getChildren(); + expect(actualNodeChildren?.[0].data.isPlaceholder).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.ts` around lines 155 - 171, The chained optional assertion is fragile; change the test in route-configuration-node-mapper.test.ts to explicitly assert intermediate values rather than relying on the optional chain: after calling mapper.getVizNodeFromProcessor and obtaining vizNode, get children via vizNode.getChildren() and assert children is defined and non-empty, then find a non-placeholder child (use children.find(c => !c.data.isPlaceholder)) and assert that this child is defined, then get that child's children (child.getChildren()), assert those are defined/non-empty, and finally assert that the first child's data.isPlaceholder is true; reference mapper.getVizNodeFromProcessor, vizNode.getChildren, and the found child.getChildren when making these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsx`:
- Around line 142-146: The placeholder drop target
(placeholderNodeDropTargetSpec) in PlaceholderNode.tsx does not return a drop
result, so nested placeholder drops yield undefined and prevent
handleValidNodeDrop from running; update the placeholderNodeDropTargetSpec to
include a drop method that returns the element (e.g., implement drop: (_item,
_monitor, _props) => element) so drops on nested placeholders propagate
correctly and allow handleValidNodeDrop to receive the dropped element.
In `@packages/ui/src/models/visualization/flows/abstract-camel-visual-entity.ts`:
- Around line 396-398: The current logic in the block that computes arrayPath
(using getArrayProperty(this.entityDef, `${data.path}.${property.name}`)) treats
missing insertAtStart as equivalent to mode === AddStepMode.InsertChildStep
which causes callers that omit insertAtStart to default to unshift(), changing
global add-order; change the condition so insertAtStart is used only when
explicitly provided (e.g., test for insertAtStart !== undefined) and otherwise
fall back to the previous default behavior (do not force InsertChildStep to mean
prepend), then call arrayPath.unshift(defaultValue) only when the explicit flag
is true (or mode truly intends it), otherwise use arrayPath.push(defaultValue)
to append.
---
Outside diff comments:
In `@packages/ui/src/components/Visualization/Custom/customComponentUtils.ts`:
- Around line 49-60: The current canDragGroup function now allows dragging
special single-clause groups (like "otherwise"); restore the prior single-clause
guard by returning false when the group's parent is a single-clause container —
specifically, in canDragGroup check both the top-level path condition
(groupVizNode.data.path?.split('.').length === 1) and the parent clause/property
condition (e.g., if groupVizNode.data.parent?.property is undefined or indicates
a single-clause parent) and return false in that case; keep
CustomGroupExpanded.tsx using canDragGroup as the drag gate and let the new
placeholder logic handle insertion into empty containers.
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`:
- Around line 124-138: PlaceholderNodeInner currently calls useReplaceStep and
useInsertStep only when vizNode is truthy which breaks the Rules of Hooks; fix
by ensuring hooks are invoked unconditionally: either move the early return
guard out of this component so PlaceholderNodeInner always receives a non-null
vizNode, or update useReplaceStep and useInsertStep to accept a nullable vizNode
(e.g., vizNode | null) and always call them at top of the component, then
compute
isSpecialPlaceholder/isSpecialChild/insertStepTargetNode/insertStepOptions after
validating vizNode; reference symbols: PlaceholderNodeInner, vizNode,
useReplaceStep, useInsertStep, insertStepTargetNode, insertStepOptions.
---
Duplicate comments:
In
`@packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts`:
- Around line 24-32: The two checks using cy.checkNodeExist('otherwise', 2)
after cy.removeNodeByName('otherwise') can pass with placeholder nodes; update
the test so after calling cy.removeNodeByName('otherwise') you also read the
flow model/YAML (via the test helper the repo exposes—e.g., the flow model/YAML
getter or editor) and assert that the actual "otherwise" branch object is
removed (e.g., no "otherwise" key, branch length decreased, or node id gone)
instead of only relying on cy.checkNodeExist; keep the initial
cy.checkNodeExist/cy.removeNodeByName steps but replace or augment the final
cy.checkNodeExist('otherwise', 2) with a concrete assertion against the
model/YAML.
In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts`:
- Around line 78-97: The code directly calls draggedVizNode.removeChild() in the
special-placeholder branch and the 'backward' case, bypassing the guarded
removal logic that checks canRemoveStep / canRemoveFlow and invokes
camelResource.removeEntity(); change those direct removeChild() calls to use the
same guarded cleanup path used by performForwardDrop (or extract a small helper
that performs the guard checks and calls camelResource.removeEntity() when
allowed) so that both special-placeholder and backward moves remove the source
entity via camelResource.removeEntity() rather than leaving a stale copy behind.
---
Nitpick comments:
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts`:
- Around line 61-70: The test description for the it(...) block in
circuit-breaker-node-mapper.test.ts is misleading: update the test name (the
string passed to the it function) to reflect that an onFallback placeholder is
expected when circuitBreaker.onFallback is undefined; use a name like "should
include an `onFallback` placeholder when `onFallback` is undefined" to match the
assertions that check vizNode.getChildren() indices [1] and [2] for placeholder
behavior.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.ts`:
- Around line 155-171: The chained optional assertion is fragile; change the
test in route-configuration-node-mapper.test.ts to explicitly assert
intermediate values rather than relying on the optional chain: after calling
mapper.getVizNodeFromProcessor and obtaining vizNode, get children via
vizNode.getChildren() and assert children is defined and non-empty, then find a
non-placeholder child (use children.find(c => !c.data.isPlaceholder)) and assert
that this child is defined, then get that child's children
(child.getChildren()), assert those are defined/non-empty, and finally assert
that the first child's data.isPlaceholder is true; reference
mapper.getVizNodeFromProcessor, vizNode.getChildren, and the found
child.getChildren when making these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 498f9658-1bbf-4f10-9ed3-4ff67e5b1e41
⛔ Files ignored due to path filters (4)
packages/ui/src/components/Visualization/Canvas/Form/__snapshots__/CanvasForm.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Custom/Node/__snapshots__/PlaceholderNode.test.tsx.snapis excluded by!**/*.snappackages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (34)
packages/ui-tests/cypress/e2e/designer/basicNodeActions/hoverToolbarActions.cy.tspackages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.tspackages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.tspackages/ui-tests/cypress/support/cypress.d.tspackages/ui-tests/cypress/support/next-commands/default.tspackages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsxpackages/ui/src/components/Visualization/Canvas/canvas.defaults.tspackages/ui/src/components/Visualization/Canvas/flow.service.tspackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.tspackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.tspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.scsspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsxpackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsxpackages/ui/src/components/Visualization/Custom/customComponentUtils.test.tspackages/ui/src/components/Visualization/Custom/customComponentUtils.tspackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsxpackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsxpackages/ui/src/models/visualization/base-visual-entity.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-visual-entity.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-verbs-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.tspackages/ui/src/models/visualization/visualization-node.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/ui-tests/cypress/support/cypress.d.ts
- packages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.ts
- packages/ui-tests/cypress/e2e/designer/basicNodeActions/hoverToolbarActions.cy.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.test.ts
- packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.scss
- packages/ui-tests/cypress/support/next-commands/default.ts
- packages/ui/src/models/visualization/flows/camel-route-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/camel-rest-visual-entity.ts
- packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsx
- packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsx
- packages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-verbs-node-mapper.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts (1)
78-83:⚠️ Potential issue | 🟠 MajorReuse the removal guards before deleting the dragged node.
These branches still call
draggedVizNode.removeChild()directly, so they bypass thecanRemoveStep/canRemoveFlowhandling already used inperformForwardDrop(). That can turn a move into an unintended duplicate or skipremoveEntity()cleanup for flow-level moves.🔧 Suggested fix
+const removeDraggedNode = (draggedVizNode: IVisualizationNode, entitiesContext: EntitiesContextResult) => { + const interaction = draggedVizNode.getNodeInteraction(); + if (interaction.canRemoveStep) { + draggedVizNode.removeChild(); + } else if (interaction.canRemoveFlow) { + const flowId = draggedVizNode.getId(); + entitiesContext.camelResource.removeEntity(flowId ? [flowId] : undefined); + } +}; + if (isSpecialChildPlaceholder) { const parentVizNode = droppedVizNode.getParentNode(); if (!parentVizNode) return; parentVizNode.pasteBaseEntityStep(draggedNodeContent, AddStepMode.InsertSpecialChildStep, true); - draggedVizNode.removeChild(); + removeDraggedNode(draggedVizNode, entitiesContext); } else { /** Handle the drag and drop operation based on the direction differently: for forward direction we append the step to the dropped node, then remove the dragged node for backward direction we remove the dragged node first, then prepend the step to the dropped node */ switch (getNodeDragAndDropDirection(draggedVizNode, droppedVizNode, droppedIntoEdge)) { case 'forward': { performForwardDrop(droppedVizNode, draggedVizNode, draggedNodeContent, droppedIntoEdge, entitiesContext); break; } case 'backward': - draggedVizNode.removeChild(); + removeDraggedNode(draggedVizNode, entitiesContext); droppedVizNode.pasteBaseEntityStep(draggedNodeContent, AddStepMode.PrependStep); break; } }Also applies to: 94-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts` around lines 78 - 83, The code calls draggedVizNode.removeChild() directly inside the isSpecialChildPlaceholder branch (and similarly at lines 94-96), bypassing existing removal guards in performForwardDrop; update this flow to reuse the same removal checks by invoking the same removal/cleanup path (e.g., call the helper that checks canRemoveStep/canRemoveFlow and calls removeEntity() when permitted) instead of directly calling draggedVizNode.removeChild() after parentVizNode.pasteBaseEntityStep(...); ensure the branch that handles special-child drops calls the same removal routine used by performForwardDrop so moves respect canRemoveStep/canRemoveFlow and run flow-level cleanup.packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx (1)
115-116:⚠️ Potential issue | 🟡 MinorNormalize the special-child sentinel before building the label.
updatedLabelonly handles'placeholder'. IfgetNodeLabel()still returns'placeholder-special-child', this will renderAdd placeholder-special-childagain instead of user-facing copy.💡 Suggested fix
- const updatedLabel = label === 'placeholder' ? 'Add step' : `Add ${label}`; + const isGenericPlaceholderLabel = label === 'placeholder' || label === 'placeholder-special-child'; + const updatedLabel = isGenericPlaceholderLabel ? 'Add step' : `Add ${label}`;Also applies to: 128-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx` around lines 115 - 116, The label builder uses updatedLabel = label === 'placeholder' ? 'Add step' : `Add ${label}` but doesn't handle the sentinel 'placeholder-special-child'; normalize the node label first (from getNodeLabel()) by mapping 'placeholder-special-child' to 'placeholder' (or checking startsWith('placeholder')) before computing updatedLabel so both updatedLabel and the similar logic at lines ~128-129 produce the user-friendly "Add step" copy; update the code around getNodeLabel(), the label variable, and the updatedLabel computation accordingly.
🧹 Nitpick comments (8)
packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts (1)
61-71: Test name contradicts the updated behavior.The test is named "should not return an
onFallbacknode if not defined", but the assertions now validate that anonFallbackplaceholder node IS returned at index 2. Consider renaming to accurately reflect the new behavior.✏️ Suggested test name update
- it('should not return an `onFallback` node if not defined', () => { + it('should return an `onFallback` placeholder when onFallback is not defined', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts` around lines 61 - 71, Rename the test's description string to reflect the current behavior: it currently reads "should not return an `onFallback` node if not defined" but the assertions expect an `onFallback` placeholder at index 2; update the it(...) description to something like "should return an `onFallback` placeholder node when onFallback is undefined" (the test block containing mapper.getVizNodeFromProcessor, vizNode, and routeDefinition) so the name matches the assertions.packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts (1)
208-211: Assert the schema lookup stays out of this path.This case now captures the new
trueresult, but it no longer proves thatcanDragGroup()bypassesCamelComponentSchemaService.getProcessorStepsProperties()forotherwise. Adding that assertion would lock in the regression fix and catch a reintroduction of the old schema-dependent branch.Suggested test tightening
- it('should return true when group is otherwise(single-clause)', () => { + it('should return true for an `otherwise` group without consulting schema steps properties', () => { const groupVizNode = getMockGroupVizNode('route.from.steps.0.choice.otherwise', 'otherwise', 'choice'); - expect(canDragGroup(groupVizNode)).toBe(true); + expect(canDragGroup(groupVizNode)).toBe(true); + expect(CamelComponentSchemaService.getProcessorStepsProperties).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts` around lines 208 - 211, The test for 'should return true when group is otherwise(single-clause)' must also assert that the schema lookup is not invoked: spy or mock CamelComponentSchemaService.getProcessorStepsProperties before calling canDragGroup (using the existing getMockGroupVizNode and canDragGroup helpers) and add an expectation that getProcessorStepsProperties was not called, while keeping the existing expect(canDragGroup(groupVizNode)).toBe(true); this locks in the regression fix that 'otherwise' bypasses the schema-dependent branch.packages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.ts (1)
170-170: Consider adding optional chaining for clearer test failures.If
getChildren()returns an empty array,[0]isundefinedand accessing.datathrows aTypeErrorinstead of a clear assertion failure.🔧 Suggested defensive assertion
- expect(children?.find((c) => !c.data.isPlaceholder)?.getChildren()?.[0].data.isPlaceholder).toBe(true); + expect(children?.find((c) => !c.data.isPlaceholder)?.getChildren()?.[0]?.data?.isPlaceholder).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.ts` at line 170, The test currently calls children?.find((c) => !c.data.isPlaceholder)?.getChildren()?.[0].data.isPlaceholder which will throw if getChildren() returns an empty array; update the assertion to use optional chaining and/or a defensive check so the assertion fails cleanly (e.g. assert that getChildren()?.length > 0 before accessing [0], or replace .getChildren()?.[0].data.isPlaceholder with .getChildren()?.[0]?.data?.isPlaceholder) — apply this change in the test using the same symbols (children, find, getChildren, isPlaceholder) so a missing child yields a readable assertion failure instead of a TypeError.packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts (1)
149-155: PopulateprocessorNameon the shared placeholder nodes.This helper is now the common factory for special-child placeholders, but it returns less metadata than the existing placeholder builders in
rest-dsl-node-mapper.tsandrest-dsl-verbs-node-mapper.ts. AddingprocessorNamehere keeps placeholder nodes consistent and avoids downstream DnD/rendering code having to branch on where the placeholder came from.♻️ Proposed fix
protected getPlaceHolderNodeForProcessor(path: string): IVisualizationNode { - return createVisualizationNode(`${path}`, { + const processorName = path.split('.').pop() as keyof ProcessorDefinition; + + return createVisualizationNode(path, { catalogKind: CatalogKind.Entity, - name: path.split('.').pop() as keyof ProcessorDefinition, + name: processorName, + processorName, isPlaceholder: true, - path: `${path}`, + path, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts` around lines 149 - 155, The shared helper getPlaceHolderNodeForProcessor returns placeholder nodes missing the processorName property which other builders (e.g., rest-dsl-node-mapper.ts and rest-dsl-verbs-node-mapper.ts) include; update getPlaceHolderNodeForProcessor to pass a processorName value into createVisualizationNode so the returned IVisualizationNode includes processorName (derive it from the same path/name logic you already use, e.g., the final segment of path) to keep placeholder metadata consistent for downstream DnD/rendering code.packages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.ts (1)
14-15: Prefer semantic duplication checks over placeholder totals.These
doCatch/when/intercept*counts now depend on placeholder nodes created bypackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts:127-147andpackages/ui/src/models/visualization/flows/nodes/mappers/when-node-mapper.ts:9-33, so they can change again without any regression in duplication behavior. The source assertions later in this spec already validate the real duplicated processor count; consider switching the UI-side assertions to selectors that exclude placeholders, or dropping the placeholder-sensitive totals entirely.Also applies to: 25-30, 59-60, 64-65, 69-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.ts` around lines 14 - 15, The UI tests use placeholder-sensitive counts (calls to cy.checkNodeExist('doCatch', 2), cy.checkNodeExist('doFinally', 1), and similar checks) which can break when placeholder nodes are added; update these assertions to either (a) select nodes with a filter that excludes placeholders (e.g., a selector/class or data attribute indicating real processors) or (b) remove the placeholder-dependent count assertions entirely and rely on the existing source-level duplication assertions later in the spec; find and update all occurrences of cy.checkNodeExist(...) in this test (including the instances at the other noted ranges) and ensure selectors target only real duplicated processors rather than placeholder nodes created by base-node-mapper / when-node-mapper.packages/ui-tests/cypress/e2e/designer/basicNodeActions/hoverToolbarActions.cy.ts (1)
74-75: Avoid asserting placeholder-inflated branch totals.
packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts:76-116andpackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.ts:9-38now add placeholder children as part of branch rendering, sowhen: 6/5andotherwise: 2are testing canvas scaffolding more than the actual route shape. That makes this spec brittle and can also mask a broken branch insertion. Prefer asserting the semantic result instead—e.g. source/YAML counts or selectors that exclude placeholder nodes.Also applies to: 90-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-tests/cypress/e2e/designer/basicNodeActions/hoverToolbarActions.cy.ts` around lines 74 - 75, The current assertions using cy.checkNodeExist('when', 6) and cy.checkNodeExist('log', 2) (also the similar assertions later) are counting placeholder scaffolding added by base-node-mapper/choice-node-mapper and are brittle; replace them with semantic assertions that exclude placeholders—either assert the flow/YAML/source node counts or query for node elements while excluding placeholder elements (e.g., filter out elements with the placeholder class/attribute) to verify the actual branch children/route shape instead of the inflated canvas totals; update the checks that call checkNodeExist to use the non-placeholder-aware selector or the source/YAML count check.packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.ts (1)
116-117: Avoid mutating suite-level fixtures in this test.
vizNode1andplaceholderNodeare shared across the whole file, so changingdata.namehere makes later cases order-dependent. Creating fresh nodes in the test keeps the suite isolated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.ts` around lines 116 - 117, The test mutates shared fixtures vizNode1 and placeholderNode by setting .data.name, making other tests order-dependent; instead, create fresh local node instances inside the test (e.g., clone or call the same factory used to construct vizNode1/placeholderNode) and set .data.name on those locals so the suite-level vizNode1 and placeholderNode remain unchanged; update the test to reference the new local variables rather than the shared vizNode1/placeholderNode.packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsx (1)
55-56: Prefer matching the selected node by viz path.This setup is for
CanvasForm, but the new lookup is still coupled to FlowService’stest|...canvas ID format. Matchingnode.data.vizNode?.data.pathkeeps the test stable if canvas IDs change again.♻️ Suggested tweak
- selectedNode = nodes.find((node) => node.id === 'test|route.from.steps.1.choice')!; // choice + selectedNode = nodes.find((node) => node.data.vizNode?.data.path === 'route.from.steps.1.choice')!; // choice🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsx` around lines 55 - 56, The test currently finds the selected node by canvas-specific ID returned from FlowService.getFlowDiagram('test', ...); update the lookup to match the viz path on the node instead (use node.data.vizNode?.data.path) so the test doesn't rely on the "test|..." canvas ID format; in the CanvasForm test replace the find predicate used for selectedNode with a comparison against the expected viz path (e.g., node.data.vizNode?.data.path === '<expected-path>' or derive it from camelRouteVisualEntity) while keeping FlowService.getFlowDiagram and the selectedNode variable intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`:
- Around line 128-139: The hooks useReplaceStep and useInsertStep are currently
only called after the early return when vizNode is defined, causing hook order
to change if vizNode toggles; move the hook invocations so they run
unconditionally at the top of the component (before the vizNode guard), calling
useReplaceStep(useReplaceStep(...)) and useInsertStep(useInsertStep(...)) with
safe inputs (e.g., pass vizNode or undefined and compute
insertStepTargetNode/insertStepOptions from vizNode if present) and ensure the
hooks return stable no-op handlers when given undefined so the component can
return early without changing hook order; update references to
insertStepTargetNode/insertStepOptions and tooltipContent to derive values after
vizNode check but keep hook calls unconditional.
---
Duplicate comments:
In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts`:
- Around line 78-83: The code calls draggedVizNode.removeChild() directly inside
the isSpecialChildPlaceholder branch (and similarly at lines 94-96), bypassing
existing removal guards in performForwardDrop; update this flow to reuse the
same removal checks by invoking the same removal/cleanup path (e.g., call the
helper that checks canRemoveStep/canRemoveFlow and calls removeEntity() when
permitted) instead of directly calling draggedVizNode.removeChild() after
parentVizNode.pasteBaseEntityStep(...); ensure the branch that handles
special-child drops calls the same removal routine used by performForwardDrop so
moves respect canRemoveStep/canRemoveFlow and run flow-level cleanup.
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`:
- Around line 115-116: The label builder uses updatedLabel = label ===
'placeholder' ? 'Add step' : `Add ${label}` but doesn't handle the sentinel
'placeholder-special-child'; normalize the node label first (from
getNodeLabel()) by mapping 'placeholder-special-child' to 'placeholder' (or
checking startsWith('placeholder')) before computing updatedLabel so both
updatedLabel and the similar logic at lines ~128-129 produce the user-friendly
"Add step" copy; update the code around getNodeLabel(), the label variable, and
the updatedLabel computation accordingly.
---
Nitpick comments:
In
`@packages/ui-tests/cypress/e2e/designer/basicNodeActions/hoverToolbarActions.cy.ts`:
- Around line 74-75: The current assertions using cy.checkNodeExist('when', 6)
and cy.checkNodeExist('log', 2) (also the similar assertions later) are counting
placeholder scaffolding added by base-node-mapper/choice-node-mapper and are
brittle; replace them with semantic assertions that exclude placeholders—either
assert the flow/YAML/source node counts or query for node elements while
excluding placeholder elements (e.g., filter out elements with the placeholder
class/attribute) to verify the actual branch children/route shape instead of the
inflated canvas totals; update the checks that call checkNodeExist to use the
non-placeholder-aware selector or the source/YAML count check.
In
`@packages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.ts`:
- Around line 14-15: The UI tests use placeholder-sensitive counts (calls to
cy.checkNodeExist('doCatch', 2), cy.checkNodeExist('doFinally', 1), and similar
checks) which can break when placeholder nodes are added; update these
assertions to either (a) select nodes with a filter that excludes placeholders
(e.g., a selector/class or data attribute indicating real processors) or (b)
remove the placeholder-dependent count assertions entirely and rely on the
existing source-level duplication assertions later in the spec; find and update
all occurrences of cy.checkNodeExist(...) in this test (including the instances
at the other noted ranges) and ensure selectors target only real duplicated
processors rather than placeholder nodes created by base-node-mapper /
when-node-mapper.
In `@packages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsx`:
- Around line 55-56: The test currently finds the selected node by
canvas-specific ID returned from FlowService.getFlowDiagram('test', ...); update
the lookup to match the viz path on the node instead (use
node.data.vizNode?.data.path) so the test doesn't rely on the "test|..." canvas
ID format; in the CanvasForm test replace the find predicate used for
selectedNode with a comparison against the expected viz path (e.g.,
node.data.vizNode?.data.path === '<expected-path>' or derive it from
camelRouteVisualEntity) while keeping FlowService.getFlowDiagram and the
selectedNode variable intact.
In
`@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts`:
- Around line 208-211: The test for 'should return true when group is
otherwise(single-clause)' must also assert that the schema lookup is not
invoked: spy or mock CamelComponentSchemaService.getProcessorStepsProperties
before calling canDragGroup (using the existing getMockGroupVizNode and
canDragGroup helpers) and add an expectation that getProcessorStepsProperties
was not called, while keeping the existing
expect(canDragGroup(groupVizNode)).toBe(true); this locks in the regression fix
that 'otherwise' bypasses the schema-dependent branch.
In
`@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.ts`:
- Around line 116-117: The test mutates shared fixtures vizNode1 and
placeholderNode by setting .data.name, making other tests order-dependent;
instead, create fresh local node instances inside the test (e.g., clone or call
the same factory used to construct vizNode1/placeholderNode) and set .data.name
on those locals so the suite-level vizNode1 and placeholderNode remain
unchanged; update the test to reference the new local variables rather than the
shared vizNode1/placeholderNode.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts`:
- Around line 149-155: The shared helper getPlaceHolderNodeForProcessor returns
placeholder nodes missing the processorName property which other builders (e.g.,
rest-dsl-node-mapper.ts and rest-dsl-verbs-node-mapper.ts) include; update
getPlaceHolderNodeForProcessor to pass a processorName value into
createVisualizationNode so the returned IVisualizationNode includes
processorName (derive it from the same path/name logic you already use, e.g.,
the final segment of path) to keep placeholder metadata consistent for
downstream DnD/rendering code.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts`:
- Around line 61-71: Rename the test's description string to reflect the current
behavior: it currently reads "should not return an `onFallback` node if not
defined" but the assertions expect an `onFallback` placeholder at index 2;
update the it(...) description to something like "should return an `onFallback`
placeholder node when onFallback is undefined" (the test block containing
mapper.getVizNodeFromProcessor, vizNode, and routeDefinition) so the name
matches the assertions.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.ts`:
- Line 170: The test currently calls children?.find((c) =>
!c.data.isPlaceholder)?.getChildren()?.[0].data.isPlaceholder which will throw
if getChildren() returns an empty array; update the assertion to use optional
chaining and/or a defensive check so the assertion fails cleanly (e.g. assert
that getChildren()?.length > 0 before accessing [0], or replace
.getChildren()?.[0].data.isPlaceholder with
.getChildren()?.[0]?.data?.isPlaceholder) — apply this change in the test using
the same symbols (children, find, getChildren, isPlaceholder) so a missing child
yields a readable assertion failure instead of a TypeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49322a41-f9df-47b8-b611-d6d3e199fa7e
⛔ Files ignored due to path filters (4)
packages/ui/src/components/Visualization/Canvas/Form/__snapshots__/CanvasForm.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Custom/Node/__snapshots__/PlaceholderNode.test.tsx.snapis excluded by!**/*.snappackages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (34)
packages/ui-tests/cypress/e2e/designer/basicNodeActions/hoverToolbarActions.cy.tspackages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.tspackages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.tspackages/ui-tests/cypress/support/cypress.d.tspackages/ui-tests/cypress/support/next-commands/default.tspackages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsxpackages/ui/src/components/Visualization/Canvas/canvas.defaults.tspackages/ui/src/components/Visualization/Canvas/flow.service.tspackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.tspackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.tspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.scsspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsxpackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsxpackages/ui/src/components/Visualization/Custom/customComponentUtils.test.tspackages/ui/src/components/Visualization/Custom/customComponentUtils.tspackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsxpackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsxpackages/ui/src/models/visualization/base-visual-entity.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-visual-entity.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-verbs-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.tspackages/ui/src/models/visualization/visualization-node.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/ui/src/components/Visualization/Canvas/flow.service.ts
- packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.ts
- packages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/abstract-camel-visual-entity.ts
- packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsx
- packages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.test.ts
- packages/ui/src/models/visualization/visualization-node.ts
- packages/ui-tests/cypress/support/next-commands/default.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.ts
- packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsx
- packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-verbs-node-mapper.ts
- packages/ui/src/components/Visualization/Custom/customComponentUtils.ts
- packages/ui/src/models/visualization/flows/camel-rest-visual-entity.ts
Comment on lines
+128
to
+139
| const isSpecialPlaceholder = vizNode.data.name !== 'placeholder' && vizNode.data.name !== 'placeholder-special-child'; | ||
| const isSpecialChildPlaceholder = vizNode.data.name === 'placeholder-special-child'; | ||
| const parentVizNode = vizNode.getParentNode(); | ||
| const insertStepTargetNode = isSpecialPlaceholder ? (parentVizNode ?? vizNode) : vizNode; | ||
| const insertStepOptions = isSpecialPlaceholder | ||
| ? { | ||
| predefinedComponent: { name: vizNode.data.name, type: CatalogKind.Processor }, | ||
| insertAtStart: true, | ||
| } | ||
| : undefined; | ||
| const { onInsertStep } = useInsertStep(insertStepTargetNode, AddStepMode.InsertSpecialChildStep, insertStepOptions); | ||
| const tooltipContent = isSpecialPlaceholder ? `Click to add ${vizNode?.data.name} branch` : 'Click to add a step'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsxRepository: KaotoIO/kaoto
Length of output: 11891
Don't call useReplaceStep and useInsertStep conditionally based on vizNode presence.
These hooks are invoked at lines 127 and 138, after the early return guard at lines 124–126. When vizNode is undefined, the component returns before hook execution. When vizNode is defined, both hooks execute. If element.getData().vizNode changes between renders, React detects a different hook count and order, breaking the component's hook state management. Move hook invocations before the conditional return, or refactor to make hooks accept undefined and return no-op handlers.
🧰 Tools
🪛 Biome (2.4.6)
[error] 138-138: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`
around lines 128 - 139, The hooks useReplaceStep and useInsertStep are currently
only called after the early return when vizNode is defined, causing hook order
to change if vizNode toggles; move the hook invocations so they run
unconditionally at the top of the component (before the vizNode guard), calling
useReplaceStep(useReplaceStep(...)) and useInsertStep(useInsertStep(...)) with
safe inputs (e.g., pass vizNode or undefined and compute
insertStepTargetNode/insertStepOptions from vizNode if present) and ensure the
hooks return stable no-op handlers when given undefined so the component can
return early without changing hook order; update references to
insertStepTargetNode/insertStepOptions and tooltipContent to derive values after
vizNode check but keep hook calls unconditional.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.ts (1)
37-44:⚠️ Potential issue | 🟡 MinorHardcoded path 'rest.placeholder' may be incorrect.
Line 42 uses hardcoded
'rest.placeholder'while line 37 correctly uses${path}.placeholderfor the node ID. Ifpathis not always'rest'(e.g., nested or indexed paths like'rest.0'), this creates an inconsistency where the node ID and thedata.pathproperty differ.🔧 Suggested fix
if (isEmptyRest) { const placeholderNode = createVisualizationNode(`${path}.placeholder`, { catalogKind: CatalogKind.Pattern, name: 'placeholder-special-child', isPlaceholder: true, processorName: REST_ELEMENT_NAME, - path: 'rest.placeholder', + path: `${path}.placeholder`, }); vizNode.addChild(placeholderNode); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.ts` around lines 37 - 44, The placeholder node uses a hardcoded data.path 'rest.placeholder' which can diverge from the node ID when the `path` variable is different; update the call to createVisualizationNode for `placeholderNode` to use `${path}.placeholder` for the `path` property (keep other fields like `processorName: REST_ELEMENT_NAME` and `isPlaceholder` unchanged) so the node's id and data.path remain consistent with the `path` variable and `vizNode.addChild(placeholderNode)` still behaves correctly.packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx (1)
141-178:⚠️ Potential issue | 🔴 CriticalMissing
drophandler prevents placeholder from being a valid drop target.The
placeholderNodeDropTargetSpecdefinesaccept,canDrop, andcollect, but lacks adropfunction. Without it, when a user drops onto this placeholder,handleValidNodeDropwon't receive the placeholder asdropResult, and the special-child insertion flow won't execute.Proposed fix: add drop handler
const placeholderNodeDropTargetSpec: DropTargetSpec<...> = useMemo( () => ({ accept: [NODE_DRAG_TYPE, GROUP_DRAG_TYPE], canDrop: (item, _monitor, _props) => { return checkNodeDropCompatibility( (item as Node).getData()?.vizNode, vizNode, (mode: AddStepMode, filterNode: IVisualizationNode, compatibilityCheckNodeName: string) => { const filter = entitiesContext.camelResource.getCompatibleComponents( mode, filterNode.data, filterNode.getNodeDefinition(), ); return catalogModalContext?.checkCompatibility(compatibilityCheckNodeName, filter) ?? false; }, ); }, + drop: (_item, _monitor, _props) => { + return element; + }, collect: (monitor) => ({ droppable: monitor.isDragging(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx` around lines 141 - 178, The drop target spec placeholderNodeDropTargetSpec is missing a drop handler, so dropped items never produce a dropResult and the special-child insertion flow doesn't run; add a drop: (item, monitor, props) => ({ dropTargetType: 'placeholder', vizNode }) (or return an object containing the placeholder's identifying info such as the vizNode or its id) to the spec so handleValidNodeDrop can detect the placeholder via the dropResult; ensure the returned shape matches what handleValidNodeDrop expects and update types if needed.packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts (1)
45-58:⚠️ Potential issue | 🟠 MajorAssert the inserted
whenin the source, not only on the canvas.After Line 46, the visual
whencount now includes placeholder-driven structure, but the Monaco assertions still only provechoiceandotherwisewere serialized. This test can miss a regression where thewhenbranch renders but is not written to the model.Suggested assertion
cy.getMonacoValue().then(({ sourceCode }) => { const choiceMatches = sourceCode.match(/- choice:/g) ?? []; expect(choiceMatches).to.have.lengthOf(1); + const whenMatches = sourceCode.match(/^\s*-?\s*when:/gm) ?? []; + expect(whenMatches).to.have.lengthOf(1); + const otherwiseMatches = sourceCode.match(/otherwise:/g) ?? []; expect(otherwiseMatches).to.have.lengthOf(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts` around lines 45 - 58, The test currently only asserts 'choice' and 'otherwise' in the Monaco source but not the serialized 'when' node; update the test after cy.openSourceCode()/cy.getMonacoValue() to also verify the serialized when branch exists by searching sourceCode for the 'when:' token (e.g., count matches of /when:/g) and asserting the expected length so the test fails if the when branch is rendered on canvas but not written to the model; locate this change around cy.checkNodeExist('when', 2), cy.openSourceCode, and cy.getMonacoValue to add the assertion.
♻️ Duplicate comments (3)
packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx (1)
124-138:⚠️ Potential issue | 🔴 CriticalHooks called conditionally after early return violates Rules of Hooks.
useReplaceStep(line 127) anduseInsertStep(line 138) are called after the early return at lines 124-126. WhenvizNodetoggles between defined and undefined across renders, React will see a different number of hooks, causing state corruption.Proposed fix: move hooks before the guard
const boxRef = useRef<Rect | null>(null); const boxXRef = useRef<number | null>(null); const boxYRef = useRef<number | null>(null); + + const isSpecialPlaceholder = vizNode?.data.name !== 'placeholder' && vizNode?.data.name !== 'placeholder-special-child'; + const isSpecialChildPlaceholder = vizNode?.data.name === 'placeholder-special-child'; + const parentVizNode = vizNode?.getParentNode(); + const insertStepTargetNode = isSpecialPlaceholder ? (parentVizNode ?? vizNode) : vizNode; + const insertStepOptions = isSpecialPlaceholder && vizNode + ? { + predefinedComponent: { name: vizNode.data.name, type: CatalogKind.Processor }, + insertAtStart: true, + } + : undefined; + + const { onReplaceNode } = useReplaceStep(vizNode); + const { onInsertStep } = useInsertStep(insertStepTargetNode, AddStepMode.InsertSpecialChildStep, insertStepOptions); useAnchor((element: Node) => { return new TargetAnchor(element); }, AnchorEnd.both); if (!vizNode) { return null; } - const { onReplaceNode } = useReplaceStep(vizNode); - const isSpecialPlaceholder = vizNode.data.name !== 'placeholder' && vizNode.data.name !== 'placeholder-special-child'; - const isSpecialChildPlaceholder = vizNode.data.name === 'placeholder-special-child'; - const parentVizNode = vizNode.getParentNode(); - const insertStepTargetNode = isSpecialPlaceholder ? (parentVizNode ?? vizNode) : vizNode; - const insertStepOptions = isSpecialPlaceholder - ? { - predefinedComponent: { name: vizNode.data.name, type: CatalogKind.Processor }, - insertAtStart: true, - } - : undefined; - const { onInsertStep } = useInsertStep(insertStepTargetNode, AddStepMode.InsertSpecialChildStep, insertStepOptions);Note: The hooks
useReplaceStepanduseInsertStepwill need to handleundefinedinputs gracefully (returning no-op handlers).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx` around lines 124 - 138, Move the hook calls out of the conditional early-return so they run on every render: call useReplaceStep(vizNode) and useInsertStep(...) before the `if (!vizNode) return null;` guard, and ensure both hooks gracefully accept undefined so they return safe no-op handlers (e.g., onReplaceNode and onInsertStep become no-ops when vizNode is undefined). Keep the same logic for computing insertStepTargetNode/insertStepOptions (using parentVizNode, isSpecialPlaceholder, AddStepMode.InsertSpecialChildStep, CatalogKind.Processor, etc.), but compute any values that depend on vizNode after the guard or make their computations tolerant of undefined so hooks are invoked unconditionally.packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts (1)
78-84:⚠️ Potential issue | 🟠 MajorAlign special-placeholder cleanup with interaction constraints.
The
removeChild()call at line 83 runs unconditionally, butperformForwardDrop(lines 161-167) correctly checkscanRemoveStepandcanRemoveFlowbefore removing. This inconsistency could bypass node-removal rules for special placeholder drops.Proposed fix to align with performForwardDrop pattern
parentVizNode.pasteBaseEntityStep(draggedNodeContent, AddStepMode.InsertSpecialChildStep, true); - draggedVizNode.removeChild(); + const draggedVizNodeInteraction = draggedVizNode.getNodeInteraction(); + if (draggedVizNodeInteraction.canRemoveStep) { + draggedVizNode.removeChild(); + } else if (draggedVizNodeInteraction.canRemoveFlow) { + const flowId = draggedVizNode?.getId(); + entitiesContext.camelResource.removeEntity(flowId ? [flowId] : undefined); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts` around lines 78 - 84, The removal of the dragged node in the isSpecialChildPlaceholder branch must respect the same constraints as performForwardDrop; after obtaining parentVizNode and calling parentVizNode.pasteBaseEntityStep(draggedNodeContent, AddStepMode.InsertSpecialChildStep, true), check the same removal guards used in performForwardDrop (canRemoveStep and canRemoveFlow on the relevant node/context) before calling draggedVizNode.removeChild(), and only call removeChild when those guards permit removal; mirror the performForwardDrop pattern to determine which flags to check.packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts (1)
28-32:⚠️ Potential issue | 🟠 MajorPost-delete
otherwisestill isn’t being verified.Line 28 and Line 32 both expect
cy.checkNodeExist('otherwise', 2), so this can still pass if deleting the realotherwisebranch leaves only placeholder nodes behind. Please assert on a changed count or, better, verify the YAML/model aftercy.removeNodeByName('otherwise').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts` around lines 28 - 32, The test currently calls cy.checkNodeExist('otherwise', 2) both before and after cy.removeNodeByName('otherwise'), which can falsely pass if placeholders remain; modify the after-delete assertion to verify a changed count (e.g., expect a lower number) or, preferably, load and assert the underlying flow model/YAML to ensure the 'otherwise' branch is actually gone: update the code around cy.removeNodeByName('otherwise') to (1) capture the node count with cy.checkNodeExist before deletion, (2) call cy.removeNodeByName('otherwise'), (3) assert the new count differs (decreased) or (better) call the app's export/serialize helper to fetch the YAML/model and assert there is no 'otherwise' node present (use the same helper used elsewhere in tests to locate the model).
🧹 Nitpick comments (4)
packages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.ts (1)
25-30: These count assertions are now coupled to placeholder rendering.The new
2/3expectations appear to include placeholder nodes, while the source assertions still validate only the real serialized branches. That makes these checks brittle to future placeholder UI changes and less explicit about verifying actual duplication. Prefer asserting YAML counts or using a helper/selector that excludes placeholders.Also applies to: 59-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.ts` around lines 25 - 30, The assertions using cy.checkNodeExist('when', 2/3) are counting placeholder-rendered nodes; update the test to assert against the actual serialized/YAML node counts or use a selector helper that excludes placeholders (e.g., replace cy.checkNodeExist with a new/alternate helper that filters out elements with the placeholder class or checks the app's serialized state). Specifically, change the checks around cy.selectDuplicateNode('when') and subsequent cy.checkNodeExist calls to either (a) read the flow/YAML output and assert the number of real 'when' branches, or (b) create/use a selector helper (e.g., checkRealNodeExist or add a :not(.placeholder) filter) to ensure only real nodes are counted.packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.ts (1)
45-80: Add the empty-choiceregression case.These updates cover the new placeholder ordering, but they still only exercise a populated
whenarray. Issue#2934is specifically about dropping into an empty container, so this suite should also assert thatchoice.whenstill yields the leading placeholder when no branches exist.Suggested test
+ it('should keep the `when` placeholder when the choice has no branches', () => { + routeDefinition.from.steps[0].choice = { when: [] }; + + const vizNode = mapper.getVizNodeFromProcessor(path, { processorName: 'choice' }, routeDefinition); + + expect(vizNode.getChildren()).toHaveLength(2); + expect(vizNode.getChildren()?.[0].data.path).toBe('from.steps.0.choice.when'); + expect(vizNode.getChildren()?.[0].data.isPlaceholder).toBe(true); + expect(vizNode.getChildren()?.[1].data.path).toBe('from.steps.0.choice.otherwise'); + expect(vizNode.getChildren()?.[1].data.isPlaceholder).toBe(true); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.ts` around lines 45 - 80, Add a test that sets routeDefinition.from.steps[0].choice!.when = [] and then calls mapper.getVizNodeFromProcessor(path, { processorName: 'choice' }, routeDefinition) to assert the returned vizNode still contains the leading placeholder for the when container (e.g., vizNode.getChildren()?.[0].data.path === 'from.steps.0.choice.when' and vizNode.getChildren()?.[0].data.isPlaceholder === true) and that the otherwise placeholder or node appears as expected; locate the relevant behavior in the existing tests around mapper.getVizNodeFromProcessor and the routeDefinition fixture and mirror the assertions used in the other cases (placeholder ordering, children length) but with an empty choice.when to reproduce the regression.packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts (1)
149-156: Minor: Redundant template literal for path.Lines 150 and 154 use
`${path}`whenpathwould suffice. This is a minor style nit.💅 Suggested simplification
protected getPlaceHolderNodeForProcessor(path: string): IVisualizationNode { - return createVisualizationNode(`${path}`, { + return createVisualizationNode(path, { catalogKind: CatalogKind.Entity, name: path.split('.').pop() as keyof ProcessorDefinition, isPlaceholder: true, - path: `${path}`, + path, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts` around lines 149 - 156, The getPlaceHolderNodeForProcessor function uses redundant template literals for path; replace the two occurrences of `${path}` in the call to createVisualizationNode (the first argument and the path property) with the plain path variable to simplify the code and keep behavior the same—update references in getPlaceHolderNodeForProcessor (which returns createVisualizationNode) while leaving CatalogKind, ProcessorDefinition and isPlaceholder logic unchanged.packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx (1)
128-129: Consider extracting'placeholder-special-child'to a shared constant.This string literal appears in multiple places across the codebase. Extracting it to a shared constant would improve maintainability and prevent typos.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx` around lines 128 - 129, Extract the repeated literal 'placeholder-special-child' into a shared constant (e.g., PLACEHOLDER_SPECIAL_CHILD) and replace direct uses in this file (references within isSpecialPlaceholder and isSpecialChildPlaceholder that compare vizNode.data.name) with that constant; also update other occurrences across the codebase to import and use the same constant so all comparisons use a single source of truth and reduce typo risk.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.ts`:
- Around line 14-16: The test calls cy.checkNodeExist('doCatch', 2) then
cy.selectDuplicateNode('doCatch') but doesn't assert that a real duplicate was
created; update the test to verify duplication by asserting the post-duplication
state — e.g., call cy.checkNodeExist('doCatch', 3) or add an assertion that the
duplicated branch appears on the canvas/YAML after invoking
cy.selectDuplicateNode('doCatch') and ensure similar verification for
'doFinally' if relevant; locate these changes near the existing
cy.checkNodeExist and cy.selectDuplicateNode calls to make the assertion
immediately after the duplication action.
In
`@packages/ui/src/components/Visualization/Custom/customComponentUtils.test.ts`:
- Around line 208-211: The test incorrectly expects canDragGroup(...) to return
true for the 'otherwise' single-clause branch; update the assertion to expect
false so that fixed branches like 'otherwise' (created via
getMockGroupVizNode('route.from.steps.0.choice.otherwise', 'otherwise',
'choice')) remain non-draggable per the logic in canDragGroup and how
CustomGroupExpanded.tsx uses it to gate dragging.
In
`@packages/ui/src/models/visualization/flows/camel-route-visual-entity.test.ts`:
- Around line 351-353: The test currently just checks child count and grabs the
actual when node but doesn't assert that the sibling placeholders exist; update
the assertions around choiceNode.getChildren() to explicitly check the
placeholder children by asserting their data.isPlaceholder flag (e.g., verify
choiceNode.getChildren()?.[0].data.isPlaceholder is true for the leading
when-placeholder and, in the empty-choice case,
choiceNode.getChildren()?.[2].data.isPlaceholder is true for the trailing
otherwise-placeholder), while keeping the existing check for the real when node
at index 1 (whenNode) to ensure the insert-at-start droppable placeholders are
validated directly.
---
Outside diff comments:
In
`@packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts`:
- Around line 45-58: The test currently only asserts 'choice' and 'otherwise' in
the Monaco source but not the serialized 'when' node; update the test after
cy.openSourceCode()/cy.getMonacoValue() to also verify the serialized when
branch exists by searching sourceCode for the 'when:' token (e.g., count matches
of /when:/g) and asserting the expected length so the test fails if the when
branch is rendered on canvas but not written to the model; locate this change
around cy.checkNodeExist('when', 2), cy.openSourceCode, and cy.getMonacoValue to
add the assertion.
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`:
- Around line 141-178: The drop target spec placeholderNodeDropTargetSpec is
missing a drop handler, so dropped items never produce a dropResult and the
special-child insertion flow doesn't run; add a drop: (item, monitor, props) =>
({ dropTargetType: 'placeholder', vizNode }) (or return an object containing the
placeholder's identifying info such as the vizNode or its id) to the spec so
handleValidNodeDrop can detect the placeholder via the dropResult; ensure the
returned shape matches what handleValidNodeDrop expects and update types if
needed.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.ts`:
- Around line 37-44: The placeholder node uses a hardcoded data.path
'rest.placeholder' which can diverge from the node ID when the `path` variable
is different; update the call to createVisualizationNode for `placeholderNode`
to use `${path}.placeholder` for the `path` property (keep other fields like
`processorName: REST_ELEMENT_NAME` and `isPlaceholder` unchanged) so the node's
id and data.path remain consistent with the `path` variable and
`vizNode.addChild(placeholderNode)` still behaves correctly.
---
Duplicate comments:
In
`@packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts`:
- Around line 28-32: The test currently calls cy.checkNodeExist('otherwise', 2)
both before and after cy.removeNodeByName('otherwise'), which can falsely pass
if placeholders remain; modify the after-delete assertion to verify a changed
count (e.g., expect a lower number) or, preferably, load and assert the
underlying flow model/YAML to ensure the 'otherwise' branch is actually gone:
update the code around cy.removeNodeByName('otherwise') to (1) capture the node
count with cy.checkNodeExist before deletion, (2) call
cy.removeNodeByName('otherwise'), (3) assert the new count differs (decreased)
or (better) call the app's export/serialize helper to fetch the YAML/model and
assert there is no 'otherwise' node present (use the same helper used elsewhere
in tests to locate the model).
In `@packages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.ts`:
- Around line 78-84: The removal of the dragged node in the
isSpecialChildPlaceholder branch must respect the same constraints as
performForwardDrop; after obtaining parentVizNode and calling
parentVizNode.pasteBaseEntityStep(draggedNodeContent,
AddStepMode.InsertSpecialChildStep, true), check the same removal guards used in
performForwardDrop (canRemoveStep and canRemoveFlow on the relevant
node/context) before calling draggedVizNode.removeChild(), and only call
removeChild when those guards permit removal; mirror the performForwardDrop
pattern to determine which flags to check.
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`:
- Around line 124-138: Move the hook calls out of the conditional early-return
so they run on every render: call useReplaceStep(vizNode) and useInsertStep(...)
before the `if (!vizNode) return null;` guard, and ensure both hooks gracefully
accept undefined so they return safe no-op handlers (e.g., onReplaceNode and
onInsertStep become no-ops when vizNode is undefined). Keep the same logic for
computing insertStepTargetNode/insertStepOptions (using parentVizNode,
isSpecialPlaceholder, AddStepMode.InsertSpecialChildStep, CatalogKind.Processor,
etc.), but compute any values that depend on vizNode after the guard or make
their computations tolerant of undefined so hooks are invoked unconditionally.
---
Nitpick comments:
In
`@packages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.ts`:
- Around line 25-30: The assertions using cy.checkNodeExist('when', 2/3) are
counting placeholder-rendered nodes; update the test to assert against the
actual serialized/YAML node counts or use a selector helper that excludes
placeholders (e.g., replace cy.checkNodeExist with a new/alternate helper that
filters out elements with the placeholder class or checks the app's serialized
state). Specifically, change the checks around cy.selectDuplicateNode('when')
and subsequent cy.checkNodeExist calls to either (a) read the flow/YAML output
and assert the number of real 'when' branches, or (b) create/use a selector
helper (e.g., checkRealNodeExist or add a :not(.placeholder) filter) to ensure
only real nodes are counted.
In `@packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsx`:
- Around line 128-129: Extract the repeated literal 'placeholder-special-child'
into a shared constant (e.g., PLACEHOLDER_SPECIAL_CHILD) and replace direct uses
in this file (references within isSpecialPlaceholder and
isSpecialChildPlaceholder that compare vizNode.data.name) with that constant;
also update other occurrences across the codebase to import and use the same
constant so all comparisons use a single source of truth and reduce typo risk.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.ts`:
- Around line 149-156: The getPlaceHolderNodeForProcessor function uses
redundant template literals for path; replace the two occurrences of `${path}`
in the call to createVisualizationNode (the first argument and the path
property) with the plain path variable to simplify the code and keep behavior
the same—update references in getPlaceHolderNodeForProcessor (which returns
createVisualizationNode) while leaving CatalogKind, ProcessorDefinition and
isPlaceholder logic unchanged.
In
`@packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.ts`:
- Around line 45-80: Add a test that sets
routeDefinition.from.steps[0].choice!.when = [] and then calls
mapper.getVizNodeFromProcessor(path, { processorName: 'choice' },
routeDefinition) to assert the returned vizNode still contains the leading
placeholder for the when container (e.g., vizNode.getChildren()?.[0].data.path
=== 'from.steps.0.choice.when' and vizNode.getChildren()?.[0].data.isPlaceholder
=== true) and that the otherwise placeholder or node appears as expected; locate
the relevant behavior in the existing tests around
mapper.getVizNodeFromProcessor and the routeDefinition fixture and mirror the
assertions used in the other cases (placeholder ordering, children length) but
with an empty choice.when to reproduce the regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f85238da-c8d1-42e2-a088-1cd90ba72ed9
⛔ Files ignored due to path filters (4)
packages/ui/src/components/Visualization/Canvas/Form/__snapshots__/CanvasForm.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snapis excluded by!**/*.snappackages/ui/src/components/Visualization/Custom/Node/__snapshots__/PlaceholderNode.test.tsx.snapis excluded by!**/*.snappackages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (34)
packages/ui-tests/cypress/e2e/designer/basicNodeActions/hoverToolbarActions.cy.tspackages/ui-tests/cypress/e2e/designer/basicNodeActions/stepDuplication.cy.tspackages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.tspackages/ui-tests/cypress/support/cypress.d.tspackages/ui-tests/cypress/support/next-commands/default.tspackages/ui/src/components/Visualization/Canvas/Form/CanvasForm.test.tsxpackages/ui/src/components/Visualization/Canvas/canvas.defaults.tspackages/ui/src/components/Visualization/Canvas/flow.service.tspackages/ui/src/components/Visualization/Custom/Group/CustomGroupExpanded.tsxpackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.test.tspackages/ui/src/components/Visualization/Custom/Node/CustomNodeUtils.tspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.scsspackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsxpackages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.tsxpackages/ui/src/components/Visualization/Custom/customComponentUtils.test.tspackages/ui/src/components/Visualization/Custom/customComponentUtils.tspackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsxpackages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.tsxpackages/ui/src/models/visualization/base-visual-entity.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.test.tspackages/ui/src/models/visualization/flows/abstract-camel-visual-entity.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.tspackages/ui/src/models/visualization/flows/camel-rest-visual-entity.tspackages/ui/src/models/visualization/flows/camel-route-visual-entity.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/rest-dsl-verbs-node-mapper.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.test.tspackages/ui/src/models/visualization/flows/nodes/mappers/route-configuration-node-mapper.tspackages/ui/src/models/visualization/visualization-node.ts
✅ Files skipped from review due to trivial changes (2)
- packages/ui/src/models/visualization/flows/camel-rest-visual-entity.test.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/choice-node-mapper.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/ui-tests/cypress/support/next-commands/default.ts
- packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.scss
- packages/ui/src/components/Visualization/Custom/customComponentUtils.ts
- packages/ui/src/models/visualization/visualization-node.ts
- packages/ui-tests/cypress/e2e/designer/basicNodeActions/hoverToolbarActions.cy.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/circuit-breaker-node-mapper.test.ts
- packages/ui/src/models/visualization/flows/nodes/mappers/base-node-mapper.test.ts
- packages/ui/src/models/visualization/base-visual-entity.ts
- packages/ui/src/components/Visualization/Custom/Node/PlaceholderNode.test.tsx
- packages/ui/src/models/visualization/flows/camel-rest-visual-entity.ts
- packages/ui/src/components/Visualization/Custom/hooks/insert-step.hook.test.tsx
- packages/ui-tests/cypress/support/cypress.d.ts
- packages/ui/src/models/visualization/flows/abstract-camel-visual-entity.ts
|
lordrip
approved these changes
Mar 11, 2026
Member
lordrip
left a comment
lordrip
left a comment
There was a problem hiding this comment.
Good job @shivamG640, I like the functionality, DnD benefits a lot from it 👍
Something we might need to give another thought in the near future, is to decide what direction to take regarding the node mappers.
The short summary is that I was moving from parsing everything at the base node mapper to parsing all the specialties in dedicated mappers. I think that's easier to reason about and also gives us the flexibility to accommodate other DSLs in the future, where we don't know if having placeholders in the same position as we do is appropriate.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2934
Summary by CodeRabbit
New Features
Bug Fixes
UI
Tests