[BI Data Mapper] Add support for the join clause #969
Conversation
…ClauseConnector from the node index
…tioning in Diagram
…d DataMapperNodeModel
…eryExprLabelWidget references and clean up QueryOutputNode by removing unused query handling code
…el signature to include query inputs
…nputOutputPortModel
…eft outer equijoin
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds JOIN/ArrayJoin support across the data mapper: core interface updates for focus tracking and clause props; new ClauseConnector node (model/factory/widget); array expansion and mapping changes; UI clause editor/store integration; Query traversal and visitor hooks; QueryExprLabelWidget removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClauseEditor
participant DMStore
participant ClausesPanel
participant Diagram
participant ClauseNode
rect rgb(240,248,255)
Note over User,ClauseEditor: Create JOIN clause via UI
User->>ClauseEditor: choose JOIN, fill lhs/rhs, submit
ClauseEditor->>DMStore: setClauseToAdd(newClause)
ClauseEditor->>DMStore: clearDraft()
DMStore-->>ClausesPanel: clauseToAdd updated
ClausesPanel->>Diagram: apply clause to model
end
rect rgb(240,255,240)
Note over Diagram,ClauseNode: Diagram creates ClauseConnectorNode
Diagram->>Diagram: traverseNode() visits model.query
Diagram->>ClauseNode: beginVisitQuery -> create node
ClauseNode->>ClauseNode: initPorts()
ClauseNode->>ClauseNode: initLinks()
Diagram-->>User: clause connector rendered
end
sequenceDiagram
participant Caller
participant expandArrayFn
participant model
Caller->>expandArrayFn: expandArrayFn(context, inputIds[], outputId, viewId)
expandArrayFn->>expandArrayFn: build baseView using sourceFields = inputIds
alt target port name ends with ".#"
expandArrayFn->>expandArrayFn: return MappingType.ArrayJoin
else
expandArrayFn->>expandArrayFn: return ArrayToArray or ArrayToSingleton
end
expandArrayFn-->>Caller: mapping type & view
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx (1)
36-79: Effect cleanup clearsclauseToAddbefore consumers read it—fix store access and cleanup timingThe effect both reacts to and clears
clauseToAddon every dependency change:useEffect(() => { if (clauseToAdd) { setAdding(clauses.length - 1); } return () => { setClauseToAdd(undefined); }; }, [clauseToAdd, clauses.length, setClauseToAdd, setAdding]);Root cause: You're accessing
clauseToAddvia.getState()(non-reactive snapshot) while depending on it in the effect. When an external trigger (e.g.,mapWithJoinin modification-utils.ts) callssetClauseToAdd({type: JOIN, ...}), ClausesPanel doesn't re-render because there's no hook subscription on that state. But ifclauses.lengthor other dependencies change during the same render cycle, the cleanup fires and resetsclauseToAddtoundefinedbeforeClauseEditor(line 37:clause ?? clauseToAdd ?? {}) can read it for pre-population.Fix:
- Use hook-based access for all store state to ensure reactivity:
const { isQueryClausesPanelOpen, setIsQueryClausesPanelOpen, clauseToAdd, setClauseToAdd, } = useDMQueryClausesPanelStore();
- Move cleanup responsibility to the component that consumes the value (ClauseEditor), or only reset on unmount:
useEffect(() => { if (clauseToAdd) { setAdding(clauses.length - 1); } }, [clauseToAdd, clauses.length, setAdding]); useEffect(() => { return () => { setClauseToAdd(undefined); }; }, [setClauseToAdd]); // Only cleanup on unmount
🧹 Nitpick comments (11)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx (1)
54-58: Remove unused loadingScreen variable.The
loadingScreenelement is defined but never rendered in the component. If loading state handling is planned for the future, consider adding a TODO comment; otherwise, remove this unused code.Apply this diff to remove the unused variable:
- const loadingScreen = ( - <div className={classnames(classes.element, classes.loadingContainer)}> - <ProgressRing sx={{ height: '16px', width: '16px' }} /> - </div> - ); - return (workspaces/ballerina/data-mapper/src/components/Diagram/hooks/useDiagramModel.ts (1)
43-105: Include query IOs in the query key to track query-structure changesDeriving
queryIOsfrommodel.query.inputsandmodel.query.outputand wiring it into theuseQuerykey is a reasonable way to refresh the diagram when the query IO shape changes, without over‑invalidating on unrelated query edits. This looks correct as long asinputs/outputare stable primitive IDs; if they ever become richer objects, consider switching to a more explicit serialization (e.g.,JSON.stringify) to avoid[object Object]collisions.workspaces/ballerina/data-mapper/src/components/Diagram/Label/ExpressionLabelFactory.tsx (1)
26-45: Skip mapping-options UI for ArrayJoin linksUsing
pendingMappingTypeto short‑circuit and render nothing forMappingType.ArrayJoinavoids showing the standard mapping options UI on JOIN links, which makes sense now that JOIN is driven via the clauses panel. You might optionally consider a small non‑interactive “JOIN” badge here for discoverability, but the current behavior is consistent with the new flow.workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx (1)
226-268: Wire hasInputsOutputsChanged into DataMapperContext; consider ClauseConnector reusePassing
hasInputsOutputsChangedintoDataMapperContexthere matches the new constructor shape and lets the context react appropriately when the IO structure changes. That part looks correct.Separately,
IntermediateNodeInitVisitoris currently seeded only with existingLinkConnectorNode/QueryExprConnectorNodeinstances:nodes.filter(node => node instanceof LinkConnectorNode || node instanceof QueryExprConnectorNode)If
ClauseConnectorNodeinstances also need their positions or other state preserved across regenerations, consider extending this filter (and the corresponding import) to includeClauseConnectorNodeas well.workspaces/ballerina/data-mapper/src/components/Diagram/Node/Input/InputNode.ts (1)
54-60: Tighten flatMap usage forsourceFields
flatMap(view => view.sourceFields).filter(Boolean)works but relies on filtering outundefinedvalues. For clearer intent and better type inference, consider returning an array explicitly:const focusedFieldFQNs = [ ...this.context.views.flatMap(view => view.sourceFields ?? []), ...(this.context.model.query?.inputs || []), ];This keeps
focusedFieldFQNsstrictly typed to string[] without relying onfilter(Boolean).workspaces/ballerina/data-mapper/src/visitors/IntermediateNodeInitVisitor.ts (1)
18-50: Query handling viaClauseConnectorNodeis consistent with existing visitor patternThe new
beginVisitQuerycorrectly wires queries into the intermediate node set usingClauseConnectorNode, aligned with the extended BaseVisitor API. This keeps query handling parallel to mapping connector nodes. If you later need to reuse existing clause connector nodes across refreshes, you might extendfindExistingNodeto coverClauseConnectorNodeas well, but that’s not required for correctness now.workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
444-472: focusInputRootMap wiring and traversal context look coherent; document mutation assumptionsThe new
focusInputRootMapplumbing inexpandDMModel,processInputRoots, andprocessArrayis internally consistent:
expandDMModelreturnsfocusInputRootMapalongsidetriggerRefresh, matching theExpandedDMModelshape.processInputRootsinitializesmodel.focusInputRootMap = {}and uses apreProcessedModelwithtraversingRootset per input, soprocessArraycan track which root an array focus member originated from.processArraywritesmodel.focusInputRootMap[parentId] = model.traversingRootonly in the focus‑member branch, which is reached via that preprocessed model.Given that
processArrayassumesmodel.focusInputRootMapis initialized, it would be good to either:
- Add a brief comment in
processInputRootsnoting that it’s responsible for initializingfocusInputRootMapused by downstream array processing, or- Defensively initialize in
processArray:if (model.traversingRoot) { model.focusInputRootMap ??= {}; model.focusInputRootMap[parentId] = model.traversingRoot; }This protects against future internal callers that might pass a DMModel without going through
processInputRoots.Also applies to: 479-501, 592-612
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx (2)
23-38: Store-backed clause selection flow is consistent and avoids stale draftsUsing
clause ?? clauseToAdd ?? {}gives a clear priority between an explicitclauseprop and a draftclauseToAdd, and clearingclauseToAddon submit/cancel (viasetClauseToAdd(undefined)) keeps the store from retaining stale draft state. Since you access the store with.getState(),clauseToAddis effectively read once per mount, which matches an “initialize-on-open” panel. If you ever need the editor to react to mid-session store changes, you’d want to switch to the subscribed hook form instead.
122-163: JOIN submit/cancel behavior and default props are reasonableClearing
clauseToAddin bothhandleSubmitandhandleCancelensures the panel doesn’t accidentally reuse a previous draft, and constructing theIntermediateClausefromDMFormFieldValueskeeps the wiring generic. For JOIN, forcingproperties.type = "var"andisOuter = falseis a sensible default for an inner join with type inference; if/when you introduce outer-join support, this will be an easy place to thread anisOutertoggle from the UI.workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (2)
62-110: Consider usingforEachinstead ofmapand strengthen the port change detection.Line 96: Using
map()without collecting the result—useforEach()for clarity since you're only iterating for side effects.Lines 103-105: The comparison logic concatenates port IDs to detect changes. This could fail to detect order-independent changes (e.g.,
["a", "b"]vs["b", "a"]) or could have false positives if IDs overlap (e.g.,["ab", "c"]vs["a", "bc"]).Apply this diff:
- if (matchedSearch && this.outPort) { - this.getModel().getNodes().map((node) => { + if (matchedSearch && this.outPort) { + this.getModel().getNodes().forEach((node) => {For a more robust port change detection, consider:
if (prevSourcePorts.length !== this.sourcePorts.length || - prevSourcePorts.map(port => port.getID()).join('') !== this.sourcePorts.map(port => port.getID()).join('')) { + !prevSourcePorts.every((port, idx) => port.getID() === this.sourcePorts[idx]?.getID())) { this.shouldInitLinks = true; }Or use a Set-based approach if order doesn't matter:
const prevIds = new Set(prevSourcePorts.map(p => p.getID())); const currentIds = new Set(this.sourcePorts.map(p => p.getID())); const changed = prevIds.size !== currentIds.size || !Array.from(prevIds).every(id => currentIds.has(id)); if (changed) { this.shouldInitLinks = true; }
122-123: Potential duplicate entries inlinkedPorts.Lines 122-123 call
addLinkedPort()without checking if the port already exists in thelinkedPortsarray. IfinitLinks()is called multiple times (thoughshouldInitLinksguards against it), or if the same port is referenced multiple times inquery.inputs, this could lead to duplicate entries.Consider adding a check in
addLinkedPortor here:if (sourcePort) { - sourcePort.addLinkedPort(this.inPort); - sourcePort.addLinkedPort(this.targetMappedPort) + if (!sourcePort.linkedPorts.some(p => p.getID() === this.inPort.getID())) { + sourcePort.addLinkedPort(this.inPort); + } + if (!sourcePort.linkedPorts.some(p => p.getID() === this.targetMappedPort?.getID())) { + sourcePort.addLinkedPort(this.targetMappedPort); + }Or, better yet, update the
addLinkedPortmethod inInputOutputPortModelto handle deduplication internally (if not already done).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts(4 hunks)workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts(3 hunks)workspaces/ballerina/ballerina-visualizer/src/views/DataMapper/DataMapperView.tsx(1 hunks)workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx(2 hunks)workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx(6 hunks)workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx(3 hunks)workspaces/ballerina/data-mapper/src/components/DataMapper/Views/DataMapperView.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Actions/IONodesScrollCanvasAction.ts(2 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Actions/utils.ts(2 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Diagram.tsx(3 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Label/ExpressionLabelFactory.tsx(2 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Label/ExpressionLabelModel.ts(0 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Label/QueryExprLabelWidget.tsx(0 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Link/DataMapperLink/DataMapperLink.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/LinkState/CreateLinkState.ts(4 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeFactory.tsx(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/index.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/Input/InputNode.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/QueryExprConnector/QueryExprConnectorNodeWidget.tsx(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/QueryOutput/QueryOutputNode.ts(0 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/commons/DataMapperNode.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/index.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Port/model/InputOutputPortModel.ts(2 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/hooks/useDiagramModel.ts(2 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/utils/common-utils.ts(3 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/utils/focus-positioning-utils.ts(2 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/utils/modification-utils.ts(2 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/utils/port-utils.ts(1 hunks)workspaces/ballerina/data-mapper/src/store/store.ts(2 hunks)workspaces/ballerina/data-mapper/src/utils/DataMapperContext/DataMapperContext.ts(2 hunks)workspaces/ballerina/data-mapper/src/utils/model-utils.ts(3 hunks)workspaces/ballerina/data-mapper/src/visitors/BaseVisitor.ts(2 hunks)workspaces/ballerina/data-mapper/src/visitors/IntermediateNodeInitVisitor.ts(2 hunks)
💤 Files with no reviewable changes (3)
- workspaces/ballerina/data-mapper/src/components/Diagram/Node/QueryOutput/QueryOutputNode.ts
- workspaces/ballerina/data-mapper/src/components/Diagram/Label/ExpressionLabelModel.ts
- workspaces/ballerina/data-mapper/src/components/Diagram/Label/QueryExprLabelWidget.tsx
🧰 Additional context used
🧬 Code graph analysis (22)
workspaces/ballerina/data-mapper/src/components/Diagram/Actions/IONodesScrollCanvasAction.ts (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
ClauseConnectorNode(36-187)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/port-utils.ts (2)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/Input/InputNode.ts (1)
InputNode(29-160)workspaces/ballerina/data-mapper/src/components/Diagram/Port/model/InputOutputPortModel.ts (1)
InputOutputPortModel(54-123)
workspaces/ballerina/data-mapper/src/components/Diagram/Port/model/InputOutputPortModel.ts (1)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/modification-utils.ts (1)
mapWithJoin(179-202)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx (5)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
ClauseConnectorNode(36-187)workspaces/ballerina/data-mapper/src/components/styles.ts (1)
useIntermediateNodeStyles(297-336)workspaces/ballerina/data-mapper/src/store/store.ts (2)
useDMExpressionBarStore(135-161)useDMQueryClausesPanelStore(171-180)workspaces/ballerina/data-mapper/src/components/Diagram/Node/LinkConnector/LinkConnectorWidgetComponents.tsx (1)
renderPortWidget(26-32)workspaces/common-libs/ui-toolkit/src/components/Codicon/Codicon.tsx (1)
Codicon(44-56)
workspaces/ballerina/data-mapper/src/components/Diagram/Actions/utils.ts (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
ClauseConnectorNode(36-187)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/QueryExprConnector/QueryExprConnectorNodeWidget.tsx (1)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/common-utils.ts (1)
expandArrayFn(226-266)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeFactory.tsx (2)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (2)
ClauseConnectorNode(36-187)CLAUSE_CONNECTOR_NODE_TYPE(33-33)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx (1)
ClauseConnectorNodeWidget(35-84)
workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx (1)
workspaces/ballerina/data-mapper/src/components/DataMapper/Views/DataMapperView.ts (1)
View(18-23)
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx (2)
workspaces/ballerina/data-mapper/src/store/store.ts (1)
useDMQueryClausesPanelStore(171-180)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (4)
DMFormField(260-271)DMFormFieldValues(273-275)IntermediateClause(216-219)IntermediateClauseProps(206-214)
workspaces/ballerina/data-mapper/src/visitors/BaseVisitor.ts (1)
workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
ExpandedDMModel(118-129)
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx (1)
workspaces/ballerina/data-mapper/src/store/store.ts (1)
useDMQueryClausesPanelStore(171-180)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts (2)
workspaces/ballerina/data-mapper/src/components/DataMapper/Views/DataMapperView.ts (1)
View(18-23)workspaces/ballerina/data-mapper/src/components/Diagram/Node/Input/InputNode.ts (1)
InputNode(29-160)
workspaces/ballerina/data-mapper/src/store/store.ts (1)
workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
IntermediateClause(216-219)
workspaces/ballerina/data-mapper/src/components/Diagram/LinkState/CreateLinkState.ts (2)
workspaces/ballerina/data-mapper/src/components/Diagram/Port/model/InputOutputPortModel.ts (1)
InputOutputPortModel(54-123)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
ClauseConnectorNode(36-187)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/focus-positioning-utils.ts (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
ClauseConnectorNode(36-187)
workspaces/ballerina/data-mapper/src/visitors/IntermediateNodeInitVisitor.ts (2)
workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
Query(191-198)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
ClauseConnectorNode(36-187)
workspaces/ballerina/data-mapper/src/utils/model-utils.ts (2)
workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
ExpandedDMModel(118-129)workspaces/ballerina/data-mapper/src/visitors/BaseVisitor.ts (1)
BaseVisitor(21-40)
workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/utils.ts (1)
workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMModel(131-144)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (8)
workspaces/ballerina/data-mapper/src/components/Diagram/Port/model/InputOutputPortModel.ts (1)
InputOutputPortModel(54-123)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMDiagnostic(70-83)workspaces/ballerina/data-mapper/src/utils/DataMapperContext/DataMapperContext.ts (1)
IDataMapperContext(21-35)workspaces/ballerina/data-mapper/src/store/store.ts (1)
useDMSearchStore(83-89)workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts (1)
findInputNode(23-47)workspaces/ballerina/data-mapper/src/components/Diagram/utils/port-utils.ts (2)
getInputPort(26-37)getTargetPortPrefix(60-73)workspaces/ballerina/data-mapper/src/components/Diagram/Node/QueryOutput/QueryOutputNode.ts (1)
QueryOutputNode(37-186)workspaces/ballerina/data-mapper/src/components/Diagram/Link/DataMapperLink/DataMapperLink.ts (1)
DataMapperLinkModel(37-95)
workspaces/ballerina/data-mapper/src/components/Diagram/Diagram.tsx (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
ClauseConnectorNode(36-187)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/common-utils.ts (1)
workspaces/ballerina/data-mapper/src/utils/DataMapperContext/DataMapperContext.ts (1)
IDataMapperContext(21-35)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/modification-utils.ts (3)
workspaces/ballerina/data-mapper/src/components/Diagram/utils/common-utils.ts (1)
expandArrayFn(226-266)workspaces/ballerina/data-mapper/src/components/Diagram/Port/model/InputOutputPortModel.ts (1)
InputOutputPortModel(54-123)workspaces/ballerina/data-mapper/src/store/store.ts (1)
useDMQueryClausesPanelStore(171-180)
🔇 Additional comments (30)
workspaces/ballerina/ballerina-visualizer/src/views/DataMapper/DataMapperView.tsx (1)
716-721: Includingqueryinputs in the signature is correct; confirmquery.inputsare names (strings)The new
inputsconstruction correctly ensures that changes tomodel.query.inputswill now be reflected in the model signature and thus inhasInputsChanged, which is important for reprocessing when query inputs change.This relies on
model.query?.inputsbeing an array of input names (strings) to stay consistent withModelSignature.inputs: string[]and with the existingmodel.inputs.map(i => i.name).If
query.inputsinstead contains IO/field objects, consider normalizing to names as well:const getModelSignature = (model: DMModel | ExpandedDMModel): ModelSignature => ({ inputs: [ ...model.inputs.map(i => i.name), ...(model.query?.inputs ? model.query.inputs.map(i => typeof i === "string" ? i : i.name) : []), ], output: model.output.name, subMappings: model.subMappings?.map(s => (s as IORoot | IOType).name) || [], refs: "refs" in model ? JSON.stringify(model.refs) : "", });If
query.inputsis already typed asstring[]inDMModel, the current implementation is fine and this comment can be ignored.workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/index.ts (1)
18-20: LGTM!The barrel exports follow the standard pattern and properly consolidate the ClauseConnector module exports.
workspaces/ballerina/data-mapper/src/components/Diagram/Node/index.ts (1)
28-28: LGTM!The ClauseConnector export is properly added to the Node module's public API, consistent with other node type exports.
workspaces/ballerina/data-mapper/src/components/Diagram/Actions/utils.ts (2)
28-29: LGTM!The imports are properly added to support the new ClauseConnectorNode functionality.
46-50: LGTM!ClauseConnectorNode is correctly added to the INTERMEDIATE_NODES array, consistent with other connector node types.
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeFactory.tsx (1)
31-38: LGTM!The conditional rendering logic correctly ensures the widget is only displayed when both input and output ports have established links.
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx (2)
35-52: LGTM!The component properly integrates with the data mapper stores and provides appropriate handlers for opening the clause panel and editing diagnostics.
60-83: LGTM!The widget layout properly renders ports, action buttons, and diagnostic information with appropriate styling and accessibility attributes.
workspaces/ballerina/data-mapper/src/components/Diagram/Actions/IONodesScrollCanvasAction.ts (2)
30-30: LGTM!The ClauseConnectorNode import is properly added to support the new node type in scroll handling.
164-164: LGTM!ClauseConnectorNode is correctly included in the instanceof check for intermediate node repositioning, ensuring consistent behavior with other connector node types.
workspaces/ballerina/data-mapper/src/components/Diagram/Node/commons/DataMapperNode.ts (1)
383-391: The referencedresolveArrayMemberFieldhelper does not exist in the codebase.The review comment's concern appears to be based on a non-existent refactoring. The code is correct:
attributes.field?.memberdirectly accesses the array member type (standard IOType property)focusedFieldFQNsis preserved via the...attributesspread operator passed toaddPortsForInputField- Focus resolution continues through the
isFocusedField()method calls during recursive field processing (lines 119, 238)Array field focus behavior is maintained through the existing focused field tracking mechanism; no prior helper function needs verification.
workspaces/ballerina/data-mapper/src/components/DataMapper/Views/DataMapperView.ts (1)
20-20: All consumers properly handle the breaking API change.The
sourceField→sourceFields(string array) migration has been correctly implemented:
- View interface (
DataMapperView.tsline 20): Updated tosourceFields?: string[]- View instantiation (
common-utils.tsline 250): Correctly setssourceFields: inputIds- View consumption (
InputNode.tsline 58): Properly accesses withflatMap(view => view.sourceFields).filter(Boolean)to handle the optional array- Initial view (
DataMapperEditor.tsxline 153): OmitssourceFieldssafely (optional property)No references to the old
sourceFieldproperty remain in the codebase.workspaces/ballerina/data-mapper/src/components/Diagram/Node/QueryExprConnector/QueryExprConnectorNodeWidget.tsx (1)
62-77: Expand array focus to use all mapping inputsPassing
mapping.inputsintoexpandArrayFnmatches the updatedinputIds: string[]signature and lets the expansion logic consider all query inputs instead of just the first one. This is consistent with multi‑input mappings (e.g., JOIN) and looks correct assumingmapping.inputsis always a defined string array.workspaces/ballerina/data-mapper/src/components/Diagram/utils/focus-positioning-utils.ts (1)
18-168: Extend focus resolution to ClauseConnectorNodeIncluding
ClauseConnectorNodein the intermediate-node union insideresolveTargetNodeAndPortcorrectly re-routes focus to the underlyingtargetMappedPortfor JOIN clause connectors, keeping behavior consistent with other intermediate nodes. The undefined‑port case falls back tonullcleanly, so this looks safe.workspaces/ballerina/data-mapper/src/components/Diagram/Link/DataMapperLink/DataMapperLink.ts (1)
27-35: Add ArrayJoin as a dedicated mapping typeIntroducing
MappingType.ArrayJoin = "array-join"cleanly separates JOIN behavior from other array mapping types and provides a clear discriminator for downstream UI/logic. No issues here.workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx (1)
153-158: Annotate initialView as View[]The explicit
View[]annotation forinitialViewlines up with theViewinterface and keeps the reducer state well-typed. No issues here.workspaces/ballerina/data-mapper/src/utils/model-utils.ts (1)
19-88: Integrate query nodes into the traversal pipelineHooking
model.queryintotraverseNodeviatraverseQuery, and delegating tobeginVisitQuery/endVisitQuery, cleanly extends the existing visitor pattern without disturbing inputs/outputs/mappings traversal. This gives visitors (e.g.,IntermediateNodeInitVisitor) a well-defined entry point for query/JOIN logic. Looks good.workspaces/ballerina/data-mapper/src/components/Diagram/Port/model/InputOutputPortModel.ts (1)
21-89: Trigger JOIN-clause flow for ArrayJoin mappingsExtending the
isPendingMappingRequiredbranch to special-caseMappingType.ArrayJoinand invokemapWithJoin(lm)before returning cleanly diverts JOIN connections into the query-clauses flow instead of creating a regular mapping. GivenmapWithJoinalready null‑checks the source port, this wiring looks correct and keeps the existing behavior for other mapping types intact.workspaces/ballerina/data-mapper/src/components/Diagram/utils/port-utils.ts (1)
26-37: getInputPort SubMappingNode support looks correctThe widened signature and conditional portId construction are consistent with existing patterns and keep the hidden‑parent walking behavior intact. No issues from a typing or runtime perspective.
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClausesPanel.tsx (2)
19-21: React import and hook usage are fineUsing
import React, { useEffect } from "react";together withReact.useStateanduseEffectis consistent and safe; no concerns here.
81-88: Panel width increaseChanging the
SidePanelwidth to 400 should just be a layout tweak. Verify visually that it doesn’t collide with other UI elements in typical editor window sizes.workspaces/ballerina/data-mapper/src/visitors/BaseVisitor.ts (1)
19-39: Query visitor hooks are a clean extensionAdding
beginVisitQuery/endVisitQuerywithQueryfrom the core library is a straightforward and backwards‑compatible extension ofBaseVisitor. This matches howExpandedDMModelalready carriesquery?: Query.workspaces/ballerina/data-mapper/src/utils/DataMapperContext/DataMapperContext.ts (1)
21-25: AddinghasInputsOutputsChangedto context is reasonableThe new
hasInputsOutputsChangedflag onIDataMapperContextandDataMapperContextcleanly exposes IO change tracking to consumers. With the default value in the constructor, existing call sites compile as long as their argument ordering has been adjusted where the flag is used (as done inDataMapperEditor).Also applies to: 37-53
workspaces/ballerina/data-mapper/src/components/Diagram/utils/modification-utils.ts (1)
25-29: JOIN mapping wiring viamapWithJoinlooks correct; be mindful of field assumptions
- Updating
mapWithQueryto callexpandArrayFn(context, [input], ...)correctly matches the newinputIds: string[]API without changing behavior.mapWithJoincleanly:
- Extracts the source port,
- Seeds a
JOIN-typeIntermediateClausewith a generated name (<fieldName>Item),type: "var", and the sourcefieldFQNas expression,- Opens the query clauses panel via the store.
Two minor points to keep in mind:
sourcePortModel.attributes.fieldand.fieldFQNare assumed to be defined. This matches existing usage ingetMapWithFnData, but if you ever allow joins from ports that don’t map to a concrete field (e.g., header-only ports), you may want to guard:if (!sourcePortModel.attributes.field || !sourcePortModel.attributes.fieldFQN) { return; }Please verify that the initial JOIN clause shape (
type: "var",isOuter: false, emptylhsExpression/rhsExpression) matches whatClauseEditorexpects to receive for a queued JOIN.Overall, the mapping and store interactions look sound.
Also applies to: 153-177, 179-202
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx (1)
40-83: JOIN-specific form configuration looks coherent with the new clause propsThe extended
clauseTypeItems, conditional labels/documentation for JOIN, and the additionallhsExpression/rhsExpressionfields all line up withIntermediateClausePropsand with how JOINs are typically modeled (join source, item alias, and ON condition sides). ThegenerateFieldsswitch producing[expressionField, nameField, lhsExpressionField, rhsExpressionField]for JOIN keeps the form structure clear and avoids surfacing irrelevant controls likeorderField.Also applies to: 98-120, 140-153
workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
56-63: JOIN clause and focus-mapping extensions align with consumer codeThe additions here are cohesive:
IntermediateClauseType.JOINmatches the new JOIN option inClauseEditor.- Optional
lhsExpression,rhsExpression, andisOuteronIntermediateClausePropscover the extra metadata JOIN needs without impacting existing clauses.focusInputRootMap?on bothExpandedDMModelandDMModel, plustraversingRoot?onDMModel, align with the updatedfindInputNodeand other JOIN-aware traversal logic.Because all the new fields are optional, these changes are backward-compatible for consumers that aren’t JOIN-aware yet.
Also applies to: 118-129, 131-144, 206-214
workspaces/ballerina/data-mapper/src/components/Diagram/utils/common-utils.ts (2)
102-113: Array join mapping detection via port suffix is reasonableThe new branch in
getMappingType:if (sourceDim > 0) { const dimDelta = sourceDim - targetDim; if (dimDelta == 0) { if (targetPort.attributes.portName.endsWith(".#")) { return MappingType.ArrayJoin; } return MappingType.ArrayToArray; } if (dimDelta > 0) { return MappingType.ArrayToSingleton; } }cleanly distinguishes
ArrayJoinfrom a regularArrayToArraymap using the".#"suffix convention on the target port name, while preserving previous behavior for other cases. As long as join targets consistently use that suffix, this keeps the mapping type classification straightforward.
226-266: All callers ofexpandArrayFnare correctly updated to passinputIds: string[]Verification confirms both call sites in the Ballerina workspace properly pass string arrays:
QueryExprConnectorNodeWidget.tsx:76— passesmapping.inputswhich is confirmed asstring[](evidenced by.join()call in QueryExprConnectorNode.ts line 68)modification-utils.ts:176— passes[input]wrapping the single input in an array for the single-input caseThe breaking change has been successfully applied across all relevant call sites.
workspaces/ballerina/data-mapper/src/components/Diagram/LinkState/CreateLinkState.ts (1)
33-33: LGTM! ClauseConnectorNode integration into link creation flow.The changes correctly integrate
ClauseConnectorNodeinto the link creation state machine by:
- Importing the new node type alongside
LinkConnectorNode- Removing
isQueryHeaderPortrestrictions that previously prevented query header ports from participating in link creation- Adding
ClauseConnectorNodeto the parent node resolution logic so that intermediate ports on clause connectors properly resolve to theirtargetMappedPortThis enables clause-based joins to work seamlessly within the existing link creation workflow.
Also applies to: 100-109, 123-143, 145-191
workspaces/ballerina/data-mapper/src/components/Diagram/Diagram.tsx (1)
39-39: LGTM! Proper registration and integration of ClauseConnectorNode.The changes follow the established pattern for integrating connector nodes:
- Importing
ClauseConnectorNodealongside existing connector node types- Registering
ClauseConnectorNodeFactorywith the diagram engine- Including
ClauseConnectorNodein the node filter and type union for position updatesThis ensures clause connectors are properly positioned based on their target ports, consistent with other connector node types.
Also applies to: 90-90, 161-169
...ces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts
Show resolved
Hide resolved
...erina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeFactory.tsx
Show resolved
Hide resolved
workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR adds JOIN clause support to the BI Data Mapper, enabling users to perform join operations on collections. The implementation includes a new clause connector visualization component, extends the data model to support multiple source fields in mappings, and updates the query clause editor with JOIN-specific form fields.
Key Changes
- Added
IntermediateClauseType.JOINenum value and related properties (lhsExpression,rhsExpression,isOuter) to support join condition configuration - Introduced
ClauseConnectorNodecomponent for visualizing clause operations in the diagram, similar to existing connector nodes - Refactored view model from single
sourceFieldto arraysourceFieldsto support multiple input sources for join operations
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
ballerina-core/src/interfaces/data-mapper.ts |
Added JOIN clause type enum and extended interface properties for join conditions |
data-mapper/src/visitors/BaseVisitor.ts |
Added query visiting methods to support traversing query structures |
data-mapper/src/visitors/IntermediateNodeInitVisitor.ts |
Implemented clause connector node creation during query visits |
data-mapper/src/utils/model-utils.ts |
Added query traversal logic to model visiting |
data-mapper/src/components/Diagram/Node/ClauseConnector/* |
New clause connector node component with widget, factory, and model |
data-mapper/src/components/Diagram/utils/modification-utils.ts |
Added mapWithJoin function to handle JOIN clause mapping initialization |
data-mapper/src/components/Diagram/utils/common-utils.ts |
Added ArrayJoin mapping type detection and updated expandArrayFn signature |
data-mapper/src/components/Diagram/Link/DataMapperLink/DataMapperLink.ts |
Added ArrayJoin mapping type enum value |
data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx |
Extended form fields for JOIN clause configuration (LHS/RHS expressions) |
data-mapper/src/components/DataMapper/Views/DataMapperView.ts |
Changed view model from single sourceField to array sourceFields |
data-mapper/src/store/store.ts |
Added clauseToAdd state for pre-populating clause editor |
ballerina-extension/src/rpc-managers/data-mapper/utils.ts |
Added focusInputRootMap tracking for mapping focused inputs to their root sources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...erina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeFactory.tsx
Show resolved
Hide resolved
...aces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx
Show resolved
Hide resolved
| let inputNode = findNodeByField(fieldStartsWith); | ||
|
|
||
| // if not found, try with parentSourceField | ||
| if (!inputNode && views && lastViewIndex) { |
There was a problem hiding this comment.
Potential issue: focusInputRootMap might be undefined if not initialized. Add a safety check before accessing it:
if (!inputNode && views && lastViewIndex && outputNode.context.model.focusInputRootMap) {
inputNode = findNodeByField(outputNode.context.model.focusInputRootMap[fieldStartsWith]);
}| if (!inputNode && views && lastViewIndex) { | |
| if ( | |
| !inputNode && | |
| views && | |
| lastViewIndex && | |
| outputNode.context.model.focusInputRootMap | |
| ) { |
| if (prevSourcePorts.length !== this.sourcePorts.length || | ||
| prevSourcePorts.map(port => port.getID()).join('') !== this.sourcePorts.map(port => port.getID()).join('')) { |
There was a problem hiding this comment.
The condition for setting shouldInitLinks compares port ID strings by joining them, which could produce false positives if individual IDs contain the same characters in different arrangements. Consider comparing arrays properly:
if (prevSourcePorts.length !== this.sourcePorts.length ||
!prevSourcePorts.every((port, idx) => port.getID() === this.sourcePorts[idx]?.getID())) {
this.shouldInitLinks = true;
}| if (prevSourcePorts.length !== this.sourcePorts.length || | |
| prevSourcePorts.map(port => port.getID()).join('') !== this.sourcePorts.map(port => port.getID()).join('')) { | |
| if ( | |
| prevSourcePorts.length !== this.sourcePorts.length || | |
| !prevSourcePorts.every((port, idx) => port.getID() === this.sourcePorts[idx]?.getID()) | |
| ) { |
| optional: false, | ||
| editable: true, | ||
| documentation: "Enter the name of the tool.", | ||
| documentation: clauseType === IntermediateClauseType.JOIN ? "Represents each record in the joined collection" : "Enter the name for variable", |
There was a problem hiding this comment.
Missing article "the" before "variable". Should be "Enter the name for the variable".
| documentation: clauseType === IntermediateClauseType.JOIN ? "Represents each record in the joined collection" : "Enter the name for variable", | |
| documentation: clauseType === IntermediateClauseType.JOIN ? "Represents each record in the joined collection" : "Enter the name for the variable", |
| import { InputOutputPortModel, IntermediatePortModel } from "../../Port"; | ||
| import { DataMapperNodeModel } from "../commons/DataMapperNode"; | ||
| import { ArrayOutputNode } from "../ArrayOutput"; | ||
| import { ObjectOutputNode } from "../ObjectOutput"; |
There was a problem hiding this comment.
Unused import ObjectOutputNode.
| import { ObjectOutputNode } from "../ObjectOutput"; |
| import { ArrayOutputNode } from "../ArrayOutput"; | ||
| import { ObjectOutputNode } from "../ObjectOutput"; | ||
| import { findInputNode } from "../../utils/node-utils"; | ||
| import { getInputPort, getOutputPort, getTargetPortPrefix } from "../../utils/port-utils"; |
There was a problem hiding this comment.
Unused import getOutputPort.
| import { getInputPort, getOutputPort, getTargetPortPrefix } from "../../utils/port-utils"; | |
| import { getInputPort, getTargetPortPrefix } from "../../utils/port-utils"; |
| import { findInputNode } from "../../utils/node-utils"; | ||
| import { getInputPort, getOutputPort, getTargetPortPrefix } from "../../utils/port-utils"; | ||
| import { OFFSETS } from "../../utils/constants"; | ||
| import { removeMapping } from "../../utils/modification-utils"; |
There was a problem hiding this comment.
Unused import removeMapping.
| import { removeMapping } from "../../utils/modification-utils"; |
| const loadingScreen = ( | ||
| <div className={classnames(classes.element, classes.loadingContainer)}> | ||
| <ProgressRing sx={{ height: '16px', width: '16px' }} /> | ||
| </div> | ||
| ); | ||
|
|
There was a problem hiding this comment.
Unused variable loadingScreen.
| const loadingScreen = ( | |
| <div className={classnames(classes.element, classes.loadingContainer)}> | |
| <ProgressRing sx={{ height: '16px', width: '16px' }} /> | |
| </div> | |
| ); |
| this.sourcePorts.push(inputPort); | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (93% of all statements in the enclosing function have an explicit semicolon).
| }) | |
| }); |
…-extensions-public into bi-dm-join-clause
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
83-86: Optimize duplicate port check for better performance.The duplicate check
!this.sourcePorts.some(port => port.getID() === inputPort.getID())runs inside theforEachloop for each input. With many inputs, this O(n²) check could become inefficient.Consider using a
Setto track port IDs for O(1) lookups:+ const existingPortIds = new Set<string>(); this.query.inputs.forEach((field) => { const inputField = field?.split('.').pop(); const matchedSearch = inputSearch === "" || inputField.toLowerCase().includes(inputSearch.toLowerCase()); if (!matchedSearch) return; const inputNode = findInputNode(field, this, views, lastViewIndex); if (inputNode) { const inputPort = getInputPort(inputNode, field?.replace(/\.\d+/g, '')); - if (!this.sourcePorts.some(port => port.getID() === inputPort.getID())) { + const portId = inputPort.getID(); + if (!existingPortIds.has(portId)) { this.sourcePorts.push(inputPort); + existingPortIds.add(portId); } } });Based on past review comments.
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx (2)
23-23: Prefer subscribing to the store via the hook instead ofgetState()Reading
clauseToAddfromuseDMQueryClausesPanelStore.getState()meansClauseEditorwill not re-render ifclauseToAddchanges while the component stays mounted, and it diverges from the usual Zustand hook pattern. Consider subscribing via the hook so the active clause stays in sync:- const { clauseToAdd, setClauseToAdd } = useDMQueryClausesPanelStore.getState(); - const { type: _clauseType, properties: clauseProps } = clause ?? clauseToAdd ?? {}; + const { clauseToAdd } = useDMQueryClausesPanelStore(); + const setClauseToAdd = useDMQueryClausesPanelStore((state) => state.setClauseToAdd); + const { type: _clauseType, properties: clauseProps } = clause ?? clauseToAdd ?? {};This keeps the editor reactive to store updates and consistent with other components using the same store.
Also applies to: 36-38
98-120: JOIN fields look correct; consider isolating JOIN-specific defaults from the generic submit handlerThe new
lhsExpression/rhsExpressionfields and the JOIN branch ingenerateFields()align withIntermediateClausePropsand should work as expected for editing JOIN clauses.The JOIN-specific defaults in
handleSubmit:if (clauseType === IntermediateClauseType.JOIN) { clause.properties.type = "var"; clause.properties.isOuter = false; }hard‑code JOIN semantics into this otherwise generic editor. As JOIN evolves (e.g., supporting outer joins or different binding types), this coupling can make it harder to extend without revisiting this component.
Consider moving these defaults into either:
- A small JOIN-specific helper (e.g.,
normalizeJoinClauseProps(data)), or- The place where
IntermediateClausePropsfor JOIN are defined/generated,so
ClauseEditorjust submitsdataand stays agnostic to clause‑specific rules.Also applies to: 128-131, 148-149
🧹 Nitpick comments (1)
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx (1)
122-137: EnsuresetClauseToAdd’s type allows clearing withundefinedBoth
handleSubmitandhandleCancelcallsetClauseToAdd(undefined)to clear the draft, which is a good pattern. However, instore.tsthe setter is currently typed as takingIntermediateClauseonly (per the provided snippet), which would be a TypeScript error if not updated.If you haven’t already, please widen the setter signature so the call here is type-safe:
- setClauseToAdd: (clauseToAdd: IntermediateClause) => set({ clauseToAdd }), + setClauseToAdd: (clauseToAdd: IntermediateClause | undefined) => set({ clauseToAdd }),Alternatively, you could expose and use a dedicated
resetQueryClausesPanelStorehelper here, but allowingundefinedis simplest given the state shape.Also applies to: 160-163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx(6 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts
🧰 Additional context used
🧬 Code graph analysis (3)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx (4)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
ClauseConnectorNode(33-184)workspaces/ballerina/data-mapper/src/store/store.ts (2)
useDMExpressionBarStore(135-161)useDMQueryClausesPanelStore(171-180)workspaces/ballerina/data-mapper/src/components/Diagram/Node/LinkConnector/LinkConnectorWidgetComponents.tsx (1)
renderPortWidget(26-32)workspaces/common-libs/ui-toolkit/src/components/Codicon/Codicon.tsx (1)
Codicon(44-56)
workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx (2)
workspaces/ballerina/data-mapper/src/store/store.ts (1)
useDMQueryClausesPanelStore(171-180)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (4)
DMFormField(260-271)DMFormFieldValues(273-275)IntermediateClause(216-219)IntermediateClauseProps(206-214)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (8)
workspaces/ballerina/data-mapper/src/components/Diagram/Port/model/InputOutputPortModel.ts (1)
InputOutputPortModel(54-123)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMDiagnostic(70-83)workspaces/ballerina/data-mapper/src/utils/DataMapperContext/DataMapperContext.ts (1)
IDataMapperContext(21-35)workspaces/ballerina/data-mapper/src/store/store.ts (1)
useDMSearchStore(83-89)workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts (1)
findInputNode(23-47)workspaces/ballerina/data-mapper/src/components/Diagram/utils/port-utils.ts (2)
getInputPort(26-37)getTargetPortPrefix(60-73)workspaces/ballerina/data-mapper/src/components/Diagram/Node/QueryOutput/QueryOutputNode.ts (1)
QueryOutputNode(37-186)workspaces/ballerina/data-mapper/src/components/Diagram/Link/DataMapperLink/DataMapperLink.ts (1)
DataMapperLinkModel(37-95)
🔇 Additional comments (4)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (2)
169-179: LGTM!The position update logic correctly guards against missing
targetMappedPortand appropriately adjusts positioning based on error state.
181-183: LGTM!The error check properly uses optional chaining to handle cases where
diagnosticsmight be undefined.workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx (1)
54-77: LGTM!The component rendering logic is clean and well-structured. The use of optional chaining in the
data-testidattribute (line 62) demonstrates good defensive coding practices. The conditional rendering of the diagnostic widget is appropriate.workspaces/ballerina/data-mapper/src/components/DataMapper/SidePanel/QueryClauses/ClauseEditor.tsx (1)
40-47: JOIN type option and conditional labels/documentation look consistentThe additions for
IntermediateClauseType.FROM/JOINinclauseTypeItemsand the JOIN‑specific labels/documentation fornameandexpressionfields (“Item Alias”, “Represents each record in the joined collection”, “Join With Collection”, “Collection to be joined”) read clearly and match the intended JOIN semantics. No changes needed here.Also applies to: 49-56, 73-80
...ces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts
Show resolved
Hide resolved
| this.sourcePorts.forEach((sourcePort) => { | ||
| const inPort = this.inPort; | ||
| const lm = new DataMapperLinkModel(undefined, this.diagnostics, true, undefined, true); | ||
|
|
||
| if (sourcePort) { | ||
| sourcePort.addLinkedPort(this.inPort); | ||
| sourcePort.addLinkedPort(this.targetMappedPort) | ||
|
|
||
| lm.setTargetPort(this.inPort); | ||
| lm.setSourcePort(sourcePort); | ||
| lm.registerListener({ | ||
| selectionChanged(event) { | ||
| if (event.isSelected) { | ||
| inPort.fireEvent({}, "link-selected"); | ||
| sourcePort.fireEvent({}, "link-selected"); | ||
| } else { | ||
| inPort.fireEvent({}, "link-unselected"); | ||
| sourcePort.fireEvent({}, "link-unselected"); | ||
| } | ||
| }, | ||
| }) | ||
| this.getModel().addAll(lm); | ||
| } | ||
| }) |
There was a problem hiding this comment.
Guard against undefined targetMappedPort when adding linked ports.
Line 120 calls sourcePort.addLinkedPort(this.targetMappedPort) without verifying that targetMappedPort is defined. Since targetMappedPort is only set when the output search matches and a QueryOutputNode is found (lines 92-106), it could be undefined here, adding an invalid entry to the linkedPorts array.
Apply this diff to add a guard:
this.sourcePorts.forEach((sourcePort) => {
const inPort = this.inPort;
const lm = new DataMapperLinkModel(undefined, this.diagnostics, true, undefined, true);
if (sourcePort) {
sourcePort.addLinkedPort(this.inPort);
- sourcePort.addLinkedPort(this.targetMappedPort)
+ if (this.targetMappedPort) {
+ sourcePort.addLinkedPort(this.targetMappedPort);
+ }
lm.setTargetPort(this.inPort);
lm.setSourcePort(sourcePort);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.sourcePorts.forEach((sourcePort) => { | |
| const inPort = this.inPort; | |
| const lm = new DataMapperLinkModel(undefined, this.diagnostics, true, undefined, true); | |
| if (sourcePort) { | |
| sourcePort.addLinkedPort(this.inPort); | |
| sourcePort.addLinkedPort(this.targetMappedPort) | |
| lm.setTargetPort(this.inPort); | |
| lm.setSourcePort(sourcePort); | |
| lm.registerListener({ | |
| selectionChanged(event) { | |
| if (event.isSelected) { | |
| inPort.fireEvent({}, "link-selected"); | |
| sourcePort.fireEvent({}, "link-selected"); | |
| } else { | |
| inPort.fireEvent({}, "link-unselected"); | |
| sourcePort.fireEvent({}, "link-unselected"); | |
| } | |
| }, | |
| }) | |
| this.getModel().addAll(lm); | |
| } | |
| }) | |
| this.sourcePorts.forEach((sourcePort) => { | |
| const inPort = this.inPort; | |
| const lm = new DataMapperLinkModel(undefined, this.diagnostics, true, undefined, true); | |
| if (sourcePort) { | |
| sourcePort.addLinkedPort(this.inPort); | |
| if (this.targetMappedPort) { | |
| sourcePort.addLinkedPort(this.targetMappedPort); | |
| } | |
| lm.setTargetPort(this.inPort); | |
| lm.setSourcePort(sourcePort); | |
| lm.registerListener({ | |
| selectionChanged(event) { | |
| if (event.isSelected) { | |
| inPort.fireEvent({}, "link-selected"); | |
| sourcePort.fireEvent({}, "link-selected"); | |
| } else { | |
| inPort.fireEvent({}, "link-unselected"); | |
| sourcePort.fireEvent({}, "link-unselected"); | |
| } | |
| }, | |
| }) | |
| this.getModel().addAll(lm); | |
| } | |
| }) |
🤖 Prompt for AI Agents
In
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts
around lines 114 to 137, sourcePort.addLinkedPort(this.targetMappedPort) is
called without checking if targetMappedPort is defined which can push undefined
into linkedPorts; before calling addLinkedPort for targetMappedPort, guard it
with an if (this.targetMappedPort) (or explicit null/undefined check) so you
only add a linked port when targetMappedPort is set, keeping the existing
behavior for inPort and preserving link creation and listener registration.
There was a problem hiding this comment.
If targetPort did not find code should hit ErrorBoundary
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the addLinkedPort method implementation to check if it validates the input
ast-grep --pattern $'addLinkedPort($$$) {
$$$
}'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Also search for the addLinkedPort definition in port-related files
rg -n "addLinkedPort" --type=ts -A 5 -B 2
Length of output: 21921
🏁 Script executed:
#!/bin/bash
# Check how linkedPorts is used - does it access properties that would throw on undefined?
rg -n "linkedPorts" --type=ts -A 3 -B 1 | head -100
Length of output: 10344
🏁 Script executed:
#!/bin/bash
# Check the ClauseConnectorNodeFactory to see if there's a guard preventing rendering when targetMappedPort is undefined
cat workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeFactory.tsx
Length of output: 1620
🏁 Script executed:
#!/bin/bash
# Check if initLinks is only called when targetMappedPort is set
rg -n "initLinks" --type=ts -B 3 -A 3 workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/
Length of output: 835
@KCSAbeywickrama, I understand your point about ErrorBoundary handling error conditions. However, there are a few concerns with the current approach:
-
Silent failure: At line 98,
getPort()can returnundefinedif the port doesn't exist, andshouldInitLinksat line 101 can still be set totruebased on sourcePorts changes. This meanstargetMappedPortcould beundefinedwhen enteringinitLinks(). -
Delayed error: If
undefinedis added tolinkedPorts, the error won't occur immediately. It will only throw whenlinkedPorts.forEach((linkedPort) => linkedPort.fireEvent(...))is called later (e.g., inCreateLinkState.tsline 128), which might happen during event handling outside the ErrorBoundary's scope. -
Unclear intent: Without an explicit guard or error throw, it's unclear whether an undefined
targetMappedPortis an expected edge case or a critical error.
Suggestion: If not finding the target port is truly an error condition that should be caught, consider either:
- Adding an explicit guard with a meaningful error:
if (!this.targetMappedPort) { throw new Error(`Target port not found for query output: ${this.query.output}`); }
- Or adding the null check before
addLinkedPortto prevent undefined from entering the array in the first place.
This makes the error handling explicit and ensures the ErrorBoundary can catch it at the right time. What's your preference?
| const onClickEdit = () => { | ||
| const targetPort = node.targetMappedPort; | ||
| setExprBarFocusedPort(targetPort); | ||
| }; |
There was a problem hiding this comment.
Add guard for undefined targetMappedPort in edit handler.
The onClickEdit handler accesses node.targetMappedPort without checking if it's defined. Since targetMappedPort is only set when the output search matches and a QueryOutputNode is found in initPorts(), it could be undefined, causing setExprBarFocusedPort to receive an invalid value.
Apply this diff to add a guard:
const onClickEdit = () => {
+ if (!node.targetMappedPort) {
+ return;
+ }
const targetPort = node.targetMappedPort;
setExprBarFocusedPort(targetPort);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onClickEdit = () => { | |
| const targetPort = node.targetMappedPort; | |
| setExprBarFocusedPort(targetPort); | |
| }; | |
| const onClickEdit = () => { | |
| if (!node.targetMappedPort) { | |
| return; | |
| } | |
| const targetPort = node.targetMappedPort; | |
| setExprBarFocusedPort(targetPort); | |
| }; |
🤖 Prompt for AI Agents
In
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx
around lines 49 to 52, the onClickEdit handler reads node.targetMappedPort
without checking for undefined; update the handler to guard against undefined by
reading targetMappedPort into a local variable, checking if it is truthy (or not
undefined) before calling setExprBarFocusedPort, and return early or no-op when
it's missing so setExprBarFocusedPort never receives an invalid value.
| </div> | ||
| </div> | ||
| ); | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing the new line at the end of the file. Pls add it in other applicable places as wel
workspaces/ballerina/data-mapper/src/components/Diagram/utils/common-utils.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (1)
74-86: Consider using a Set for more efficient duplicate checking.The current implementation checks for duplicate ports using
.some()inside the loop, resulting in O(n·m) complexity. For better performance with many inputs, consider using a Set to track port IDs.+ const existingPortIds = new Set<string>(); this.query.inputs.forEach((field) => { const inputField = field?.split('.').pop(); const matchedSearch = inputSearch === "" || inputField.toLowerCase().includes(inputSearch.toLowerCase()); if (!matchedSearch) return; const inputNode = findInputNode(field, this, views, lastViewIndex); if (inputNode) { const inputPort = getInputPort(inputNode, field?.replace(/\.\d+/g, '')); - if (!this.sourcePorts.some(port => port.getID() === inputPort.getID())) { + if (!existingPortIds.has(inputPort.getID())) { this.sourcePorts.push(inputPort); + existingPortIds.add(inputPort.getID()); } } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeFactory.tsx(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx(1 hunks)workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/index.ts
- workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeFactory.tsx
- workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNodeWidget.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (8)
workspaces/ballerina/data-mapper/src/components/Diagram/Port/model/InputOutputPortModel.ts (1)
InputOutputPortModel(54-123)workspaces/ballerina/ballerina-core/src/interfaces/data-mapper.ts (1)
DMDiagnostic(70-83)workspaces/ballerina/data-mapper/src/utils/DataMapperContext/DataMapperContext.ts (1)
IDataMapperContext(21-35)workspaces/ballerina/data-mapper/src/store/store.ts (1)
useDMSearchStore(83-89)workspaces/ballerina/data-mapper/src/components/Diagram/utils/node-utils.ts (1)
findInputNode(23-47)workspaces/ballerina/data-mapper/src/components/Diagram/utils/port-utils.ts (2)
getInputPort(26-37)getTargetPortPrefix(60-73)workspaces/ballerina/data-mapper/src/components/Diagram/Node/QueryOutput/QueryOutputNode.ts (1)
QueryOutputNode(37-186)workspaces/ballerina/data-mapper/src/components/Diagram/Link/DataMapperLink/DataMapperLink.ts (1)
DataMapperLinkModel(37-95)
🔇 Additional comments (3)
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts (3)
18-28: LGTM!All imports are properly used throughout the implementation.
109-167: LGTM!The link creation logic correctly handles port connections and event propagation. The concerns about
targetMappedPortpotentially being undefined were previously discussed, and the fail-fast error handling approach via ErrorBoundary is an architectural decision that has been clarified.
169-183: LGTM!Both
updatePositionandhasErrormethods are implemented correctly with appropriate guards and optional chaining.
| export class ClauseConnectorNode extends DataMapperNodeModel { | ||
|
|
||
| public sourcePorts: InputOutputPortModel[] = []; | ||
| public targetMappedPort: InputOutputPortModel; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add undefined to the type union for targetMappedPort.
The property is set to undefined at line 62 and is only conditionally assigned (lines 92-106), but the type declaration doesn't reflect this. This bypasses TypeScript's null-safety checks.
Apply this diff:
- public targetMappedPort: InputOutputPortModel;
+ public targetMappedPort: InputOutputPortModel | undefined;This will ensure TypeScript enforces proper null checks wherever targetMappedPort is accessed.
🤖 Prompt for AI Agents
In
workspaces/ballerina/data-mapper/src/components/Diagram/Node/ClauseConnector/ClauseConnectorNode.ts
around line 36, the property declaration for targetMappedPort currently omits
undefined even though it's assigned undefined at line 62 and only conditionally
set later (lines 92–106); update the type declaration to include undefined in
the union (e.g., InputOutputPortModel | undefined) so TypeScript enforces
null-safety, then run typecheck and add any necessary null/undefined guards
where targetMappedPort is accessed.
2 participants
Purpose
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.