From e35343537cced3f92ac012202d434af135231e2e Mon Sep 17 00:00:00 2001 From: Pulkit Sharma Date: Sat, 5 Apr 2025 01:46:57 +0530 Subject: [PATCH] Add proper support for progress flow during tool calling --- client/src/App.tsx | 44 +++++++++------ client/src/components/Sidebar.tsx | 2 +- client/src/components/ToolsTab.tsx | 31 +++++++++-- .../src/components/__tests__/Sidebar.test.tsx | 48 +++++++++++++++++ .../components/__tests__/ToolsTab.test.tsx | 54 ++++++++++++++++--- client/src/lib/configurationTypes.ts | 4 +- client/src/lib/constants.ts | 7 +-- .../hooks/__tests__/useConnection.test.tsx | 7 ++- client/src/lib/hooks/useConnection.ts | 42 ++++++++++----- client/src/utils/configUtils.ts | 4 +- package-lock.json | 22 ++++---- 11 files changed, 204 insertions(+), 61 deletions(-) diff --git a/client/src/App.tsx b/client/src/App.tsx index 61dcad7..dd7894f 100644 --- a/client/src/App.tsx +++ b/client/src/App.tsx @@ -411,21 +411,34 @@ const App = () => { }; const callTool = async (name: string, params: Record) => { - const response = await makeConnectionRequest( - { - method: "tools/call" as const, - params: { - name, - arguments: params, - _meta: { - progressToken: progressTokenRef.current++, + try { + const response = await makeConnectionRequest( + { + method: "tools/call" as const, + params: { + name, + arguments: params, + _meta: { + progressToken: progressTokenRef.current++, + }, }, }, - }, - CompatibilityCallToolResultSchema, - "tools", - ); - setToolResult(response); + CompatibilityCallToolResultSchema, + "tools", + ); + setToolResult(response); + } catch (e) { + const toolResult: CompatibilityCallToolResult = { + content: [ + { + type: "text", + text: (e as Error).message ?? String(e), + }, + ], + isError: true, + }; + setToolResult(toolResult); + } }; const handleRootsChange = async () => { @@ -633,9 +646,10 @@ const App = () => { setTools([]); setNextToolCursor(undefined); }} - callTool={(name, params) => { + callTool={async (name, params) => { clearError("tools"); - callTool(name, params); + setToolResult(null); + await callTool(name, params); }} selectedTool={selectedTool} setSelectedTool={(tool) => { diff --git a/client/src/components/Sidebar.tsx b/client/src/components/Sidebar.tsx index 19f8020..039453e 100644 --- a/client/src/components/Sidebar.tsx +++ b/client/src/components/Sidebar.tsx @@ -325,7 +325,7 @@ const Sidebar = ({ return (
-
diff --git a/client/src/components/__tests__/Sidebar.test.tsx b/client/src/components/__tests__/Sidebar.test.tsx index e527f6d..4d0e3f1 100644 --- a/client/src/components/__tests__/Sidebar.test.tsx +++ b/client/src/components/__tests__/Sidebar.test.tsx @@ -350,6 +350,54 @@ describe("Sidebar Environment Variables", () => { ); }); + it("should update MCP server proxy address", () => { + const setConfig = jest.fn(); + renderSidebar({ config: DEFAULT_INSPECTOR_CONFIG, setConfig }); + + openConfigSection(); + + const proxyAddressInput = screen.getByTestId( + "MCP_PROXY_FULL_ADDRESS-input", + ); + fireEvent.change(proxyAddressInput, { + target: { value: "http://localhost:8080" }, + }); + + expect(setConfig).toHaveBeenCalledWith( + expect.objectContaining({ + MCP_PROXY_FULL_ADDRESS: { + description: + "Set this if you are running the MCP Inspector Proxy on a non-default address. Example: http://10.1.1.22:5577", + value: "http://localhost:8080", + }, + }), + ); + }); + + it("should update max total timeout", () => { + const setConfig = jest.fn(); + renderSidebar({ config: DEFAULT_INSPECTOR_CONFIG, setConfig }); + + openConfigSection(); + + const maxTotalTimeoutInput = screen.getByTestId( + "MCP_REQUEST_MAX_TOTAL_TIMEOUT-input", + ); + fireEvent.change(maxTotalTimeoutInput, { + target: { value: "10000" }, + }); + + expect(setConfig).toHaveBeenCalledWith( + expect.objectContaining({ + MCP_REQUEST_MAX_TOTAL_TIMEOUT: { + description: + "Maximum total timeout for requests sent to the MCP server (ms) (Use with progress notifications)", + value: 10000, + }, + }), + ); + }); + it("should handle invalid timeout values entered by user", () => { const setConfig = jest.fn(); renderSidebar({ config: DEFAULT_INSPECTOR_CONFIG, setConfig }); diff --git a/client/src/components/__tests__/ToolsTab.test.tsx b/client/src/components/__tests__/ToolsTab.test.tsx index 349977a..c46a32f 100644 --- a/client/src/components/__tests__/ToolsTab.test.tsx +++ b/client/src/components/__tests__/ToolsTab.test.tsx @@ -1,4 +1,4 @@ -import { render, screen, fireEvent } from "@testing-library/react"; +import { render, screen, fireEvent, act } from "@testing-library/react"; import { describe, it, expect, jest } from "@jest/globals"; import "@testing-library/jest-dom"; import ToolsTab from "../ToolsTab"; @@ -43,7 +43,7 @@ describe("ToolsTab", () => { tools: mockTools, listTools: jest.fn(), clearTools: jest.fn(), - callTool: jest.fn(), + callTool: jest.fn(async () => {}), selectedTool: null, setSelectedTool: jest.fn(), toolResult: null, @@ -59,14 +59,16 @@ describe("ToolsTab", () => { ); }; - it("should reset input values when switching tools", () => { + it("should reset input values when switching tools", async () => { const { rerender } = renderToolsTab({ selectedTool: mockTools[0], }); // Enter a value in the first tool's input const input = screen.getByRole("spinbutton") as HTMLInputElement; - fireEvent.change(input, { target: { value: "42" } }); + await act(async () => { + fireEvent.change(input, { target: { value: "42" } }); + }); expect(input.value).toBe("42"); // Switch to second tool @@ -80,7 +82,8 @@ describe("ToolsTab", () => { const newInput = screen.getByRole("spinbutton") as HTMLInputElement; expect(newInput.value).toBe(""); }); - it("should handle integer type inputs", () => { + + it("should handle integer type inputs", async () => { renderToolsTab({ selectedTool: mockTools[1], // Use the tool with integer type }); @@ -93,10 +96,49 @@ describe("ToolsTab", () => { expect(input.value).toBe("42"); const submitButton = screen.getByRole("button", { name: /run tool/i }); - fireEvent.click(submitButton); + await act(async () => { + fireEvent.click(submitButton); + }); expect(defaultProps.callTool).toHaveBeenCalledWith(mockTools[1].name, { count: 42, }); }); + + it("should disable button and change text while tool is running", async () => { + // Create a promise that we can resolve later + let resolvePromise: ((value: unknown) => void) | undefined; + const mockPromise = new Promise((resolve) => { + resolvePromise = resolve; + }); + + // Mock callTool to return our promise + const mockCallTool = jest.fn().mockReturnValue(mockPromise); + + renderToolsTab({ + selectedTool: mockTools[0], + callTool: mockCallTool, + }); + + const submitButton = screen.getByRole("button", { name: /run tool/i }); + expect(submitButton.getAttribute("disabled")).toBeNull(); + + // Click the button and verify immediate state changes + await act(async () => { + fireEvent.click(submitButton); + }); + + // Verify button is disabled and text changed + expect(submitButton.getAttribute("disabled")).not.toBeNull(); + expect(submitButton.textContent).toBe("Running..."); + + // Resolve the promise to simulate tool completion + await act(async () => { + if (resolvePromise) { + await resolvePromise({}); + } + }); + + expect(submitButton.getAttribute("disabled")).toBeNull(); + }); }); diff --git a/client/src/lib/configurationTypes.ts b/client/src/lib/configurationTypes.ts index d0c1263..b4dd86e 100644 --- a/client/src/lib/configurationTypes.ts +++ b/client/src/lib/configurationTypes.ts @@ -20,13 +20,13 @@ export type InspectorConfig = { * Whether to reset the timeout on progress notifications. Useful for long-running operations that send periodic progress updates. * Refer: https://spec.modelcontextprotocol.io/specification/2025-03-26/basic/utilities/progress/#progress-flow */ - MCP_SERVER_REQUEST_TIMEOUT_RESET_ON_PROGRESS: ConfigItem; + MCP_REQUEST_TIMEOUT_RESET_ON_PROGRESS: ConfigItem; /** * Maximum total time in milliseconds to wait for a response from the MCP server before timing out. Used in conjunction with MCP_SERVER_REQUEST_TIMEOUT_RESET_ON_PROGRESS. * Refer: https://spec.modelcontextprotocol.io/specification/2025-03-26/basic/utilities/progress/#progress-flow */ - MCP_SERVER_REQUEST_TIMEOUT_MAX_TOTAL_TIMEOUT: ConfigItem; + MCP_REQUEST_MAX_TOTAL_TIMEOUT: ConfigItem; /** * The full address of the MCP Proxy Server, in case it is running on a non-default address. Example: http://10.1.1.22:5577 diff --git a/client/src/lib/constants.ts b/client/src/lib/constants.ts index 9caf4bc..f594827 100644 --- a/client/src/lib/constants.ts +++ b/client/src/lib/constants.ts @@ -25,12 +25,13 @@ export const DEFAULT_INSPECTOR_CONFIG: InspectorConfig = { description: "Timeout for requests to the MCP server (ms)", value: 10000, }, - MCP_SERVER_REQUEST_TIMEOUT_RESET_ON_PROGRESS: { + MCP_REQUEST_TIMEOUT_RESET_ON_PROGRESS: { description: "Reset timeout on progress notifications", value: true, }, - MCP_SERVER_REQUEST_TIMEOUT_MAX_TOTAL_TIMEOUT: { - description: "Maximum total timeout for requests sent to the MCP server (ms)", + MCP_REQUEST_MAX_TOTAL_TIMEOUT: { + description: + "Maximum total timeout for requests sent to the MCP server (ms) (Use with progress notifications)", value: 60000, }, MCP_PROXY_FULL_ADDRESS: { diff --git a/client/src/lib/hooks/__tests__/useConnection.test.tsx b/client/src/lib/hooks/__tests__/useConnection.test.tsx index 7a96802..1671b70 100644 --- a/client/src/lib/hooks/__tests__/useConnection.test.tsx +++ b/client/src/lib/hooks/__tests__/useConnection.test.tsx @@ -95,11 +95,10 @@ describe("useConnection", () => { expect.objectContaining({ timeout: DEFAULT_INSPECTOR_CONFIG.MCP_SERVER_REQUEST_TIMEOUT.value, maxTotalTimeout: - DEFAULT_INSPECTOR_CONFIG - .MCP_SERVER_REQUEST_TIMEOUT_MAX_TOTAL_TIMEOUT.value, + DEFAULT_INSPECTOR_CONFIG.MCP_REQUEST_MAX_TOTAL_TIMEOUT.value, resetTimeoutOnProgress: - DEFAULT_INSPECTOR_CONFIG - .MCP_SERVER_REQUEST_TIMEOUT_RESET_ON_PROGRESS.value, + DEFAULT_INSPECTOR_CONFIG.MCP_REQUEST_TIMEOUT_RESET_ON_PROGRESS + .value, }), ); }); diff --git a/client/src/lib/hooks/useConnection.ts b/client/src/lib/hooks/useConnection.ts index d67c623..ac4605c 100644 --- a/client/src/lib/hooks/useConnection.ts +++ b/client/src/lib/hooks/useConnection.ts @@ -8,7 +8,6 @@ import { ClientRequest, CreateMessageRequestSchema, ListRootsRequestSchema, - ProgressNotificationSchema, ResourceUpdatedNotificationSchema, LoggingMessageNotificationSchema, Request, @@ -23,6 +22,7 @@ import { ResourceListChangedNotificationSchema, ToolListChangedNotificationSchema, PromptListChangedNotificationSchema, + Progress, } from "@modelcontextprotocol/sdk/types.js"; import { RequestOptions } from "@modelcontextprotocol/sdk/shared/protocol.js"; import { useState } from "react"; @@ -99,21 +99,38 @@ export function useConnection({ if (!mcpClient) { throw new Error("MCP client not connected"); } - try { const abortController = new AbortController(); + + // prepare MCP Client request options + const mcpRequestOptions: RequestOptions = { + signal: options?.signal ?? abortController.signal, + resetTimeoutOnProgress: + options?.resetTimeoutOnProgress ?? + resetRequestTimeoutOnProgress(config), + timeout: options?.timeout ?? getMCPServerRequestTimeout(config), + maxTotalTimeout: + options?.maxTotalTimeout ?? + getMCPServerRequestMaxTotalTimeout(config), + }; + + // If progress notifications are enabled, add an onprogress hook to the MCP Client request options + // This is required by SDK to reset the timeout on progress notifications + if (mcpRequestOptions.resetTimeoutOnProgress) { + mcpRequestOptions.onprogress = (params: Progress) => { + // Add progress notification to `Server Notification` window in the UI + if (onNotification) { + onNotification({ + method: "notification/progress", + params, + }); + } + }; + } + let response; try { - response = await mcpClient.request(request, schema, { - signal: options?.signal ?? abortController.signal, - resetTimeoutOnProgress: - options?.resetTimeoutOnProgress ?? - resetRequestTimeoutOnProgress(config), - timeout: options?.timeout ?? getMCPServerRequestTimeout(config), - maxTotalTimeout: - options?.maxTotalTimeout ?? - getMCPServerRequestMaxTotalTimeout(config), - }); + response = await mcpClient.request(request, schema, mcpRequestOptions); pushHistory(request, response); } catch (error) { @@ -291,7 +308,6 @@ export function useConnection({ if (onNotification) { [ CancelledNotificationSchema, - ProgressNotificationSchema, LoggingMessageNotificationSchema, ResourceUpdatedNotificationSchema, ResourceListChangedNotificationSchema, diff --git a/client/src/utils/configUtils.ts b/client/src/utils/configUtils.ts index 577e86a..86e1643 100644 --- a/client/src/utils/configUtils.ts +++ b/client/src/utils/configUtils.ts @@ -16,11 +16,11 @@ export const getMCPServerRequestTimeout = (config: InspectorConfig): number => { export const resetRequestTimeoutOnProgress = ( config: InspectorConfig, ): boolean => { - return config.MCP_SERVER_REQUEST_TIMEOUT_RESET_ON_PROGRESS.value as boolean; + return config.MCP_REQUEST_TIMEOUT_RESET_ON_PROGRESS.value as boolean; }; export const getMCPServerRequestMaxTotalTimeout = ( config: InspectorConfig, ): number => { - return config.MCP_SERVER_REQUEST_TIMEOUT_MAX_TOTAL_TIMEOUT.value as number; + return config.MCP_REQUEST_MAX_TOTAL_TIMEOUT.value as number; }; diff --git a/package-lock.json b/package-lock.json index e626474..1f0b1a0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,20 +1,20 @@ { "name": "@modelcontextprotocol/inspector", - "version": "0.8.0", + "version": "0.8.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@modelcontextprotocol/inspector", - "version": "0.8.0", + "version": "0.8.1", "license": "MIT", "workspaces": [ "client", "server" ], "dependencies": { - "@modelcontextprotocol/inspector-client": "^0.8.0", - "@modelcontextprotocol/inspector-server": "^0.8.0", + "@modelcontextprotocol/inspector-client": "^0.8.1", + "@modelcontextprotocol/inspector-server": "^0.8.1", "concurrently": "^9.0.1", "shell-quote": "^1.8.2", "spawn-rx": "^5.1.2", @@ -32,10 +32,10 @@ }, "client": { "name": "@modelcontextprotocol/inspector-client", - "version": "0.8.0", + "version": "0.8.1", "license": "MIT", "dependencies": { - "@modelcontextprotocol/sdk": "^1.6.1", + "@modelcontextprotocol/sdk": "^1.8.0", "@radix-ui/react-checkbox": "^1.1.4", "@radix-ui/react-dialog": "^1.1.3", "@radix-ui/react-icons": "^1.3.0", @@ -1329,11 +1329,13 @@ "link": true }, "node_modules/@modelcontextprotocol/sdk": { - "version": "1.6.1", - "license": "MIT", + "version": "1.8.0", + "resolved": "https://registry.npmjs.org/@modelcontextprotocol/sdk/-/sdk-1.8.0.tgz", + "integrity": "sha512-e06W7SwrontJDHwCawNO5SGxG+nU9AAx+jpHHZqGl/WrDBdWOpvirC+s58VpJTB5QemI4jTRcjWT4Pt3Q1NPQQ==", "dependencies": { "content-type": "^1.0.5", "cors": "^2.8.5", + "cross-spawn": "^7.0.3", "eventsource": "^3.0.2", "express": "^5.0.1", "express-rate-limit": "^7.5.0", @@ -9483,10 +9485,10 @@ }, "server": { "name": "@modelcontextprotocol/inspector-server", - "version": "0.8.0", + "version": "0.8.1", "license": "MIT", "dependencies": { - "@modelcontextprotocol/sdk": "^1.6.1", + "@modelcontextprotocol/sdk": "^1.8.0", "cors": "^2.8.5", "express": "^4.21.0", "ws": "^8.18.0",