Conversation
Greptile SummaryThis PR migrates MCP server configuration from browser extension storage ( Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Ext as Browser Extension
participant BG as Background Script
participant UI as React UI (useMcpConfig)
participant SRV as Agent Server (/mcp-config)
participant FS as mcp.json (disk)
participant Agent as AiSdkAgent
Note over BG,FS: One-time migration on startup
BG->>FS: migrateMcpToServer() → POST /mcp-config/migrate
FS-->>BG: migrated count
Note over UI,FS: CRUD via REST hooks
UI->>SRV: GET /mcp-config
SRV->>FS: readMcpConfig()
FS-->>SRV: { version, servers[] }
SRV-->>UI: { servers[] }
UI->>SRV: POST /mcp-config (add server)
SRV->>FS: addMcpServer() → atomic write
FS-->>SRV: McpServerEntry (with id)
SRV-->>UI: { server } 201
UI->>UI: invalidateQueries([mcp-config])
Note over Agent,FS: Per-session MCP tool building
Agent->>FS: buildMcpServerSpecs() → getMcpServers()
FS-->>Agent: McpServerEntry[]
Agent->>Agent: createMcpClients(specs)
Agent-->>Agent: { tools } merged toolset
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Claude finished @DaniAkash's task in 2m 4s —— View job Code Review
The architecture is solid — migrating MCP config to a server-side Issues Remaining from Previous Review1. const addCustomServer = async (config: CustomServerConfig) => {
await addServerMutation({ // throws on non-2xx response
...
})
track(CUSTOM_MCP_ADDED_EVENT, ...)
}All sibling handlers ( 2. Custom server removal silently drops errors — } else {
removeServerMutation(server.id) // promise dropped, no .catch()
}Two issues: (a) errors are silently lost with no user feedback, and (b) 3. Dialog closes synchronously before async mutation settles — interface AddCustomMCPDialogProps {
onAddServer: (config: CustomServerConfig) => void // typed as void
}
// In onUrlSubmit:
onAddServer({...}) // fire-and-forget
resetForms() // dialog closes immediately
onOpenChange(false)The prop should be Remaining Issue from Greptile (Lower Priority)4. Stale After a successful migration, New Findings5. No loading state on custom server deletion (also part of issue #2 above)
6. import { Experimental_StdioMCPTransport as StdioMCPTransport } from '@ai-sdk/mcp/mcp-stdio'The What Looks Good
|
|
Claude finished @DaniAkash's task in 2m 6s —— View job Code Review
Good progress — the critical issues from the previous round are resolved. Here's the updated status: Previously Raised Issues — Status✅ Fixed: The method now reads directly from ✅ Fixed: Now wrapped in ✅ Fixed: Custom server removal dropped promise ( Now uses Note: The redundant Remaining Issues (2 low-priority items from previous review)1. Stale schema fields still present —
// These lines should be removed:
enabledMcpServers: z.array(z.string()).optional(),
customMcpServers: z.array(CustomMcpServerSchema).optional(),
2. Stale After a successful migration, the original if (response.ok) {
await mcpServerStorage.setValue([]) // clear legacy data
await storage.setItem(MCP_MIGRATED_KEY, true)
}Minor Note (no action needed)Dialog closes before async mutation resolves —
Args whitespace splitting —
|
| const onStdioSubmit = (values: StdioFormValues) => { | ||
| // Parse space-separated args string into array | ||
| const args = values.args?.trim() | ||
| ? values.args.trim().split(/\s+/) | ||
| : undefined |
There was a problem hiding this comment.
Args with spaces cannot be represented
Splitting the args string on whitespace (split(/\s+/)) breaks for any argument containing a space — the most common case being file paths (e.g. /Users/John Doe/project or C:\Program Files\my-server). Such a path would be silently split into multiple incorrect arguments, causing the stdio MCP server to fail at start-up with no clear error in the UI.
Consider replacing the single args string with a dynamic list of individual argument fields (similar to the env-var key/value pairs already used below), or at minimum support simple shell quoting so that "arg with spaces" is kept as one token.
// Current — breaks on spaces:
const args = values.args?.trim()
? values.args.trim().split(/\s+/)
: undefined
// Safer alternative using shell-quote or a per-argument list UI:
// e.g. import { parse } from 'shell-quote'
// const args = values.args?.trim() ? parse(values.args.trim()) as string[] : undefinedPrompt To Fix With AI
This is a comment left during a code review.
Path: apps/agent/entrypoints/app/connect-mcp/AddCustomMCPDialog.tsx
Line: 159-163
Comment:
**Args with spaces cannot be represented**
Splitting the args string on whitespace (`split(/\s+/)`) breaks for any argument containing a space — the most common case being file paths (e.g. `/Users/John Doe/project` or `C:\Program Files\my-server`). Such a path would be silently split into multiple incorrect arguments, causing the stdio MCP server to fail at start-up with no clear error in the UI.
Consider replacing the single args string with a dynamic list of individual argument fields (similar to the env-var key/value pairs already used below), or at minimum support simple shell quoting so that `"arg with spaces"` is kept as one token.
```ts
// Current — breaks on spaces:
const args = values.args?.trim()
? values.args.trim().split(/\s+/)
: undefined
// Safer alternative using shell-quote or a per-argument list UI:
// e.g. import { parse } from 'shell-quote'
// const args = values.args?.trim() ? parse(values.args.trim()) as string[] : undefined
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
This pull request refactors how MCP server configuration is managed and accessed throughout the application. The main change is the migration from the legacy
useMcpServersand related utilities to a new set of hooks and methods (useMcpConfig,useAddMcpServer,useRemoveMcpServer) that centralize and streamline MCP server state management. Additionally, the codebase is simplified by removing now-unnecessary references to enabled MCP/custom servers in chat and workflow contexts. A data migration is also triggered on background startup to ensure existing user data is preserved.MCP Server Management Refactor:
useMcpServerswith the newuseMcpConfig,useAddMcpServer, anduseRemoveMcpServerhooks across the app, including inAppSelector.tsx,ConnectMCP.tsx,NewTab.tsx, andChatFooter.tsx. This change unifies MCP server state management and enables more robust server mutations and cache updates. [1] [2] [3] [4] [5] [6] [7] [8]mutateMcpConfig, ensuring UI consistency after server changes. [1] [2] [3] [4] [5]Custom Server Enhancements:
transport,headers,command,args,cwd, andenv, and updated the UI to display transport type and command line for stdio transports. [1] [2]Legacy Code and Data Migration:
enabledMcpServersRefandenabledCustomServersReffrom chat, workflow, and session hooks (useChatRefs,useChatSession,useRunWorkflow,CreateGraph.tsx), simplifying the context and reducing unnecessary data passing. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]migrateMcpToServer()in the background script to migrate legacy MCP server data to the new storage format on startup, ensuring user data continuity. [1] [2]These changes modernize and consolidate MCP server management, reduce technical debt, and prepare the codebase for future MCP-related enhancements.