fix(mcp-crud-ui): address greptile 3/5 review

- remove non-functional XIcon remove-server button (no onRemoveServer prop wired)
- fix stale closure in MCPAppsPanel auto-enable effect: use serversRef/selectedServersRef
- remove utility re-export from McpCrudPermissionPanel (classifyToolOp, groupToolsByCrud)
- remove redundant selectedTools.length === 0 guard (always true when !toolPermissions[id])
This commit is contained in:
Ishaan Jaffer 2026-03-11 18:39:51 -07:00
parent 1ba6ab71f1
commit 409c5f795f
3 changed files with 16 additions and 18 deletions

View file

@ -1,6 +1,6 @@
"use client";
import React, { useCallback, useEffect, useState } from "react";
import React, { useCallback, useEffect, useRef, useState } from "react";
import { Spin, Input, Button, Skeleton } from "antd";
import { SearchOutlined, ArrowLeftOutlined, RightOutlined, ToolOutlined, CheckCircleOutlined } from "@ant-design/icons";
import { deleteMCPOAuthUserCredential, fetchMCPServers, getMCPOAuthUserCredentialStatus, listMCPTools } from "../networking";
@ -99,6 +99,14 @@ const MCPAppsPanel: React.FC<Props> = ({ accessToken, selectedServers, onChange
// OAuth2 connect state — tracks which server_ids have a stored user credential
const [oauthConnected, setOauthConnected] = useState<Set<string>>(new Set());
// Refs keep the latest values for the auto-enable effect so it always reads
// the current servers/selectedServers without needing them as dependencies
// (which would cause the effect to fire on every render).
const serversRef = useRef<MCPServer[]>([]);
useEffect(() => { serversRef.current = servers; }, [servers]);
const selectedServersRef = useRef<string[]>(selectedServers);
useEffect(() => { selectedServersRef.current = selectedServers; }, [selectedServers]);
const nameOf = (s: MCPServer) => s.server_name ?? s.alias ?? s.server_id;
useEffect(() => {
@ -157,15 +165,17 @@ const MCPAppsPanel: React.FC<Props> = ({ accessToken, selectedServers, onChange
// Auto-enable oauth2 servers for the current chat session when a valid
// credential is detected (either on mount or after a fresh OAuth sign-in).
// Uses refs for servers/selectedServers to avoid stale closures without
// adding them as deps (which would cause the effect to re-fire on every render).
useEffect(() => {
if (oauthConnected.size === 0) return;
const namesToAdd = servers
.filter((s) => oauthConnected.has(s.server_id) && !selectedServers.includes(nameOf(s)))
const namesToAdd = serversRef.current
.filter((s) => oauthConnected.has(s.server_id) && !selectedServersRef.current.includes(nameOf(s)))
.map(nameOf);
if (namesToAdd.length > 0) {
onChange([...selectedServers, ...namesToAdd]);
onChange([...selectedServersRef.current, ...namesToAdd]);
}
}, [oauthConnected]); // eslint-disable-line react-hooks/exhaustive-deps
}, [oauthConnected, onChange]);
const handleToggle = async (serverName: string, checked: boolean, serverId?: string) => {
if (!checked) {

View file

@ -3,7 +3,6 @@ import { listMCPTools } from "../networking";
import { MCPTool, MCPServer } from "../mcp_tools/types";
import { Text } from "@tremor/react";
import { Spin, Radio } from "antd";
import { XIcon } from "lucide-react";
import { useMCPServers } from "../../app/(dashboard)/hooks/mcpServers/useMCPServers";
import McpCrudPermissionPanel from "../mcp_tools/McpCrudPermissionPanel";
import { classifyToolOp } from "../../utils/mcpToolCrudClassification";
@ -157,15 +156,6 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
</button>
</>
)}
<button
type="button"
className="text-gray-400 hover:text-gray-600"
onClick={() => {
// Handle remove server if needed
}}
>
<XIcon className="w-4 h-4" />
</button>
</div>
</div>
@ -191,7 +181,7 @@ const MCPToolPermissions: React.FC<MCPToolPermissionsProps> = ({
{!isLoading && !error && tools.length > 0 && viewMode === "crud" && (
<McpCrudPermissionPanel
tools={tools}
value={selectedTools.length === 0 && !toolPermissions[server.server_id] ? undefined : selectedTools}
value={!toolPermissions[server.server_id] ? undefined : selectedTools}
onChange={(allowed) => handleCrudPanelChange(server.server_id, allowed)}
readOnly={disabled}
/>

View file

@ -244,5 +244,3 @@ const McpCrudPermissionPanel: React.FC<McpCrudPermissionPanelProps> = ({
};
export default McpCrudPermissionPanel;
export { classifyToolOp, groupToolsByCrud };