feat: Update MCPServer interface to make command and args optional, e…#28
Merged
Conversation
…nhance transport type handling in MCP components
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Closed
📝 WalkthroughWalkthroughThree UI components are updated to support multiple MCP server transport types (stdio, sse, http, direct) by making Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/MCPServerForm.tsx (1)
8-20:⚠️ Potential issue | 🟠 MajorForm cannot safely edit non-stdio MCP servers due to missing transport fields.
The
MCPServerFormPropsinterface (lines 8-20) omitsurlandtransportType, which exist inMCPManagementPanel's server interface and are required for SSE/HTTP transport types. When editing a non-stdio server:
- The form lacks inputs for
urlandtransportType, causing these fields to be lost on save (line 131 sends onlycommand,args, etc.)- The required command validation (line 72-74) conflicts with the backend design where only stdio transport requires a command
- The form can be invoked for any server type (line 259 passes
server as any) with no filteringThis creates a data-loss vulnerability and type inconsistency. The backend's
addMCPServervalidates transport-specific fields conditionally, butupdateMCPServerlacks this validation, allowing inconsistent state.Either:
- Add
urlandtransportTypefields with conditional rendering/validation based on transport type- Filter non-stdio servers from the edit button in
MCPManagementPanel- Add transport-specific validation to
updateMCPServerto matchaddMCPServer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MCPServerForm.tsx` around lines 8 - 20, The MCPServerFormProps type and MCPServerForm are missing transport fields (url and transportType), causing edits of non-stdio servers to drop required transport-specific data and clash with validation; update the MCPServerFormProps to include url and transportType, add conditional rendering and validation in MCPServerForm (show url input only when transportType !== "stdio" and require command only when transportType === "stdio"), and ensure the save path used by the form calls updateMCPServer with transportType and url included; alternatively, if you intend to disallow editing non-stdio servers, change MCPManagementPanel to stop passing non-stdio servers into the form (remove the server as any cast and filter edit button) or add matching transport-specific checks to updateMCPServer to validate url/transportType like addMCPServer does.
🧹 Nitpick comments (4)
src/components/MCPManagementPanel.tsx (3)
43-72: Duplicated helper functions across components.
getTransportTypeand the transport label logic are duplicated inMCPServerSelector.tsx(lines 52-81). Consider extracting these to a shared utility module to maintain consistency and reduce duplication.♻️ Suggested extraction
Create a shared utility, e.g.,
src/utils/mcpTransport.ts:export type TransportType = "stdio" | "sse" | "http" | "direct"; export function getTransportType(server: { transportType?: TransportType; url?: string; command?: string; }): TransportType { if (server.transportType) return server.transportType; if (server.url) return "sse"; if (server.command) return "stdio"; return "direct"; } export function getTransportLabel(transportType: TransportType): string { const labels: Record<TransportType, string> = { stdio: "STDIO", sse: "SSE", http: "HTTP", direct: "Direct", }; return labels[transportType]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MCPManagementPanel.tsx` around lines 43 - 72, Extract the duplicated getTransportType and getTransportLabel logic into a shared utility (e.g., a new module exporting TransportType, getTransportType, and getTransportLabel) and replace the local implementations in MCPManagementPanel.tsx (functions getTransportType and getTransportLabel) and MCPServerSelector.tsx with imports from that module; ensure the new getTransportType signature accepts an object with optional transportType, url, and command fields and that getTransportLabel accepts the TransportType union so both components import the same types and functions and delete the duplicated local functions.
198-211: IIFE pattern works but consider extracting to a named component.The IIFE works correctly for scoping computed values (
transportType,transportLabel,args), but the 180+ line inline function reduces readability. AServerCardcomponent would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MCPManagementPanel.tsx` around lines 198 - 211, The inline IIFE that renders each server card is large and should be extracted into a named presentational component (e.g., ServerCard) to improve readability and reuse; create a ServerCard component that accepts props (server, selectedServer, onSelect, any handlers/props used inside), move the computed values and JSX (transportType via getTransportType, transportLabel via getTransportLabel, args, className logic referencing selectedServer._id, etc.) into that component, replace the IIFE with a simple <ServerCard key={server._id} server={server} selectedServer={selectedServer} .../> invocation, and ensure any event callbacks or state lifts (selection/updating) are passed as props so behavior remains identical.
69-70: Unreachable default case.The
defaultbranch is unreachable since the parametertransportTypeis typed as a union of the four literal types. TypeScript will error if a new transport is added without updating the switch. Consider removing it or usingsatisfiesfor exhaustiveness checking.♻️ Exhaustiveness check pattern
function getTransportLabel(transportType: "stdio" | "sse" | "http" | "direct") { switch (transportType) { case "stdio": return "STDIO"; case "sse": return "SSE"; case "http": return "HTTP"; case "direct": return "Direct"; - default: - return "Unknown"; } + // Exhaustiveness check - will error if a case is missing + const _exhaustive: never = transportType; + return _exhaustive; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MCPManagementPanel.tsx` around lines 69 - 70, The switch over transportType in MCPManagementPanel currently has an unreachable default that masks non-exhaustiveness; remove the default return "Unknown" and replace it with an exhaustive-check pattern: add a small helper function assertUnreachable(x: never): never and call assertUnreachable(transportType) in the switch's fallback so TypeScript will error if a new transport literal is added and not handled; reference the transportType switch in MCPManagementPanel and the new assertUnreachable helper to locate where to change code.src/components/MCPServerSelector.tsx (1)
278-280: Consider consistent label casing withMCPManagementPanel.
MCPManagementPanelusesgetTransportLabel()which returns "Direct" (capitalized), but here you usetransportType.toUpperCase()which returns "DIRECT" (all caps). This creates visual inconsistency between components.♻️ Use shared label function
- <span className="text-xs px-2 py-0.5 bg-black/30 text-green-300 rounded border border-green-900/30"> - {transportType.toUpperCase()} - </span> + <span className="text-xs px-2 py-0.5 bg-black/30 text-green-300 rounded border border-green-900/30"> + {getTransportLabel(transportType)} + </span>Then add a
getTransportLabelfunction (or import from shared utility) for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MCPServerSelector.tsx` around lines 278 - 280, The MCPServerSelector component renders the transport badge using transportType.toUpperCase(), causing inconsistent casing with MCPManagementPanel which uses getTransportLabel(); replace the direct toUpperCase() usage by calling the shared getTransportLabel function (or import it from the shared utility) when rendering the span so both components show the same label format; update the span that currently contains {transportType.toUpperCase()} to use getTransportLabel(transportType) and ensure the import or local helper is added/used consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/MCPManagementPanel.tsx`:
- Line 259: The inline cast "server as any" hides a type mismatch between the
object passed from the MCP list and the props expected by MCPServerForm; remove
the cast and either (a) update handleEdit to accept the concrete server type and
map/transform that server into the exact props MCPServerForm expects before
opening the form, or (b) extend MCPServerForm's props (or add optional fields)
to include url and transportType so the types align; update the signature of
handleEdit and the place where it calls/opens MCPServerForm (referencing
handleEdit, MCPServerForm, and the server variable) accordingly so no `as any`
is needed.
---
Outside diff comments:
In `@src/components/MCPServerForm.tsx`:
- Around line 8-20: The MCPServerFormProps type and MCPServerForm are missing
transport fields (url and transportType), causing edits of non-stdio servers to
drop required transport-specific data and clash with validation; update the
MCPServerFormProps to include url and transportType, add conditional rendering
and validation in MCPServerForm (show url input only when transportType !==
"stdio" and require command only when transportType === "stdio"), and ensure the
save path used by the form calls updateMCPServer with transportType and url
included; alternatively, if you intend to disallow editing non-stdio servers,
change MCPManagementPanel to stop passing non-stdio servers into the form
(remove the server as any cast and filter edit button) or add matching
transport-specific checks to updateMCPServer to validate url/transportType like
addMCPServer does.
---
Nitpick comments:
In `@src/components/MCPManagementPanel.tsx`:
- Around line 43-72: Extract the duplicated getTransportType and
getTransportLabel logic into a shared utility (e.g., a new module exporting
TransportType, getTransportType, and getTransportLabel) and replace the local
implementations in MCPManagementPanel.tsx (functions getTransportType and
getTransportLabel) and MCPServerSelector.tsx with imports from that module;
ensure the new getTransportType signature accepts an object with optional
transportType, url, and command fields and that getTransportLabel accepts the
TransportType union so both components import the same types and functions and
delete the duplicated local functions.
- Around line 198-211: The inline IIFE that renders each server card is large
and should be extracted into a named presentational component (e.g., ServerCard)
to improve readability and reuse; create a ServerCard component that accepts
props (server, selectedServer, onSelect, any handlers/props used inside), move
the computed values and JSX (transportType via getTransportType, transportLabel
via getTransportLabel, args, className logic referencing selectedServer._id,
etc.) into that component, replace the IIFE with a simple <ServerCard
key={server._id} server={server} selectedServer={selectedServer} .../>
invocation, and ensure any event callbacks or state lifts (selection/updating)
are passed as props so behavior remains identical.
- Around line 69-70: The switch over transportType in MCPManagementPanel
currently has an unreachable default that masks non-exhaustiveness; remove the
default return "Unknown" and replace it with an exhaustive-check pattern: add a
small helper function assertUnreachable(x: never): never and call
assertUnreachable(transportType) in the switch's fallback so TypeScript will
error if a new transport literal is added and not handled; reference the
transportType switch in MCPManagementPanel and the new assertUnreachable helper
to locate where to change code.
In `@src/components/MCPServerSelector.tsx`:
- Around line 278-280: The MCPServerSelector component renders the transport
badge using transportType.toUpperCase(), causing inconsistent casing with
MCPManagementPanel which uses getTransportLabel(); replace the direct
toUpperCase() usage by calling the shared getTransportLabel function (or import
it from the shared utility) when rendering the span so both components show the
same label format; update the span that currently contains
{transportType.toUpperCase()} to use getTransportLabel(transportType) and ensure
the import or local helper is added/used consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22d858cc-d63d-4837-bf7e-e894fdaa10d1
📒 Files selected for processing (3)
src/components/MCPManagementPanel.tsxsrc/components/MCPServerForm.tsxsrc/components/MCPServerSelector.tsx
| }`} /> | ||
| </button> | ||
| <button | ||
| onClick={() => handleEdit(server as any)} |
There was a problem hiding this comment.
Avoid as any cast — masks type mismatch with MCPServerForm.
This cast hides the fact that MCPServerForm doesn't accept url or transportType props. When the form is extended to support other transports, this cast should be removed and types aligned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/MCPManagementPanel.tsx` at line 259, The inline cast "server
as any" hides a type mismatch between the object passed from the MCP list and
the props expected by MCPServerForm; remove the cast and either (a) update
handleEdit to accept the concrete server type and map/transform that server into
the exact props MCPServerForm expects before opening the form, or (b) extend
MCPServerForm's props (or add optional fields) to include url and transportType
so the types align; update the signature of handleEdit and the place where it
calls/opens MCPServerForm (referencing handleEdit, MCPServerForm, and the server
variable) accordingly so no `as any` is needed.
Collaborator
|
First, verify whether MikePfunk28 really owns a Projects v2 board numbered 4. Second, verify whether the token used in Actions can see that board. Third, if the board is org-owned, fix --owner. Fourth, stop letting this step fail the entire workflow unless adding to the project is actually mission-critical. |
Collaborator
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
2 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.
…nhance transport type handling in MCP components
Summary by CodeRabbit
Release Notes