diff --git a/ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.test.ts b/ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.test.ts index be910acf7e..567e1d2301 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.test.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.test.ts @@ -38,7 +38,7 @@ describe("useMCPServerHealth", () => { vi.clearAllMocks(); }); - it("should fetch health status for given server IDs", async () => { + it("should fetch health status for all servers", async () => { const mockHealthStatuses = [ { server_id: "server-1", status: "healthy" }, { server_id: "server-2", status: "unhealthy" }, @@ -46,27 +46,6 @@ describe("useMCPServerHealth", () => { vi.mocked(networking.fetchMCPServerHealth).mockResolvedValue(mockHealthStatuses); - const { result } = renderHook(() => useMCPServerHealth(["server-1", "server-2"]), { - wrapper, - }); - - await waitFor(() => { - expect(result.current.isSuccess).toBe(true); - }); - - expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("test-token-123", ["server-1", "server-2"]); - expect(result.current.data).toEqual(mockHealthStatuses); - }); - - it("should fetch health status for all servers when no server IDs provided", async () => { - const mockHealthStatuses = [ - { server_id: "server-1", status: "healthy" }, - { server_id: "server-2", status: "healthy" }, - { server_id: "server-3", status: "unhealthy" }, - ]; - - vi.mocked(networking.fetchMCPServerHealth).mockResolvedValue(mockHealthStatuses); - const { result } = renderHook(() => useMCPServerHealth(), { wrapper, }); @@ -75,30 +54,15 @@ describe("useMCPServerHealth", () => { expect(result.current.isSuccess).toBe(true); }); - expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("test-token-123", undefined); + expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("test-token-123"); expect(result.current.data).toEqual(mockHealthStatuses); }); - it("should handle empty server list", async () => { - vi.mocked(networking.fetchMCPServerHealth).mockResolvedValue([]); - - const { result } = renderHook(() => useMCPServerHealth([]), { - wrapper, - }); - - await waitFor(() => { - expect(result.current.isSuccess).toBe(true); - }); - - expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("test-token-123", []); - expect(result.current.data).toEqual([]); - }); - it("should handle errors when fetching health status", async () => { const mockError = new Error("Failed to fetch health status"); vi.mocked(networking.fetchMCPServerHealth).mockRejectedValue(mockError); - const { result } = renderHook(() => useMCPServerHealth(["server-1"]), { + const { result } = renderHook(() => useMCPServerHealth(), { wrapper, }); @@ -116,7 +80,7 @@ describe("useMCPServerHealth", () => { accessToken: null, } as any); - const { result } = renderHook(() => useMCPServerHealth(["server-1"]), { + const { result } = renderHook(() => useMCPServerHealth(), { wrapper, }); @@ -124,4 +88,18 @@ describe("useMCPServerHealth", () => { expect(result.current.status).toBe("pending"); expect(networking.fetchMCPServerHealth).not.toHaveBeenCalled(); }); + + it("should use a stable query key that does not include server IDs", () => { + // Regression test: deleting a server used to pass a changing serverIds array into the + // hook, which was embedded in the query key. React Query would see a new key and fire + // a health check for every remaining server. + // + // The fix: the hook takes no serverIds parameter and uses a constant query key, so + // deleting (or adding) a server never causes an extra health check request. + // + // We verify the contract here by confirming the hook accepts no arguments. + // The stable-key behaviour is further exercised by mcp_servers.test.tsx. + const hookLength = useMCPServerHealth.length; + expect(hookLength).toBe(0); + }); }); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.ts b/ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.ts index 95d7f3bcee..f81ade047e 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/hooks/mcpServers/useMCPServerHealth.ts @@ -10,11 +10,11 @@ interface MCPServerHealth { status: string; } -export const useMCPServerHealth = (serverIds?: string[]) => { +export const useMCPServerHealth = () => { const { accessToken } = useAuthorized(); return useQuery({ - queryKey: [...mcpServerHealthKeys.lists(), { serverIds }], - queryFn: async () => await fetchMCPServerHealth(accessToken!, serverIds), + queryKey: mcpServerHealthKeys.lists(), + queryFn: async () => await fetchMCPServerHealth(accessToken!), enabled: !!accessToken, // Refetch health status every 30 seconds to keep it up to date refetchInterval: 30000, 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 8385fc7ac7..0ca3d69895 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 @@ -185,9 +185,10 @@ describe("MCPServers", () => { expect(getByText("MCP Servers")).toBeInTheDocument(); }); - // Verify the health check API was called with server IDs + // Verify the health check API was called (without a server ID filter — the hook always + // fetches health for all servers so the query key stays stable) await waitFor(() => { - expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("123", ["server-1", "server-2"]); + expect(networking.fetchMCPServerHealth).toHaveBeenCalledWith("123"); }); }); @@ -348,4 +349,86 @@ describe("MCPServers", () => { // Team B server should not be visible expect(screen.queryByText("Team B Server")).not.toBeInTheDocument(); }); + + it("should not trigger an extra health check when the server list changes after deletion", async () => { + // Regression test: previously useMCPServerHealth received serverIds derived from the + // server list. Deleting a server changed serverIds, which changed the React Query key, + // which caused a new health check request for every remaining server. + // + // Fix: useMCPServerHealth uses a stable, argument-free query key. The component + // re-rendering with a shorter server list must NOT produce a second health fetch. + const twoServers = [ + { + server_id: "server-1", + server_name: "Test Server 1", + alias: "test-server-1", + url: "https://example.com/mcp", + transport: "http", + auth_type: "none", + created_at: "2024-01-01T00:00:00Z", + created_by: "user-1", + updated_at: "2024-01-01T00:00:00Z", + updated_by: "user-1", + teams: [], + mcp_access_groups: [], + }, + { + server_id: "server-2", + server_name: "Test Server 2", + alias: "test-server-2", + url: "https://example2.com/mcp", + transport: "sse", + auth_type: "api_key", + created_at: "2024-01-02T00:00:00Z", + created_by: "user-2", + updated_at: "2024-01-02T00:00:00Z", + updated_by: "user-2", + teams: [], + mcp_access_groups: [], + }, + ]; + 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.fetchMCPServerHealth).mockResolvedValue([ + { server_id: "server-1", status: "healthy" }, + { server_id: "server-2", status: "healthy" }, + ]); + + // Use a shared queryClient with a non-zero gcTime so cached health data survives + // the re-render triggered by the server list refresh + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false, gcTime: 60_000 } }, + }); + + const { rerender } = render( + + + , + ); + + // Wait for the initial health fetch to complete + await waitFor(() => { + expect(networking.fetchMCPServerHealth).toHaveBeenCalledTimes(1); + }); + + // Simulate what happens after a server is deleted: the server list query is + // refetched (returns oneServer), causing the component to re-render with the + // shorter list. + await act(async () => { + await queryClient.invalidateQueries({ queryKey: ["mcpServers"] }); + }); + + rerender( + + + , + ); + + // The server list refresh must NOT trigger a second health check + expect(networking.fetchMCPServerHealth).toHaveBeenCalledTimes(1); + }); }); 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 0edf8c2a4c..26f5ab4df3 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_servers.tsx @@ -29,8 +29,7 @@ const MCPServers: React.FC = ({ accessToken, userRole, userID }) const { data: mcpServers, isLoading: isLoadingServers, refetch } = useMCPServers(); // Fetch health status for all servers - const serverIds = useMemo(() => mcpServers?.map((server) => server.server_id), [mcpServers]); - const { data: healthStatuses, isLoading: isLoadingHealth } = useMCPServerHealth(serverIds); + const { data: healthStatuses, isLoading: isLoadingHealth } = useMCPServerHealth(); // Merge health status data into servers const serversWithHealth = useMemo(() => {