diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 49beb743..2a865cf6 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -499,6 +499,11 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({ + {/* The header's explicit Disconnect, which routes to the standalone + `onDisconnect` rather than through the toggle. */} + @@ -3878,6 +3883,70 @@ describe("App background command rejections (#2049)", () => { rejections.stop(); } }); + + // `onToggleConnection` and `onDisconnect` both close a live session with + // `try { await disconnect() } finally { finalizeExplicitDisconnect() }` — a + // `finally`, not a `catch` — so a transport that fails to close rejects out + // of the handler. App discards neither: both are terminated with a `.catch` + // that toasts, because a bare `void` would turn an ordinary click into a + // global unhandled rejection with nothing shown to the user (#2130 review). + const CLOSE_FAILURE = new Error("close boom"); + + it("toasts a failed transport close from the connection toggle instead of leaking it", async () => { + const rejections = captureUnhandledRejections(); + try { + const user = await connect(); + const client = clientInstances[0] as EventTarget & { + disconnect: ReturnType; + }; + client.disconnect.mockRejectedValueOnce(CLOSE_FAILURE); + + // Second click on the same, now-connected server takes the disconnect + // branch of the toggle. + await user.click(screen.getByText("connect")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to change the connection", + message: "close boom", + 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 { + const user = await connect(); + const client = clientInstances[0] as EventTarget & { + disconnect: ReturnType; + }; + client.disconnect.mockRejectedValueOnce(CLOSE_FAILURE); + + await user.click(screen.getByText("disconnect")); + + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to disconnect", + message: "close boom", + color: "red", + }), + ), + ); + await settleRejections(); + expect(rejections.seen).toEqual([]); + } finally { + rejections.stop(); + } + }); }); /** A complete `useManagedResources` return, so the mock stays type-checked against the hook's contract. */ diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index f005b69f..f5d11b32 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1550,19 +1550,44 @@ function App() { [], ); - // The seven wrappers below discard a promise the callee already owns — each - // of these 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. + // 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", + }); + }; + }, []); + const dispatchToggleConnection = useCallback( (id: string) => { - void onToggleConnection(id); + onToggleConnection(id).catch( + reportConnectionFailure("Failed to change the connection"), + ); }, - [onToggleConnection], + [onToggleConnection, reportConnectionFailure], ); const dispatchDisconnect = useCallback(() => { - void onDisconnect(); - }, [onDisconnect]); + 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. const dispatchCallTool = useCallback( (name: string, args: Record, runAsTask?: boolean) => { void onCallTool(name, args, runAsTask); diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index e152fad8..20fd24db 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -61,11 +61,7 @@ const noopBridgeFactory: BridgeFactory = () => /** * Per-bundle overrides. Each key takes a `Partial` of that bundle, so a test - * names only the field it cares about — `makeProps({ - tools: { - tools: { tools: [t] }, - }, - })` + * names only the field it cares about — `makeProps({ tools: { tools: [t] } })` * — and inherits the rest of the bundle's defaults. */ type PropOverrides = {