XS⚠️ ◾ Moved MCP types to shared folder#656
Conversation
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
There was a problem hiding this comment.
Pull request overview
Refactors MCP server-related types/interfaces so they live in a shared module that can be consumed by both the UI and backend (supporting the broader effort to deduplicate transport/type declarations).
Changes:
- Moved MCP server config/transport types from UI/backend into
src/shared/types/mcp.ts. - Updated UI
McpServerFormto import/re-export shared MCP types and adjusted new-configidinitialization. - Updated backend MCP types module to re-export shared types and tweaked in-memory transport option detection in
MCPServerManager.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ui/src/components/settings/mcp/McpServerForm.tsx | Switches to shared MCP types and removes local duplicated type declarations. |
| src/shared/types/mcp.ts | Introduces shared Transport + MCP server config interfaces/unions. |
| src/backend/services/mcp/types.ts | Re-exports MCP types from the shared module to eliminate duplication. |
| src/backend/services/mcp/mcp-server-manager.ts | Changes how in-memory transport options are derived from config. |
Comments suppressed due to low confidence (1)
src/backend/services/mcp/mcp-server-manager.ts:154
- This change switches the in-memory options detection from checking property presence (
"inMemoryServerId" in config) to a truthy check (config.inMemoryServerId). IfinMemoryServerIdis missing/empty, the client will now be created withoutoptions.inMemoryClientTransport, causingcreateClientAsyncto throw and be swallowed by thecreationPromisecatch (returningnull) rather than failing fast with a clear error. Consider explicitly validating/throwing whenconfig.transport === "inMemory"andinMemoryServerIdis not a non-empty string, so misconfigured internal servers don’t fail silently.
let options: CreateClientOptions | undefined;
if (config.transport === "inMemory" && config.inMemoryServerId) {
const transport = MCPServerManager.internalClientTransports.get(config.inMemoryServerId);
if (!transport) {
throw new Error(
`No in-memory transport registered for server '${config.name}'. Ensure it was initialized correctly.`,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export type Transport = "streamableHttp" | "stdio" | "inMemory"; | ||
|
|
||
| export interface MCPBaseConfig { | ||
| id: string; |
There was a problem hiding this comment.
MCPBaseConfig.id is typed as a required string, but the UI now creates new configs with id: "" (see McpServerFormWrapper) as a sentinel for “not yet persisted”. That means the shared type doesn’t reflect the real contract and can lead to accidental empty IDs leaking into places that expect a stable identifier. Consider making id optional/nullable for “draft” configs (or introducing a separate Draft/New config type), and keep a non-empty id for persisted configs only (validated server-side).
| id: string; | |
| /** | |
| * Unique identifier for persisted configs. | |
| * For draft / not-yet-persisted configs this may be omitted, and the server | |
| * is responsible for assigning and validating a non-empty id. | |
| */ | |
| id?: string; |
|
🚀 Pre-release build is available for this PR: |
|
✅ Automated Release Created Successfully Release Details:
You can monitor the build progress in the Actions tab. |
✏️
✏️ #639
✏️ Moved MCP types to shared folder
✏️