Merged
Conversation
…ow already selected tools
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 document URI and range to the MCP tools RPC and client call; migrates MCP selection model from string[] to McpTool[] with derived selectedToolNames; deduplicates MCP tools by mcpConnection in visitors; and updates connector rendering and connection-opening flows to be MCP-aware. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Form / MCPtoolsSelection
participant Client as ExtendedLanguageClient
participant Server as Language Server
participant Diagram as Diagram Components
UI->>Client: getMcpTools({ connectionName, documentUri, range })
Client->>Server: RPC request (connectionName, documentUri, range)
Server-->>Client: McpToolsResponse (McpTool[] + optional selectedTools)
Client-->>UI: resolved McpTool[] (mapped)
UI->>UI: derive selectedToolNames (Set<string>) and update selection state
UI->>Diagram: re-render nodes/connectors (use mcpToolNames / MCP-aware flows)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx (1)
362-423:⚠️ Potential issue | 🟡 MinorUse Tool.mcpConnection for MCP connection label/icon.
The label/icon currently rely on
connectorNode.mcpConnection, which can be undefined when MCP connection data lives on theTool. This can leave the connection label blank for MCP tools.🧩 Suggested fix
- const connectorNode = ((node.stNode as Tool).mediator ?? node.stNode) as Connector; + const toolNode = node.stNode as Tool; + const connectorNode = (toolNode.mediator ?? node.stNode) as Connector; const tooltip = hasDiagnotics ? node.getDiagnostics().map(diagnostic => diagnostic.message).join("\n") : undefined; - const isMCPTool = (node.stNode as Tool).isMcpTool; + const isMCPTool = toolNode.isMcpTool; + const connectionLabel = isMCPTool ? toolNode.mcpConnection : connectorNode.configKey; ... - {(connectorNode.configKey || isMCPTool ) && + {(connectionLabel || isMCPTool ) && <S.CircleContainer ... - {connectorNode.mcpConnection && <g transform="translate(68,7)"> + {isMCPTool && connectionLabel && <g transform="translate(68,7)"> <foreignObject width="25" height="25"> <S.IconContainer>{getMediatorIconsFromFont('mcp')}</S.IconContainer> </foreignObject> </g>} ... {(connectorNode.configKey || isMCPTool) && <S.ConnectionContainer> <S.ConnectionText> - {connectorNode.configKey || connectorNode.mcpConnection} + {connectionLabel} </S.ConnectionText> </S.ConnectionContainer>}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx` around lines 362 - 423, The connector label/icon uses connectorNode.mcpConnection but MCP tools may store the data on the Tool object; update usages to fall back to Tool.mcpConnection when connectorNode.mcpConnection is falsy. Specifically, in the JSX blocks that render the icon and the connection label (references: connectorNode.mcpConnection, getMediatorIconsFromFont('mcp'), and the ConnectionText that renders connectorNode.configKey || connectorNode.mcpConnection), change the expression to use connectorNode.mcpConnection || Tool.mcpConnection (or an equivalent accessor from props/context that provides the Tool) so the icon and text render for MCP tools. Ensure the tooltip/conditional checks (connectorNode.configKey || isMCPTool) still gate rendering.workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx (2)
1281-1302:⚠️ Potential issue | 🟠 Major
documentUriandrangemay beundefinedbutMcpToolsSelectionexpects non-optional values.In
FormGeneratorProps, bothdocumentUriandrangeare optional (documentUri?: string,range?: Range), butMcpToolsSelectionPropsdeclares them as required (documentUri: string,range: Range). ThemcpToolsSelectioncase has no guard to ensure they're defined before rendering.This means
undefinedcan be passed toMcpToolsSelection, which then forwards them to the RPC call at line 472/474 ofMcpToolsSelection.tsx, potentially causing a runtime error.Proposed fix: guard the render
case 'mcpToolsSelection': + if (!documentUri || !range) { + return null; + } const selectedToolsSet = new Set<string>( Array.isArray(selectedMcpTools) ? selectedMcpTools : [] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx` around lines 1281 - 1302, The mcpToolsSelection branch in FormGenerator (case 'mcpToolsSelection') can pass undefined documentUri/range to McpToolsSelection (props documentUri: string, range: Range); add a guard that checks FormGenerator's documentUri and range are defined before returning McpToolsSelection and otherwise render a safe fallback (e.g., null or a validation message) or disable the control; ensure the check happens before creating selectedToolsSet and before rendering so McpToolsSelection never receives undefined values.
1282-1301:⚠️ Potential issue | 🟠 Major
selectedToolsSetis constructed incorrectly —McpTool[]objects are passed tonew Set<string>().After this PR,
mcpToolsSelectionstoresMcpTool[](objects withnameanddescription).useWatchon line 243 returns these objects. Passing them tonew Set<string>()produces a Set of"[object Object]"strings, not tool names. TheselectedToolsprop becomes unusable for any.has(toolName)check.This is currently masked because
McpToolsSelectioninternally computesselectedToolNamesfromgetValuesrather than using theselectedToolsprop. However, if the component is fixed to use the prop (per the earlier comment), this will break.Proposed fix: map to tool names
case 'mcpToolsSelection': const selectedToolsSet = new Set<string>( - Array.isArray(selectedMcpTools) ? selectedMcpTools : [] + Array.isArray(selectedMcpTools) + ? selectedMcpTools.map((tool: any) => typeof tool === 'string' ? tool : tool.name) + : [] );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx` around lines 1282 - 1301, The selectedToolsSet is being built from selectedMcpTools (which now contains McpTool objects) but is passed to new Set<string>() without mapping to names, producing "[object Object]" entries; update the construction of selectedToolsSet (used where selectedMcpTools is referenced and passed into the McpToolsSelection component) to map selectedMcpTools to their name strings (e.g., Array.isArray(selectedMcpTools) ? selectedMcpTools.map(t => t.name) : []) before creating the Set so selectedTools prop is a Set of tool names that .has(toolName) checks will work as intended.workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts (1)
2108-2126:⚠️ Potential issue | 🟡 MinorRemove the unused
McpToolsWithDescriptioninterface.The
McpToolsWithDescriptioninterface (lines 2123-2126) is not imported or used anywhere in the codebase. Since it duplicates theMcpToolinterface concept with only a minor difference (required vs. optionaldescriptionfield), and is not referenced anywhere, it should be removed as dead code.The
McpToolsRequest,McpToolsResponse, and changes to adddocumentUri,range, andselectedToolsare otherwise consistent with the PR objectives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts` around lines 2108 - 2126, Remove the dead interface McpToolsWithDescription by deleting its declaration (the interface named McpToolsWithDescription) from the file; ensure no other code references it (search for McpToolsWithDescription) and rely on the existing McpToolsResponse/McpToolsRequest types instead so you don't introduce any new type changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx`:
- Around line 443-451: The memo for selectedToolNames currently depends on
getValues('mcpToolsSelection') which is non-reactive and causes
incorrect/ineffective memoization; replace this by deriving the Set from the
reactive selectedTools prop passed by the parent (or subscribe directly with
useWatch using the component's _control) and memoize on that reactive value
instead — update the selectedToolNames computation to map the incoming
selectedTools (or the useWatch result) to a Set of tool.name, and remove
getValues from the dependency array so the displayed selection actually follows
form changes.
In `@workspaces/mi/mi-diagram/src/visitors/NodeFactoryVisitor.ts`:
- Around line 732-759: The filter for toolsWithUniqueConnections currently
excludes MCP tools lacking mcpConnection (tool.mcpConnection falsy) which hides
legacy/new tools; change the filter so missing mcpConnection is treated as a
unique connection (i.e., if connectionName is null/undefined, return true to
include that MCP tool), and keep the rest of the dedup logic the same; also
ensure the grouping logic that builds toolNode.mcpToolNames and the calls to
createNodeAndLinks (in this file) remains compatible, and mirror this fallback
behavior in PositionVisitor and SizingVisitor so layout/positioning treat
missing mcpConnection as its own unique group.
---
Outside diff comments:
In `@workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts`:
- Around line 2108-2126: Remove the dead interface McpToolsWithDescription by
deleting its declaration (the interface named McpToolsWithDescription) from the
file; ensure no other code references it (search for McpToolsWithDescription)
and rely on the existing McpToolsResponse/McpToolsRequest types instead so you
don't introduce any new type changes.
In `@workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx`:
- Around line 1281-1302: The mcpToolsSelection branch in FormGenerator (case
'mcpToolsSelection') can pass undefined documentUri/range to McpToolsSelection
(props documentUri: string, range: Range); add a guard that checks
FormGenerator's documentUri and range are defined before returning
McpToolsSelection and otherwise render a safe fallback (e.g., null or a
validation message) or disable the control; ensure the check happens before
creating selectedToolsSet and before rendering so McpToolsSelection never
receives undefined values.
- Around line 1282-1301: The selectedToolsSet is being built from
selectedMcpTools (which now contains McpTool objects) but is passed to new
Set<string>() without mapping to names, producing "[object Object]" entries;
update the construction of selectedToolsSet (used where selectedMcpTools is
referenced and passed into the McpToolsSelection component) to map
selectedMcpTools to their name strings (e.g., Array.isArray(selectedMcpTools) ?
selectedMcpTools.map(t => t.name) : []) before creating the Set so selectedTools
prop is a Set of tool names that .has(toolName) checks will work as intended.
In
`@workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx`:
- Around line 362-423: The connector label/icon uses connectorNode.mcpConnection
but MCP tools may store the data on the Tool object; update usages to fall back
to Tool.mcpConnection when connectorNode.mcpConnection is falsy. Specifically,
in the JSX blocks that render the icon and the connection label (references:
connectorNode.mcpConnection, getMediatorIconsFromFont('mcp'), and the
ConnectionText that renders connectorNode.configKey ||
connectorNode.mcpConnection), change the expression to use
connectorNode.mcpConnection || Tool.mcpConnection (or an equivalent accessor
from props/context that provides the Tool) so the icon and text render for MCP
tools. Ensure the tooltip/conditional checks (connectorNode.configKey ||
isMCPTool) still gate rendering.
---
Duplicate comments:
In `@workspaces/mi/mi-diagram/src/visitors/PositionVisitor.ts`:
- Around line 533-549: The filter for toolsWithUniqueConnections in
PositionVisitor excludes MCP tools that have no mcpConnection (returning false),
which drops valid tools; update the filter so non-MCP tools remain included, and
MCP tools without a connection are also included (only MCP tools with a defined
mcpConnection should be deduplicated by checking firstIndex). Locate the
toolsList.filter block in PositionVisitor (the toolsWithUniqueConnections
variable) and change the connectionName check from "if (!connectionName) {
return false; }" to include those tools (e.g., "if (!connectionName) { return
true; }" or equivalent logic) so only MCP tools with a connection are subject to
the findIndex uniqueness filter.
In `@workspaces/mi/mi-diagram/src/visitors/SizingVisitor.ts`:
- Around line 606-633: The filter for toolsWithUniqueConnections currently
excludes MCP tools that lack an mcpConnection (returns false) which drops them
unintentionally; update the filter inside SizingVisitor.ts so that: if
!isMcpTool return true as before, but if isMcpTool and the tool has no
mcpConnection also return true (treat them like non-MCP), and only apply the
"firstIndex" uniqueness check when a connectionName exists; reference the
toolsWithUniqueConnections filter, toolsList, Tool, isMcpTool, and mcpConnection
(and mirror the same fix you applied in NodeFactoryVisitor).
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+732
to
759
| const toolsWithUniqueConnections = toolsList.filter((tool: Tool, index: number) => { | ||
| const isMcpTool = tool.isMcpTool; | ||
| if (!isMcpTool) { | ||
| return true; // Include all non-MCP tools | ||
| } | ||
| // For MCP tools, only include the first one with each unique connection name | ||
| const connectionName = tool.mcpConnection; | ||
| if (!connectionName) { | ||
| return false; | ||
| } | ||
| const firstIndex = toolsList.findIndex((t: Tool) => t.mcpConnection === connectionName); | ||
| return index === firstIndex; | ||
| }); | ||
|
|
||
| for (let i = 0; i < toolsWithUniqueConnections.length; i++) { | ||
| const toolNode = toolsWithUniqueConnections[i]; | ||
| const isMCPTool = toolNode.isMcpTool; | ||
| if (isMCPTool) { | ||
| if (toolNode.mcpConnection) { | ||
| toolNode.mcpToolNames = toolsList | ||
| .filter((t: Tool) => t.mcpConnection === toolNode.mcpConnection) | ||
| .map((t: Tool) => t.name); | ||
| } | ||
| this.createNodeAndLinks({ node: toolNode, name: toolNode.tag, type: NodeTypes.CONNECTOR_NODE, dontLink: true }); | ||
| continue; | ||
| } | ||
| if(toolNode.mediator == undefined) { | ||
| if (toolNode.mediator == undefined) { | ||
| continue; |
Contributor
There was a problem hiding this comment.
Avoid hiding MCP tools without a connection.
The filter excludes MCP tools when mcpConnection is missing, which can make newly added/legacy tools disappear from the diagram. Consider keeping such tools visible (treat missing connection as unique), and mirror the fallback in PositionVisitor and SizingVisitor for consistent layout.
🔧 Suggested adjustment
- const toolsWithUniqueConnections = toolsList.filter((tool: Tool, index: number) => {
- const isMcpTool = tool.isMcpTool;
- if (!isMcpTool) {
- return true; // Include all non-MCP tools
- }
- // For MCP tools, only include the first one with each unique connection name
- const connectionName = tool.mcpConnection;
- if (!connectionName) {
- return false;
- }
- const firstIndex = toolsList.findIndex((t: Tool) => t.mcpConnection === connectionName);
- return index === firstIndex;
- });
+ const toolsWithUniqueConnections = toolsList.filter((tool: Tool, index: number) => {
+ const isMcpTool = tool.isMcpTool;
+ if (!isMcpTool) {
+ return true; // Include all non-MCP tools
+ }
+ // For MCP tools, only include the first one with each unique connection name.
+ // If there's no connection yet, keep the tool visible.
+ const connectionName = tool.mcpConnection;
+ if (!connectionName) {
+ return true;
+ }
+ const firstIndex = toolsList.findIndex((t: Tool) => t.mcpConnection === connectionName);
+ return index === firstIndex;
+ });📝 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.
Suggested change
| const toolsWithUniqueConnections = toolsList.filter((tool: Tool, index: number) => { | |
| const isMcpTool = tool.isMcpTool; | |
| if (!isMcpTool) { | |
| return true; // Include all non-MCP tools | |
| } | |
| // For MCP tools, only include the first one with each unique connection name | |
| const connectionName = tool.mcpConnection; | |
| if (!connectionName) { | |
| return false; | |
| } | |
| const firstIndex = toolsList.findIndex((t: Tool) => t.mcpConnection === connectionName); | |
| return index === firstIndex; | |
| }); | |
| for (let i = 0; i < toolsWithUniqueConnections.length; i++) { | |
| const toolNode = toolsWithUniqueConnections[i]; | |
| const isMCPTool = toolNode.isMcpTool; | |
| if (isMCPTool) { | |
| if (toolNode.mcpConnection) { | |
| toolNode.mcpToolNames = toolsList | |
| .filter((t: Tool) => t.mcpConnection === toolNode.mcpConnection) | |
| .map((t: Tool) => t.name); | |
| } | |
| this.createNodeAndLinks({ node: toolNode, name: toolNode.tag, type: NodeTypes.CONNECTOR_NODE, dontLink: true }); | |
| continue; | |
| } | |
| if(toolNode.mediator == undefined) { | |
| if (toolNode.mediator == undefined) { | |
| continue; | |
| const toolsWithUniqueConnections = toolsList.filter((tool: Tool, index: number) => { | |
| const isMcpTool = tool.isMcpTool; | |
| if (!isMcpTool) { | |
| return true; // Include all non-MCP tools | |
| } | |
| // For MCP tools, only include the first one with each unique connection name. | |
| // If there's no connection yet, keep the tool visible. | |
| const connectionName = tool.mcpConnection; | |
| if (!connectionName) { | |
| return true; | |
| } | |
| const firstIndex = toolsList.findIndex((t: Tool) => t.mcpConnection === connectionName); | |
| return index === firstIndex; | |
| }); | |
| for (let i = 0; i < toolsWithUniqueConnections.length; i++) { | |
| const toolNode = toolsWithUniqueConnections[i]; | |
| const isMCPTool = toolNode.isMcpTool; | |
| if (isMCPTool) { | |
| if (toolNode.mcpConnection) { | |
| toolNode.mcpToolNames = toolsList | |
| .filter((t: Tool) => t.mcpConnection === toolNode.mcpConnection) | |
| .map((t: Tool) => t.name); | |
| } | |
| this.createNodeAndLinks({ node: toolNode, name: toolNode.tag, type: NodeTypes.CONNECTOR_NODE, dontLink: true }); | |
| continue; | |
| } | |
| if (toolNode.mediator == undefined) { | |
| continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/mi/mi-diagram/src/visitors/NodeFactoryVisitor.ts` around lines 732
- 759, The filter for toolsWithUniqueConnections currently excludes MCP tools
lacking mcpConnection (tool.mcpConnection falsy) which hides legacy/new tools;
change the filter so missing mcpConnection is treated as a unique connection
(i.e., if connectionName is null/undefined, return true to include that MCP
tool), and keep the rest of the dedup logic the same; also ensure the grouping
logic that builds toolNode.mcpToolNames and the calls to createNodeAndLinks (in
this file) remains compatible, and mirror this fallback behavior in
PositionVisitor and SizingVisitor so layout/positioning treat missing
mcpConnection as its own unique group.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request implements improvements to MCP (Model Context Protocol) tools functionality in the MI (Micro Integrator) diagram and visualization components. The changes focus on improving user experience when working with MCP tools, including better grouping, selection management, and visual representation.
Changes:
- Extended Tool interface to include MCP-specific metadata (connection name and tool names)
- Modified MCP tools request/response to include document context and selected tools
- Implemented grouping of MCP tools by connection in diagram visualization
- Changed MCP tool selection from string arrays to full tool objects with descriptions
- Updated connector icons to use AI connector icon for MCP tools
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/mi/syntax-tree/src/syntax-tree-interfaces.ts | Added mcpConnection and mcpToolNames properties to Tool interface |
| workspaces/mi/mi-extension/src/lang-client/ExtendedLanguageClient.ts | Updated getMcpTools request to include documentUri and range parameters |
| workspaces/mi/mi-diagram/src/visitors/SizingVisitor.ts | Implemented filtering to show only unique MCP connection nodes in diagram sizing |
| workspaces/mi/mi-diagram/src/visitors/PositionVisitor.ts | Applied same unique connection filtering for MCP tools positioning |
| workspaces/mi/mi-diagram/src/visitors/NodeFactoryVisitor.ts | Grouped MCP tools by connection and populated mcpToolNames array |
| workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx | Updated to fetch AI connector icon for MCP tools and display grouped tool names |
| workspaces/mi/mi-diagram/src/components/nodes/BaseNodeModel.ts | Added handling for tool tag and MCP tools in connector operations |
| workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx | Changed selection management from string[] to McpTool[] objects with pre-selection support |
| workspaces/mi/mi-diagram/src/components/Form/FormGenerator.tsx | Passed documentUri and range to McpToolsSelection component |
| workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts | Extended McpToolsRequest and McpToolsResponse with additional fields |
| workspaces/mi/mi-diagram/src/resources/constants.ts | Adjusted MCP tools list max height offset |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx
Show resolved
Hide resolved
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx
Show resolved
Hide resolved
Comment on lines
+606
to
+618
| const toolsWithUniqueConnections = toolsList.filter((tool: Tool, index: number) => { | ||
| const isMcpTool = tool.isMcpTool; | ||
| if (!isMcpTool) { | ||
| return true; // Include all non-MCP tools | ||
| } | ||
| // For MCP tools, only include the first one with each unique connection name | ||
| const connectionName = tool.mcpConnection; | ||
| if (!connectionName) { | ||
| return false; | ||
| } | ||
| const firstIndex = toolsList.findIndex((t: Tool) => t.mcpConnection === connectionName); | ||
| return index === firstIndex; | ||
| }); |
There was a problem hiding this comment.
The filtering logic for tools with unique connections is duplicated across three visitor files (SizingVisitor, PositionVisitor, and NodeFactoryVisitor). Consider extracting this into a shared utility function to improve maintainability and ensure consistency. This would make it easier to maintain and modify this logic in the future.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx (2)
517-520:documentUriandrangeare missing from thefetchMcpToolseffect's dependency array.
fetchMcpToolscloses over bothdocumentUriandrange, which are used in thegetMcpToolsRPC call to retrieve previously-selected tools for the current document position. React docs recommend that the dependency array includes all values from component scope that change over time and are used by the effect; "otherwise, your code will reference stale values from previous renders."If
documentUriorrangechanges (e.g., the user edits nodes before opening the tool selector, shifting its range) whileselectedConnectionstays the same, the effect won't re-fire andfetchMcpToolswon't be called with the updated context. For this PR's core feature ("show already-selected tools"), that means stale pre-selections could be loaded.♻️ Proposed fix
useEffect(() => { // Retrieve mcp tools when selected connection changes fetchMcpTools(); -}, [rpcClient, selectedConnection]); +}, [rpcClient, selectedConnection, documentUri, range]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx` around lines 517 - 520, The useEffect that calls fetchMcpTools is missing documentUri and range in its dependency array, so it can close over stale values used by getMcpTools; update the dependency array for the effect in McpToolsSelection (the useEffect that currently lists [rpcClient, selectedConnection]) to include documentUri and range (or include fetchMcpTools if you memoize it) so the effect re-runs when the document URI or selection range changes and the fetched pre-selections are correct.
608-622: Redundant{fetchMcpTools && (...)}guard —fetchMcpToolsis always truthy.
fetchMcpToolsis a function defined unconditionally in the component body; it is nevernullorundefined. The check at line 610 is alwaystrueand can be simplified.♻️ Proposed fix
- {fetchMcpTools && ( - <div style={{ padding: '0 12px 12px 12px', display: 'flex', alignItems: 'center' }}> + <div style={{ padding: '0 12px 12px 12px', display: 'flex', alignItems: 'center' }}> <Button onClick={fetchMcpTools} disabled={loading} appearance="secondary" > <Codicon name="refresh" sx={{ marginRight: '6px' }} iconSx={{ width: 18, height: 18, fontSize: 18 }} /> Retry </Button> - </div> - )} + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx` around lines 608 - 622, Remove the redundant conditional guard around the retry button: the fetchMcpTools function is defined unconditionally in the MCPtoolsSelection component, so replace the "{fetchMcpTools && (...)}" block with the button markup rendered directly. Locate the JSX block that currently uses the fetchMcpTools guard (the div containing the Button which calls fetchMcpTools) and render it unconditionally, keeping the onClick, disabled={loading}, appearance, and Codicon props intact.workspaces/mi/mi-diagram/src/components/sidePanel/mediators/List.tsx (1)
301-312: Latent bug in version comparison: missing major-version lower-bound guard.The algorithm correctly returns
truewhenversion[0] > minVersion[0](line 308), but immediately jumps to comparingversion[1]without first checkingversion[0] < minVersion[0]. WhenminVersionis[0, 2, 1]this is harmless (major is always≥ 0), but ifminVersionis ever bumped to e.g.[1, 2, 1], a version like[0, 3, 0]would incorrectly returntrueat line 309.A loop-based or explicit three-way comparison avoids the issue entirely:
♻️ Proposed fix — general semver three-way comparison
- // Compare major.minor.patch - if (version[0] > minVersion[0]) return true; - if (version[1] > minVersion[1]) return true; - if (version[1] < minVersion[1]) return false; - return version[2] >= minVersion[2]; + // Compare major.minor.patch (each component in order) + for (let i = 0; i < minVersion.length; i++) { + if ((version[i] ?? 0) > minVersion[i]) return true; + if ((version[i] ?? 0) < minVersion[i]) return false; + } + return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/components/sidePanel/mediators/List.tsx` around lines 301 - 312, The version check in isAIMediatorVersionValid incorrectly skips a lower-bound check for the major version; update isAIMediatorVersionValid to perform a proper three-way semver comparison between the parsed version (from allMediators.AI.version) and minVersion (currently [0,2,1]) — either by first returning false if version[0] < minVersion[0], returning true if version[0] > minVersion[0], and only then comparing minor/patch, or by implementing a loop that compares each component in order and returns accordingly; ensure you handle missing parts (e.g., undefined) when mapping to Number.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx`:
- Around line 517-520: The useEffect that calls fetchMcpTools is missing
documentUri and range in its dependency array, so it can close over stale values
used by getMcpTools; update the dependency array for the effect in
McpToolsSelection (the useEffect that currently lists [rpcClient,
selectedConnection]) to include documentUri and range (or include fetchMcpTools
if you memoize it) so the effect re-runs when the document URI or selection
range changes and the fetched pre-selections are correct.
- Around line 608-622: Remove the redundant conditional guard around the retry
button: the fetchMcpTools function is defined unconditionally in the
MCPtoolsSelection component, so replace the "{fetchMcpTools && (...)}" block
with the button markup rendered directly. Locate the JSX block that currently
uses the fetchMcpTools guard (the div containing the Button which calls
fetchMcpTools) and render it unconditionally, keeping the onClick,
disabled={loading}, appearance, and Codicon props intact.
In `@workspaces/mi/mi-diagram/src/components/sidePanel/mediators/List.tsx`:
- Around line 301-312: The version check in isAIMediatorVersionValid incorrectly
skips a lower-bound check for the major version; update isAIMediatorVersionValid
to perform a proper three-way semver comparison between the parsed version (from
allMediators.AI.version) and minVersion (currently [0,2,1]) — either by first
returning false if version[0] < minVersion[0], returning true if version[0] >
minVersion[0], and only then comparing minor/patch, or by implementing a loop
that compares each component in order and returns accordingly; ensure you handle
missing parts (e.g., undefined) when mapping to Number.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsxworkspaces/mi/mi-diagram/src/components/sidePanel/mediators/List.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx (1)
608-622:⚠️ Potential issue | 🟡 Minor
{fetchMcpTools && ...}is always truthy — dead conditional.
fetchMcpToolsis a locally defined function and can never be falsy. The guard provides no protection and can be removed.♻️ Proposed fix
- {fetchMcpTools && ( - <div style={{ padding: '0 12px 12px 12px', display: 'flex', alignItems: 'center' }}> - <Button - onClick={fetchMcpTools} - disabled={loading} - appearance="secondary" - > - <Codicon name="refresh" sx={{ marginRight: '6px' }} iconSx={{ width: 18, height: 18, fontSize: 18 }} /> - Retry - </Button> - </div> - )} + <div style={{ padding: '0 12px 12px 12px', display: 'flex', alignItems: 'center' }}> + <Button + onClick={fetchMcpTools} + disabled={loading} + appearance="secondary" + > + <Codicon name="refresh" sx={{ marginRight: '6px' }} iconSx={{ width: 18, height: 18, fontSize: 18 }} /> + Retry + </Button> + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx` around lines 608 - 622, The conditional guard `{fetchMcpTools && (...)}` is dead because `fetchMcpTools` is a locally defined function and always truthy; remove the guard and render the Retry button block unconditionally (the div containing Button/Codicon/Retry) so the UI isn't wrapped in an unnecessary check, or if the original intent was to only render when an external prop exists, rename or change `fetchMcpTools` to the optional prop (e.g., `onFetchMcpTools`) and check that instead; update references to `fetchMcpTools`/`onFetchMcpTools` in the `McpToolsSelection` component accordingly.workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx (1)
375-420:⚠️ Potential issue | 🟠 MajorFix incorrect source for MCP connection value at lines 381 and 418.
When
isMCPToolis true,connectorNodeis set to(node.stNode as Tool).mediator(aConnectorinstance). However, the actualmcpConnectionvalue lives on theToolobject itself, not on its mediator. This causes two issues:
- Line 381 — the condition
connectorNode.mcpConnectionchecks the wrong object and should useisMCPToolinstead (when true, the MCP icon should render).- Line 418 — the expression
connectorNode.configKey || connectorNode.mcpConnectiondoesn't properly handle the MCP case; for MCP tools, the value should come from(node.stNode as Tool).mcpConnectionas shown in the correct pattern at line 254.🐛 Proposed fixes
- {connectorNode.mcpConnection && <g transform="translate(68,7)"> + {isMCPTool && <g transform="translate(68,7)"> <foreignObject width="25" height="25"> <S.IconContainer>{getMediatorIconsFromFont('mcp')}</S.IconContainer> </foreignObject> </g>}- {connectorNode.configKey || connectorNode.mcpConnection} + {isMCPTool ? (node.stNode as Tool).mcpConnection : connectorNode.configKey}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx` around lines 375 - 420, The MCP rendering and value lookup use the mediator (connectorNode) instead of the Tool when isMCPTool is true; change the icon condition to check isMCPTool (not connectorNode.mcpConnection) so the MCP icon renders for tools, and change the displayed text expression to use connectorNode.configKey || (isMCPTool ? (node.stNode as Tool).mcpConnection : connectorNode.mcpConnection) so MCP tools pull mcpConnection from the Tool instance rather than from connectorNode; update both places where connectorNode.mcpConnection is currently referenced (the icon render block using getMediatorIconsFromFont('mcp') and the ConnectionText expression) to use the corrected checks/lookup.
🧹 Nitpick comments (1)
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx (1)
423-436:selectedToolsprop is destructured but never consumed — misleading API surface.The
selectedTools: Set<string>prop is assigned a default and destructured, butselectedToolNamesderived fromuseWatchis used throughout the component for all rendering and logic. Any value passed in viaselectedToolsis silently ignored, which will mislead callers.Consider removing
selectedToolsfromMcpToolsSelectionPropsif the component's selection state is wholly owned by the form viamcpToolsSelection, or document clearly that the prop is deprecated/unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx` around lines 423 - 436, The prop selectedTools is destructured in McpToolsSelection but never used (the component reads selection from useWatch via selectedToolNames/mcpToolsSelection), so remove selectedTools from the McpToolsSelectionProps type and from the McpToolsSelection parameter list (and any default value assignment) to avoid a misleading API surface; alternatively, if you intend to allow external initialization, wire selectedTools into the form state by setting the form default or calling setValue('mcpToolsSelection', Array.from(selectedTools)) inside McpToolsSelection (keep only one approach—prefer removing the unused prop if the form fully owns selection).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx`:
- Around line 257-259: The handler in ConnectorNodeWidget currently returns
silently when no match is found for connectionName in
connectionData.connections, leaving users with no feedback; update the code that
checks `if (!connection) { return; }` (in the click/connection handler in
ConnectorNodeWidget) to surface an explicit user-visible error (for example call
the app's notification/toast API, set a local error state that briefly
highlights the connector, or log a clear warning) and include the missing
`connectionName` in the message; ensure you still guard against null but replace
the silent return with a user-facing action so misconfigured MCP connections are
obvious.
---
Outside diff comments:
In
`@workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx`:
- Around line 608-622: The conditional guard `{fetchMcpTools && (...)}` is dead
because `fetchMcpTools` is a locally defined function and always truthy; remove
the guard and render the Retry button block unconditionally (the div containing
Button/Codicon/Retry) so the UI isn't wrapped in an unnecessary check, or if the
original intent was to only render when an external prop exists, rename or
change `fetchMcpTools` to the optional prop (e.g., `onFetchMcpTools`) and check
that instead; update references to `fetchMcpTools`/`onFetchMcpTools` in the
`McpToolsSelection` component accordingly.
In
`@workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx`:
- Around line 375-420: The MCP rendering and value lookup use the mediator
(connectorNode) instead of the Tool when isMCPTool is true; change the icon
condition to check isMCPTool (not connectorNode.mcpConnection) so the MCP icon
renders for tools, and change the displayed text expression to use
connectorNode.configKey || (isMCPTool ? (node.stNode as Tool).mcpConnection :
connectorNode.mcpConnection) so MCP tools pull mcpConnection from the Tool
instance rather than from connectorNode; update both places where
connectorNode.mcpConnection is currently referenced (the icon render block using
getMediatorIconsFromFont('mcp') and the ConnectionText expression) to use the
corrected checks/lookup.
---
Nitpick comments:
In
`@workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsx`:
- Around line 423-436: The prop selectedTools is destructured in
McpToolsSelection but never used (the component reads selection from useWatch
via selectedToolNames/mcpToolsSelection), so remove selectedTools from the
McpToolsSelectionProps type and from the McpToolsSelection parameter list (and
any default value assignment) to avoid a misleading API surface; alternatively,
if you intend to allow external initialization, wire selectedTools into the form
state by setting the form default or calling setValue('mcpToolsSelection',
Array.from(selectedTools)) inside McpToolsSelection (keep only one
approach—prefer removing the unused prop if the form fully owns selection).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/mi/mi-diagram/src/components/Form/MCPtoolsSelection/McpToolsSelection.tsxworkspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx
workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
workspaces/mi/mi-diagram/src/visitors/PositionVisitor.ts (1)
533-545:findIndexinsidefilteris O(n²); prefer aSetfor seen connections.
toolsList.findIndex(...)is called once per MCP tool in thefiltercallback, making the whole block O(n²). While tool counts are small today, the intent (first-occurrence deduplication) is more clearly expressed with aSet.♻️ O(1) deduplication with a Set
- const toolsWithUniqueConnections = toolsList.filter((tool: Tool, index: number) => { - const isMcpTool = tool.isMcpTool; - if (!isMcpTool) { - return true; // Include all non-MCP tools - } - // For MCP tools, only include the first one with each unique connection name - const connectionName = tool.mcpConnection; - if (!connectionName) { - return false; - } - const firstIndex = toolsList.findIndex((t: Tool) => t.mcpConnection === connectionName); - return index === firstIndex; - }); + const seenMcpConnections = new Set<string>(); + const toolsWithUniqueConnections = toolsList.filter((tool: Tool) => { + if (!tool.isMcpTool) { + return true; // Include all non-MCP tools + } + // For MCP tools, only include the first one with each unique connection name + const connectionName = tool.mcpConnection; + if (!connectionName) { + return false; + } + if (seenMcpConnections.has(connectionName)) { + return false; + } + seenMcpConnections.add(connectionName); + return true; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-diagram/src/visitors/PositionVisitor.ts` around lines 533 - 545, The current filter for toolsWithUniqueConnections is O(n²) because it calls toolsList.findIndex inside the callback; replace that logic by using a Set to track seen mcpConnection values: iterate/filter toolsList, include all non-MCP tools (tool.isMcpTool false), and for MCP tools check if tool.mcpConnection is falsy (skip) or not in the Set (add it and include) otherwise skip; update references to Tool, toolsList, tool.isMcpTool, and tool.mcpConnection accordingly to preserve the “first-occurrence” semantics while making the operation O(n).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@workspaces/mi/mi-diagram/src/visitors/PositionVisitor.ts`:
- Around line 533-545: The current filter for toolsWithUniqueConnections is
O(n²) because it calls toolsList.findIndex inside the callback; replace that
logic by using a Set to track seen mcpConnection values: iterate/filter
toolsList, include all non-MCP tools (tool.isMcpTool false), and for MCP tools
check if tool.mcpConnection is falsy (skip) or not in the Set (add it and
include) otherwise skip; update references to Tool, toolsList, tool.isMcpTool,
and tool.mcpConnection accordingly to preserve the “first-occurrence” semantics
while making the operation O(n).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsxworkspaces/mi/mi-diagram/src/visitors/PositionVisitor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/mi/mi-diagram/src/components/nodes/ConnectorNode/ConnectorNodeWidget.tsx
gigara
approved these changes
Feb 23, 2026
Contributor
gigara
left a comment
gigara
left a comment
There was a problem hiding this comment.
LGTM. Add a test case in a separate PR to cover the new flow.
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.
Purpose
MCP tools improvements
Goals
Approach
UI Component Development
npm run storybookfrom the root directory to view current components.Manage Icons
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Chores