From 37add967cda70363fcfa8bbeb07f7ab356a45e04 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 28 Aug 2026 00:18:03 -0400 Subject: [PATCH] chore(web): address Copilot review on #2130 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, all valid on verification. `onToggleConnection` and `onDisconnect` do not own every rejection, so a bare `void` on them turns an ordinary Connect/Disconnect click into a global unhandled rejection with nothing shown to the user. The toggle 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. Both are now terminated with a `.catch` that toasts the failure. The blanket comment claiming all seven wrappers own their failures was wrong. The five command handlers (`onCallTool`, `onGetPrompt`, `onReadResource`, `onCancelTask`, `onOpenApp`) do each end in a `catch`, so `void` stays correct for them; the comment now splits the two groups and says which is which. Adds the two tests the review asked for, covering the rejected-close path through both the toggle and the explicit Disconnect. Each fails against the previous `void` code, so they pin the fix rather than merely passing. The `InspectorView` double grows a `disconnect` button, since the standalone `onDisconnect` was otherwise unreachable from it. Also restores the `PropOverrides` doc example, which the scripted regrouping had rewritten inside its own JSDoc into a triple-nested shape that would not type-check if copied. Signed-off-by: cliffhall --- clients/web/src/App.test.tsx | 69 +++++++++++++++++++ clients/web/src/App.tsx | 41 ++++++++--- .../InspectorView/InspectorView.test.tsx | 6 +- 3 files changed, 103 insertions(+), 13 deletions(-) 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 = {