From 37e156c035f926f365ea39b29b7fd68df223160d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Thu, 27 Aug 2026 22:21:37 -0400 Subject: [PATCH] fix(web): surface failed resource subscribe/unsubscribe (#2174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both handlers discarded their promise with a bare `void`. Nothing owned those failures: `subscribeToResource` throws when the client is disconnected, when the server declares no subscription support, and (wrapped) on a failed request — so a subscribe that failed did nothing visible and surfaced only as an unhandled browser rejection, while every neighbouring command in the hook already routed through the shared recovery. Both now go through `runCommandInBackground`, so a mid-session 401 recovers the way it does on a refresh and any other failure toasts. The source is `ambient`, not `resource`: a `resource` step-up failure is routed into `readResourceState` — the read-preview panel — which describes a read of whatever is selected rather than this subscribe, so marking it errored would contradict a read that succeeded. Subscribing has no panel of its own, the same position the refreshes and the pagination toggle are in. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev Signed-off-by: cliffhall --- .../web/src/hooks/useServerCommands.test.tsx | 112 ++++++++++++++++++ clients/web/src/hooks/useServerCommands.tsx | 33 +++++- 2 files changed, 141 insertions(+), 4 deletions(-) diff --git a/clients/web/src/hooks/useServerCommands.test.tsx b/clients/web/src/hooks/useServerCommands.test.tsx index 8704d1b6b..b8de9e2dd 100644 --- a/clients/web/src/hooks/useServerCommands.test.tsx +++ b/clients/web/src/hooks/useServerCommands.test.tsx @@ -862,6 +862,118 @@ describe("subscriptions and completion", () => { expect(h.api().onSubscribeResource).toBeTypeOf("function"); }); + // #2174. Both used to discard the promise with a bare `void`, so a failure + // was invisible in the UI and surfaced only as an unhandled rejection. + it("toasts a failed subscribe rather than swallowing it", async () => { + const h = harness({ + client: client({ + subscribeToResource: vi + .fn() + .mockRejectedValue( + new Error("Server does not support resource subscriptions"), + ), + }), + }); + await act(async () => h.api().onSubscribeResource("u")); + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to subscribe to resource", + message: "Server does not support resource subscriptions", + }), + ), + ); + }); + + it("toasts a failed unsubscribe rather than swallowing it", async () => { + const h = harness({ + client: client({ + unsubscribeFromResource: vi + .fn() + .mockRejectedValue(new Error("Client is not connected")), + }), + }); + await act(async () => h.api().onUnsubscribeResource("u")); + await waitFor(() => + expect(notificationsMock.show).toHaveBeenCalledWith( + expect.objectContaining({ + title: "Failed to unsubscribe from resource", + message: "Client is not connected", + }), + ), + ); + }); + + it("recovers a lapsed authorization on subscribe, and retries", async () => { + const recover = vi.fn().mockResolvedValue(true); + const subscribeToResource = vi + .fn() + .mockRejectedValueOnce(authError()) + .mockResolvedValue(undefined); + const h = harness({ + client: client({ subscribeToResource }), + activeServerId: "a", + recovery: { handleCommandScopedAuthRecovery: recover }, + }); + await act(async () => h.api().onSubscribeResource("u")); + await waitFor(() => expect(subscribeToResource).toHaveBeenCalledTimes(2)); + // `ambient`, not `resource`: a `resource` step-up failure is routed into + // the read-preview panel, which this command has nothing to do with. + expect(recover).toHaveBeenCalledWith( + expect.any(AuthRecoveryRequiredError), + { + serverId: "a", + source: "ambient", + retryOperation: expect.any(Function), + }, + ); + // The recovery owns the prompt, so the command itself stays quiet. + expect(notificationsMock.show).not.toHaveBeenCalled(); + }); + + it("recovers a lapsed authorization on unsubscribe, and retries", async () => { + const recover = vi.fn().mockResolvedValue(true); + const unsubscribeFromResource = vi + .fn() + .mockRejectedValueOnce(authError()) + .mockResolvedValue(undefined); + const h = harness({ + client: client({ unsubscribeFromResource }), + activeServerId: "a", + recovery: { handleCommandScopedAuthRecovery: recover }, + }); + await act(async () => h.api().onUnsubscribeResource("u")); + await waitFor(() => + expect(unsubscribeFromResource).toHaveBeenCalledTimes(2), + ); + expect(recover).toHaveBeenCalledWith( + expect.any(AuthRecoveryRequiredError), + { + serverId: "a", + source: "ambient", + retryOperation: expect.any(Function), + }, + ); + expect(notificationsMock.show).not.toHaveBeenCalled(); + }); + + it("does not toast a subscribe whose recovery was left unsatisfied", async () => { + // The recovery has taken over — a redirect is pending or a step-up prompt + // is open — so the wrapper resolves `undefined` rather than rejecting, and + // a toast here would talk over the prompt the user is looking at. + const recover = vi.fn().mockResolvedValue(false); + const h = harness({ + client: client({ + subscribeToResource: vi.fn().mockRejectedValue(authError()), + }), + activeServerId: "a", + recovery: { handleCommandScopedAuthRecovery: recover }, + }); + await act(async () => h.api().onSubscribeResource("u")); + await waitFor(() => expect(recover).toHaveBeenCalled()); + expect(notificationsMock.show).not.toHaveBeenCalled(); + }); + it("returns the completion values", async () => { const c = client(); const h = harness({ client: c }); diff --git a/clients/web/src/hooks/useServerCommands.tsx b/clients/web/src/hooks/useServerCommands.tsx index f4cb49f14..61aabae87 100644 --- a/clients/web/src/hooks/useServerCommands.tsx +++ b/clients/web/src/hooks/useServerCommands.tsx @@ -564,20 +564,45 @@ export function useServerCommands({ [inspectorClient, activeServerId, handleCommandScopedAuthRecovery], ); + // Both route through the shared recovery like every other command (#2174). + // They used to discard the promise with a bare `void`, which neither the + // callee nor anything else owned: `subscribeToResource` throws when the + // client is disconnected, when the server declares no subscription support, + // and (wrapped) on a failed request, so a subscribe that failed did nothing + // visible and surfaced only as an unhandled rejection. + // + // The source is `ambient`, not `resource`. A `resource` step-up failure is + // routed into `readResourceState` — the *preview* panel — which describes a + // read of whatever resource is selected, not this subscribe; marking it + // errored would contradict a read that succeeded. Subscribing has no panel + // of its own (the tile only flips its button label), which is the same + // position the refreshes and the pagination toggle are in, and they are + // `ambient` for the same reason. + // + // Both pass an `errorTitle`: nothing else records these failures, so without + // one the button would go on silently doing nothing. const onSubscribeResource = useCallback( (uri: string) => { if (!inspectorClient) return; - void inspectorClient.subscribeToResource(uri); + runCommandInBackground( + () => inspectorClient.subscribeToResource(uri), + "ambient", + "Failed to subscribe to resource", + ); }, - [inspectorClient], + [inspectorClient, runCommandInBackground], ); const onUnsubscribeResource = useCallback( (uri: string) => { if (!inspectorClient) return; - void inspectorClient.unsubscribeFromResource(uri); + runCommandInBackground( + () => inspectorClient.unsubscribeFromResource(uri), + "ambient", + "Failed to unsubscribe from resource", + ); }, - [inspectorClient], + [inspectorClient, runCommandInBackground], ); const onCompleteArgument = useCallback(