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(