app/components/mcp-market.tsx (1)

231-244: 🛠️ Refactor suggestion

Loading indicator never toggles during loadTools

loadTools fetches data asynchronously but never flips isLoading, so the modal may show “No tools available” before the request completes. A dedicated loading flag (or re-use isLoading) fixes the UX:

 const loadTools = async (id: string) => {
-  try {
+  try {
+    setIsLoading(true);
     const result = await getClientTools(id);
     if (result) {
-      setTools(result);
+      setToolsResponse(result);
     } else {
       throw new Error("Failed to load tools");
     }
   } catch (error) {
@@
-    setTools(null);
+    setToolsResponse(null);
   } finally {
+    setIsLoading(false);
   }
 };

app/mcp/client.ts (1)

27-29: Consider adding more context in the error message.

The error message when attempting to use stdio transport in export mode could be more helpful by explaining alternatives.

-      throw new Error("Cannot use stdio transport in export mode");
+      throw new Error("Cannot use stdio transport in export mode. Please use SSE transport configuration instead.");

app/components/mcp-market.tsx (2)

49-53: tools state now holds the whole response – consider renaming for clarity

tools is no longer an array but a ListToolsResponse object. The plural name can mis-lead future readers into thinking it is an array, and makes the later tools.tools access look odd.

-const [tools, setTools] = useState<ListToolsResponse | null>(null);
+const [toolsResponse, setToolsResponse] = useState<ListToolsResponse | null>(null);

A small rename avoids the double-tools accessor and improves readability.

---

735-744: Two minor list-rendering issues

1. index is used as key, risking re-ordering bugs; prefer a stable identifier (e.g. tool.name ?? index).
2. tool.name and tool.description are optional, so rendering may output empty <div>s. Guard or fall back to placeholders.

-  tools.tools.map((tool: ToolSchema, index: number) => (
-    <div key={index} className={styles["tool-item"]}>
+  toolsResponse.tools.map((tool: ToolSchema, index: number) => (
+    <div
+      key={tool.name ?? index}
+      className={styles["tool-item"]}
+    >
       <div className={styles["tool-name"]}>{tool.name ?? "Unnamed Tool"}</div>
       <div className={styles["tool-description"]}>
         {tool.description ?? "No description"}
       </div>
     </div>
   ))

app/mcp/types.ts (1)

71-76: inputSchema?: object is too loose

Using object loses type information and prevents validation. Prefer a more precise type:

inputSchema?: Record<string, unknown>; // or a Zod schema if available

This will still accept arbitrary JSON shapes while keeping unknown safety.

app/mcp/actions.ts (1)

24-29: Top-level await inside constant initialisation can break older Node builds

await at module scope requires ES 2022 modules; some bundlers/serverless targets still transpile to CommonJS. Compute CONFIG_PATH lazily inside an async helper to stay compatible.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/mcp/actions.ts (3)

1-3: "use server" directive won't work conditionally.

React/NextJS recognizes the directive only when it is the very first statement. Wrapping it in an if block makes it ineffective.

Consider using separate entry files or the webpack alias approach, or split into actions.client.ts & actions.server.ts with conditional imports.

---

347-354: Fallback client search uses O(n²) scan and fragile name matching.

1. Scanning all clients on every request is O(#clients × #tools).
2. Comparing t.name == request.params?.name assumes params.name exists and is string-typed.

Maintain a Map<toolName, clientId> during client initialization for O(1) lookup and add proper type guards.

---

369-393: 🛠️ Refactor suggestion

Incorrect merge logic between bundled config and localStorage override.

The loop iterates over config.mcpServers, so any additional servers present only in storedConfig are ignored.

Apply this fix:

-for (const mcpId in config.mcpServers) {
+for (const mcpId in storedConfig.mcpServers) {
  if (mcpId in mcpServers) {
    mcpServers[mcpId] = {
      ...mcpServers[mcpId],
      ...storedConfig.mcpServers[mcpId],
    };
  } else {
    mcpServers[mcpId] = storedConfig.mcpServers[mcpId];
  }
}

app/mcp/actions.ts (1)

417-419: Useless catch clause that only rethrows the error.

The catch block on line 417-419 simply rethrows the original error, which is redundant.

-  try {
    // 确保目录存在
    if (EXPORT_MODE) {
      const storage = localStorage;
      storage.setItem("McpConfig", JSON.stringify(config));
    } else {
      const fs = await import("fs/promises");
      const path = await import("path");
      await fs.mkdir(path.dirname(CONFIG_PATH), { recursive: true });
      await fs.writeFile(CONFIG_PATH, JSON.stringify(config, null, 2));
    }
-  } catch (error) {
-    throw error;
-  }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/mcp/actions.ts (3)

1-3: ⚠️ Potential issue

"use server" directive will not be effective inside the conditional block

React/NextJS directive "use server" must be the very first statement in a file to work properly. Wrapping it in an if block turns it into a regular string expression that won't be recognized by the compiler.

Consider one of these alternatives:

1. Use separate files (actions.client.ts and actions.server.ts) with conditional imports
2. Use build-time code transformation in your Webpack/Next.js configuration to inject the directive correctly

---

347-354: 🛠️ Refactor suggestion

Fallback client search uses O(n²) scan and fragile name matching

1. Scanning all clients on every request is O(#clients × #tools).
2. Comparing t.name == request.params?.name uses loose equality and assumes params.name exists.

Consider these improvements:

1. Maintain a Map<toolName, clientId> when clients are initialized for O(1) lookup.
2. Use strict equality (===) and proper type guards.

let client = clientsMap.get(clientId);
if (!client) {
-  client = [...clientsMap.values()].find(
-    (c) =>
-      c.tools?.tools &&
-      c.tools.tools.find((t) => t.name == request.params?.name),
-  );
+  // Use type guard to ensure params.name exists and is a string
+  const toolName = typeof request.params?.name === "string" ? request.params.name : null;
+  if (toolName) {
+    client = [...clientsMap.values()].find(
+      (c) =>
+        c.tools?.tools &&
+        c.tools.tools.some((t) => t.name === toolName),
+    );
+  }
}

---

369-393: ⚠️ Potential issue

Incorrect merge logic between bundled config and localStorage override

The loop iterates over config.mcpServers, so any additional servers present only in storedConfig are ignored.

Apply this fix to properly merge the configurations:

if (storedConfig_str) {
  const storedConfig: McpConfigData = JSON.parse(storedConfig_str);
-  const mcpServers = config.mcpServers;
-  if (storedConfig.mcpServers) {
-    for (const mcpId in config.mcpServers) {
-      if (mcpId in mcpServers) {
-        mcpServers[mcpId] = {
-          ...mcpServers[mcpId],
-          ...storedConfig.mcpServers[mcpId],
-        };
-      } else {
-        mcpServers[mcpId] = storedConfig.mcpServers[mcpId];
-      }
-    }
-  }
-
-  config.mcpServers = mcpServers;
+  if (storedConfig.mcpServers) {
+    const merged = { ...config.mcpServers };
+    for (const mcpId in storedConfig.mcpServers) {
+      merged[mcpId] = { 
+        ...merged[mcpId], 
+        ...storedConfig.mcpServers[mcpId] 
+      };
+    }
+    config.mcpServers = merged;
+  }
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/mcp/actions.ts (2)

1-3: ⚠️ Potential issue

The "use server" directive inside a conditional won't have the intended effect

This conditional directive won't be recognized by Next.js, as directives must be the first statement of a file. The compiler cannot evaluate the EXPORT_MODE condition at compilation time.

Consider using separate files (e.g., actions.server.ts and actions.client.ts) with appropriate directives at the top of each file, and re-export from a common entry point based on the build config.

---

349-356: ⚠️ Potential issue

Fallback client search uses inefficient lookup and lacks type safety

While adding a fallback mechanism is good, the implementation has two issues:

1. The lookup is inefficient, scanning all clients and tools on each request
2. The comparison t.name == request.params?.name assumes params.name exists and is string-typed

Consider implementing a tool-to-client mapping:

-let client = clientsMap.get(clientId);
-if (!client) {
-  client = [...clientsMap.values()].find(
-    (c) =>
-      c.tools?.tools &&
-      c.tools.tools.find((t) => t.name == request.params?.name),
-  );
-}
+let client = clientsMap.get(clientId);
+if (!client && request.params?.name && typeof request.params.name === 'string') {
+  // Use a tool-to-client mapping that's maintained when tools are initialized
+  const toolName = request.params.name;
+  client = [...clientsMap.values()].find(
+    (c) => c.tools?.tools && c.tools.tools.some(t => t.name === toolName)
+  );
+}

app/mcp/actions.ts (2)

371-392: Config merging logic is improved but could be refined

The logic for merging configurations has been improved compared to previous implementations. However, consider a more thorough approach to ensure no entries are missed.

-const merged = { ...config.mcpServers };
-if (storedConfig.mcpServers) {
-  for (const id in storedConfig.mcpServers) {
-    merged[id] = { ...merged[id], ...storedConfig.mcpServers[id] };
-  }
-}
+// Create a merged configuration that combines both sources
+const merged = { ...config.mcpServers };
+if (storedConfig.mcpServers) {
+  // Ensure we process all servers from stored config
+  for (const id in storedConfig.mcpServers) {
+    merged[id] = { ...merged[id], ...storedConfig.mcpServers[id] };
+  }
+}

---

422-424: Unnecessary catch clause

This catch clause simply rethrows the original error without any additional processing.

-try {
  // ... existing code ...
-} catch (error) {
-  throw error;
-}
+// ... existing code ...

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/mcp/actions.ts (2)

1-3: "use server" directive inside a conditional will not work as expected

The "use server" directive must be the very first statement in a file to be recognized by the NextJS compiler. Wrapping it in a conditional turns it into a regular string expression without any special meaning.

Consider splitting this into separate files:

• actions.server.ts with "use server" at the top
• actions.client.ts without the directive
• A common entry point that conditionally imports based on EXPORT_MODE

---

349-360: 🛠️ Refactor suggestion

Optimize client search with a tool-to-client mapping

The current implementation performs an O(n²) operation by iterating through all clients and their tools for each request that doesn't specify a clientId. This can become inefficient as the number of clients and tools grows.

Consider maintaining a Map<toolName, clientId> when tools are initialized:

// Add at file level
+ const toolToClientMap = new Map<string, string>();

// Update in places where tools are initialized (e.g., line 136)
  clientsMap.set(clientId, { client, tools, errorMsg: null });
+ if (tools?.tools) {
+   for (const tool of tools.tools) {
+     if (tool.name) {
+       toolToClientMap.set(tool.name, clientId);
+     }
+   }
+ }

// Then in executeMcpAction:
  if (
    !client &&
    request.params?.name &&
    typeof request.params.name === "string"
  ) {
    // Use a direct lookup instead of scanning all clients
+   const toolClientId = toolToClientMap.get(request.params.name);
+   if (toolClientId) {
+     client = clientsMap.get(toolClientId);
+   }
-   const toolName = request.params.name;
-   client = [...clientsMap.values()].find(
-     (c) => c.tools?.tools && c.tools.tools.some((t) => t.name == toolName),
-   );
  }

Don't forget to update the map when clients/tools are removed or changed.

app/mcp/actions.ts (2)

424-426: Small suggestion: Consider using a constant for the formatting configuration

The indentation value '2' is hardcoded in the JSON.stringify call. For consistency, consider extracting this to a constant.

+ const JSON_INDENT = 2;

// Then use it in both places where JSON is formatted
  await fs.writeFile(
    await getConfigPath(),
-   JSON.stringify(config, null, 2),
+   JSON.stringify(config, null, JSON_INDENT),
  );

---

428-430: Useless catch clause that only rethrows the error

The catch clause simply rethrows the original error without any additional processing, making it redundant.

  try {
    // ...code...
- } catch (error) {
-   throw error;
  }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/mcp/actions.ts (1)

1-3: ⚠️ Potential issue

The "use server" directive will be ineffective due to conditional wrapping

Next.js requires the "use server" directive to be the very first statement in a file to be recognized. Placing it inside a conditional statement effectively turns it into a no-op string expression.

Consider separating the client and server code into different files, such as actions.client.ts and actions.server.ts, and use dynamic imports to load the appropriate file based on EXPORT_MODE.

app/mcp/actions.ts (1)

263-269: Consider extracting duplicate mapping logic to a function

This code block duplicates the logic from lines 139-145 for populating the tool-to-client map.

+// Helper function to populate tool to client map
+function populateToolToClientMap(tools: any, clientId: string) {
+  if (tools?.tools) {
+    for (const tool of tools.tools) {
+      if (tool.name) {
+        toolToClientMap.set(tool.name, clientId);
+      }
+    }
+  }
+}

// In initializeSingleClient
-if (tools?.tools) {
-  for (const tool of tools.tools) {
-    if (tool.name) {
-      toolToClientMap.set(tool.name, clientId);
-    }
-  }
-}
+populateToolToClientMap(tools, clientId);

// In resumeMcpServer
-if (tools?.tools) {
-  for (const tool of tools.tools) {
-    if (tool.name) {
-      toolToClientMap.set(tool.name, clientId);
-    }
-  }
-}
+populateToolToClientMap(tools, clientId);

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/mcp/actions.base.ts (1)

109-153: initializeSingleClient resolves before the client is ready

The function is declared async yet it never awaits the createClient chain—it sets up a .then/.catch handler and returns immediately.
Callers like initializeMcpSystem therefore “await” a promise that is already resolved, giving a false sense of completion while clients are still starting in the background.

If the intent is fire-and-forget, mark the function void and drop async to avoid confusion.
Otherwise, return the inner promise to let callers truly wait:

-  createClient(clientId, serverConfig)
+  return createClient(clientId, serverConfig)
     .then(async (client) => { /* … */ })
     .catch((error) => { /* … */ });

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/mcp/actions.base.ts (5)

31-32: ⚠️ Potential issue

The toolToClientMap lacks cleanup when clients are removed or restarted.

When a client is removed, paused, or all clients are restarted, the entries in toolToClientMap are not cleaned up. This can lead to stale mappings that cause requests to be sent to non-existent clients.

Implement a helper function to purge tool mappings:

const toolToClientMap = new Map<string, string>();

+// Helper function to purge tool mappings for a specific client
+function purgeToolMappings(clientId: string) {
+  for (const [tool, mappedId] of toolToClientMap.entries()) {
+    if (mappedId === clientId) {
+      toolToClientMap.delete(tool);
+    }
+  }
+}

Then call this function in pauseMcpServer, removeMcpServer, and use toolToClientMap.clear() in restartAllClients.

---

388-418: 🛠️ Refactor suggestion

Improve localStorage error handling with nested try-catch blocks.

The current implementation falls back to the default config if either fetch fails or localStorage operations fail. It should preserve the successfully fetched config even if localStorage operations fail.

if (EXPORT_MODE) {
  const res = await fetch(await getConfigPath());
+  if (!res.ok) {
+    throw new Error(`Failed to fetch config: ${res.status} ${res.statusText}`);
+  }
  const config: McpConfigData = await res.json();
-  const storage = localStorage;
-  const storedConfig_str = storage.getItem("McpConfig");
-  if (storedConfig_str) {
-    const storedConfig: McpConfigData = JSON.parse(storedConfig_str);
+  try {
+    const storedConfigStr = localStorage.getItem("McpConfig");
+    if (storedConfigStr) {
+      try {
+        const storedConfig: McpConfigData = JSON.parse(storedConfigStr);
        // Create a merged configuration that combines both sources
        const merged = { ...config.mcpServers };
        if (storedConfig.mcpServers) {
          // Ensure we process all servers from stored config
          for (const id in storedConfig.mcpServers) {
            merged[id] = { ...merged[id], ...storedConfig.mcpServers[id] };
          }
        }
        config.mcpServers = merged;
+      } catch (parseError) {
+        logger.warn(`Failed to parse stored MCP config: ${parseError}`);
+        // Continue with default config
+      }
    }
+  } catch (storageError) {
+    logger.warn(`Failed to access localStorage: ${storageError}`);
+    // Continue with default config
+  }
  return config;

---

318-323: ⚠️ Potential issue

Add tool mapping cleanup when removing a server.

When a server is removed, the tool mappings for that client should be removed to prevent routing requests to a non-existent client.

clientsMap.delete(clientId);
+// Purge tool mappings for this client
+for (const [tool, mappedId] of toolToClientMap.entries()) {
+  if (mappedId === clientId) {
+    toolToClientMap.delete(tool);
+  }
+}

---

343-344: ⚠️ Potential issue

Clear tool-to-client mappings when restarting all clients.

When all clients are restarted, the tool-to-client mappings should be cleared to prevent routing requests to non-existent clients.

// 清空状态
clientsMap.clear();
+toolToClientMap.clear();

---

232-237: ⚠️ Potential issue

Add tool mapping cleanup when pausing a server.

When a server is paused, the tool mappings for that client should be removed to prevent routing requests to a non-existent client.

clientsMap.delete(clientId);
+// Purge tool mappings for this client
+for (const [tool, mappedId] of toolToClientMap.entries()) {
+  if (mappedId === clientId) {
+    toolToClientMap.delete(tool);
+  }
+}

app/mcp/actions.ts (2)

3-3: Consider adding type guard for consistent error handling.

The variable actionsHost is initialized with a type but no value. When accessing this variable before initialization, TypeScript's type checking won't catch potential issues.

-let actionsHost: typeof import("./actions.base");
+let actionsHost: typeof import("./actions.base") | undefined;

---

48-53: Add error handling for the dynamic import chain.

The current implementation doesn't handle potential failures in the dynamic import chain. Consider adding error handling to provide better diagnostics.

export const executeMcpAction = async (
  clientId: string,
  request: McpRequestMessage,
) => {
-  return (await actions()).executeMcpAction(clientId, request);
+  try {
+    return (await actions()).executeMcpAction(clientId, request);
+  } catch (error) {
+    console.error(`Failed to execute MCP action: ${error}`);
+    throw error;
+  }
};

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

app/mcp/actions.base.ts (1)

417-447: 🛠️ Refactor suggestion

Fragile localStorage error handling.

This implementation has the same issue that was flagged in a previous review. If localStorage is unavailable or the stored JSON is malformed, you'll lose the remote config data.

Apply the solution from the previous review to ensure resilient config handling:

      const res = await fetch(await getConfigPath());
      const config: McpConfigData = await res.json();
-      const storage = localStorage;
-      const storedConfig_str = storage.getItem("McpConfig");
-      if (storedConfig_str) {
-        const storedConfig: McpConfigData = JSON.parse(storedConfig_str);
+      try {
+        const storedConfigStr = localStorage.getItem("McpConfig");
+        if (storedConfigStr) {
+          const storedConfig: McpConfigData = JSON.parse(storedConfigStr);
           // merge logic …
-      }
-      return config;
+        }
+      } catch (lsError) {
+        logger.warn(`Ignoring invalid localStorage MCP config: ${lsError}`);
+      }
+      return config;   // always return the remote (possibly merged) config

app/mcp/actions.base.ts (1)

218-248: Potential issue with config status handling.

In the addMcpServer function, there's a check if (isNewServer && !config.status) that only sets default status for new servers. This might cause issues if an existing server's config is provided without a status.

Consider adding a default status for all servers without status, not just new ones:

-    // 如果是新服务器，设置默认状态为 active
-    if (isNewServer && !config.status) {
+    // 设置默认状态为 active（如果未指定）
+    if (!config.status) {
       config.status = "active";
     }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro