Support MCP( WIP) #5974
Support MCP( WIP) #5974
Conversation
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
WalkthroughThis pull request introduces a comprehensive implementation of the Multi-Channel Protocol (MCP) system for NextChat. The changes span multiple files, adding new functionality for managing multiple AI clients, server configurations, and a dedicated market page for MCP servers. The implementation includes client management, logging, configuration handling, and a new user interface component for exploring and configuring MCP servers. Changes
Sequence DiagramsequenceDiagram
participant User
participant McpMarketPage
participant McpActions
participant McpClient
User->>McpMarketPage: Open MCP Market
McpMarketPage->>McpActions: Get MCP Config
McpActions-->>McpMarketPage: Return Config
User->>McpMarketPage: Add New Server
McpMarketPage->>McpActions: Add MCP Server
McpActions->>McpClient: Initialize Client
McpClient-->>McpActions: Client Created
McpActions-->>McpMarketPage: Server Added
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Your build has completed! |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
app/store/chat.ts (1)
429-447: Validate JSON parsing & protect against malformed message content
- The regular expression approach to identify ```json:mcp blocks is functional, but consider graceful fallbacks if the block is missing or incomplete.
- You already catch JSON parsing errors, which is good. In case of invalid JSON from user-provided data, ensuring a user-facing error response could be beneficial.
- If multiple MCP calls can happen concurrently, verify the system’s concurrency handling to avoid race conditions.
app/mcp/actions.ts (1)
1-33: Leverage stronger typing and ensure concurrency safety
- Currently,
fsClientis typed asany—consider adding an explicit interface or type to make maintenance easier and reduce accidental misuse.- When multiple calls happen in quick succession, ensure
initFileSystemClientcan safely handle simultaneous initialization requests.app/mcp/logger.ts (1)
1-60: Ensure cross-platform log compatibility
- ANSI color codes might not display as intended across all terminals (e.g., some CI/CD environments or Windows shells).
- Consider adding timestamps or structured logging for easier troubleshooting and correlation with other logs.
app/mcp/client.ts (4)
1-5: Use absolute imports selectively.Imports from the "@modelcontextprotocol" package and Zod library are fine. However, ensure that these external imports remain minimal for faster builds and better maintainability. If future code only requires submodules, consider selective imports from large packages (e.g.,
import { ... } from "@modelcontextprotocol/sdk/client").
6-10: Clarify optional fields inServerConfiginterface.Currently,
argsandenvare optional. Provide doc comments describing the scenarios in which these fields would be omitted to improve code clarity.
44-47: ValidatePrimitivestructure and values.As
Primitive.valueis typed toany, adding a Zod schema or another validation layer for resource, tool, or prompt objects can improve type safety and help catch unexpected data structures from the server.
83-87: Use structured logging for request execution.Currently, you only do
console.log(r)after executing the request. Consider usinglogger.infoor a more structured approach to log request/response pairs for debugging and auditing.... const r = client.request(request, z.any()); - console.log(r); + logger.info("Request response:", r); return r; }tsconfig.json (1)
26-26: Cleanup references to removed files ininclude.Removing
"app/calcTextareaHeight.ts"from theincludearray is fine. Ensure that all references (e.g., imports) to that file are also removed throughout the codebase to avoid confusion.package.json (1)
25-25: Leverage the@modelcontextprotocol/sdkdependency effectively.Ensure that all relevant modules from the new SDK are utilized. If only a small subset is needed, consider partial imports or code splitting to reduce bundle size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
app/mcp/actions.ts(1 hunks)app/mcp/client.ts(1 hunks)app/mcp/example.ts(1 hunks)app/mcp/logger.ts(1 hunks)app/mcp/mcp_config.ts(1 hunks)app/store/chat.ts(2 hunks)next.config.mjs(3 hunks)package.json(2 hunks)tsconfig.json(2 hunks)
🔇 Additional comments (8)
app/store/chat.ts (1)
32-32: Add test coverage for 'executeMcpAction' usage
The new import integrates MCP action processing into the chat flow. To reduce the risk of unhandled edge cases (e.g., unavailable server, invalid JSON structures, etc.), please ensure the code path invoking executeMcpAction is tested thoroughly.
app/mcp/mcp_config.ts (1)
1-40: Secure storage of credentials and externalize paths
- The placeholders for API keys and tokens should never be replaced by real secrets in the repository; instead, reading from environment variables or a vault is recommended.
- The path
"/Users/kadxy/Desktop"is hard-coded. Consider making this path configurable for different environments.
app/mcp/example.ts (1)
1-92: Harden the file reading mechanism
- The code showcases reading arbitrary files from a specified path. Confirm that server-side controls or directory whitelisting is in place to guard against unauthorized access.
- Good job using Zod schemas for validation—this helps ensure the integrity of server responses.
app/mcp/client.ts (1)
49-81: Parallel asynchronous operations are well handled; keep an eye on performance.
The approach of collecting promises in an array and awaiting them simultaneously is a good practice. For large-scale data, ensure the server handles concurrent calls effectively to avoid resource contention or throttling.
next.config.mjs (2)
35-35: Ensure experimental features are stable enough for production.
serverActions: true is an experimental feature. Confirm that your production environment accepts potential breaking changes or instabilities. Consider adding a feature gate or fallback logic.
75-78: Maintain consistent pattern for rewrite placeholders.
Rewrite placeholders, such as :resource_name and :deploy_name, are used in the destination URL. Confirm that you handle all edge cases (like missing or invalid resource_name / deploy_name). Potentially add server-side validation to avoid runtime errors.
tsconfig.json (1)
3-3: Check compatibility of ES2022 target.
Confirm that your deployment environment supports ES2022 features, or use a transpilation step if targeting older browsers. Some hosting platforms may not yet fully support ES2022 natively.
package.json (1)
57-58: Check versions & synergy for zod and zustand.
zod is now widely used for schema validation. Keep an eye on potential conflicts when using zustand for state management if any store-based validations are needed.
app/mcp/client.ts
Outdated
| export async function createClient( | ||
| serverConfig: ServerConfig, | ||
| name: string, | ||
| ): Promise<Client> { | ||
| logger.info(`Creating client for server ${name}`); | ||
|
|
||
| const transport = new StdioClientTransport({ | ||
| command: serverConfig.command, | ||
| args: serverConfig.args, | ||
| env: serverConfig.env, | ||
| }); | ||
| const client = new Client( | ||
| { | ||
| name: `nextchat-mcp-client-${name}`, | ||
| version: "1.0.0", | ||
| }, | ||
| { | ||
| capabilities: { | ||
| roots: { | ||
| // listChanged indicates whether the client will emit notifications when the list of roots changes. | ||
| // listChanged 指示客户端在根列表更改时是否发出通知。 | ||
| listChanged: true, | ||
| }, | ||
| }, | ||
| }, | ||
| ); | ||
| await client.connect(transport); | ||
| return client; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider robust error handling for client creation.
When creating the client and connecting to the transport (line 40), include error handling for potential failures (e.g., invalid command, environment issues). This ensures unexpected errors are caught gracefully.
...
await client.connect(transport);
+ // TODO: Consider wrapping this in try/catch:
+ // try {
+ // await client.connect(transport);
+ // } catch (err) {
+ // logger.error('Failed to connect to transport', err);
+ // throw err;
+ // }
return client;
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -49,11 +50,12 @@ | |||
| "remark-breaks": "^3.0.2", | |||
| "remark-gfm": "^3.0.1", | |||
| "remark-math": "^5.1.1", | |||
| "rt-client": "https://github.com/Azure-Samples/aoai-realtime-audio-sdk/releases/download/js/v0.5.0/rt-client-0.5.0.tgz", | |||
There was a problem hiding this comment.
💡 Codebase verification
Warning: The rt-client dependency is actively used and cannot be removed
The rt-client package is an essential dependency that is actively used in the realtime chat functionality:
- It's imported and used extensively in
app/components/realtime-chat/realtime-chat.tsx - Core functionalities depend on it:
- Real-time audio streaming and processing
- Voice chat features
- Server communication for real-time responses
- Audio transcription and playback
Removing this dependency would break the real-time chat feature. The package should be kept unless there's a planned replacement or the real-time chat functionality is being removed.
🔗 Analysis chain
Confirm removal of the deprecated rt-client dependency.
Verify that no modules still rely on rt-client. Skipping this verification might lead to runtime errors if references remain.
Let me gather more information about the actual usage of rt-client in the code.
Let me check the actual usage in the realtime-chat.tsx file to understand if this is an active dependency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to rt-client in the codebase:
rg 'rt-client' -A 3 -B 3
Length of output: 2671
Script:
#!/bin/bash
# Check for imports and actual usage of rt-client in the realtime components
ast-grep --pattern 'import { $$$ } from "rt-client"'
# Check for any direct usage of rt-client package
rg -l 'new RTClient|RTClient\.'
Length of output: 675
Script:
#!/bin/bash
# Check the implementation in realtime-chat.tsx
cat app/components/realtime-chat/realtime-chat.tsx
Length of output: 10998
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
app/mcp/actions.ts (3)
7-17: Global variables and maps
Storing clients in a globalMapand a globalinitializedflag may be problematic in multi-instance or serverless environments. Consider a more robust design (e.g., an initialization manager) if concurrency or horizontal scaling are required.
18-51: initializeMcpClients function
- Code loops over
conf.mcpServersand initializes each client with proper logging.- Catches errors and tracks failed clients in
errorClients.- Marks
initialized = trueto prevent reinitialization.- Logs final status and available clients.
This flow is user-friendly and understandable. Consider adding retries or fallback logic for partial initialization failures.
53-70: executeMcpAction function
- Fetches the client from
clientsMapby ID and executes the request.- Logs any execution errors and rethrows them.
Consider returning a standardized error object for better client-side handling, especially if this function may be extended for user-facing error messages in the future.app/store/chat.ts (2)
363-367: MCP check on new messages
Line 366 callscheckMcpJsonwhenever a new message arrives. This integration ensures automatic detection of MCP requests without additional code in the message pipeline. Be sure to handle potential performance impacts if message volume is very high.
773-798: checkMcpJson function
- Validates if message content matches MCP JSON format.
- On success, extracts the MCP payload and invokes
executeMcpAction.- Uses
onUserInputto feed back results as a new message.
This is a clean, modular approach. However, watch for large JSON payloads or malicious content. Consider adding rate-limits or size checks for production.app/mcp/utils.ts (2)
1-3: isMcpJson function
Returns the match result of a regex. This function is concise but slightly at risk of returning null or array. Consider returning a boolean explicitly (!!content.match(...)) if you only need a truthy check.
5-11: extractMcpJson function
CapturesclientIdand parses JSON from the second capture group. The approach is straightforward. A try-catch block for JSON parsing (or an upfront validity check) might guard against syntax errors or maliciously malformed JSON.app/page.tsx (1)
9-10: Await MCP initialization
Blocking initialization ensures MCP dependencies are ready before UI rendering. Ensure that this call won’t degrade page responsiveness for users if the initialization is slow. Consider deferring or parallelizing if needed.app/mcp/logger.ts (1)
24-65: Logging methods
- Provides functions for info, success, error, warn, and debug with consistent output formatting.
debugis controlled by a flag to prevent noisy logs.- A structured
formatMessagemethod safely handles objects.
Well-structured, though consider a fallback or custom transport in production to integrate with external logging systems (e.g., Winston, Bunyan).app/mcp/client.ts (3)
32-35: Document or remove commented capabilities configuration.The commented capabilities configuration lacks explanation. Either document why it's preserved for future use or remove it if unnecessary.
28-28: Consider extracting version to a configuration constant.The hardcoded version "1.0.0" should be moved to a configuration constant for better maintainability.
+const CLIENT_VERSION = "1.0.0"; export async function createClient( serverConfig: ServerConfig, name: string, ): Promise<Client> { // ... const client = new Client( { name: `nextchat-mcp-client-${name}`, - version: "1.0.0", + version: CLIENT_VERSION, }, // ... );
52-76: Refactor duplicate promise handling pattern.The promise handling pattern is repeated for resources, tools, and prompts. Consider extracting this to a helper function.
+const createPrimitivePromise = ( + client: Client, + type: Primitive["type"], + listFn: () => Promise<{ [key: string]: any[] }> +) => { + return listFn().then((result) => { + const items = result[`${type}s`]; + items.forEach((item) => primitives.push({ type, value: item })); + }); +}; if (capabilities?.resources) { - promises.push( - client.listResources().then(({ resources }) => { - resources.forEach((item) => - primitives.push({ type: "resource", value: item }), - ); - }), - ); + promises.push(createPrimitivePromise(client, "resource", () => client.listResources())); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
.eslintignore(1 hunks)app/mcp/actions.ts(1 hunks)app/mcp/client.ts(1 hunks)app/mcp/example.ts(1 hunks)app/mcp/logger.ts(1 hunks)app/mcp/mcp_config.json(1 hunks)app/mcp/utils.ts(1 hunks)app/page.tsx(1 hunks)app/store/chat.ts(4 hunks)package.json(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- .eslintignore
- app/mcp/mcp_config.json
🚧 Files skipped from review as they are similar to previous changes (2)
- app/mcp/example.ts
- package.json
🔇 Additional comments (10)
app/mcp/actions.ts (3)
1-2: Use of "use server" directive
These lines establish a server-only execution context. Be mindful of referencing browser-specific APIs in server files to avoid runtime errors.
3-6: Import dependencies
Imports for createClient, executeRequest, MCPClientLogger, and the JSON config look consistent with the intended usage. Ensure that underlying modules handle errors gracefully.
72-77: getAvailableClients function
Filters out clients that failed to initialize. This is straightforward and aligns with the error management strategy. Ensure future expansions remain in sync (e.g., re-checking previously error-prone clients).
app/store/chat.ts (2)
32-33: New MCP imports
Imports from mcp/actions and mcp/utils introduce the MCP functionality into the chat store. This is a logical location given the store’s role in handling messages.
434-434: Asynchronous onFinish callback
This line suggests the onFinish callback is asynchronous. Confirm that all references to onFinish handle promises and potential errors correctly to avoid unhandled promise rejections.
app/page.tsx (1)
4-4: Import initializeMcpClients
This sets up a new dependency on actions.ts. Verify that all required environment variables and config references are available at runtime.
app/mcp/logger.ts (2)
1-10: Color codes definition
Defines ANSI codes for styling the log output. Straightforward and correct for Node.js environments.
12-22: Logger constructor and fields
The prefix and debugMode options provide flexible logging. This is a neat approach for customizing logs.
app/mcp/client.ts (2)
1-10: LGTM! Well-structured imports and interface definition.
The ServerConfig interface is well-designed with appropriate optional parameters for args and env configuration.
14-40: Add error handling for client creation and connection.
The client creation process should include proper error handling for transport creation and connection failures.
app/mcp/client.ts
Outdated
|
|
||
| interface Primitive { | ||
| type: "resource" | "tool" | "prompt"; | ||
| value: any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'any' type with a more specific type.
Using 'any' type reduces type safety. Consider defining specific types for resource, tool, and prompt values.
app/mcp/client.ts
Outdated
| /** Execute a request */ | ||
| export async function executeRequest(client: Client, request: any) { | ||
| return client.request(request, z.any()); | ||
| } |
There was a problem hiding this comment.
Improve request validation and error handling.
The current implementation has several concerns:
- Using
z.any()bypasses the benefits of Zod's type validation - No request validation before execution
- No error handling for failed requests
Consider implementing proper validation and error handling:
-export async function executeRequest(client: Client, request: any) {
- return client.request(request, z.any());
+export async function executeRequest(client: Client, request: unknown) {
+ const requestSchema = z.object({
+ // Define your request schema here
+ // Example:
+ // method: z.string(),
+ // params: z.record(z.unknown())
+ });
+
+ try {
+ const validatedRequest = requestSchema.parse(request);
+ return await client.request(validatedRequest, z.unknown());
+ } catch (error) {
+ if (error instanceof z.ZodError) {
+ throw new Error(`Invalid request format: ${error.message}`);
+ }
+ throw error;
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/store/chat.ts (2)
Line range hint
376-402: Enhance type safety in onUserInput function.The function could benefit from improved type safety:
- The
isMcpResponseparameter should have a default value since it's optional- The content type assertion should be more explicit
Consider applying these changes:
async onUserInput( content: string, attachImages?: string[], - isMcpResponse?: boolean, + isMcpResponse: boolean = false, ) { const session = get().currentSession(); const modelConfig = session.mask.modelConfig; - let mContent: string | MultimodalContent[] = isMcpResponse + let mContent: string | MultimodalContent[] = isMcpResponse === true ? content : fillTemplateWith(content, modelConfig);
782-810: Enhance error handling and maintainability in checkMcpJson.Consider the following improvements:
- Add specific error type handling
- Add debug logging for the response formatting
- Extract JSON response formatting to a separate function
Consider applying these changes:
checkMcpJson(message: ChatMessage) { const content = getMessageTextContent(message); if (isMcpJson(content)) { try { const mcpRequest = extractMcpJson(content); if (mcpRequest) { console.debug("[MCP Request]", mcpRequest); executeMcpAction(mcpRequest.clientId, mcpRequest.mcp) .then((result) => { console.log("[MCP Response]", result); - const mcpResponse = - typeof result === "object" - ? JSON.stringify(result) - : String(result); + const mcpResponse = formatMcpResponse(result); + console.debug("[MCP Formatted Response]", mcpResponse); get().onUserInput( `\`\`\`json:mcp:${mcpRequest.clientId}\n${mcpResponse}\n\`\`\``, [], true, ); }) - .catch((error) => showToast(String(error))); + .catch((error) => { + console.error("[MCP Execution Error]", error); + showToast(error instanceof Error ? error.message : String(error)); + }); } - } catch (error) { + } catch (error: unknown) { console.error("[MCP Error]", error); + showToast("Failed to process MCP request"); } } }, + function formatMcpResponse(result: unknown): string { + return typeof result === "object" ? JSON.stringify(result) : String(result); + }app/mcp/types.ts (2)
1-2: Optional: Confirm the specification link remains stable.The comment references an external spec URL. If it’s crucial, ensure the link remains valid. If the spec is versioned, consider pinning a specific version for documentation stability.
21-32: Response interface structure is flexible and aligns well with standard JSON-RPC patterns.The
errorstructure is appropriately typed, ensuring systematic error reporting. Consider adding clearer distinction for success vs. error states if your logic requires it.app/mcp/client.ts (2)
43-46: Primitive interface uses 'any'.Using
anytype forvaluereduces type safety. Define a union or more explicit shape if the data is known.-interface Primitive { - type: "resource" | "tool" | "prompt"; - value: any; -} +interface Resource { /* shape */} +interface Tool { /* shape */} +interface Prompt { /* shape */} +type PrimitiveValue = Resource | Tool | Prompt; + +interface Primitive { + type: "resource" | "tool" | "prompt"; + value: PrimitiveValue; +}
48-80: listPrimitives merges capability checks with asynchronous calls.
- The approach is valid and merges results from multiple promises.
- Consider an early break if no capabilities exist to optimize performance in some edge cases.
- See if any concurrency issues might arise if the server is large or if many resources are loaded at once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/mcp/actions.ts(1 hunks)app/mcp/client.ts(1 hunks)app/mcp/types.ts(1 hunks)app/store/chat.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/mcp/actions.ts
🔇 Additional comments (14)
app/store/chat.ts (4)
Line range hint 1-37: LGTM! Import changes are well-organized.
The new imports for MCP functionality are properly structured and necessary for the implementation.
62-62: LGTM! Type extension is clean and minimal.
The addition of isMcpResponse to ChatMessage type is well-typed and follows TypeScript best practices.
368-372: LGTM! Clean integration of MCP checking.
The addition of checkMcpJson call is well-placed and maintains the existing message processing flow.
Line range hint 1-810: Verify integration with existing chat functionality.
Since this is a WIP PR adding MCP support, please ensure:
- The changes don't affect existing chat functionality
- Error states are properly propagated to the UI
- MCP responses don't break the chat history display
✅ Verification successful
Based on the verification results, I can now provide a final response:
MCP integration maintains existing chat functionality with proper error handling
The code review shows that the MCP (Message Control Protocol) integration:
-
Properly maintains existing chat functionality:
- Core message handling (
onNewMessage,onUserInput) remains intact - Message rendering and display components are unaffected
- Existing error states and UI feedback are preserved
- Core message handling (
-
Has appropriate error handling:
- MCP errors are caught and displayed via
showToast - Failed MCP actions are properly logged:
console.error("[MCP Error]", error) - Error states are propagated to the UI through existing message error flags
- MCP errors are caught and displayed via
-
Safely integrates with chat history:
- MCP responses are clearly marked with
isMcpResponseflag - Messages are properly added to chat history with correct formatting
- Template handling is properly skipped for MCP responses
- MCP responses are clearly marked with
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential regressions in chat functionality
echo "Checking for chat message handling..."
rg -A 5 "onUserInput|onNewMessage" --type ts
echo "Checking for error handling patterns..."
rg "showToast|console\.error" --type ts
echo "Checking for message display components..."
rg -A 5 "ChatMessage|renderMessage" --type tsx
Length of output: 10897
Script:
#!/bin/bash
# Let's check for message display components with proper extension
echo "Checking for message display components..."
rg -A 5 "ChatMessage|renderMessage" --type ts --type tsx
# Check for MCP-related changes
echo "Checking for MCP-related code..."
rg -A 5 "mcp|MCP" --type ts --type tsx
# Check for error handling in chat store
echo "Checking error handling in chat store..."
ast-grep --pattern 'onError(error) {
$$$
}'
Length of output: 464
Script:
#!/bin/bash
# Let's try with just ts extension first
echo "Checking for message display components..."
rg -A 5 "ChatMessage|renderMessage" ./*.ts ./app/**/*.ts
echo "Checking for MCP-related code..."
rg -A 5 "mcp|MCP" ./*.ts ./app/**/*.ts
echo "Checking error handling in chat store..."
rg -A 5 "onError.*{" ./app/store/chat.ts
# Check for message rendering components
echo "Checking message components..."
fd -e tsx . -x cat {} | rg -A 5 "ChatMessage|renderMessage"
Length of output: 23199
app/mcp/types.ts (5)
5-12: Interfaces with typed methods and flexible params are well structured.
The McpRequestMessage interface design uses a union for the id and retains flexibility in params. This is good for requests where the schema can evolve over time.
14-20: Schema validation ensures robust request handling.
Good use of Zod for optional jsonrpc and union types for id. This schema can mitigate malformed requests by typechecking at runtime.
34-47: Robust schema for JSON-RPC responses.
The optional error object’s shape is well-defined. Ensure the client code handles partial responses (e.g., result or error fields) gracefully.
49-55: Encapsulated notifications match JSON-RPC patterns.
Separating notifications into a distinct interface clarifies the difference between requests and notifications.
57-61: Notifications schema aligns with the interface.
The use of z.record(z.unknown()) for params preserves flexibility while still enforcing structural constraints.
app/mcp/client.ts (5)
1-6: Imports are consistent and descriptive.
All necessary dependencies (Client, StdioClientTransport, custom logger, and McpRequestMessage) are imported cleanly. The use of zod is appropriate for runtime type checks.
7-11: ServerConfig is concise and extendable.
Storing command, arguments, and environment variables in an interface fosters clarity. If future expansions are likely (e.g., working directories, timeouts), consider grouping them under optional sub-objects or adding additional properties.
13-14: Logger usage is straightforward.
A dedicated logger is beneficial for debugging and operational visibility. Ensure logging levels are adjustable in production environments if needed.
15-41: createClient handles basic client setup.
- Logging the server name is helpful for troubleshooting.
- Consider error handling around
client.connectin case the process fails to start or times out.
Would you like a script to search for unhandled promise rejections around connect calls and propose structured error-handling code if discovered?
82-88: 🛠️ Refactor suggestion
executeRequest returns data without intermediate validation or error handling.
Even though z.any() is used, you might want stronger type checks if the response format is critical. Also, consider how errors from client.request are caught and propagated.
export async function executeRequest(
client: Client,
request: McpRequestMessage,
) {
- return client.request(request, z.any());
+ try {
+ const response = await client.request(request, z.unknown());
+ // Potentially parse the response with the McpResponseMessageSchema
+ return McpResponseMessageSchema.parse(response);
+ } catch (err) {
+ logger.error('Failed to execute MCP request', err);
+ throw err;
+ }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/mcp/actions.ts (1)
66-86: Enhance error handling with specific error types.The error handling could be more specific to help with debugging and user feedback.
export async function executeMcpAction( clientId: string, request: McpRequestMessage, ) { try { const client = clientsMap.get(clientId)?.client; if (!client) { - logger.error(`Client ${clientId} not found`); - return; + throw new Error(`MCP client '${clientId}' not found`); } logger.info(`Executing MCP request for ${clientId}`); return await executeRequest(client, request); } catch (error) { - logger.error(`MCP execution error: ${error}`); - throw error; + const errorMessage = error instanceof Error ? error.message : String(error); + logger.error(`MCP execution error for client '${clientId}':`, errorMessage); + throw new Error(`Failed to execute MCP action: ${errorMessage}`); } }app/store/chat.ts (1)
813-840: Enhance error handling in MCP JSON processing.The error handling could be more specific and provide better user feedback.
checkMcpJson(message: ChatMessage) { const content = getMessageTextContent(message); if (isMcpJson(content)) { try { const mcpRequest = extractMcpJson(content); if (mcpRequest) { console.debug("[MCP Request]", mcpRequest); executeMcpAction(mcpRequest.clientId, mcpRequest.mcp) .then((result) => { console.log("[MCP Response]", result); const mcpResponse = typeof result === "object" ? JSON.stringify(result) : String(result); get().onUserInput( `\`\`\`json:mcp-response:${mcpRequest.clientId}\n${mcpResponse}\n\`\`\``, [], true, ); }) - .catch((error) => showToast("MCP execution failed", error)); + .catch((error) => { + const errorMessage = error instanceof Error ? error.message : String(error); + showToast( + "MCP Execution Failed", + `Failed to execute MCP action for client '${mcpRequest.clientId}': ${errorMessage}` + ); + console.error("[MCP Execution Error]", error); + }); } } catch (error) { - console.error("[MCP Error]", error); + const errorMessage = error instanceof Error ? error.message : String(error); + console.error("[MCP Processing Error]", error); + showToast( + "MCP Processing Failed", + `Failed to process MCP message: ${errorMessage}` + ); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/chat.tsx(11 hunks)app/constant.ts(1 hunks)app/mcp/actions.ts(1 hunks)app/mcp/client.ts(1 hunks)app/mcp/example.ts(1 hunks)app/store/chat.ts(11 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/components/chat.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/mcp/example.ts
🔇 Additional comments (7)
app/constant.ts (2)
256-261: LGTM! Well-structured template for MCP primitives.The template clearly separates clientId and primitives sections with good formatting.
263-365: LGTM! Comprehensive system template with clear documentation.The template provides:
- Clear sections for tools, usage rules, and interaction flow
- Well-documented examples
- Proper markdown formatting for code blocks
app/mcp/client.ts (3)
43-46: 🛠️ Refactor suggestionReplace 'any' type with a more specific type.
The
valuefield uses theanytype which reduces type safety. Consider defining a proper type or interface.export interface Primitive { type: "resource" | "tool" | "prompt"; - value: any; + value: { + name: string; + description?: string; + parameters?: Record<string, unknown>; + }; }Likely invalid or redundant comment.
82-88: 🛠️ Refactor suggestionImprove request validation.
The function uses
z.any()which bypasses Zod's type validation benefits. Consider implementing proper validation schema.export async function executeRequest( client: Client, request: McpRequestMessage, ) { - return client.request(request, z.any()); + const responseSchema = z.object({ + // Define expected response schema + result: z.unknown(), + error: z.string().optional(), + }); + return client.request(request, responseSchema); }Likely invalid or redundant comment.
15-41:⚠️ Potential issueAdd error handling for client connection.
The client connection lacks error handling which could lead to unhandled promise rejections.
export async function createClient( serverConfig: ServerConfig, name: string, ): Promise<Client> { logger.info(`Creating client for server ${name}`); const transport = new StdioClientTransport({ command: serverConfig.command, args: serverConfig.args, env: serverConfig.env, }); const client = new Client( { name: `nextchat-mcp-client-${name}`, version: "1.0.0", }, { capabilities: { // roots: { // listChanged: true, // }, }, }, ); - await client.connect(transport); + try { + await client.connect(transport); + } catch (error) { + logger.error(`Failed to connect client ${name}:`, error); + throw new Error(`Failed to initialize MCP client ${name}: ${error.message}`); + } return client; }Likely invalid or redundant comment.
app/mcp/actions.ts (1)
28-63: LGTM! Well-implemented initialization with proper error handling.The function includes:
- Singleton pattern to prevent multiple initializations
- Comprehensive error handling
- Good logging of success and failure cases
app/store/chat.ts (1)
201-219: LGTM! Well-implemented system prompt generation.The function effectively:
- Filters relevant primitives
- Handles template replacement
- Maintains clear structure
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (14)
app/constant.ts (1)
264-366: Consider enhancing the system template documentation.The template is well-structured with comprehensive documentation. Consider these improvements:
- Add version information to track template changes
- Use more realistic paths in examples (e.g.,
~/DesktoporC:\Users\username\Desktop)export const MCP_SYSTEM_TEMPLATE = ` +// Template Version: 1.0.0 You are an AI assistant with access to system tools... 5. EXAMPLE INTERACTION: User: "What files do I have on my desktop?" Assistant: "I'll check which directories I have access to. \`\`\`json:mcp:filesystem { "method": "tools/call", "params": { "name": "list_allowed_directories", "arguments": {} } } \`\`\`" User: "\`\`\`json:mcp-response:filesystem { - "directories": ["/path/to/desktop"] + "directories": ["~/Desktop"] } \`\`\`"app/mcp/actions.ts (3)
52-68: ConsolidatereinitializeMcpClientsandrestartAllClientsFunctionsThe functions
reinitializeMcpClients(lines 52-68) andrestartAllClients(lines 179-194) perform similar actions: closing clients, clearing states, and reinitializing MCP clients. To avoid code duplication and improve maintainability, consider refactoringrestartAllClientsto callreinitializeMcpClients.Apply this refactor:
export async function restartAllClients() { logger.info("Restarting all MCP clients..."); - // 清空状态 - clientsMap.clear(); - errorClients = []; - initialized = false; - - // 重新初始化 - await initializeMcpClients(); + // Delegate to reinitializeMcpClients + await reinitializeMcpClients(); return { success: errorClients.length === 0, errorClients, }; }
123-127: Improve Error Handling When Client Is Not FoundIn the
executeMcpActionfunction (lines 123-127), when a client is not found, the function logs an error but does not throw an exception or provide feedback to the caller. This could lead to silent failures. Consider throwing an error to ensure that the calling function can handle this scenario appropriately.Apply this change:
const client = clientsMap.get(clientId)?.client; if (!client) { logger.error(`Client ${clientId} not found`); - return; + throw new Error(`Client ${clientId} not found`); }
197-216: CombinegetAllClientStatusandgetClientErrorsFunctionsThe functions
getAllClientStatus(lines 197-205) andgetClientErrors(lines 208-216) both return a record of client IDs to their error messages. To reduce redundancy, consider combining these functions into one or clarifying their distinct purposes if they are intended to provide different information.app/components/mcp-market.tsx (2)
185-250: RefactorrenderConfigFormto Improve ReadabilityThe
renderConfigFormfunction (lines 181-250) handles multiple input types and contains nested loops and conditional logic, making it complex. Consider refactoring by extracting input field rendering into separate components to enhance readability and maintainability.
436-438: Handle Long Descriptions in Server InfoIn the server info section (lines 436-438), long descriptions may overflow or break the layout. Consider adding CSS styles to handle text overflow, such as ellipsis, or provide tooltips for full descriptions.
app/components/home.tsx (1)
77-83: Maintain Consistent Import OrderThe import order of
useEffectanduseState(line 5) deviates from the convention used in the rest of the file, where hooks are typically imported after other modules. Consider reorganizing the imports for consistency.app/mcp/types.ts (2)
5-61: LGTM! Well-structured message types with proper validation.The MCP message interfaces and schemas are well-defined and properly validated using Zod. The implementation follows the JSON-RPC 2.0 specification.
Consider adding:
- JSDoc comments for better IDE integration
- Examples of valid message structures
- Constants for common error codes
Example:
/** MCP request message following JSON-RPC 2.0 specification */ export interface McpRequestMessage { /** JSON-RPC version. Must be "2.0" when present */ jsonrpc?: "2.0"; // ... rest of the interface } /** Common MCP error codes */ export const McpErrorCodes = { PARSE_ERROR: -32700, INVALID_REQUEST: -32600, METHOD_NOT_FOUND: -32601, // ... add more error codes } as const;
63-99: LGTM! Well-designed server configuration types.The server configuration interfaces provide good type safety and flexibility for different server configurations.
Consider adding:
- Type guards for runtime validation
- Utility functions for configuration validation
- Constants for common server types
Example:
export const SERVER_TYPES = { FILESYSTEM: 'filesystem', DOCKER: 'docker-mcp', POSTGRES: 'postgres', // ... add more server types } as const; export type ServerType = typeof SERVER_TYPES[keyof typeof SERVER_TYPES]; export function isValidServerConfig(config: unknown): config is ServerConfig { // Add runtime validation logic }app/mcp/preset-server.json (2)
74-78: Enhance database connection securityBoth MongoDB and PostgreSQL configurations use plain connection strings. Consider:
- Supporting connection options (SSL, timeout, etc.)
- Implementing credential management
- Adding connection pooling configuration
Also applies to: 138-142
161-165: Add API configuration safeguardsFor both Brave Search and Google Maps APIs, consider adding:
- Rate limiting configuration
- Error handling guidance
- API key rotation support
Also applies to: 184-188
app/components/mcp-market.module.scss (3)
86-95: Extract status colors to CSS variablesReplace hardcoded colors with CSS variables for consistency:
- background-color: #10b981; + background-color: var(--status-success); - background-color: #ef4444; + background-color: var(--status-error); - background-color: #f59e0b; + background-color: var(--status-warning);
188-389: Reduce style duplication with mixinsConsider extracting common input styles into SCSS mixins:
- Form input styles
- Button hover/focus states
- Common transitions
574-612: Move global styles to a separate fileGlobal styles should be moved to a dedicated global stylesheet to:
- Prevent scope conflicts
- Improve maintainability
- Better separate concerns
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/icons/mcp.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
app/components/home.tsx(4 hunks)app/components/mcp-market.module.scss(1 hunks)app/components/mcp-market.tsx(1 hunks)app/components/sidebar.tsx(2 hunks)app/constant.ts(2 hunks)app/locales/cn.ts(1 hunks)app/mcp/actions.ts(1 hunks)app/mcp/mcp_config.json(1 hunks)app/mcp/preset-server.json(1 hunks)app/mcp/types.ts(1 hunks)app/mcp/utils.ts(1 hunks)app/store/chat.ts(11 hunks)next.config.mjs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- next.config.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- app/mcp/utils.ts
- app/store/chat.ts
🔇 Additional comments (11)
app/constant.ts (3)
50-50: LGTM! Path addition follows conventions.The new
McpMarketpath is well-structured and consistent with existing path definitions.
257-262: LGTM! Clean and focused template structure.The template follows the existing patterns and provides a clear structure for clientId and primitives interpolation.
257-367: LGTM! Well-integrated with existing constants.The new MCP-related constants are properly placed within the file's organization, maintaining logical grouping and following existing patterns.
app/mcp/actions.ts (1)
23-66:⚠️ Potential issueEnsure Thread-Safe Access to Shared State Variables
The module uses shared state variables (
clientsMap,initialized,errorClients) without synchronization mechanisms. If functions likeinitializeMcpClientsorreinitializeMcpClientsare called concurrently, it may lead to race conditions or inconsistent state. Consider implementing synchronization to ensure thread-safe operations.To verify concurrency issues, please review how and where these functions are invoked. If they can be called concurrently, synchronization mechanisms like mutexes or atomic variables should be introduced to protect shared state.
app/components/mcp-market.tsx (1)
253-270: Ensure Robust Error Handling When Loading PrimitivesIn the
loadPrimitivesfunction (lines 253-270), if the server is not running, an empty array is set forprimitives. Ensure that components consumingprimitivescan handle empty arrays without errors to prevent potential runtime issues.app/components/home.tsx (1)
77-83: Verify Dynamic Import Path forMcpMarketPageEnsure that the dynamic import path
'./mcp-market'(lines 77-83) correctly resolves to theMcpMarketPagecomponent. This is important for the component to load properly, especially in build and deployment environments.app/components/sidebar.tsx (1)
254-262: LGTM!The new MCP icon button is well-integrated into the sidebar, following the existing patterns for icon buttons and conditional rendering.
app/locales/cn.ts (1)
629-631: LGTM!The MCP localization entry follows the established pattern and maintains consistency with other localization entries.
app/mcp/preset-server.json (3)
11-16: Add path validation constraints for filesystem accessThe filesystem paths configuration lacks validation constraints. Consider adding:
- Path pattern restrictions
- Absolute/relative path validation
- Directory traversal prevention
35-39: Specify required GitHub token scopesThe GitHub PAT configuration should specify the minimum required scopes for security. Consider adding:
- List of required scopes in the description
- Scope validation in the implementation
199-204: Enhance Docker configuration securityThe Docker module lacks essential configuration options. Consider adding:
- Container resource limits
- Security options (e.g., no-new-privileges)
- Network access controls
app/mcp/actions.ts
Outdated
| } catch (error) { | ||
| console.error("Failed to read MCP config:", error); | ||
| return { mcpServers: {} }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Missing Configuration File Gracefully
In the getMcpConfig function (lines 35-38), if the configuration file is missing (e.g., on the first run), it logs an error. Instead, consider checking if the error is due to the file not existing (ENOENT) and initialize with a default configuration without logging an error. This provides a smoother user experience.
Apply this change:
} catch (error) {
- console.error("Failed to read MCP config:", error);
- return { mcpServers: {} };
+ if (error.code === 'ENOENT') {
+ logger.warn("MCP config file not found, initializing with default configuration.");
+ return { mcpServers: {} };
+ } else {
+ logger.error("Failed to read MCP config:", error);
+ throw error;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error("Failed to read MCP config:", error); | |
| return { mcpServers: {} }; | |
| } | |
| } catch (error) { | |
| if (error.code === 'ENOENT') { | |
| logger.warn("MCP config file not found, initializing with default configuration."); | |
| return { mcpServers: {} }; | |
| } else { | |
| logger.error("Failed to read MCP config:", error); | |
| throw error; | |
| } | |
| } |
app/components/mcp-market.tsx
Outdated
|
|
||
| try { | ||
| // 构建服务器配置 | ||
| const args = [...preset.baseArgs]; | ||
| const env: Record<string, string> = {}; | ||
|
|
There was a problem hiding this comment.
Add Null Checks for preset.configSchema Before Use
In the saveServerConfig function (lines 130-135), preset.configSchema is accessed without null checks. If preset.configSchema is undefined or null, it may cause runtime errors. Ensure that preset.configSchema is validated before accessing its properties.
Apply this change:
if (!preset || !preset.configSchema || !editingServerId) return;
+ if (!preset.configSchema) {
+ showToast("Server configuration schema is missing.");
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
app/components/mcp-market.tsx
Outdated
| const bError = clientErrors[b.id] !== null; | ||
|
|
||
| if (aAdded !== bAdded) { | ||
| return aAdded ? -1 : 1; | ||
| } | ||
| if (aAdded && bAdded) { | ||
| if (aError !== bError) { | ||
| return aError ? -1 : 1; | ||
| } | ||
| } | ||
| return 0; | ||
| }) |
There was a problem hiding this comment.
Correct Sorting Logic for Server Error Status
In the server list sorting logic (lines 389-404), the comparison of aError and bError may be inverted, causing servers with errors to appear before those without. Adjust the sorting condition to ensure servers without errors are prioritized.
Apply this fix:
if (aAdded && bAdded) {
if (aError !== bError) {
- return aError ? -1 : 1;
+ return aError ? 1 : -1;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bError = clientErrors[b.id] !== null; | |
| if (aAdded !== bAdded) { | |
| return aAdded ? -1 : 1; | |
| } | |
| if (aAdded && bAdded) { | |
| if (aError !== bError) { | |
| return aError ? -1 : 1; | |
| } | |
| } | |
| return 0; | |
| }) | |
| const bError = clientErrors[b.id] !== null; | |
| if (aAdded !== bAdded) { | |
| return aAdded ? -1 : 1; | |
| } | |
| if (aAdded && bAdded) { | |
| if (aError !== bError) { | |
| return aError ? 1 : -1; | |
| } | |
| } | |
| return 0; | |
| }) |
app/mcp/mcp_config.json
Outdated
| { | ||
| "mcpServers": { | ||
| "filesystem": { | ||
| "command": "npx", | ||
| "args": [ | ||
| "-y", | ||
| "@modelcontextprotocol/server-filesystem", | ||
| "/Users/kadxy/Desktop" | ||
| ] | ||
| }, | ||
| "docker-mcp": { | ||
| "command": "uvx", | ||
| "args": ["docker-mcp"] | ||
| }, | ||
| "difyworkflow": { | ||
| "command": "mcp-difyworkflow-server", | ||
| "args": ["-base-url", "23"], | ||
| "env": { | ||
| "DIFY_WORKFLOW_NAME": "23", | ||
| "DIFY_API_KEYS": "23" | ||
| } | ||
| }, | ||
| "postgres": { | ||
| "command": "docker", | ||
| "args": ["run", "-i", "--rm", "mcp/postgres", null] | ||
| }, | ||
| "playwright": { | ||
| "command": "npx", | ||
| "args": ["-y", "@executeautomation/playwright-mcp-server"] | ||
| }, | ||
| "gdrive": { | ||
| "command": "npx", | ||
| "args": ["-y", "@modelcontextprotocol/server-gdrive"] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and validation guidance.
The configuration file lacks documentation for server configurations and validation requirements.
Consider adding:
- A documentation section describing each server type and its purpose
- Required environment variables for each server
- Validation rules for configuration parameters
- Error handling guidance
Example structure:
{
"documentation": {
"version": "1.0",
"servers": {
"filesystem": {
"description": "Local filesystem MCP server",
"required_env": ["MCP_FILESYSTEM_PATH"],
"validation": {
"MCP_FILESYSTEM_PATH": "Must be a valid directory path"
}
},
// ... other servers
}
},
"mcpServers": {
// ... existing configurations
}
}
app/mcp/mcp_config.json
Outdated
| "postgres": { | ||
| "command": "docker", | ||
| "args": ["run", "-i", "--rm", "mcp/postgres", null] | ||
| }, |
There was a problem hiding this comment.
Fix null argument in postgres configuration.
The postgres server configuration contains a null argument which could cause runtime issues.
Remove the null argument or replace it with a valid value:
- "args": ["run", "-i", "--rm", "mcp/postgres", null]
+ "args": ["run", "-i", "--rm", "mcp/postgres"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "postgres": { | |
| "command": "docker", | |
| "args": ["run", "-i", "--rm", "mcp/postgres", null] | |
| }, | |
| "postgres": { | |
| "command": "docker", | |
| "args": ["run", "-i", "--rm", "mcp/postgres"] | |
| }, |
app/mcp/mcp_config.json
Outdated
| "difyworkflow": { | ||
| "command": "mcp-difyworkflow-server", | ||
| "args": ["-base-url", "23"], | ||
| "env": { | ||
| "DIFY_WORKFLOW_NAME": "23", | ||
| "DIFY_API_KEYS": "23" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Replace placeholder values in difyworkflow configuration.
The configuration contains placeholder values "23" for base URL and API keys, which are not valid production values.
Consider using environment variables:
- "args": ["-base-url", "23"],
+ "args": ["-base-url", "${DIFY_BASE_URL}"],
"env": {
- "DIFY_WORKFLOW_NAME": "23",
- "DIFY_API_KEYS": "23"
+ "DIFY_WORKFLOW_NAME": "${DIFY_WORKFLOW_NAME}",
+ "DIFY_API_KEYS": "${DIFY_API_KEYS}"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "difyworkflow": { | |
| "command": "mcp-difyworkflow-server", | |
| "args": ["-base-url", "23"], | |
| "env": { | |
| "DIFY_WORKFLOW_NAME": "23", | |
| "DIFY_API_KEYS": "23" | |
| } | |
| }, | |
| "difyworkflow": { | |
| "command": "mcp-difyworkflow-server", | |
| "args": ["-base-url", "${DIFY_BASE_URL}"], | |
| "env": { | |
| "DIFY_WORKFLOW_NAME": "${DIFY_WORKFLOW_NAME}", | |
| "DIFY_API_KEYS": "${DIFY_API_KEYS}" | |
| } | |
| }, |
app/mcp/mcp_config.json
Outdated
| "filesystem": { | ||
| "command": "npx", | ||
| "args": [ | ||
| "-y", | ||
| "@modelcontextprotocol/server-filesystem", | ||
| "/Users/kadxy/Desktop" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Remove hardcoded filesystem path.
The filesystem server configuration contains a hardcoded path /Users/kadxy/Desktop which will not work across different environments and could pose security risks.
Consider using environment variables or configuration parameters for the path:
- "/Users/kadxy/Desktop"
+ "${MCP_FILESYSTEM_PATH}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "filesystem": { | |
| "command": "npx", | |
| "args": [ | |
| "-y", | |
| "@modelcontextprotocol/server-filesystem", | |
| "/Users/kadxy/Desktop" | |
| ] | |
| }, | |
| "filesystem": { | |
| "command": "npx", | |
| "args": [ | |
| "-y", | |
| "@modelcontextprotocol/server-filesystem", | |
| "${MCP_FILESYSTEM_PATH}" | |
| ] | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
app/mcp/actions.ts (2)
295-310: 🛠️ Refactor suggestionAdd request validation to prevent invalid actions
The
executeMcpActionfunction should validate the structure of therequestobject before execution to ensure robustness and prevent unexpected errors.Apply this diff to add validation:
+ // Validate request structure + if (!request || typeof request !== 'object') { + throw new Error('Invalid request format'); + } + if (!request.method || typeof request.method !== 'string') { + throw new Error('Missing or invalid method in request'); + }
269-292: 🛠️ Refactor suggestionEnhance error resilience in client restart process
In the
restartAllClientsfunction, individual client failures during restart should not prevent other clients from restarting. Implementing error handling for each client ensures that one failure doesn't halt the entire process.Apply this change:
+ // 使用 Promise.allSettled 来处理并发和错误 + await Promise.allSettled( + Array.from(clientsMap.values()).map(async (client) => { + if (client.client) { + try { + await removeClient(client.client); + } catch (error) { + logger.error(`Failed to remove client: ${error}`); + } + } + }) + ); + // 并行初始化所有客户端 + await Promise.all( + Object.entries(config.mcpServers).map(([clientId, serverConfig]) => + initializeSingleClient(clientId, serverConfig) + ) + );
🧹 Nitpick comments (4)
app/mcp/actions.ts (1)
29-60: Improve variable naming for clarityThe variable
statusat line 29 is used to store client data fromclientsMap, which could lead to confusion with thestatusfield in the returnedServerStatusResponse. Consider renamingstatustoclientDatafor better readability and to prevent ambiguity.Apply this change:
- const status = clientsMap.get(clientId); + const clientData = clientsMap.get(clientId); - if (!status) { + if (!clientData) { - if (status.errorMsg) { + if (clientData.errorMsg) { - return { status: "error", errorMsg: status.errorMsg }; + return { status: "error", errorMsg: clientData.errorMsg }; - if (status.client) { + if (clientData.client) {app/mcp/preset-server.json (2)
5-5: Remove duplicated text in the descriptionThe
descriptionfield for the "Filesystem" preset server contains repeated text. This appears to be unintentional and should be corrected for clarity.Apply this change:
- "description": "Secure file operations with configurable access controlsSecure file operations with configurable access controlsSecure file operations with configurable access controlsSecure file operations with configurable access controls", + "description": "Secure file operations with configurable access controls",
68-68: Correct typographical error in descriptionThere's a typo in the description of the "Playwright" preset server. "Webscrapping" should be "web scraping".
Apply this change:
- "description": "Browser automation and webscrapping with Playwright", + "description": "Browser automation and web scraping with Playwright",app/components/mcp-market.tsx (1)
254-269: Remove commented code.The commented
removeServerfunction should either be implemented or removed. Commented code adds noise and can become outdated.- // const removeServer = async (id: string) => { - // try { - // setIsLoading(true); - // const newConfig = await removeMcpServer(id); - // setConfig(newConfig); - - // // 移除状态 - // setClientStatuses((prev) => { - // const newStatuses = { ...prev }; - // delete newStatuses[id]; - // return newStatuses; - // }); - // } finally { - // setIsLoading(false); - // } - // };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/icons/pause.svgis excluded by!**/*.svgapp/icons/play.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
app/components/mcp-market.module.scss(1 hunks)app/components/mcp-market.tsx(1 hunks)app/mcp/actions.ts(1 hunks)app/mcp/preset-server.json(1 hunks)app/mcp/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/mcp-market.module.scss
🔇 Additional comments (5)
app/mcp/actions.ts (1)
323-330: Implement atomic file writes for configuration updatesTo prevent potential file corruption during writes, use a temporary file and an atomic rename operation when updating the MCP configuration file.
Apply this change:
+ const tempPath = `${CONFIG_PATH}.tmp`; + await fs.writeFile(tempPath, JSON.stringify(config, null, 2)); + await fs.rename(tempPath, CONFIG_PATH); + // Clean up temp file if an error occurs + try { + await fs.unlink(tempPath); + } catch { + // Ignore cleanup errors + }app/mcp/types.ts (1)
68-74: Improve type safety forListToolsResponseinterfaceThe index signature
[key: string]: anyin thetoolsobject reduces type safety by allowing any additional properties without restriction. Consider defining explicit properties or using a generic type to enhance type checking.Apply this change:
- [key: string]: any; + // Define additional known properties explicitly + parameters?: Record<string, unknown>; + version?: string; + // Remove or limit the index signature + // [key: string]: unknown; // If necessary, use a less permissive type };app/components/mcp-market.tsx (3)
615-621: Add accessibility improvements to the search input.The search input lacks proper accessibility attributes.
<input type="text" className={styles["search-bar"]} placeholder={"Search MCP Server"} autoFocus onInput={(e) => setSearchText(e.currentTarget.value)} + aria-label="Search MCP servers" + role="searchbox" + aria-describedby="search-description" /> +<span id="search-description" className="sr-only"> + Search for MCP servers by name, description, or tags +</span>
351-360: Implement proper input sanitization.The input values are directly used without sanitization, which could lead to XSS vulnerabilities.
<input type="text" value={value} placeholder={`${itemLabel} ${index + 1}`} onChange={(e) => { + const sanitizedValue = sanitizeInput(e.target.value); const newValue = [...currentValue] as string[]; - newValue[index] = e.target.value; + newValue[index] = sanitizedValue; setUserConfig({ ...userConfig, [key]: newValue }); }} />
576-694: LGTM! Well-structured component rendering.The component's rendering logic is well-organized with proper error boundaries and loading states.
app/components/mcp-market.tsx
Outdated
| useEffect(() => { | ||
| console.log("MCP Market - Current config:", config); | ||
| console.log("MCP Market - Current clientStatuses:", clientStatuses); | ||
| }, [config, clientStatuses]); |
There was a problem hiding this comment.
Remove debug console logs before production deployment.
Debug console logs should not be present in production code as they may expose sensitive information and impact performance.
- useEffect(() => {
- console.log("MCP Market - Current config:", config);
- console.log("MCP Market - Current clientStatuses:", clientStatuses);
- }, [config, clientStatuses]);
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/components/mcp-market.tsx (1)
640-646: 🛠️ Refactor suggestionAdd accessibility improvements to the search input.
The search input lacks proper accessibility attributes.
<input type="text" className={styles["search-bar"]} placeholder={"Search MCP Server"} autoFocus onInput={(e) => setSearchText(e.currentTarget.value)} + aria-label="Search MCP servers" + role="searchbox" + aria-describedby="search-description" /> +<span id="search-description" className="sr-only"> + Search for MCP servers by name, description, or tags +</span>
🧹 Nitpick comments (3)
app/components/mcp-market.tsx (3)
34-40: Enhance type safety of ConfigProperty interface.Consider improving type safety by:
- Making the
typefield more specific using a union type of allowed values- Adding validation for
minItemsto ensure it's positiveinterface ConfigProperty { - type: string; + type: 'string' | 'array' | 'number' | 'boolean'; description?: string; required?: boolean; - minItems?: number; + minItems?: number & { [K in number]: K extends number ? (K extends Positive ? K : never) : never }; }
43-54: Consider consolidating related state using useReducer.The component uses multiple useState hooks for related state management. Consider using useReducer to consolidate server-related states (config, status, tools) into a single reducer for better maintainability and state updates consistency.
interface ServerState { config: McpConfigData | undefined; clientStatuses: Record<string, ServerStatusResponse>; tools: ListToolsResponse["tools"] | null; isLoading: boolean; } type ServerAction = | { type: 'SET_CONFIG'; payload: McpConfigData } | { type: 'UPDATE_STATUS'; payload: { id: string; status: ServerStatusResponse } } | { type: 'SET_TOOLS'; payload: ListToolsResponse["tools"] | null } | { type: 'SET_LOADING'; payload: boolean };
285-304: Improve error handling in pauseServer function.The error handling could be more specific and informative. Consider:
- Adding error type checking
- Providing more detailed error messages
- Implementing retry logic for transient failures
try { setIsLoading(true); showToast("Stopping server..."); const newConfig = await pauseMcpServer(id); setConfig(newConfig); setClientStatuses((prev) => ({ ...prev, [id]: { status: "paused", errorMsg: null }, })); showToast("Server stopped successfully"); } catch (error) { - showToast("Failed to stop server"); - console.error(error); + const errorMessage = error instanceof Error + ? `Failed to stop server: ${error.message}` + : "Failed to stop server: Unknown error"; + logger.error(errorMessage, { serverId: id, error }); + showToast(errorMessage); } finally {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/components/mcp-market.tsx(1 hunks)
🔇 Additional comments (2)
app/components/mcp-market.tsx (2)
364-373: 🛠️ Refactor suggestionAdd input validation and accessibility improvements.
The input field lacks validation and proper accessibility attributes.
<input type="text" value={value} + aria-label={`${itemLabel} ${index + 1}`} + aria-required={prop.required} placeholder={`${itemLabel} ${index + 1}`} onChange={(e) => { + const sanitizedValue = e.target.value.trim(); + if (prop.pattern && !new RegExp(prop.pattern).test(sanitizedValue)) { + showToast(`Invalid ${itemLabel} format`); + return; + } const newValue = [...currentValue] as string[]; - newValue[index] = e.target.value; + newValue[index] = sanitizedValue; setUserConfig({ ...userConfig, [key]: newValue }); }} />Likely invalid or redundant comment.
66-69: 🛠️ Refactor suggestionReplace console.error with proper error logging.
Console statements should not be used in production code. Consider implementing a proper logging service.
- console.error("Failed to load preset servers:", error); + logger.error("Failed to load preset servers:", error); showToast("Failed to load preset servers");Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/constant.ts (2)
258-263: Consider improving template readability.The template structure is functional but could be more maintainable.
Consider using this more structured format:
-export const MCP_TOOLS_TEMPLATE = ` -[clientId] -{{ clientId }} -[tools] -{{ tools }} -`; +export const MCP_TOOLS_TEMPLATE = ` +# Client Information +Client ID: {{ clientId }} + +# Available Tools +{{ tools }} +`;
265-380: Review and enhance the MCP system template.The template has several areas that could be improved:
- The example interaction shows Chinese text which may not be universally understood
- There are multiple exclamation marks used for emphasis which could be replaced with more professional formatting
- The error examples could be better structured
Consider these improvements:
4. INTERACTION FLOW: A. When user makes a request: - IMMEDIATELY use appropriate tool if available - DO NOT ask if user wants you to use the tool - DO NOT just describe what you could do B. After receiving tool response: - Explain results clearly - Take next appropriate action if needed C. If tools fail: - Explain the error - Try alternative approach immediately 5. EXAMPLE INTERACTION: - good example: + # Correct Example: + # List allowed directories ```json:mcp:filesystem { "method": "tools/call", "params": { "name": "list_allowed_directories", "arguments": {} } } ``` + # Write a file ```json:mcp-response:filesystem { "method": "tools/call", "params": { "name": "write_file", "arguments": { - "path": "/Users/river/dev/nextchat/test/joke.txt", - "content": "为什么数学书总是感到忧伤？因为它有太多的问题。" + "path": "/Users/example/docs/greeting.txt", + "content": "Hello, World!" } } } ``` - follwing is the wrong! mcp json example: + # Common Mistakes: + # 1. Incorrect method name ```json:mcp:filesystem { "method": "write_file", # Wrong: should be "tools/call" "params": { "path": "NextChat_Information.txt", "content": "1" } } ``` - This is wrong because the method is not tools/call. + # 2. Missing mcp prefix in code block ```{ "method": "search_repositories", "params": { "query": "2oeee" } } ``` - This is wrong because the method is not tools/call.!!!!!!!!!!! + # Correct Format Reference: - the right format is: ```json:mcp:filesystem { "method": "tools/call", "params": { "name": "search_repositories", "arguments": { "query": "2oeee" } } } ``` - please follow the format strictly ONLY use tools/call method!!!!!!!!!!! + Note: Always use the "tools/call" method and follow the format strictly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/constant.ts(3 hunks)
🔇 Additional comments (3)
app/constant.ts (3)
50-50: LGTM! Path enum updated correctly.The new
McpMarketpath follows the existing naming convention and is properly formatted.
91-91: LGTM! StoreKey enum updated correctly.The new
Mcpkey follows the existing naming convention and is properly formatted.
258-380: Verify the usage of MCP constants across the codebase.Let's ensure these new constants are properly imported and used in other files.
✅ Verification successful
Constants are properly used across the codebase
The MCP constants are correctly imported and used in app/store/chat.ts with proper template substitution. No issues found with hardcoded values or inconsistent usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for imports of the new MCP constants echo "Checking imports of MCP constants..." rg -l "MCP_TOOLS_TEMPLATE|MCP_SYSTEM_TEMPLATE" --type ts --type tsx # Check for potential usages of hardcoded strings that should use these constants echo "Checking for hardcoded MCP strings that should use constants..." rg -l "tools/call" --type ts --type tsxLength of output: 385
Script:
#!/bin/bash # Find all TypeScript files including React ones echo "TypeScript files containing MCP constants:" fd -e ts -e tsx | xargs rg -l "MCP_TOOLS_TEMPLATE|MCP_SYSTEM_TEMPLATE" echo -e "\nFiles with potential hardcoded 'tools/call' usage:" fd -e ts -e tsx | xargs rg -l "tools/call" # Get context around tools/call usage to verify if they should use the constants echo -e "\nContext of tools/call usage:" fd -e ts -e tsx | xargs rg -C 3 "tools/call"Length of output: 3039
Script:
#!/bin/bash # Check how the constants are used in chat.ts echo "Usage of MCP constants in chat.ts:" rg -C 5 "MCP_.*_TEMPLATE" app/store/chat.tsLength of output: 872
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
app/mcp/actions.ts (2)
26-60: Consider refactoring status checks for better maintainability.The function has multiple return points and nested conditions. Consider using a single status object that's modified based on conditions, then returned at the end.
export async function getClientStatus( clientId: string, ): Promise<ServerStatusResponse> { + let status: ServerStatusResponse = { status: "undefined", errorMsg: null }; const clientData = clientsMap.get(clientId); const config = await getMcpConfigFromFile(); const serverConfig = config.mcpServers[clientId]; - if (!serverConfig) { - return { status: "undefined", errorMsg: null }; + if (serverConfig) { + if (serverConfig.status === "paused") { + status = { status: "paused", errorMsg: null }; + } else if (clientData) { + if (clientData.errorMsg) { + status = { status: "error", errorMsg: clientData.errorMsg }; + } else if (clientData.client) { + status = { status: "active", errorMsg: null }; + } else { + status = { status: "error", errorMsg: "Client not found" }; + } + } } - if (serverConfig.status === "paused") { - return { status: "paused", errorMsg: null }; - } - - if (!status) { - return { status: "undefined", errorMsg: null }; - } - - if (status.errorMsg) { - return { status: "error", errorMsg: status.errorMsg }; - } - - if (status.client) { - return { status: "active", errorMsg: null }; - } - - return { status: "error", errorMsg: "Client not found" }; + return status; }
68-72: Optimize client count calculation.The count can be calculated more efficiently using Array methods.
export async function getAvailableClientsCount() { - let count = 0; - clientsMap.forEach((map) => !map.errorMsg && count++); - return count; + return Array.from(clientsMap.values()).filter(map => !map.errorMsg).length; }app/components/mcp-market.module.scss (4)
8-14: Add ARIA attributes to loading indicator.Enhance accessibility for screen readers.
.loading-indicator { font-size: 12px; color: var(--primary); margin-left: 8px; font-weight: normal; opacity: 0.8; + &[aria-busy="true"] { + cursor: progress; + } }
30-30: Define animation duration as a variable.Extract the animation duration to a variable for better maintainability.
+$animation-duration: 0.3s; + animation: slide-in ease 0.3s;
115-116: Use CSS variables for consistent colors.Replace hardcoded color values with CSS variables for better maintainability.
- background-color: #16a34a; + background-color: var(--success-color); - background-color: #22c55e; + background-color: var(--active-color); - background-color: #ef4444; + background-color: var(--error-color); - background-color: #6b7280; + background-color: var(--disabled-color);Also applies to: 159-160, 163-164, 167-168
88-106: Optimize loading animation performance.Use
transforminstead ofopacityfor better performance, and consider usingwill-changefor hardware acceleration.&.loading { position: relative; &::after { content: ""; position: absolute; top: 0; left: 0; right: 0; bottom: 0; + will-change: transform; background: linear-gradient( 90deg, transparent, rgba(255, 255, 255, 0.2), transparent ); background-size: 200% 100%; - animation: loading-pulse 1.5s infinite; + animation: loading-pulse 1.5s infinite linear; } }app/components/mcp-market.tsx (1)
43-57: Consider using useReducer for complex state management.The component has multiple related state variables that could be managed more effectively with useReducer.
interface McpMarketState { searchText: string; userConfig: Record<string, string | string[]>; editingServerId?: string; tools: ListToolsResponse["tools"] | null; viewingServerId?: string; isLoading: boolean; config?: McpConfigData; clientStatuses: Record<string, ServerStatusResponse>; loadingPresets: boolean; presetServers: PresetServer[]; loadingStates: Record<string, string>; } type McpMarketAction = | { type: 'SET_SEARCH_TEXT'; payload: string } | { type: 'SET_USER_CONFIG'; payload: Record<string, string | string[]> } | { type: 'SET_EDITING_SERVER'; payload?: string } // ... other action types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/mcp-market.module.scss(1 hunks)app/components/mcp-market.tsx(1 hunks)app/mcp/actions.ts(1 hunks)
🔇 Additional comments (6)
app/mcp/actions.ts (4)
259-281: Fix potential memory leak in removeMcpServer.The client's resources should be cleaned up before updating the configuration to prevent potential memory leaks.
284-306: Enhance error resilience in client restart process.Individual client failures during restart shouldn't prevent other clients from restarting.
337-344: Implement atomic file writes for configuration updates.Use temporary files and atomic rename operations to prevent corruption during writes.
117-124: Protect against concurrent initialization race conditions.Multiple clients initializing simultaneously could lead to race conditions. Consider adding a lock mechanism or using a queue for initialization.
Consider implementing a queue system for client initialization:
const initQueue = new PQueue({concurrency: 1}); await initQueue.add(() => initializeSingleClient(clientId, serverConfig));app/components/mcp-market.tsx (2)
143-215: Enhance error handling in saveServerConfig.The function should validate configuration values before saving.
682-688: Add accessibility improvements to the search input.The search input lacks proper accessibility attributes.
| export async function getMcpConfigFromFile(): Promise<McpConfigData> { | ||
| try { | ||
| const configStr = await fs.readFile(CONFIG_PATH, "utf-8"); | ||
| return JSON.parse(configStr); | ||
| } catch (error) { | ||
| logger.error(`Failed to load MCP config, using default config: ${error}`); | ||
| return DEFAULT_MCP_CONFIG; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add configuration schema validation.
The configuration loading process should validate the structure of the loaded data against a schema to ensure integrity.
interface McpConfigSchema {
mcpServers: Record<string, ServerConfig>;
}
function validateConfig(config: unknown): config is McpConfigData {
const schema = z.object({
mcpServers: z.record(z.custom<ServerConfig>())
});
return schema.safeParse(config).success;
}
app/mcp/actions.ts
Outdated
| export async function reinitializeClient(clientId: string) { | ||
| const config = await getMcpConfigFromFile(); | ||
| const serverConfig = config.mcpServers[clientId]; | ||
| if (!serverConfig) { | ||
| throw new Error(`Server config not found for client ${clientId}`); | ||
| } | ||
| await initializeSingleClient(clientId, serverConfig); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in reinitializeClient.
The function should handle initialization failures gracefully and provide detailed error information.
export async function reinitializeClient(clientId: string) {
+ logger.info(`Attempting to reinitialize client [${clientId}]...`);
const config = await getMcpConfigFromFile();
const serverConfig = config.mcpServers[clientId];
if (!serverConfig) {
- throw new Error(`Server config not found for client ${clientId}`);
+ const error = `Server config not found for client ${clientId}`;
+ logger.error(error);
+ throw new Error(error);
+ }
+ try {
await initializeSingleClient(clientId, serverConfig);
+ logger.success(`Successfully reinitialized client [${clientId}]`);
+ } catch (error) {
+ const message = `Failed to reinitialize client [${clientId}]: ${error}`;
+ logger.error(message);
+ throw new Error(message);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function reinitializeClient(clientId: string) { | |
| const config = await getMcpConfigFromFile(); | |
| const serverConfig = config.mcpServers[clientId]; | |
| if (!serverConfig) { | |
| throw new Error(`Server config not found for client ${clientId}`); | |
| } | |
| await initializeSingleClient(clientId, serverConfig); | |
| } | |
| export async function reinitializeClient(clientId: string) { | |
| logger.info(`Attempting to reinitialize client [${clientId}]...`); | |
| const config = await getMcpConfigFromFile(); | |
| const serverConfig = config.mcpServers[clientId]; | |
| if (!serverConfig) { | |
| const error = `Server config not found for client ${clientId}`; | |
| logger.error(error); | |
| throw new Error(error); | |
| } | |
| try { | |
| await initializeSingleClient(clientId, serverConfig); | |
| logger.success(`Successfully reinitialized client [${clientId}]`); | |
| } catch (error) { | |
| const message = `Failed to reinitialize client [${clientId}]: ${error}`; | |
| logger.error(message); | |
| throw new Error(message); | |
| } | |
| } |
| &:focus { | ||
| background-color: var(--white); | ||
| border-color: var(--primary); | ||
| outline: none; | ||
| box-shadow: 0 0 0 2px var(--primary-10); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance focus visibility for accessibility.
Improve focus state visibility for keyboard navigation.
&:focus {
background-color: var(--white);
border-color: var(--primary);
outline: none;
- box-shadow: 0 0 0 2px var(--primary-10);
+ box-shadow: 0 0 0 3px var(--primary-20);
+ outline: 2px solid transparent;
+ outline-offset: 2px;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| &:focus { | |
| background-color: var(--white); | |
| border-color: var(--primary); | |
| outline: none; | |
| box-shadow: 0 0 0 2px var(--primary-10); | |
| } | |
| &:focus { | |
| background-color: var(--white); | |
| border-color: var(--primary); | |
| outline: none; | |
| box-shadow: 0 0 0 3px var(--primary-20); | |
| outline: 2px solid transparent; | |
| outline-offset: 2px; | |
| } |
| export function McpMarketPage() { | ||
| const navigate = useNavigate(); | ||
| const [searchText, setSearchText] = useState(""); | ||
| const [userConfig, setUserConfig] = useState<Record<string, any>>({}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'any' type with a proper type definition.
Using 'any' type reduces type safety and could lead to runtime errors.
- const [userConfig, setUserConfig] = useState<Record<string, any>>({});
+ const [userConfig, setUserConfig] = useState<Record<string, string | string[]>>({});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [userConfig, setUserConfig] = useState<Record<string, any>>({}); | |
| const [userConfig, setUserConfig] = useState<Record<string, string | string[]>>({}); |
app/components/mcp-market.tsx
Outdated
| useEffect(() => { | ||
| const loadPresetServers = async () => { | ||
| try { | ||
| setLoadingPresets(true); | ||
| const response = await fetch("https://nextchat.club/mcp/list"); | ||
| if (!response.ok) { | ||
| throw new Error("Failed to load preset servers"); | ||
| } | ||
| const data = await response.json(); | ||
| setPresetServers(data?.data ?? []); | ||
| } catch (error) { | ||
| console.error("Failed to load preset servers:", error); | ||
| showToast("Failed to load preset servers"); | ||
| } finally { | ||
| setLoadingPresets(false); | ||
| } | ||
| }; | ||
| loadPresetServers().then(); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in loadPresetServers.
Add proper error handling and remove console.error calls.
const loadPresetServers = async () => {
try {
setLoadingPresets(true);
const response = await fetch("https://nextchat.club/mcp/list");
if (!response.ok) {
- throw new Error("Failed to load preset servers");
+ throw new Error(`Failed to load preset servers: ${response.statusText}`);
}
const data = await response.json();
+ if (!data?.data) {
+ throw new Error("Invalid server response format");
+ }
setPresetServers(data?.data ?? []);
} catch (error) {
- console.error("Failed to load preset servers:", error);
+ logger.error("Failed to load preset servers:", error);
showToast("Failed to load preset servers");
} finally {
setLoadingPresets(false);
}
};Committable suggestion skipped: line range outside the PR's diff.
| // 获取服务器支持的 Tools | ||
| const loadTools = async (id: string) => { | ||
| try { | ||
| const result = await getClientTools(id); | ||
| if (result) { | ||
| setTools(result); | ||
| } else { | ||
| throw new Error("Failed to load tools"); | ||
| } | ||
| } catch (error) { | ||
| showToast("Failed to load tools"); | ||
| console.error(error); | ||
| setTools(null); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in loadTools.
Replace console.error with proper logging and add more specific error messages.
const loadTools = async (id: string) => {
try {
const result = await getClientTools(id);
if (result) {
setTools(result);
} else {
- throw new Error("Failed to load tools");
+ throw new Error(`Failed to load tools for server ${id}: No tools returned`);
}
} catch (error) {
- showToast("Failed to load tools");
- console.error(error);
+ const message = error instanceof Error ? error.message : "Unknown error";
+ logger.error(`Failed to load tools for server ${id}:`, message);
+ showToast(`Failed to load tools: ${message}`);
setTools(null);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 获取服务器支持的 Tools | |
| const loadTools = async (id: string) => { | |
| try { | |
| const result = await getClientTools(id); | |
| if (result) { | |
| setTools(result); | |
| } else { | |
| throw new Error("Failed to load tools"); | |
| } | |
| } catch (error) { | |
| showToast("Failed to load tools"); | |
| console.error(error); | |
| setTools(null); | |
| } | |
| }; | |
| // 获取服务器支持的 Tools | |
| const loadTools = async (id: string) => { | |
| try { | |
| const result = await getClientTools(id); | |
| if (result) { | |
| setTools(result); | |
| } else { | |
| throw new Error(`Failed to load tools for server ${id}: No tools returned`); | |
| } | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : "Unknown error"; | |
| logger.error(`Failed to load tools for server ${id}:`, message); | |
| showToast(`Failed to load tools: ${message}`); | |
| setTools(null); | |
| } | |
| }; |
| <Modal | ||
| title={`Configure Server - ${editingServerId}`} | ||
| onClose={() => !isLoading && setEditingServerId(undefined)} | ||
| actions={[ | ||
| <IconButton | ||
| key="cancel" | ||
| text="Cancel" | ||
| onClick={() => setEditingServerId(undefined)} | ||
| bordered | ||
| disabled={isLoading} | ||
| />, | ||
| <IconButton | ||
| key="confirm" | ||
| text="Save" | ||
| type="primary" | ||
| onClick={saveServerConfig} | ||
| bordered | ||
| disabled={isLoading} | ||
| />, | ||
| ]} | ||
| > | ||
| <List>{renderConfigForm()}</List> | ||
| </Modal> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance modal accessibility.
Add proper ARIA attributes and keyboard navigation support to the modal.
<Modal
title={`Configure Server - ${editingServerId}`}
onClose={() => !isLoading && setEditingServerId(undefined)}
+ role="dialog"
+ aria-modal="true"
+ aria-labelledby="modal-title"
actions={[
<IconButton
key="cancel"
text="Cancel"
onClick={() => setEditingServerId(undefined)}
bordered
disabled={isLoading}
+ aria-label="Cancel configuration"
/>,
<IconButton
key="confirm"
text="Save"
type="primary"
onClick={saveServerConfig}
bordered
disabled={isLoading}
+ aria-label="Save configuration"
/>,
]}
>
+ <h2 id="modal-title" className="sr-only">
+ Configure Server - {editingServerId}
+ </h2>
<List>{renderConfigForm()}</List>
</Modal>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Modal | |
| title={`Configure Server - ${editingServerId}`} | |
| onClose={() => !isLoading && setEditingServerId(undefined)} | |
| actions={[ | |
| <IconButton | |
| key="cancel" | |
| text="Cancel" | |
| onClick={() => setEditingServerId(undefined)} | |
| bordered | |
| disabled={isLoading} | |
| />, | |
| <IconButton | |
| key="confirm" | |
| text="Save" | |
| type="primary" | |
| onClick={saveServerConfig} | |
| bordered | |
| disabled={isLoading} | |
| />, | |
| ]} | |
| > | |
| <List>{renderConfigForm()}</List> | |
| </Modal> | |
| </div> | |
| <Modal | |
| title={`Configure Server - ${editingServerId}`} | |
| onClose={() => !isLoading && setEditingServerId(undefined)} | |
| role="dialog" | |
| aria-modal="true" | |
| aria-labelledby="modal-title" | |
| actions={[ | |
| <IconButton | |
| key="cancel" | |
| text="Cancel" | |
| onClick={() => setEditingServerId(undefined)} | |
| bordered | |
| disabled={isLoading} | |
| aria-label="Cancel configuration" | |
| />, | |
| <IconButton | |
| key="confirm" | |
| text="Save" | |
| type="primary" | |
| onClick={saveServerConfig} | |
| bordered | |
| disabled={isLoading} | |
| aria-label="Save configuration" | |
| />, | |
| ]} | |
| > | |
| <h2 id="modal-title" className="sr-only"> | |
| Configure Server - {editingServerId} | |
| </h2> | |
| <List>{renderConfigForm()}</List> | |
| </Modal> | |
| </div> |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
app/components/mcp-market.tsx (4)
47-47: 🛠️ Refactor suggestionReplace 'any' type with a proper type definition.
Using 'any' type reduces type safety and could lead to runtime errors.
Apply this diff:
-const [userConfig, setUserConfig] = useState<Record<string, any>>({}); +const [userConfig, setUserConfig] = useState<Record<string, string | string[]>>({});
238-252: 🛠️ Refactor suggestionImprove error handling in loadTools.
Replace console.error with proper error messages.
Apply this diff:
const loadTools = async (id: string) => { try { const result = await getClientTools(id); if (result) { setTools(result); } else { - throw new Error("Failed to load tools"); + throw new Error(`Failed to load tools for server ${id}: No tools returned`); } } catch (error) { - showToast("Failed to load tools"); - console.error(error); + const message = error instanceof Error ? error.message : "Unknown error"; + showToast(`Failed to load tools: ${message}`); setTools(null); } };
701-707: 🛠️ Refactor suggestionAdd accessibility improvements to the search input.
The search input lacks proper accessibility attributes.
Apply this diff:
<input type="text" className={styles["search-bar"]} placeholder={"Search MCP Server"} autoFocus onInput={(e) => setSearchText(e.currentTarget.value)} + aria-label="Search MCP servers" + role="searchbox" + aria-describedby="search-description" /> +<span id="search-description" className="sr-only"> + Search for MCP servers by name, description, or tags +</span>
716-739: 🛠️ Refactor suggestionEnhance modal accessibility.
Add proper ARIA attributes and keyboard navigation support to the modal.
Apply this diff:
<Modal title={`Configure Server - ${editingServerId}`} onClose={() => !isLoading && setEditingServerId(undefined)} + role="dialog" + aria-modal="true" + aria-labelledby="modal-title" actions={[ <IconButton key="cancel" text="Cancel" onClick={() => setEditingServerId(undefined)} bordered disabled={isLoading} + aria-label="Cancel configuration" />, <IconButton key="confirm" text="Save" type="primary" onClick={saveServerConfig} bordered disabled={isLoading} + aria-label="Save configuration" />, ]} > + <h2 id="modal-title" className="sr-only"> + Configure Server - {editingServerId} + </h2> <List>{renderConfigForm()}</List> </Modal>
🧹 Nitpick comments (8)
app/locales/en.ts (1)
638-640: LGTM on structure, consider expanding the MCP label for clarity.The addition follows the file's structure and naming conventions correctly. However, since MCP (Multi-Channel Protocol) is a technical term, consider making it more user-friendly.
Consider this alternative to improve clarity:
Mcp: { - Name: "MCP", + Name: "Multi-Channel Protocol (MCP)", },app/mcp/actions.ts (4)
71-71: Improve readability of client counting logicThe use of logical AND (
&&) for incrementingcountwithingetAvailableClientsCountmay reduce code clarity.Consider refactoring the code for better readability:
- clientsMap.forEach((map) => !map.errorMsg && count++); + clientsMap.forEach((map) => { + if (!map.errorMsg) { + count++; + } + });
217-259: Handle client initialization failures without affecting configurationIn
resumeMcpServer, when client initialization fails, the server configuration is updated to"error"status, which may not be ideal as it persists an error state in the configuration file.Instead of modifying the server configuration on initialization failure, consider handling the error within the clients map only. This keeps the configuration file consistent and avoids unintended side effects:
// 初始化失败 clientsMap.set(clientId, { client: null, tools: null, errorMsg: error instanceof Error ? error.message : String(error), }); logger.error(`Failed to initialize client [${clientId}]: ${error}`); - return false; + // Do not modify the configuration file + return await getClientStatus(clientId).then( + (status) => status.status === "active" + );
64-66: Ensure consistent null checks ingetClientToolsThe function
getClientToolsreturnsnullif the client is not found or if tools are not available.For better consistency, consider explicitly checking if the client exists before accessing its tools:
export async function getClientTools(clientId: string) { - return clientsMap.get(clientId)?.tools ?? null; + const clientData = clientsMap.get(clientId); + if (clientData && clientData.tools) { + return clientData.tools; + } + return null; }
364-371: Avoid redundant error logging inisMcpEnabledIn
isMcpEnabled, logging the error may be unnecessary if the function simply returnsfalse.Consider removing the error logging to reduce noise:
export async function isMcpEnabled() { try { const serverConfig = getServerSideConfig(); return !!serverConfig.enableMcp; } catch (error) { - logger.error(`Failed to check MCP status: ${error}`); return false; } }app/layout.tsx (1)
9-9: Optimize import placement for better readabilityThe import of
getServerSideConfigcould be moved closer to its usage withinRootLayoutfor code clarity.If
getServerSideConfigis only used insideRootLayout, consider moving the import statement inside the function scope.app/config/server.ts (2)
85-85: Document the new environment variableENABLE_MCPThe addition of
ENABLE_MCPlacks documentation, which could cause confusion for other developers.Add a comment explaining the purpose and expected values of
ENABLE_MCP:ENABLE_MCP?: string; // Set to 'true' to enable MCP functionality
256-256: Consistent casting of environment variables to booleanIn the configuration object,
enableMcpuses!!to cast the environment variable to a boolean. Ensure this is consistent across all boolean configurations.Review other boolean configurations to ensure consistent usage:
return { ... enableMcp: !!process.env.ENABLE_MCP, // For consistency, cast other booleans similarly hideUserApiKey: !!process.env.HIDE_USER_API_KEY, disableGPT4: !!process.env.DISABLE_GPT4, hideBalanceQuery: !!process.env.ENABLE_BALANCE_QUERY, disableFastLink: !!process.env.DISABLE_FAST_LINK, ... };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.env.template(1 hunks)Dockerfile(1 hunks)app/components/chat.tsx(15 hunks)app/components/home.tsx(6 hunks)app/components/mcp-market.tsx(1 hunks)app/components/sidebar.tsx(6 hunks)app/config/server.ts(3 hunks)app/layout.tsx(2 hunks)app/locales/en.ts(1 hunks)app/mcp/actions.ts(1 hunks)app/page.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- app/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/chat.tsx
🔇 Additional comments (9)
app/mcp/actions.ts (4)
334-341: Handle missing configuration file gracefullyIn
getMcpConfigFromFile, if the configuration file is missing (e.g., on the first run), the error is logged, which might be confusing for users.This issue was previously noted. Consider checking if the error is due to the file not existing (
ENOENT) and initialize with a default configuration without logging an error.
345-351: Use atomic writes to prevent configuration file corruptionWhen updating the MCP configuration, writing directly to the file could lead to corruption if the process is interrupted.
As previously suggested, consider implementing atomic file writes using temporary files and renaming:
async function updateMcpConfig(config: McpConfigData): Promise<void> { try { + const tempPath = `${CONFIG_PATH}.tmp`; + await fs.writeFile(tempPath, JSON.stringify(config, null, 2)); + await fs.rename(tempPath, CONFIG_PATH); } catch (error) { + // Clean up temp file if it exists + try { + await fs.unlink(`${CONFIG_PATH}.tmp`); + } catch { + // Ignore cleanup errors + } throw error; } }
316-331: Add request validation to ensure correct executionThe
executeMcpActionfunction should validate the request structure to prevent potential errors during execution.This was highlighted earlier. Ensure that the request contains the necessary properties:
export async function executeMcpAction( clientId: string, request: McpRequestMessage, ) { try { + // Validate request structure + if (!request || typeof request !== 'object') { + throw new Error('Invalid request format'); + } + if (!request.action || typeof request.action !== 'string') { + throw new Error('Missing or invalid action in request'); + } const client = clientsMap.get(clientId); if (!client?.client) { throw new Error(`Client ${clientId} not found`); } logger.info(`Executing request for [${clientId}]`); return await executeRequest(client.client, request); } catch (error) { logger.error(`Failed to execute request for [${clientId}]: ${error}`); throw error; } }
291-313: 🛠️ Refactor suggestionEnsure all clients are restarted even if some fail
In
restartAllClients, a failure in one client could prevent other clients from restarting due to the synchronousforloops.Refactor the function to handle client restarts asynchronously and independently:
// 关闭所有客户端 - for (const client of clientsMap.values()) { - if (client.client) { - await removeClient(client.client); - } - } + await Promise.allSettled( + Array.from(clientsMap.values()).map(async (client) => { + if (client.client) { + try { + await removeClient(client.client); + } catch (error) { + logger.error(`Failed to remove client: ${error}`); + } + } + }) + ); // 清空状态 clientsMap.clear(); // 重新初始化 const config = await getMcpConfigFromFile(); - for (const [clientId, serverConfig] of Object.entries(config.mcpServers)) { - await initializeSingleClient(clientId, serverConfig); - } + await Promise.all( + Object.entries(config.mcpServers).map(([clientId, serverConfig]) => + initializeSingleClient(clientId, serverConfig) + ) + );Likely invalid or redundant comment.
Dockerfile (1)
45-45: Verify the existence of mcp_config.json.The COPY command assumes
mcp_config.jsonexists in the builder stage. If this file is missing, the build will fail..env.template (1)
10-14: LGTM!The documentation for the
ENABLE_MCPenvironment variable is clear, follows the existing pattern, and correctly specifies the default value and usage.app/components/home.tsx (1)
78-83: LGTM!The dynamic import for
McpMarketPagefollows the existing pattern and includes proper loading state handling.app/components/sidebar.tsx (1)
267-277: LGTM!The conditional rendering of the MCP button is well-implemented with proper icon, text, and navigation handling.
app/components/mcp-market.tsx (1)
76-94: 🛠️ Refactor suggestionImprove error handling in loadPresetServers.
The function uses console.error and could benefit from better error handling.
Apply this diff:
const loadPresetServers = async () => { if (!mcpEnabled) return; try { setLoadingPresets(true); const response = await fetch("https://nextchat.club/mcp/list"); if (!response.ok) { - throw new Error("Failed to load preset servers"); + throw new Error(`Failed to load preset servers: ${response.statusText}`); } const data = await response.json(); + if (!data?.data) { + throw new Error("Invalid server response format"); + } setPresetServers(data?.data ?? []); } catch (error) { - console.error("Failed to load preset servers:", error); showToast("Failed to load preset servers"); } finally { setLoadingPresets(false); } };Likely invalid or redundant comment.
| Mcp: { | ||
| Name: "MCP", | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Add comprehensive localization for MCP-related UI elements
The verification revealed that most MCP-related strings are not properly localized. Please add the following keys to the Mcp object in the locale files:
- Market page elements:
app/components/mcp-market.tsx: "MCP Market", "Search MCP Server", "servers configured"app/components/mcp-market.tsx: Status messages like "Creating MCP client...", "Starting server...", "Stopping server..."
- Error messages:
app/store/chat.ts: "MCP execution failed"app/components/mcp-market.tsx: "Failed to load preset servers"
- Button labels and tooltips in the market interface
🔗 Analysis chain
Verify comprehensive localization coverage for MCP feature.
As this is a WIP PR implementing the Multi-Channel Protocol system, ensure that all user-facing strings related to MCP functionality (such as error messages, tooltips, or configuration options) are properly localized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential unlocalized MCP-related strings in the codebase
echo "Searching for MCP-related strings that might need localization..."
rg -i "mcp" --type ts --type tsx --type js --type jsx -g '!app/locales/*' -g '!*test*'
Length of output: 255
Script:
#!/bin/bash
# Search for MCP-related strings in TypeScript and JavaScript files
echo "Searching for MCP-related strings in source files..."
rg -i "mcp" --type ts --type js -g '!app/locales/*' -g '!*test*' -g '!*.d.ts'
# Search specifically for string literals containing MCP
echo -e "\nSearching for potential string literals containing MCP..."
ast-grep --pattern 'StringLiteral: /.*[Mm][Cc][Pp].*/'
# Search for potential React components or features related to MCP
echo -e "\nSearching for MCP-related components or features..."
rg -i "mcp" --type ts --type js -g '!app/locales/*' -g '!*test*' -A 2 -B 2
Length of output: 54077
| const serverConfig = getServerSideConfig(); | ||
|
|
There was a problem hiding this comment.
Ensure server-side code is not executed on the client
The usage of getServerSideConfig inside a component may cause issues if the component is rendered on the client side.
Since getServerSideConfig accesses server-side environment variables, ensure that RootLayout is a server component or modify the code to prevent client-side execution.
|
|
||
| COPY --from=builder /app/public ./public | ||
| COPY --from=builder /app/.next/standalone ./ | ||
| COPY --from=builder /app/.next/static ./.next/static | ||
| COPY --from=builder /app/.next/server ./.next/server | ||
|
|
||
| RUN mkdir -p /app/app/mcp && chmod 777 /app/app/mcp |
There was a problem hiding this comment.
Restrict directory permissions for security.
Setting 777 permissions (read, write, execute for all users) on the /app/app/mcp directory is too permissive and poses a security risk. Consider using more restrictive permissions.
Apply this diff to use more restrictive permissions:
-RUN mkdir -p /app/app/mcp && chmod 777 /app/app/mcp
+RUN mkdir -p /app/app/mcp && chown node:node /app/app/mcp && chmod 755 /app/app/mcp📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN mkdir -p /app/app/mcp && chmod 777 /app/app/mcp | |
| RUN mkdir -p /app/app/mcp && chown node:node /app/app/mcp && chmod 755 /app/app/mcp |
| const initMcp = async () => { | ||
| try { | ||
| const enabled = await isMcpEnabled(); | ||
| if (enabled) { | ||
| console.log("[MCP] initializing..."); | ||
| await initializeMcpSystem(); | ||
| console.log("[MCP] initialized"); | ||
| } | ||
| } catch (err) { | ||
| console.error("[MCP] failed to initialize:", err); | ||
| } | ||
| }; | ||
| initMcp(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console logs and improve error handling.
The initialization code includes console logs and could benefit from better error handling.
Apply this diff to improve the code:
const initMcp = async () => {
try {
const enabled = await isMcpEnabled();
if (enabled) {
- console.log("[MCP] initializing...");
await initializeMcpSystem();
- console.log("[MCP] initialized");
}
} catch (err) {
- console.error("[MCP] failed to initialize:", err);
+ showToast(
+ err instanceof Error
+ ? `Failed to initialize MCP: ${err.message}`
+ : "Failed to initialize MCP"
+ );
}
};Committable suggestion skipped: line range outside the PR's diff.
| useEffect(() => { | ||
| // 检查 MCP 是否启用 | ||
| const checkMcpStatus = async () => { | ||
| const enabled = await isMcpEnabled(); | ||
| setMcpEnabled(enabled); | ||
| console.log("[SideBar] MCP enabled:", enabled); | ||
| }; | ||
| checkMcpStatus(); | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console logs and add missing dependency.
The effect includes console logs and is missing mcpEnabled in its dependency array.
Apply this diff to improve the code:
useEffect(() => {
- // 检查 MCP 是否启用
const checkMcpStatus = async () => {
const enabled = await isMcpEnabled();
setMcpEnabled(enabled);
- console.log("[SideBar] MCP enabled:", enabled);
if (!enabled) {
navigate(Path.Home);
}
};
checkMcpStatus();
-}, [navigate]);
+}, [navigate, mcpEnabled]);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/mcp/actions.ts (1)
337-352: 🛠️ Refactor suggestionAdd request validation in
executeMcpActionIn
executeMcpAction, therequestobject is used without validation, which could lead to unexpected errors or security vulnerabilities. Consider validating the structure and content of therequestparameter to ensure it meets the expected format.Apply this change:
export async function executeMcpAction( clientId: string, request: McpRequestMessage, ) { try { + // Validate request structure + if (!request || typeof request !== 'object') { + throw new Error('Invalid request format'); + } + if (!request.action || typeof request.action !== 'string') { + throw new Error('Missing or invalid action in request'); + } + const client = clientsMap.get(clientId); if (!client?.client) { throw new Error(`Client ${clientId} not found`); } logger.info(`Executing request for [${clientId}]`); return await executeRequest(client.client, request); } catch (error) { logger.error(`Failed to execute request for [${clientId}]: ${error}`); throw error; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/components/mcp-market.module.scss(1 hunks)app/components/mcp-market.tsx(1 hunks)app/mcp/actions.ts(1 hunks)app/mcp/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/mcp/types.ts
🔇 Additional comments (6)
app/mcp/actions.ts (2)
355-363: Handle missing configuration file gracefullyIn
getMcpConfigFromFile, when the configuration file is missing (e.g., on the first run), it logs an error and returns the default configuration. Instead, consider checking if the error is due to the file not existing (ENOENT) and avoid logging an error in that case. This provides a smoother user experience.Apply this change:
} catch (error) { - logger.error(`Failed to load MCP config, using default config: ${error}`); - return DEFAULT_MCP_CONFIG; + if (error.code === 'ENOENT') { + logger.warn("MCP config file not found, using default configuration."); + return DEFAULT_MCP_CONFIG; + } else { + logger.error(`Failed to load MCP config: ${error}`); + throw error; + } }
366-372: Implement atomic file writes for configuration updatesWhen updating the MCP configuration file in
updateMcpConfig, there is a risk of file corruption if the write operation is interrupted. Consider using a temporary file and an atomic rename operation to ensure the configuration file remains consistent.Apply this change:
async function updateMcpConfig(config: McpConfigData): Promise<void> { try { + const tempPath = `${CONFIG_PATH}.tmp`; + await fs.writeFile(tempPath, JSON.stringify(config, null, 2)); + await fs.rename(tempPath, CONFIG_PATH); } catch (error) { + // Clean up temp file if it exists + try { + await fs.unlink(`${CONFIG_PATH}.tmp`); + } catch { + // Ignore cleanup errors + } throw error; } }app/components/mcp-market.module.scss (1)
269-274: Enhance focus visibility for better accessibilityThe focus styles on input elements can be improved to enhance keyboard navigation and accessibility. Consider adjusting the
box-shadowand addingoutline-offsetto make focused elements more noticeable.Apply this change:
&:focus { background-color: var(--white); border-color: var(--primary); outline: none; - box-shadow: 0 0 0 2px var(--primary-10); + box-shadow: 0 0 0 3px var(--primary-20); + outline: 2px solid transparent; + outline-offset: 2px; }app/components/mcp-market.tsx (3)
47-47: Replace 'any' with specific types inuserConfigUsing
anyreduces type safety and can lead to runtime errors. Specify the expected types to enhance type checking.Apply this change:
- const [userConfig, setUserConfig] = useState<Record<string, any>>({}); + const [userConfig, setUserConfig] = useState<Record<string, string | string[]>>({});
676-682: Improve accessibility of the search inputAdd ARIA attributes to the search input to enhance accessibility for users relying on assistive technologies.
Apply this change:
<input type="text" className={styles["search-bar"]} placeholder={"Search MCP Server"} autoFocus onInput={(e) => setSearchText(e.currentTarget.value)} + aria-label="Search MCP servers" + role="searchbox" />
691-714: Enhance modal accessibilityInclude appropriate ARIA attributes in the modal to improve accessibility for assistive technologies.
Apply this change:
<Modal title={`Configure Server - ${editingServerId}`} onClose={() => !isLoading && setEditingServerId(undefined)} + role="dialog" + aria-modal="true" + aria-labelledby="modal-title" actions={[ // Actions... ]} > + <h2 id="modal-title" className="sr-only"> + Configure Server - {editingServerId} + </h2> <List>{renderConfigForm()}</List> </Modal>
| export async function getAvailableClientsCount() { | ||
| let count = 0; | ||
| clientsMap.forEach((map) => !map.errorMsg && count++); | ||
| return count; | ||
| } |
There was a problem hiding this comment.
Ensure accurate count of available clients
In getAvailableClientsCount, clients with errorMsg set to null are counted as available. However, during initialization, errorMsg is null, but the client may not yet be fully initialized. To accurately count only active clients, consider checking if map.client is not null and errorMsg is null.
Apply this diff to fix the issue:
export async function getAvailableClientsCount() {
let count = 0;
- clientsMap.forEach((map) => !map.errorMsg && count++);
+ clientsMap.forEach((map) => map.client && !map.errorMsg && count++);
return count;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getAvailableClientsCount() { | |
| let count = 0; | |
| clientsMap.forEach((map) => !map.errorMsg && count++); | |
| return count; | |
| } | |
| export async function getAvailableClientsCount() { | |
| let count = 0; | |
| clientsMap.forEach((map) => map.client && !map.errorMsg && count++); | |
| return count; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/constant.ts (1)
277-392: Consider improving the system template structure and examples.While the template is comprehensive, there are a few areas for improvement:
- The example section mixes Chinese and English content
- The error examples contain excessive exclamation marks
- The formatting could be more consistent
Consider these improvements:
- follwing is the wrong! mcp json example: + Following are examples of incorrect MCP JSON format: - This is wrong because the method is not tools/call.!!!!!!!!!!! + This is incorrect because the method is not "tools/call". - please follow the format strictly ONLY use tools/call method!!!!!!!!!!! + Please strictly follow the format and ONLY use the "tools/call" method. - "content": "为什么数学书总是感到忧伤？因为它有太多的问题。" + "content": "Why do math books look sad? Because they have too many problems."app/store/chat.ts (1)
Line range hint
403-429: Consider adding validation for MCP response content.While the implementation handles MCP responses correctly, it would be beneficial to add validation for the response content.
Consider adding validation:
async onUserInput( content: string, attachImages?: string[], isMcpResponse?: boolean, ) { + if (isMcpResponse && !content.trim()) { + console.warn("[MCP] Empty response content"); + return; + } const session = get().currentSession(); const modelConfig = session.mask.modelConfig;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README_CN.md(3 hunks)app/components/chat.tsx(15 hunks)app/components/sidebar.tsx(6 hunks)app/config/server.ts(2 hunks)app/constant.ts(3 hunks)app/locales/cn.ts(1 hunks)app/locales/en.ts(1 hunks)app/store/chat.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- app/locales/en.ts
- README_CN.md
- app/locales/cn.ts
- app/config/server.ts
- app/components/sidebar.tsx
- app/components/chat.tsx
🔇 Additional comments (8)
app/constant.ts (3)
52-52: LGTM!The new path follows the existing naming convention and is appropriately placed in the enum.
94-94: LGTM!The new store key follows the existing naming convention and is appropriately placed in the enum.
270-275: LGTM!The template is well-structured and provides clear placeholders for dynamic content.
app/store/chat.ts (5)
Line range hint
1-39: LGTM!The imports are well-organized and include all necessary dependencies for the MCP functionality.
64-64: LGTM!The
isMcpResponseflag is appropriately added to the ChatMessage type.
201-220: Add error handling and validation in getMcpSystemPrompt.The implementation needs improvements in error handling and validation as previously suggested.
395-399: LGTM!The MCP JSON check is appropriately added to the message processing flow.
817-845: Address security concerns in MCP JSON handling.The implementation needs security improvements as previously suggested.
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
📝 补充信息 | Additional Information
Summary by CodeRabbit
Release Notes
New Features
Model Support
User Interface
Performance
Documentation
Chores
.gitignoreformcp_config.json.