warn if user try to attach edge for already connected MCP, remove 0 badge in MCP nodes, in favour of No tools#1910
Conversation
🦋 Changeset detectedLatest commit: 7fa2099 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
@claude --verify and add a changeset |
Co-authored-by: Dimitri POSTOLOV <dimaMachina@users.noreply.github.com>
There was a problem hiding this comment.
PR Review Summary
1 Key Finding | Risk: High
🔴❗ Critical (1) ❗🔴
- 🔴 Critical:
orphaned-tools-detector.ts:29Function signature mismatch breaksmcp-node-editor.tsx— see inline comment
🟠⚠️ Major (0) 🟠⚠️
No major issues identified.
🟡 Minor (0) 🟡
No minor issues identified.
💭 Consider (0) 💭
No additional suggestions.
🧹 While You're Here (1) 🧹
🧹 1) node-types.tsx MCPNodeData interface missing temp properties
Issue: The MCPNodeData interface is missing tempSelectedTools and tempHeaders properties that are accessed via (node.data as any) casts in orphaned-tools-detector.ts.
Why: Since you're already cleaning up this file by removing unused parameters and improving property access patterns (using destructuring for relationshipId), this is a natural opportunity to complete the type safety improvement.
Fix: Add the missing properties to MCPNodeData in node-types.tsx:
export interface MCPNodeData extends Record<string, unknown>, AnimatedNode {
toolId: string;
subAgentId?: string | null;
relationshipId?: string | null;
name?: string;
imageUrl?: string;
provider?: string;
tempSelectedTools?: string[] | null; // Add this
tempHeaders?: Record<string, string>; // Add this
tempToolPolicies?: Record<string, { needsApproval?: boolean }>;
}Refs: node-types.tsx:41-49 — current MCPNodeData interface
🚫 REQUEST CHANGES
Summary: The PR introduces a breaking change that will cause TypeScript compilation errors. The edges parameter was removed from three utility functions in orphaned-tools-detector.ts, but mcp-node-editor.tsx still passes 3 arguments to these functions. Please update mcp-node-editor.tsx to remove the edges argument from the affected function calls (lines 74, 97, 99-103) and their dependency arrays.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
orphaned-tools-detector.ts:32-33 |
Type inconsistency with tempSelectedTools access |
Reclassified as "While You're Here" — pre-existing issue worth addressing alongside this PR's cleanup |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-frontend |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
1 | 0 | 0 | 1 | 0 | 0 | 0 |
| Total | 2 | 0 | 0 | 1 | 1 | 0 | 0 |
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low
Re-Review Delta Assessment
This is a delta-scoped re-review following my previous REQUEST_CHANGES review. The single commit since the last review (7fa20992 fix typecheck) correctly addresses the critical issue I identified:
Previous Critical Issue (RESOLVED): The edges parameter was removed from utility functions in orphaned-tools-detector.ts, but mcp-node-editor.tsx was still passing 3 arguments.
Fix Applied:
- ✅ Removed
edgesfrom theuseAgentStoreselector (line 56) - ✅ Updated
getCurrentHeadersForNodecall to 2 arguments (line 71) - ✅ Updated
getCurrentSelectedToolsForNodecall to 2 arguments (line 94) - ✅ Updated
getCurrentToolPoliciesForNodecall to 2 arguments (line 95) - ✅ Removed
edgesfromuseCallbackdependency array (line 72)
The fix is clean and correctly aligns the caller with the updated function signatures. No new issues introduced.
🟡 Remaining Consideration (from prior review)
The "While You're Here" suggestion from the prior review about adding tempSelectedTools, tempHeaders, and tempToolPolicies to the MCPNodeData interface to eliminate (node.data as any) casts remains valid but is not blocking — it's a type safety improvement that can be addressed in a follow-up PR.
✅ APPROVE
Summary: The critical breaking change has been fixed. The PR now correctly implements the warning for connecting already-connected MCPs and the "No tools" display improvement. Ship it! 🚀
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Delta-only review — no subagents dispatched | — | — | — | — | — | — | — |
Note: This was a delta-scoped re-review. The previous full review already dispatched specialized reviewers for the complete PR.
Screen.Recording.2026-02-11.at.05.35.17.mov