From 67e81f794f1737243eea20a20bcf056e750953b4 Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Thu, 4 Jun 2026 16:00:42 +0000 Subject: [PATCH] fix(ui): move MCP env-var loading to React Query to drop lint suppressions The frontend-lint baseline this branch had grown carried three react-hooks/set-state-in-effect entries and four raw-fetch entries that were added rather than fixed. Load the per-user env-var data in UserEnvVarsModal and mcp_servers through React Query (useQuery/useMutation) so the setState-in-effect findings go away instead of being baselined, and capture the ?fill_env_vars deep link in lazy initial state so the modal target is derived during render rather than set from an effect Delete the unused clearMCPUserEnvVars wrapper, the one new networking fetch with no caller. The remaining three wrappers (GET status, GET server vars, POST save) still need a raw fetch because networking.tsx is the only HTTP layer and React Query consumes it, so they stay grandfathered; the baseline only ratchets down: UserEnvVarsModal 1->0, mcp_servers set-state 4->2, networking fetch 274->273 --- ui/litellm-dashboard/eslint-suppressions.json | 9 +-- .../components/mcp_tools/UserEnvVarsModal.tsx | 76 ++++++++---------- .../components/mcp_tools/mcp_servers.test.tsx | 11 +-- .../src/components/mcp_tools/mcp_servers.tsx | 78 +++++++++---------- .../src/components/networking.tsx | 14 ---- 5 files changed, 73 insertions(+), 115 deletions(-) diff --git a/ui/litellm-dashboard/eslint-suppressions.json b/ui/litellm-dashboard/eslint-suppressions.json index d0a0a8f66f5..b1b8963ecc8 100644 --- a/ui/litellm-dashboard/eslint-suppressions.json +++ b/ui/litellm-dashboard/eslint-suppressions.json @@ -1307,11 +1307,6 @@ "count": 1 } }, - "src/components/mcp_tools/UserEnvVarsModal.tsx": { - "react-hooks/set-state-in-effect": { - "count": 1 - } - }, "src/components/mcp_tools/create_mcp_server.tsx": { "no-restricted-imports": { "count": 1 @@ -1382,7 +1377,7 @@ "count": 1 }, "react-hooks/set-state-in-effect": { - "count": 4 + "count": 2 } }, "src/components/mcp_tools/mcp_tool_configuration.tsx": { @@ -1509,7 +1504,7 @@ "count": 23 }, "no-restricted-syntax": { - "count": 274 + "count": 273 } }, "src/components/object_permissions_view.tsx": { diff --git a/ui/litellm-dashboard/src/components/mcp_tools/UserEnvVarsModal.tsx b/ui/litellm-dashboard/src/components/mcp_tools/UserEnvVarsModal.tsx index 4d27bf9aa17..08a285cd56b 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/UserEnvVarsModal.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/UserEnvVarsModal.tsx @@ -1,5 +1,6 @@ -import React, { useEffect, useState } from "react"; +import React from "react"; import { Modal, Form, Input, Button, Alert, Spin, Tag, Typography } from "antd"; +import { useMutation, useQuery } from "@tanstack/react-query"; import { MCPServer, MCPUserEnvVarsStatus } from "./types"; import { getMCPUserEnvVars, storeMCPUserEnvVars } from "../networking"; import NotificationsManager from "../molecules/notifications_manager"; @@ -23,59 +24,41 @@ interface UserEnvVarsModalProps { */ const UserEnvVarsModal: React.FC = ({ server, open, accessToken, onClose, onSaved }) => { const [form] = Form.useForm(); - const [status, setStatus] = useState(null); - const [isLoading, setIsLoading] = useState(false); - const [isSaving, setIsSaving] = useState(false); - useEffect(() => { - if (!open || !server || !accessToken) { - return; - } - let cancelled = false; - setIsLoading(true); - (async () => { - try { - const fetched = await getMCPUserEnvVars(accessToken, server.server_id); - if (cancelled) return; - setStatus(fetched); - form.resetFields(); - } catch (err) { - if (!cancelled) { - NotificationsManager.fromBackend( - `Failed to load env vars: ${err instanceof Error ? err.message : String(err)}`, - ); - } - } finally { - if (!cancelled) setIsLoading(false); - } - })(); - return () => { - cancelled = true; - }; - }, [open, server, accessToken, form]); + const { + data: status, + isLoading, + isError, + } = useQuery({ + queryKey: ["mcpUserEnvVars", server?.server_id], + queryFn: () => getMCPUserEnvVars(accessToken!, server!.server_id), + enabled: open && !!server && !!accessToken, + }); - const handleSave = async (values: Record) => { - if (!server || !accessToken) return; - setIsSaving(true); - try { - const trimmed: Record = {}; - for (const [k, v] of Object.entries(values)) { - trimmed[k] = (v ?? "").trim(); - } - const saved = await storeMCPUserEnvVars(accessToken, server.server_id, trimmed); - setStatus(saved); + const saveMutation = useMutation({ + mutationFn: (values: Record) => storeMCPUserEnvVars(accessToken!, server!.server_id, values), + onSuccess: (saved) => { NotificationsManager.success("Credentials saved"); - if (onSaved) onSaved(saved); + onSaved?.(saved); onClose(); - } catch (err) { + }, + onError: (err) => { NotificationsManager.fromBackend(`Failed to save env vars: ${err instanceof Error ? err.message : String(err)}`); - } finally { - setIsSaving(false); + }, + }); + + const handleSave = (values: Record) => { + if (!server || !accessToken) return; + const trimmed: Record = {}; + for (const [k, v] of Object.entries(values)) { + trimmed[k] = (v ?? "").trim(); } + saveMutation.mutate(trimmed); }; const displayName = server?.server_name || server?.alias || server?.server_id || "MCP Server"; const required = status?.required ?? []; + const isSaving = saveMutation.isPending; return ( = ({ server, open, acces footer={null} width={520} destroyOnHidden + afterOpenChange={(opened) => { + if (opened) form.resetFields(); + }} title={
@@ -103,6 +89,8 @@ const UserEnvVarsModal: React.FC = ({ server, open, acces
+ ) : isError ? ( + ) : required.length === 0 ? ( ) : ( diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.test.tsx index fc1d941c177..446fdd8c22d 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.test.tsx @@ -15,6 +15,7 @@ vi.mock("../networking", () => ({ getGeneralSettingsCall: vi.fn().mockResolvedValue([]), updateConfigFieldSetting: vi.fn().mockResolvedValue(undefined), deleteConfigFieldSetting: vi.fn().mockResolvedValue(undefined), + listMCPUserEnvVarStatus: vi.fn().mockResolvedValue([]), })); // Mock NotificationsManager @@ -213,7 +214,7 @@ describe("MCPServers", () => { vi.mocked(networking.fetchMCPServers).mockResolvedValue(mockServers); // Mock health check to never resolve (to test loading state) vi.mocked(networking.fetchMCPServerHealth).mockImplementation( - () => new Promise(() => { }), // Never resolves + () => new Promise(() => {}), // Never resolves ); const queryClient = createQueryClient(); @@ -330,9 +331,7 @@ describe("MCPServers", () => { // Find and click on "Team A" option const dropdownOptions = document.querySelectorAll(".ant-select-item-option"); - const teamAOption = Array.from(dropdownOptions).find((option) => - option.textContent?.includes("Team A"), - ); + const teamAOption = Array.from(dropdownOptions).find((option) => option.textContent?.includes("Team A")); expect(teamAOption).toBeTruthy(); act(() => { @@ -390,9 +389,7 @@ describe("MCPServers", () => { const oneServer = twoServers.slice(0, 1); // First call returns two servers; second (after deletion) returns one - vi.mocked(networking.fetchMCPServers) - .mockResolvedValueOnce(twoServers) - .mockResolvedValueOnce(oneServer); + vi.mocked(networking.fetchMCPServers).mockResolvedValueOnce(twoServers).mockResolvedValueOnce(oneServer); vi.mocked(networking.fetchMCPServerHealth).mockResolvedValue([ { server_id: "server-1", status: "healthy" }, { server_id: "server-2", status: "healthy" }, diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.tsx index 23d9ff142f5..4b383f701cb 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.tsx @@ -4,6 +4,7 @@ import { Button, Tab, TabGroup, TabList, TabPanel, TabPanels, Text, Title } from import NewBadge from "../common_components/NewBadge"; import { Descriptions, Empty, Input, Modal, Select, Spin, Tooltip, Typography } from "antd"; import React, { useEffect, useState, useMemo, useCallback } from "react"; +import { useQuery } from "@tanstack/react-query"; import { useMCPServers } from "../../app/(dashboard)/hooks/mcpServers/useMCPServers"; import { useMCPServerHealth } from "../../app/(dashboard)/hooks/mcpServers/useMCPServerHealth"; import NotificationsManager from "../molecules/notifications_manager"; @@ -112,63 +113,51 @@ const MCPServers: React.FC = ({ accessToken, userRole, userID }) const [prefillData, setPrefillData] = useState(null); const [isDeletingServer, setIsDeletingServer] = useState(false); const [byokModalServer, setByokModalServer] = useState(null); - // Per-user env-var fill modal target + bulk status across accessible servers. + // Per-user env-var fill modal target + deep-link source captured once from the URL. const [envVarsModalServer, setEnvVarsModalServer] = useState(null); - const [envVarStatusByServer, setEnvVarStatusByServer] = useState>({}); + const [deepLinkServerId, setDeepLinkServerId] = useState(() => + typeof window === "undefined" ? null : new URLSearchParams(window.location.search).get("fill_env_vars"), + ); const [searchQuery, setSearchQuery] = useState(""); const [sortKey, setSortKey] = useState("created_desc"); const isInternalUser = userRole === "Internal User"; // Single bulk fetch of this user's per-server env-var status. Drives the // red "N user fields missing" footer on each card with no per-row request. - const refetchEnvVarStatus = useCallback(async () => { - if (!accessToken) { - setEnvVarStatusByServer({}); - return; - } - try { - const statuses = await listMCPUserEnvVarStatus(accessToken); - const map: Record = {}; - for (const s of statuses) { - map[s.server_id] = s; - } - setEnvVarStatusByServer(map); - } catch (err) { - console.warn("Failed to load MCP env-var status", err); - } - }, [accessToken]); - - useEffect(() => { - refetchEnvVarStatus(); - }, [refetchEnvVarStatus, mcpServers]); + const { data: envVarStatuses, refetch: refetchEnvVarStatus } = useQuery({ + queryKey: ["mcpUserEnvVarStatus"], + queryFn: () => listMCPUserEnvVarStatus(accessToken!), + enabled: !!accessToken, + }); // Per-server list of per-user fields this user still needs to fill in. const missingFieldsByServer = useMemo(() => { const map: Record = {}; - for (const [serverId, status] of Object.entries(envVarStatusByServer)) { - map[serverId] = (status.required ?? []).filter((spec) => !spec.is_set).map((spec) => spec.name); + for (const status of envVarStatuses ?? []) { + map[status.server_id] = (status.required ?? []).filter((spec) => !spec.is_set).map((spec) => spec.name); } return map; - }, [envVarStatusByServer]); + }, [envVarStatuses]); // Deep-link via ?fill_env_vars= — the link users follow from the - // friendly error the proxy returns when a per-user var is missing. Opens the - // fill modal for the matching server, then strips the param. + // friendly error the proxy returns when a per-user var is missing. The id is + // captured into state above and resolved to a server below; here we only strip + // the param so a refresh doesn't reopen the modal. useEffect(() => { - if (typeof window === "undefined") return; - if (!serversWithHealth || serversWithHealth.length === 0) return; + if (!deepLinkServerId || typeof window === "undefined") return; const params = new URLSearchParams(window.location.search); - const targetId = params.get("fill_env_vars"); - if (!targetId) return; - const match = serversWithHealth.find((s) => s.server_id === targetId); - if (match) { - setEnvVarsModalServer(match); - params.delete("fill_env_vars"); - const newSearch = params.toString(); - const newUrl = window.location.pathname + (newSearch ? `?${newSearch}` : "") + window.location.hash; - window.history.replaceState({}, "", newUrl); - } - }, [serversWithHealth]); + if (!params.has("fill_env_vars")) return; + params.delete("fill_env_vars"); + const newSearch = params.toString(); + const newUrl = window.location.pathname + (newSearch ? `?${newSearch}` : "") + window.location.hash; + window.history.replaceState({}, "", newUrl); + }, [deepLinkServerId]); + + const deepLinkServer = useMemo( + () => (deepLinkServerId ? serversWithHealth.find((s) => s.server_id === deepLinkServerId) ?? null : null), + [deepLinkServerId, serversWithHealth], + ); + const activeEnvVarsServer = envVarsModalServer ?? deepLinkServer; useEffect(() => { if (typeof window === "undefined") { @@ -653,10 +642,13 @@ const MCPServers: React.FC = ({ accessToken, userRole, userID }) {/* Per-user env-var fill modal — backed by /v1/mcp/server/{id}/user-env-vars */} setEnvVarsModalServer(null)} + onClose={() => { + setEnvVarsModalServer(null); + setDeepLinkServerId(null); + }} onSaved={() => { // Refresh the bulk status so the red "N user fields missing" footer // on each card clears once the user has filled in their values. diff --git a/ui/litellm-dashboard/src/components/networking.tsx b/ui/litellm-dashboard/src/components/networking.tsx index 7d08c1ce5e5..b16f166c7ca 100644 --- a/ui/litellm-dashboard/src/components/networking.tsx +++ b/ui/litellm-dashboard/src/components/networking.tsx @@ -10124,20 +10124,6 @@ export const storeMCPUserEnvVars = async ( return response.json(); }; -export const clearMCPUserEnvVars = async (accessToken: string, serverId: string): Promise => { - const url = proxyBaseUrl - ? `${proxyBaseUrl}/v1/mcp/server/${serverId}/user-env-vars` - : `/v1/mcp/server/${serverId}/user-env-vars`; - const response = await fetch(url, { - method: "DELETE", - headers: { [globalLitellmHeaderName]: `Bearer ${accessToken}` }, - }); - if (!response.ok) { - throw new Error("Failed to clear env vars"); - } - return response.json(); -}; - export const listMCPUserEnvVarStatus = async (accessToken: string): Promise => { const url = proxyBaseUrl ? `${proxyBaseUrl}/v1/mcp/user-env-vars/status` : `/v1/mcp/user-env-vars/status`; const response = await fetch(url, {