diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 2a865cf6..2b7f0a9b 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -594,6 +594,11 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ edit-server + {/* The real drag-and-drop reorder lives inside this mocked view, so the + callback is only reachable through a control like this one. */} + {(props.servers.highlightedServerIds ?? []).join(",") || "none"} @@ -3806,6 +3811,30 @@ describe("App background command rejections (#2049)", () => { return user; }; + // A complete `useServers` return with one injected mutator, so the mock stays + // type-checked against the hook's contract rather than spread from a call. + const serversWithReorder = ( + reorderServers: ReturnType["reorderServers"], + ): ReturnType => ({ + servers: [ + { + id: "A", + name: "PlotRocket", + config: { type: "streamable-http", url: "https://api.example.com/mcp" }, + connection: { status: "disconnected" }, + }, + ], + loading: false, + error: undefined, + refresh: vi.fn().mockResolvedValue(undefined), + addServer: vi.fn().mockResolvedValue(undefined), + updateServer: vi.fn().mockResolvedValue(undefined), + updateServerSettings: vi.fn().mockResolvedValue(undefined), + removeServer: vi.fn().mockResolvedValue(undefined), + reorderServers, + importSource: vi.fn().mockResolvedValue({ servers: {} }), + }); + it("does not leak an unhandled rejection when an all-pages Refresh fails", async () => { vi.mocked(useManagedTools).mockReturnValue({ tools: [], @@ -3921,6 +3950,92 @@ describe("App background command rejections (#2049)", () => { } }); + // The four command handlers await `handleCommandScopedAuthRecovery` from + // *inside* their catch blocks, and a rejection thrown from a catch is not + // caught by that same catch — so it escapes the handler entirely. That helper + // awaits `checkAuthChallengeSatisfied`, which reaches the backend and can + // reject. Before #2130's review this escaped a bare `void` as a global + // unhandled rejection with nothing shown to the user. + it("toasts an auth-recovery failure that escapes a tool call instead of leaking it", async () => { + const rejections = captureUnhandledRejections(); + try { + const user = await connect(); + const client = clientInstances[0] as EventTarget & { + callTool: ReturnType; + }; + client.callTool.mockRejectedValueOnce( + new AuthRecoveryRequiredError( + new URL("https://as.example.com/authorize"), + { reason: "unauthorized" }, + ), + ); + // The recovery helper's first await is the one that breaks. + rejectNextChallengeCheck(new Error("challenge check failed")); + + await user.click(screen.getByText("call")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Tool call failed", + message: "challenge check failed", + color: "red", + }), + ), + ); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + }); + + // The reorder handler was lifted out of the JSX into a named callback with a + // `.catch` that toasts (#2130). `reorderServers` reverts its own optimistic + // ordering and re-throws — on a 409 from a racing external edit, or a network + // error — so without the toast the drag just silently bounces back. + it("forwards a reorder to the server list", async () => { + const reorderServers = vi.fn().mockResolvedValue(undefined); + vi.mocked(useServers).mockReturnValue(serversWithReorder(reorderServers)); + const user = userEvent.setup(); + renderWithMantine(); + + await user.click(screen.getByText("reorder-servers")); + + await waitFor(() => + expect(reorderServers).toHaveBeenCalledWith(["B", "A"]), + ); + expect(notificationsMock.show).not.toHaveBeenCalled(); + }); + + it("toasts a failed reorder instead of leaking it", async () => { + const reorderServers = vi + .fn() + .mockRejectedValue(new Error("stale server list")); + vi.mocked(useServers).mockReturnValue(serversWithReorder(reorderServers)); + const rejections = captureUnhandledRejections(); + try { + const user = userEvent.setup(); + renderWithMantine(); + + await user.click(screen.getByText("reorder-servers")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to reorder servers", + message: "stale server list", + color: "red", + }), + ), + ); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + }); + it("toasts a failed transport close from the explicit Disconnect instead of leaking it", async () => { const rejections = captureUnhandledRejections(); try { diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index f5d11b32..b1bc9f28 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -122,6 +122,21 @@ import { FetchBodyDroppedToastMessage } from "./components/elements/Toasts/Fetch import { OutputValidationToastMessage } from "./components/elements/Toasts/OutputValidationToastMessage"; import { ReAuthBannerBar } from "./components/groups/ReAuthBanner/ReAuthBannerBar"; +/** + * Terminates a dispatched handler's promise by reporting the failure, for the + * `InspectorView` action wrappers below. Module scope because it closes over + * nothing — the title is the only thing that varies per action. + */ +function reportDispatchFailure(title: string) { + return (err: unknown) => { + notifications.show({ + title, + message: err instanceof Error ? err.message : String(err), + color: "red", + }); + }; +} + function App() { const { onToggleTheme } = useThemeToggle(); @@ -1550,71 +1565,75 @@ function App() { [], ); - // The two connection-lifecycle handlers do NOT own every rejection, so they - // are terminated with a `.catch` rather than discarded. `onToggleConnection` - // has three escape paths outside its own try/catch — awaiting - // `initialConfigSettledRef`, constructing the client via - // `setupClientForServer`, and the already-connected disconnect branch, whose - // `try/finally` runs `finalizeExplicitDisconnect()` and then lets a failed - // transport close propagate — and `onDisconnect` is that same `try/finally` - // on its own. A bare `void` there turns an ordinary Connect/Disconnect click - // into a global unhandled rejection with nothing shown to the user, so each - // reports instead. Connect *handshake* failures are already surfaced by the - // hook's own catch (`connectErrorMessage` + the failed-card border) and never - // reach here. - const reportConnectionFailure = useCallback((title: string) => { - return (err: unknown) => { - notifications.show({ - title, - message: err instanceof Error ? err.message : String(err), - color: "red", - }); - }; - }, []); - + // Every `InspectorView` action prop is synchronous while the handler behind + // it is async, so each wrapper below has to terminate its promise somehow. + // They all do it the same way: a reporting `.catch`, never a bare `void`. + // + // The uniformity is the point. Deciding per handler whether it "owns its + // failures" is a judgement this PR got wrong twice (#2130 review) — the + // handlers really do catch their ordinary errors, but a rejection can still + // escape by a route the reading misses: + // + // - `onToggleConnection` awaits `initialConfigSettledRef` and constructs + // the client via `setupClientForServer` outside its own try/catch, and + // its already-connected branch is a `try/finally` that lets a failed + // transport close propagate. `onDisconnect` is that `try/finally` alone. + // - `onCallTool` / `onGetPrompt` / `onReadResource` / `onOpenApp` await + // `handleCommandScopedAuthRecovery` from *inside* their catch blocks, and + // a rejection thrown from a catch is not caught by that same catch. That + // helper awaits `checkAuthChallengeSatisfied` and `pushRemoteAuthState`, + // both of which reach the backend and can reject. + // + // A handler that does surface its own failure resolves, so it never reaches + // this `.catch` and there is no double-toast; what lands here is only the + // escape, which would otherwise be a global unhandled rejection with nothing + // shown to the user. const dispatchToggleConnection = useCallback( (id: string) => { onToggleConnection(id).catch( - reportConnectionFailure("Failed to change the connection"), + reportDispatchFailure("Failed to change the connection"), ); }, - [onToggleConnection, reportConnectionFailure], + [onToggleConnection], ); const dispatchDisconnect = useCallback(() => { - onDisconnect().catch(reportConnectionFailure("Failed to disconnect")); - }, [onDisconnect, reportConnectionFailure]); - - // The five wrappers below DO discard a promise the callee already owns — - // each of those handlers ends in its own `catch` that surfaces the failure as - // a toast or a panel error — and the view's props are synchronous. `void` is - // the explicit discard `@typescript-eslint/no-floating-promises` requires. + onDisconnect().catch(reportDispatchFailure("Failed to disconnect")); + }, [onDisconnect]); const dispatchCallTool = useCallback( (name: string, args: Record, runAsTask?: boolean) => { - void onCallTool(name, args, runAsTask); + onCallTool(name, args, runAsTask).catch( + reportDispatchFailure("Tool call failed"), + ); }, [onCallTool], ); const dispatchGetPrompt = useCallback( (name: string, args: Record) => { - void onGetPrompt(name, args); + onGetPrompt(name, args).catch( + reportDispatchFailure("Failed to get prompt"), + ); }, [onGetPrompt], ); const dispatchReadResource = useCallback( (uri: string) => { - void onReadResource(uri); + onReadResource(uri).catch( + reportDispatchFailure("Failed to read resource"), + ); }, [onReadResource], ); const dispatchCancelTask = useCallback( (taskId: string) => { - void onCancelTask(taskId); + onCancelTask(taskId).catch( + reportDispatchFailure("Failed to cancel task"), + ); }, [onCancelTask], ); const dispatchOpenApp = useCallback( (name: string, args: Record) => { - void onOpenApp(name, args); + onOpenApp(name, args).catch(reportDispatchFailure("Failed to open app")); }, [onOpenApp], );