diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 674a106ec..f8c2bcd35 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1,4 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { ErrorCode, McpError } from "@modelcontextprotocol/sdk/types.js"; +import { UrlElicitationLoopError } from "@inspector/core/mcp/urlElicitation.js"; import { renderWithMantine, screen, @@ -720,6 +722,61 @@ describe("App task wiring", () => { ); }); + it("shows a URL-elicitation toast when a tool call fails with a no-list -32042", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + ( + clientInstances[0] as unknown as { + callTool: ReturnType; + } + ).callTool.mockRejectedValueOnce( + new McpError( + ErrorCode.UrlElicitationRequired, + "This request requires browser-based authorization.", + ), + ); + + await user.click(screen.getByText("call")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "URL elicitation required", + color: "yellow", + }), + ), + ); + }); + + it("shows a loop toast when a tool call aborts on a repeated URL elicitation", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + ( + clientInstances[0] as unknown as { + callTool: ReturnType; + } + ).callTool.mockRejectedValueOnce( + new UrlElicitationLoopError("https://example.com/authorize"), + ); + + await user.click(screen.getByText("call")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "URL elicitation loop", + color: "yellow", + }), + ), + ); + }); + it("surfaces a refresh failure as a red toast", async () => { vi.mocked(useManagedRequestorTasks).mockReturnValue({ tasks: [], diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 5dc00dab6..c8b84f80b 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -23,6 +23,10 @@ import type { InspectorClientEventMap, JsonValue, } from "@inspector/core/mcp/index.js"; +import { + getUrlElicitationsFromError, + UrlElicitationLoopError, +} from "@inspector/core/mcp/urlElicitation.js"; import type { TypedEventGeneric } from "@inspector/core/mcp/typedEventTarget.js"; import type { InspectorServerSettings, @@ -98,6 +102,7 @@ import { import { ServerSettingsModal } from "./components/groups/ServerSettingsModal/ServerSettingsModal"; import { ConnectionInfoModal } from "./components/groups/ConnectionInfoModal/ConnectionInfoModal"; import { OutputValidationModal } from "./components/groups/OutputValidationModal/OutputValidationModal"; +import { UrlElicitationErrorModal } from "./components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal"; import type { OAuthDetails } from "./components/groups/ConnectionInfoContent/ConnectionInfoContent"; import { ServerRemoveConfirmModal } from "./components/groups/ServerRemoveConfirmModal/ServerRemoveConfirmModal"; import { @@ -223,6 +228,46 @@ const OutputValidationToastMessage = ({ ); +// Body of the non-spec URLElicitationRequired toast: the server returned a +// -32042 error with no `elicitations` list, so there's no URL to open. We keep +// the toast short and link to a modal with the raw error body. +const UrlElicitationErrorToastMessage = ({ + onViewDetails, +}: { + onViewDetails: () => void; +}) => ( + + + The server reported a URLElicitationRequired error but listed no required + elicitations, so there's nothing to open. + + + View error details + + +); + +function errorMessage(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} + +// Pretty-print a thrown error for the URL-elicitation details modal: an McpError +// carries a `code`/`data` worth showing alongside the message, so include them +// when present; otherwise fall back to the plain message. +function formatErrorDetails(err: unknown): string { + if (err && typeof err === "object") { + const e = err as { code?: unknown; message?: unknown; data?: unknown }; + if (e.code !== undefined || e.data !== undefined) { + return JSON.stringify( + { code: e.code, message: e.message, data: e.data }, + null, + 2, + ); + } + } + return errorMessage(err); +} + // How long a progress toast lingers after its last tick. Each new tick on the // same progress stream resets this window (via `notifications.update`), so a // steady stream keeps one toast alive; the toast clears a few seconds after @@ -339,6 +384,12 @@ function App() { toolName: string; message: string; } | null>(null); + // Raw body for the non-spec URLElicitationRequired (-32042, no elicitations) + // modal opened from its warning toast. + const [urlElicitationErrorDetails, setUrlElicitationErrorDetails] = useState<{ + toolName: string; + details: string; + } | null>(null); // The active connection target. `null` between sessions; set as soon as // the user toggles a server card on. Drives state-manager lifetime. @@ -1258,9 +1309,50 @@ function App() { error: invocation.error, }); } catch (err) { + // The server kept asking for a URL the user already completed this call, + // so callTool aborted to avoid an endless re-prompt loop. Surface that + // explicitly rather than as a generic failure. + if (err instanceof UrlElicitationLoopError) { + setToolCallState({ status: "error", error: err.message }); + notifications.show({ + autoClose: false, + title: "URL elicitation loop", + color: "yellow", + message: ( + + The server requested the same URL again after you completed it ( + {err.url}), so the call was cancelled to avoid an endless loop. + + ), + }); + return; + } + // A URLElicitationRequired (-32042) error that reaches here carried no + // elicitations (a non-spec response — the with-list case is handled and + // retried inside callTool). There's no URL to open, so surface a short + // toast that links to the raw error rather than a bare error panel. + const urlElicitations = getUrlElicitationsFromError(err); + if (urlElicitations !== null && urlElicitations.length === 0) { + const details = { + toolName: name, + details: formatErrorDetails(err), + }; + setToolCallState({ status: "error", error: errorMessage(err) }); + notifications.show({ + autoClose: false, + title: "URL elicitation required", + color: "yellow", + message: ( + setUrlElicitationErrorDetails(details)} + /> + ), + }); + return; + } setToolCallState({ status: "error", - error: err instanceof Error ? err.message : String(err), + error: errorMessage(err), }); } }, @@ -1953,6 +2045,12 @@ function App() { message={outputValidationDetails?.message} onClose={() => setOutputValidationDetails(null)} /> + setUrlElicitationErrorDetails(null)} + /> = { args: { onCopyUrl: fn(), onOpenInBrowser: fn(), + onComplete: fn(), onCancel: fn(), message: "Please authenticate with the external service.", requestId: "elicit-abc-123", diff --git a/clients/web/src/components/groups/ElicitationUrlPanel/ElicitationUrlPanel.test.tsx b/clients/web/src/components/groups/ElicitationUrlPanel/ElicitationUrlPanel.test.tsx index 37007c3c7..82eea6930 100644 --- a/clients/web/src/components/groups/ElicitationUrlPanel/ElicitationUrlPanel.test.tsx +++ b/clients/web/src/components/groups/ElicitationUrlPanel/ElicitationUrlPanel.test.tsx @@ -10,6 +10,7 @@ const baseProps = { isWaiting: false, onCopyUrl: vi.fn(), onOpenInBrowser: vi.fn(), + onComplete: vi.fn(), onCancel: vi.fn(), }; @@ -33,16 +34,29 @@ describe("ElicitationUrlPanel", () => { ).toBeInTheDocument(); }); - it("does not render waiting indicator when isWaiting is false", () => { + it("does not render the waiting indicator or complete action when not waiting", () => { renderWithMantine(); expect( - screen.queryByText("Waiting for completion..."), + screen.queryByText(/Waiting for completion/), ).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "I've completed it" }), + ).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Open in Browser" }), + ).toBeInTheDocument(); }); - it("renders waiting indicator when isWaiting is true", () => { + it("renders the waiting indicator and complete action when waiting", () => { renderWithMantine(); - expect(screen.getByText("Waiting for completion...")).toBeInTheDocument(); + expect(screen.getByText(/Waiting for completion/)).toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "I've completed it" }), + ).toBeInTheDocument(); + // The open action is relabelled so the user can reopen the tab. + expect( + screen.getByRole("button", { name: "Reopen in Browser" }), + ).toBeInTheDocument(); }); it("invokes onCopyUrl when Copy URL is clicked", async () => { @@ -65,6 +79,16 @@ describe("ElicitationUrlPanel", () => { expect(onOpenInBrowser).toHaveBeenCalledTimes(1); }); + it("invokes onComplete when I've completed it is clicked while waiting", async () => { + const user = userEvent.setup(); + const onComplete = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: "I've completed it" })); + expect(onComplete).toHaveBeenCalledTimes(1); + }); + it("invokes onCancel when Cancel is clicked", async () => { const user = userEvent.setup(); const onCancel = vi.fn(); @@ -74,4 +98,12 @@ describe("ElicitationUrlPanel", () => { await user.click(screen.getByRole("button", { name: "Cancel" })); expect(onCancel).toHaveBeenCalledTimes(1); }); + + it("disables the completion and cancel actions when busy", () => { + renderWithMantine(); + expect( + screen.getByRole("button", { name: "I've completed it" }), + ).toBeDisabled(); + expect(screen.getByRole("button", { name: "Cancel" })).toBeDisabled(); + }); }); diff --git a/clients/web/src/components/groups/ElicitationUrlPanel/ElicitationUrlPanel.tsx b/clients/web/src/components/groups/ElicitationUrlPanel/ElicitationUrlPanel.tsx index 5e9f57638..df69fcf36 100644 --- a/clients/web/src/components/groups/ElicitationUrlPanel/ElicitationUrlPanel.tsx +++ b/clients/web/src/components/groups/ElicitationUrlPanel/ElicitationUrlPanel.tsx @@ -13,9 +13,16 @@ export interface ElicitationUrlPanelProps { message: string; url: string; requestId: string; + /** + * The URL has been opened and we're waiting for the user to confirm they + * finished the external flow. Reveals the "I've completed it" action; opening + * the URL does not resolve the elicitation on its own. + */ isWaiting: boolean; onCopyUrl: () => void; onOpenInBrowser: () => void; + /** User confirms the external flow is done — sends `accept`. */ + onComplete: () => void; onCancel: () => void; /** * A response has been dispatched; lock the responding actions so a second @@ -56,6 +63,7 @@ export function ElicitationUrlPanel({ isWaiting, onCopyUrl, onOpenInBrowser, + onComplete, onCancel, busy = false, }: ElicitationUrlPanelProps) { @@ -70,7 +78,7 @@ export function ElicitationUrlPanel({ Copy URL @@ -88,6 +96,11 @@ export function ElicitationUrlPanel({ + {isWaiting && ( + + )} ); diff --git a/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.test.tsx b/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.test.tsx index 6a09d3c08..e1469e397 100644 --- a/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.test.tsx +++ b/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.test.tsx @@ -214,18 +214,39 @@ describe("PendingClientRequestModal", () => { }); }); - it("opens the URL and accepts a URL elicitation", async () => { + it("opens the URL into a waiting state without resolving the elicitation", async () => { const user = userEvent.setup(); const openSpy = vi.spyOn(window, "open").mockReturnValue(null); renderWithMantine( , ); + // Before opening there is no completion action. + expect( + screen.queryByRole("button", { name: "I've completed it" }), + ).not.toBeInTheDocument(); await user.click(screen.getByRole("button", { name: "Open in Browser" })); expect(openSpy).toHaveBeenCalledWith( "https://example.com/authorize", "_blank", "noopener,noreferrer", ); + // Opening alone must not resolve the elicitation; it only reveals the + // explicit completion step. + expect(baseProps.onElicitationRespond).not.toHaveBeenCalled(); + expect( + screen.getByRole("button", { name: "I've completed it" }), + ).toBeInTheDocument(); + openSpy.mockRestore(); + }); + + it("accepts a URL elicitation only after the user confirms completion", async () => { + const user = userEvent.setup(); + const openSpy = vi.spyOn(window, "open").mockReturnValue(null); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: "Open in Browser" })); + await user.click(screen.getByRole("button", { name: "I've completed it" })); expect(baseProps.onElicitationRespond).toHaveBeenCalledWith({ action: "accept", }); @@ -246,14 +267,38 @@ describe("PendingClientRequestModal", () => { expect(writeText).toHaveBeenCalledWith("https://example.com/authorize"); }); - it("cancels a URL elicitation", async () => { + it("reveals the completion step after Copy URL so a copy-paste flow can accept", async () => { const user = userEvent.setup(); + Object.defineProperty(navigator, "clipboard", { + value: { writeText: vi.fn().mockResolvedValue(undefined) }, + configurable: true, + }); renderWithMantine( , ); + // Before copying there is no completion action. + expect( + screen.queryByRole("button", { name: "I've completed it" }), + ).not.toBeInTheDocument(); + await user.click(screen.getByRole("button", { name: "Copy URL" })); + await user.click(screen.getByRole("button", { name: "I've completed it" })); + expect(baseProps.onElicitationRespond).toHaveBeenCalledWith({ + action: "accept", + }); + }); + + it("cancels a URL elicitation after opening without sending accept", async () => { + const user = userEvent.setup(); + const openSpy = vi.spyOn(window, "open").mockReturnValue(null); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: "Open in Browser" })); await user.click(screen.getByRole("button", { name: "Cancel" })); + expect(baseProps.onElicitationRespond).toHaveBeenCalledTimes(1); expect(baseProps.onElicitationRespond).toHaveBeenCalledWith({ action: "cancel", }); + openSpy.mockRestore(); }); }); diff --git a/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.tsx b/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.tsx index 8e128672b..deb62880a 100644 --- a/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.tsx +++ b/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.tsx @@ -174,25 +174,30 @@ function ElicitationUrlModalBody({ url: string; onRespond: (result: ElicitResult) => void; }) { + const [isWaiting, setIsWaiting] = useState(false); const { responded, once } = useRespondOnce(); return ( { void navigator.clipboard?.writeText(url); + // Copying the URL is the other way to start the external flow (paste + // into a browser). Reveal the completion step too, otherwise a user who + // copies rather than clicking "Open in Browser" could only Cancel. + setIsWaiting(true); }} - onOpenInBrowser={once(() => { + onOpenInBrowser={() => { window.open(url, "_blank", "noopener,noreferrer"); - // Accept-on-open: the inspector can't observe completion of an external - // flow, so opening the URL is treated as acceptance. This is optimistic - // — the user could close the tab without finishing. A proper two-step - // "open, then confirm completion" flow (using ElicitationUrlPanel's - // isWaiting state) is tracked as a follow-up; see #1415. - onRespond({ action: "accept" }); - })} + // Opening the URL only moves the panel into its waiting state — the + // inspector can't observe completion of an external flow, so the + // elicitation resolves only when the user explicitly confirms + // completion (accept) or cancels. + setIsWaiting(true); + }} + onComplete={once(() => onRespond({ action: "accept" }))} onCancel={once(() => onRespond({ action: "cancel" }))} busy={responded} /> diff --git a/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.stories.tsx b/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.stories.tsx new file mode 100644 index 000000000..4a6c64e8b --- /dev/null +++ b/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.stories.tsx @@ -0,0 +1,59 @@ +import { AppShell } from "@mantine/core"; +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, fn, within } from "storybook/test"; +import { + UrlElicitationErrorModal, + type UrlElicitationErrorModalProps, +} from "./UrlElicitationErrorModal"; + +const SAMPLE_DETAILS = JSON.stringify( + { + code: -32042, + message: "This request requires browser-based authorization.", + data: {}, + }, + null, + 2, +); + +function InteractiveRender(args: UrlElicitationErrorModalProps) { + return ( + + + + + + ); +} + +const meta: Meta = { + title: "Groups/UrlElicitationErrorModal", + component: UrlElicitationErrorModal, + parameters: { layout: "fullscreen" }, + render: InteractiveRender, + args: { + opened: true, + onClose: fn(), + toolName: "trigger-url-elicitation", + details: SAMPLE_DETAILS, + }, +}; + +export default meta; +type Story = StoryObj; + +export const Default: Story = { + play: async ({ canvasElement }) => { + // Mantine renders the modal in a portal at document.body. + const body = within(canvasElement.ownerDocument.body); + await body.findByText("URL elicitation required"); + expect(body.getByText(/trigger-url-elicitation/)).toBeInTheDocument(); + const details = body.getByLabelText("Error details") as HTMLTextAreaElement; + expect(details.value).toContain("-32042"); + expect(details.readOnly).toBe(true); + }, +}; + +export const WithoutToolName: Story = { + args: { toolName: undefined }, +}; diff --git a/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.test.tsx b/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.test.tsx new file mode 100644 index 000000000..fe52b5b24 --- /dev/null +++ b/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.test.tsx @@ -0,0 +1,63 @@ +import { describe, it, expect, vi } from "vitest"; +import userEvent from "@testing-library/user-event"; +import { renderWithMantine, screen } from "../../../test/renderWithMantine"; +import { UrlElicitationErrorModal } from "./UrlElicitationErrorModal"; + +const DETAILS = JSON.stringify( + { + code: -32042, + message: "This request requires browser-based authorization.", + data: {}, + }, + null, + 2, +); + +describe("UrlElicitationErrorModal", () => { + it("renders the tool name and the raw error body in a read-only field", () => { + renderWithMantine( + , + ); + expect(screen.getByText("URL elicitation required")).toBeInTheDocument(); + expect(screen.getByText(/trigger-url-elicitation/)).toBeInTheDocument(); + const details = screen.getByLabelText( + "Error details", + ) as HTMLTextAreaElement; + expect(details.value).toBe(DETAILS); + expect(details.readOnly).toBe(true); + }); + + it("falls back to a generic message when no tool name is provided", () => { + renderWithMantine( + , + ); + expect(screen.getByText(/no required elicitations/i)).toBeInTheDocument(); + }); + + it("calls onClose when the close button is clicked", async () => { + const onClose = vi.fn(); + renderWithMantine( + , + ); + await userEvent.click(screen.getByRole("button", { name: "Close" })); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("renders nothing visible when closed", () => { + renderWithMantine( + , + ); + expect( + screen.queryByText("URL elicitation required"), + ).not.toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.tsx b/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.tsx new file mode 100644 index 000000000..5f1421b40 --- /dev/null +++ b/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.tsx @@ -0,0 +1,64 @@ +import { + CloseButton, + Group, + Modal, + Stack, + Text, + Textarea, + Title, +} from "@mantine/core"; + +export interface UrlElicitationErrorModalProps { + opened: boolean; + onClose: () => void; + /** The tool whose call returned the URL-elicitation-required error. */ + toolName?: string; + /** The raw error body (message + data), pretty-printed for inspection. */ + details?: string; +} + +/** + * Surfaces the raw body of a `URLElicitationRequired` (`-32042`) error that + * carried no `elicitations` list — a non-spec server response the inspector + * can't act on (there's no URL to open). The Tools screen shows a short toast; + * this modal, opened from that toast, exposes the full error so a server + * developer can see what the server actually returned. + */ +export function UrlElicitationErrorModal({ + opened, + onClose, + toolName, + details, +}: UrlElicitationErrorModalProps) { + return ( + + + + + URL elicitation required + + + + + {toolName + ? `"${toolName}" returned a URLElicitationRequired (-32042) error with no required elicitations. Per the MCP spec the error must list the URL elicitations to complete before retrying, so the inspector has nothing to open.` + : "The server returned a URLElicitationRequired (-32042) error with no required elicitations. Per the MCP spec the error must list the URL elicitations to complete before retrying, so the inspector has nothing to open."} + +