feat: configure MCP servers in settings page #3
Conversation
# Conflicts: # lib/db/queries.ts # pnpm-lock.yaml
|
@tobiasbueschel is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
|
lets gooo! |
|
This is sick, we need this! |
|
While I do hope this PR gets accepted, I believe it may be difficult due to some practical limitations. Why it might not be a good fit for the template: ⸻ That’s why I decided to build a dedicated MCP client project based on what I learned here. 👉 MCP Client Chatbot Hope it’s helpful—and if you have feedback or ideas, I’d love to hear them. Thanks! https://github.com/cgoinglove/mcp-client-chatbot |
|
/codehelper please review this pr |
![do-refacto[bot]](https://avatars.githubusercontent.com/in/1142269?s=60&v=4){kind=small}
| result.mergeIntoDataStream(dataStream, { | ||
| sendReasoning: true, | ||
| }); | ||
| } catch (dbError) { |
There was a problem hiding this comment.
Error Handling: The mcpClientsToClose array is only cleared in the onFinish handler but not in the error cases. This could lead to memory leaks if errors occur frequently. Consider adding a finally block to ensure clients are always properly closed.
| try { | ||
| // Create MCP client based on server config | ||
| const config = server.config as any; | ||
| let clientOptions: { transport: any; name: string }; |
There was a problem hiding this comment.
Type Safety: The config object is cast as any which bypasses TypeScript's type checking. Consider defining a proper interface for the config object to improve type safety and code maintainability.
| console.error("Invalid SSE config for server", server.id, config); | ||
| throw new Error("Invalid SSE configuration: URL is missing or not a string."); | ||
| } | ||
| clientOptions = { |
There was a problem hiding this comment.
UX Improvement: When a server fails to load its schema, the error message is only logged to the console and a generic message is displayed to the user. Consider displaying a more specific error message to help users troubleshoot connection issues.
| setError(null); | ||
| try { | ||
| const fetchedServers = await fetchMcpServers(); | ||
| setServers(fetchedServers); |
There was a problem hiding this comment.
Performance Concern: The fetchServerSchema function is called for all enabled servers on initial load, which could lead to performance issues if there are many servers. Consider implementing pagination or lazy loading of schemas.
| config.url = newUrl; | ||
| // Add headers to config if implemented | ||
| } else { // stdio | ||
| if (!newCommand) { |
There was a problem hiding this comment.
Security Improvement: When handling user input for stdio transport commands, consider implementing additional validation beyond just checking if the command is non-empty. For example, you might want to validate against a whitelist of allowed commands.
|
|
||
| export async function fetchMcpServers() { | ||
| const session = await auth(); | ||
| if (!session?.user?.id) { |
There was a problem hiding this comment.
Code Duplication: There's repeated error handling logic for unauthorized users across multiple functions. Consider extracting this into a reusable middleware or helper function to reduce duplication.
| } | ||
| } | ||
|
|
||
| export async function addMcpServerAction({ |
There was a problem hiding this comment.
Error Handling Inconsistency: Some functions throw errors directly while others return error objects with a success: false property. Consider standardizing the error handling approach across all functions for better maintainability.
| <p className="text-sm text-muted-foreground italic"> | ||
| {server.isEnabled | ||
| ? "No tools available or unable to connect to server" | ||
| : "Enable the server to view available tools"} |
There was a problem hiding this comment.
Accessibility Improvement: The switch component for toggling server status lacks proper ARIA attributes. Consider enhancing it with appropriate ARIA roles and states to improve accessibility.
| type: 'stdio'; | ||
| command: string; | ||
| args?: string[]; | ||
| }; |
There was a problem hiding this comment.
Potential Memory Leak: The MCP clients are stored in a state variable but there's no cleanup in the useEffect return function. Consider adding a cleanup function that closes all active MCP clients when components unmount.
| throw new Error('Missing required fields: name and config.'); | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
Error Handling: The addMcpServerAction function returns a success object with an error message on failure, but doesn't include the original error details which could be useful for debugging. Consider adding more detailed error information.
|
hello |
|
hello again |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: MCP Client Integration👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| const client = await experimental_createMCPClient(clientOptions); | ||
|
|
There was a problem hiding this comment.
Missing error handling in MCP client creation
The MCP client creation lacks error handling, which could cause unhandled promise rejections if connection fails. This would result in an uncaught exception that terminates the schema loading process without proper cleanup.
| const client = await experimental_createMCPClient(clientOptions); | |
| 165: + try { | |
| 166: + const client = await experimental_createMCPClient(clientOptions); | |
| 167: + |
Standards
- ISO-IEC-25010-Reliability-FaultTolerance
- SRE-ErrorHandling
| } finally { | ||
| // Always close the client | ||
| await client.close(); | ||
| } |
There was a problem hiding this comment.
Unclosed MCP clients causing resource leaks
If an exception occurs during tools() method call, the client.close() won't be executed. This may lead to resource leaks as MCP clients could remain open, potentially exhausting system resources over time.
| } | |
| 182: + } finally { | |
| 183: + await client.close(); | |
| 184: + } |
Standards
- ISO-IEC-25010-Reliability-ResourceUtilization
- SRE-ResourceManagement
| for (const client of mcpClientsToClose) { | ||
| try { | ||
| await client.close(); |
There was a problem hiding this comment.
Missing null check in MCP client cleanup
The code logs the number of MCP clients to close but doesn't handle the case when mcpClientsToClose is undefined. This could cause a runtime error if the variable is not properly initialized, leading to failure in cleanup process.
| for (const client of mcpClientsToClose) { | |
| try { | |
| await client.close(); | |
| 192: + console.log(`Closing ${mcpClientsToClose?.length || 0} MCP clients in onFinish...`); | |
| 193: + for (const client of mcpClientsToClose || []) { | |
| 194: + try { |
Standards
- ISO-IEC-25010-Reliability-FaultTolerance
- SRE-DefensiveProgramming
This is a super early (+ very rough) implementation of MCPs for the
ai-chatbottemplate. I've only had 30 minutes and tried to see how far I could get using the new Gemini 2.5 and Sonnet 3.7 models, hence, here is the result. This resolves the following issue: vercel#575The PR adds the following:
stdioif running locally orsse(we might want to disablestdioaltogether for a deployed version)Here are some screenshots of how it looks like at the moment:
Chat page
Server logs showing output of MPC call
Settings page
Adding new MCPs
Remaining tasks
Hoping to find some time to hand-craft this properly over the weekend, but raising this PR for early feedback and for anyone who would like to help contribute to this!
Cheers
References