From de60983ccc2248f2c474d4d0ff9ec2c82163776c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 17 Aug 2026 20:40:23 -0400 Subject: [PATCH] fix: surface list-fetch failures in paginated mode (#1998) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1954 wired the list-load error off the aggregate managed stores only. With `paginatedLists` on, those stores deliberately skip their all-page walk, so their error is permanently null — while the paged stores that actually drive the sidebar had no error state at all and `loadPage` had a try/finally with no catch. A failing list showed an empty panel with no alert and no Retry, and the connect-time `void loadPage(undefined)` left an unhandled rejection. - Give PagedTools/Prompts/ResourcesState observable error state: an `errorChange` event plus `getError()`, set in a new catch in `loadPage`, cleared on the next success and on disconnect. The rejection is still re-thrown, matching ManagedListState — callers' auth-recovery wrappers key off it to detect a 401. - Catch the connect-time load's rejection (`.catch(() => {})`) now that the failure is recorded as state rather than lost. - Select the error by mode in `usePaginatedList`, the same way `items` already is, and read it in App.tsx from the pagination model. - Rename `useManagedListError` to `useListError`: both store families expose an identical `errorChange`, so the hook declares that one-event contract itself instead of importing ManagedListEventMap. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- clients/web/src/App.tsx | 17 ++++- .../web/src/hooks/usePaginatedList.test.tsx | 33 ++++++++++ clients/web/src/hooks/usePaginatedList.ts | 14 ++++ .../core/mcp/state/pagedPromptsState.test.ts | 65 +++++++++++++++++++ .../mcp/state/pagedResourcesState.test.ts | 65 +++++++++++++++++++ .../core/mcp/state/pagedToolsState.test.ts | 65 +++++++++++++++++++ ...stError.test.tsx => useListError.test.tsx} | 20 +++--- .../test/core/react/usePagedPrompts.test.tsx | 27 ++++++++ .../core/react/usePagedResources.test.tsx | 27 ++++++++ .../test/core/react/usePagedTools.test.tsx | 27 ++++++++ core/mcp/state/pagedPromptsState.ts | 39 ++++++++++- core/mcp/state/pagedResourcesState.ts | 39 ++++++++++- core/mcp/state/pagedToolsState.ts | 39 ++++++++++- ...useManagedListError.ts => useListError.ts} | 39 ++++++----- core/react/useManagedPrompts.ts | 4 +- core/react/useManagedResourceTemplates.ts | 4 +- core/react/useManagedResources.ts | 4 +- core/react/useManagedTools.ts | 4 +- core/react/usePagedPrompts.ts | 11 +++- core/react/usePagedResources.ts | 11 +++- core/react/usePagedTools.ts | 11 +++- 21 files changed, 523 insertions(+), 42 deletions(-) rename clients/web/src/test/core/react/{useManagedListError.test.tsx => useListError.test.tsx} (84%) rename core/react/{useManagedListError.ts => useListError.ts} (58%) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 910557478..3738044d7 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -1031,18 +1031,21 @@ function App() { tools: pagedTools, nextCursor: pagedToolsCursor, pageCount: pagedToolsPageCount, + error: pagedToolsLoadError, loadPage: loadToolsPage, } = usePagedTools(inspectorClient, pagedToolsState); const { prompts: pagedPrompts, nextCursor: pagedPromptsCursor, pageCount: pagedPromptsPageCount, + error: pagedPromptsLoadError, loadPage: loadPromptsPage, } = usePagedPrompts(inspectorClient, pagedPromptsState); const { resources: pagedResources, nextCursor: pagedResourcesCursor, pageCount: pagedResourcesPageCount, + error: pagedResourcesLoadError, loadPage: loadResourcesPage, } = usePagedResources(inspectorClient, pagedResourcesState); // The active server's persisted paginated setting drives the display mode. @@ -1073,9 +1076,11 @@ function App() { paginated: paginatedLists, managedItems: managedTools, managedRefresh: refreshTools, + managedError: toolsLoadError, pagedItems: pagedTools, pagedNextCursor: pagedToolsCursor, pagedPageCount: pagedToolsPageCount, + pagedError: pagedToolsLoadError, loadPage: loadToolsPage, }); const promptsPagination = usePaginatedList({ @@ -1083,9 +1088,11 @@ function App() { paginated: paginatedLists, managedItems: managedPrompts, managedRefresh: refreshPrompts, + managedError: promptsLoadError, pagedItems: pagedPrompts, pagedNextCursor: pagedPromptsCursor, pagedPageCount: pagedPromptsPageCount, + pagedError: pagedPromptsLoadError, loadPage: loadPromptsPage, }); const resourcesPagination = usePaginatedList({ @@ -1093,9 +1100,11 @@ function App() { paginated: paginatedLists, managedItems: managedResources, managedRefresh: refreshResources, + managedError: resourcesLoadError, pagedItems: pagedResources, pagedNextCursor: pagedResourcesCursor, pagedPageCount: pagedResourcesPageCount, + pagedError: pagedResourcesLoadError, loadPage: loadResourcesPage, }); const tools = toolsPagination.items; @@ -4407,9 +4416,11 @@ function App() { toolsListChanged={toolsListChanged} promptsListChanged={promptsListChanged} resourcesListChanged={resourcesListChanged} - toolsLoadError={toolsLoadError} - promptsLoadError={promptsLoadError} - resourcesLoadError={resourcesLoadError ?? resourceTemplatesLoadError} + toolsLoadError={toolsPagination.error} + promptsLoadError={promptsPagination.error} + resourcesLoadError={ + resourcesPagination.error ?? resourceTemplatesLoadError + } subscriptions={subscriptions} subscriptionStreamState={subscriptionStreamState} logs={logs} diff --git a/clients/web/src/hooks/usePaginatedList.test.tsx b/clients/web/src/hooks/usePaginatedList.test.tsx index 558716e88..c03e6a9e6 100644 --- a/clients/web/src/hooks/usePaginatedList.test.tsx +++ b/clients/web/src/hooks/usePaginatedList.test.tsx @@ -7,9 +7,11 @@ interface Params { paginated: boolean; managedItems: string[]; managedRefresh: () => Promise; + managedError: Error | null; pagedItems: string[]; pagedNextCursor?: string; pagedPageCount: number; + pagedError: Error | null; loadPage: (cursor?: string) => Promise; } @@ -19,9 +21,11 @@ function makeParams(over: Partial = {}): Params { paginated: false, managedItems: ["m1", "m2"], managedRefresh: vi.fn(async () => []), + managedError: null, pagedItems: ["p1"], pagedNextCursor: undefined, pagedPageCount: 0, + pagedError: null, loadPage: vi.fn(async () => ({})), ...over, }; @@ -88,6 +92,35 @@ describe("usePaginatedList", () => { expect(loadPage).toHaveBeenCalledWith(undefined); }); + // #1998: the managed store deliberately never fetches in paginated mode, so + // reading its (permanently null) error there would leave a failed page load + // showing an empty panel with no alert and no Retry. + it("reports the managed error in all-pages mode", () => { + const managedError = new Error("aggregate failed"); + const pagedError = new Error("page failed"); + const params = makeParams({ managedError, pagedError }); + const { result } = renderHook(() => usePaginatedList(params)); + expect(result.current.error).toBe(managedError); + }); + + it("reports the paged error in paginated mode", () => { + const managedError = new Error("aggregate failed"); + const pagedError = new Error("page failed"); + const params = makeParams({ paginated: true, managedError, pagedError }); + const { result } = renderHook(() => usePaginatedList(params)); + expect(result.current.error).toBe(pagedError); + }); + + it("reports no error when the active source succeeded", () => { + const params = makeParams({ + paginated: true, + managedError: new Error("aggregate failed"), + pagedError: null, + }); + const { result } = renderHook(() => usePaginatedList(params)); + expect(result.current.error).toBeNull(); + }); + it("onRefresh re-fetches the aggregate in all-pages mode", () => { const managedRefresh = vi.fn(async () => []); const params = makeParams({ managedRefresh }); diff --git a/clients/web/src/hooks/usePaginatedList.ts b/clients/web/src/hooks/usePaginatedList.ts index 7729e592a..9ece24e5d 100644 --- a/clients/web/src/hooks/usePaginatedList.ts +++ b/clients/web/src/hooks/usePaginatedList.ts @@ -19,6 +19,13 @@ export interface PaginatedListModel { * promise so the caller can wrap it in auth recovery. */ onLoadMore: () => Promise; + /** + * The active source's last load failure, or `null`. Selected by mode for the + * same reason `items` is: in paginated mode the managed store never fetches, + * so its error is permanently `null` and reading it would leave a failed page + * showing an empty panel with no alert and no Retry (#1998). + */ + error: Error | null; /** * Refresh the list: reload page 1 in paginated mode, or re-fetch the whole * aggregate in all-pages mode. This is what the list-changed indicator's @@ -37,12 +44,16 @@ export interface UsePaginatedListParams { managedItems: T[]; /** Re-fetch the whole aggregate (all-pages mode Refresh). */ managedRefresh: () => Promise; + /** The aggregate store's last-fetch error (all-pages mode). */ + managedError: Error | null; /** The accumulated paged list (paginated mode display source). */ pagedItems: T[]; /** The paged store's current `nextCursor` (undefined = at the end). */ pagedNextCursor?: string; /** The paged store's page count (page 1 = 1). */ pagedPageCount: number; + /** The paged store's last page-load error (paginated mode). */ + pagedError: Error | null; /** Fetch one page; `undefined` cursor = page 1 (replaces the paged list). */ loadPage: (cursor?: string) => Promise; } @@ -64,9 +75,11 @@ export function usePaginatedList({ paginated, managedItems, managedRefresh, + managedError, pagedItems, pagedNextCursor, pagedPageCount, + pagedError, loadPage, }: UsePaginatedListParams): PaginatedListModel { const onLoadMore = useCallback((): Promise => { @@ -80,6 +93,7 @@ export function usePaginatedList({ return { items: paginated ? pagedItems : managedItems, + error: paginated ? pagedError : managedError, paginated, // Masked by `connected`: while disconnected there is no page to load and no // meaningful page count (the store resets on disconnect). diff --git a/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts b/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts index 40584cf9f..b82828b80 100644 --- a/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedPromptsState.test.ts @@ -28,6 +28,14 @@ function waitForChange(state: PagedPromptsState): Promise { }); } +function waitForError(state: PagedPromptsState): Promise { + return new Promise((resolve) => { + state.addEventListener("errorChange", (e) => resolve(e.detail), { + once: true, + }); + }); +} + describe("PagedPromptsState", () => { let client: FakeInspectorClient; let state: PagedPromptsState; @@ -201,4 +209,61 @@ describe("PagedPromptsState", () => { expect(client.listPrompts).not.toHaveBeenCalled(); }); }); + + // #1998: in paginated mode this store — not the managed one — drives the + // sidebar, so a failed page must surface here or the panel renders empty + // with no alert and no Retry. + describe("load errors (#1998)", () => { + it("records the failure, dispatches errorChange, and re-throws", async () => { + client.setStatus("connected"); + const boom = new Error("list failed"); + client.listPrompts.mockRejectedValueOnce(boom); + const errorPromise = waitForError(state); + await expect(state.loadPage()).rejects.toThrow("list failed"); + expect(await errorPromise).toBe(boom); + expect(state.getError()).toBe(boom); + }); + + it("wraps a non-Error rejection", async () => { + client.setStatus("connected"); + client.listPrompts.mockRejectedValueOnce("plain string"); + await expect(state.loadPage()).rejects.toBe("plain string"); + expect(state.getError()?.message).toBe("plain string"); + }); + + it("clears the error on the next successful load", async () => { + client.setStatus("connected"); + client.listPrompts.mockRejectedValueOnce(new Error("list failed")); + await expect(state.loadPage()).rejects.toThrow(); + client.queuePromptPages({ prompts: [prompt("a")] }); + const cleared = waitForError(state); + await state.loadPage(); + expect(await cleared).toBeNull(); + expect(state.getError()).toBeNull(); + }); + + it("clears the error on disconnect", async () => { + client.setStatus("connected"); + client.listPrompts.mockRejectedValueOnce(new Error("list failed")); + await expect(state.loadPage()).rejects.toThrow(); + const cleared = waitForError(state); + client.setStatus("disconnected"); + expect(await cleared).toBeNull(); + }); + + it("records a connect-time auto-load failure instead of floating it", async () => { + const spClient = new FakeInspectorClient({ + serverSettings: PAGINATED_SETTINGS, + }); + spClient.setStatus("connected"); + const spState = new PagedPromptsState(spClient); + const boom = new Error("connect-time list failed"); + spClient.listPrompts.mockRejectedValueOnce(boom); + const errored = waitForError(spState); + spClient.dispatchTypedEvent("connect"); + expect(await errored).toBe(boom); + expect(spState.getError()).toBe(boom); + spState.destroy(); + }); + }); }); diff --git a/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts b/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts index c60d75a39..c8fee7e79 100644 --- a/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedResourcesState.test.ts @@ -28,6 +28,14 @@ function waitForChange(state: PagedResourcesState): Promise { }); } +function waitForError(state: PagedResourcesState): Promise { + return new Promise((resolve) => { + state.addEventListener("errorChange", (e) => resolve(e.detail), { + once: true, + }); + }); +} + describe("PagedResourcesState", () => { let client: FakeInspectorClient; let state: PagedResourcesState; @@ -206,4 +214,61 @@ describe("PagedResourcesState", () => { expect(client.listResources).not.toHaveBeenCalled(); }); }); + + // #1998: in paginated mode this store — not the managed one — drives the + // sidebar, so a failed page must surface here or the panel renders empty + // with no alert and no Retry. + describe("load errors (#1998)", () => { + it("records the failure, dispatches errorChange, and re-throws", async () => { + client.setStatus("connected"); + const boom = new Error("list failed"); + client.listResources.mockRejectedValueOnce(boom); + const errorPromise = waitForError(state); + await expect(state.loadPage()).rejects.toThrow("list failed"); + expect(await errorPromise).toBe(boom); + expect(state.getError()).toBe(boom); + }); + + it("wraps a non-Error rejection", async () => { + client.setStatus("connected"); + client.listResources.mockRejectedValueOnce("plain string"); + await expect(state.loadPage()).rejects.toBe("plain string"); + expect(state.getError()?.message).toBe("plain string"); + }); + + it("clears the error on the next successful load", async () => { + client.setStatus("connected"); + client.listResources.mockRejectedValueOnce(new Error("list failed")); + await expect(state.loadPage()).rejects.toThrow(); + client.queueResourcePages({ resources: [resource("a://1")] }); + const cleared = waitForError(state); + await state.loadPage(); + expect(await cleared).toBeNull(); + expect(state.getError()).toBeNull(); + }); + + it("clears the error on disconnect", async () => { + client.setStatus("connected"); + client.listResources.mockRejectedValueOnce(new Error("list failed")); + await expect(state.loadPage()).rejects.toThrow(); + const cleared = waitForError(state); + client.setStatus("disconnected"); + expect(await cleared).toBeNull(); + }); + + it("records a connect-time auto-load failure instead of floating it", async () => { + const spClient = new FakeInspectorClient({ + serverSettings: PAGINATED_SETTINGS, + }); + spClient.setStatus("connected"); + const spState = new PagedResourcesState(spClient); + const boom = new Error("connect-time list failed"); + spClient.listResources.mockRejectedValueOnce(boom); + const errored = waitForError(spState); + spClient.dispatchTypedEvent("connect"); + expect(await errored).toBe(boom); + expect(spState.getError()).toBe(boom); + spState.destroy(); + }); + }); }); diff --git a/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts b/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts index fb1509d5e..b8ee12f50 100644 --- a/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts +++ b/clients/web/src/test/core/mcp/state/pagedToolsState.test.ts @@ -28,6 +28,14 @@ function waitForChange(state: PagedToolsState): Promise { }); } +function waitForError(state: PagedToolsState): Promise { + return new Promise((resolve) => { + state.addEventListener("errorChange", (e) => resolve(e.detail), { + once: true, + }); + }); +} + describe("PagedToolsState", () => { let client: FakeInspectorClient; let state: PagedToolsState; @@ -238,4 +246,61 @@ describe("PagedToolsState", () => { expect(state.getTools()).toEqual([]); }); }); + + // #1998: in paginated mode this store — not the managed one — drives the + // sidebar, so a failed page must surface here or the panel renders empty + // with no alert and no Retry. + describe("load errors (#1998)", () => { + it("records the failure, dispatches errorChange, and re-throws", async () => { + client.setStatus("connected"); + const boom = new Error("list failed"); + client.listTools.mockRejectedValueOnce(boom); + const errorPromise = waitForError(state); + await expect(state.loadPage()).rejects.toThrow("list failed"); + expect(await errorPromise).toBe(boom); + expect(state.getError()).toBe(boom); + }); + + it("wraps a non-Error rejection", async () => { + client.setStatus("connected"); + client.listTools.mockRejectedValueOnce("plain string"); + await expect(state.loadPage()).rejects.toBe("plain string"); + expect(state.getError()?.message).toBe("plain string"); + }); + + it("clears the error on the next successful load", async () => { + client.setStatus("connected"); + client.listTools.mockRejectedValueOnce(new Error("list failed")); + await expect(state.loadPage()).rejects.toThrow(); + client.queueToolPages({ tools: [tool("a")] }); + const cleared = waitForError(state); + await state.loadPage(); + expect(await cleared).toBeNull(); + expect(state.getError()).toBeNull(); + }); + + it("clears the error on disconnect", async () => { + client.setStatus("connected"); + client.listTools.mockRejectedValueOnce(new Error("list failed")); + await expect(state.loadPage()).rejects.toThrow(); + const cleared = waitForError(state); + client.setStatus("disconnected"); + expect(await cleared).toBeNull(); + }); + + it("records a connect-time auto-load failure instead of floating it", async () => { + const spClient = new FakeInspectorClient({ + serverSettings: PAGINATED_SETTINGS, + }); + spClient.setStatus("connected"); + const spState = new PagedToolsState(spClient); + const boom = new Error("connect-time list failed"); + spClient.listTools.mockRejectedValueOnce(boom); + const errored = waitForError(spState); + spClient.dispatchTypedEvent("connect"); + expect(await errored).toBe(boom); + expect(spState.getError()).toBe(boom); + spState.destroy(); + }); + }); }); diff --git a/clients/web/src/test/core/react/useManagedListError.test.tsx b/clients/web/src/test/core/react/useListError.test.tsx similarity index 84% rename from clients/web/src/test/core/react/useManagedListError.test.tsx rename to clients/web/src/test/core/react/useListError.test.tsx index 1eac0afde..4d7c34c15 100644 --- a/clients/web/src/test/core/react/useManagedListError.test.tsx +++ b/clients/web/src/test/core/react/useListError.test.tsx @@ -2,12 +2,12 @@ import { describe, it, expect, beforeEach } from "vitest"; import { act, renderHook } from "@testing-library/react"; import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; import { ManagedToolsState } from "@inspector/core/mcp/state/managedToolsState"; -import { useManagedListError } from "@inspector/core/react/useManagedListError"; +import { useListError } from "@inspector/core/react/useListError"; // The shared subscription behind the four `useManaged*` hooks' `error` field // (#1953). Exercised through ManagedToolsState — any managed list would do, // since the error lives entirely in the shared base. -describe("useManagedListError", () => { +describe("useListError", () => { let client: FakeInspectorClient; let state: ManagedToolsState; const boom = new Error("Invalid result for tools/list: ttlMs required"); @@ -21,12 +21,12 @@ describe("useManagedListError", () => { }); it("returns null when there is no state", () => { - const { result } = renderHook(() => useManagedListError(null)); + const { result } = renderHook(() => useListError(null)); expect(result.current).toBeNull(); }); it("returns null before any load fails", () => { - const { result } = renderHook(() => useManagedListError(state)); + const { result } = renderHook(() => useListError(state)); expect(result.current).toBeNull(); }); @@ -34,12 +34,12 @@ describe("useManagedListError", () => { client.listAllTools.mockRejectedValueOnce(boom); await expect(state.refresh()).rejects.toThrow(boom); - const { result } = renderHook(() => useManagedListError(state)); + const { result } = renderHook(() => useListError(state)); expect(result.current).toBe(boom); }); it("updates when the state dispatches errorChange", async () => { - const { result } = renderHook(() => useManagedListError(state)); + const { result } = renderHook(() => useListError(state)); client.listAllTools.mockRejectedValueOnce(boom); await act(async () => { @@ -51,7 +51,7 @@ describe("useManagedListError", () => { it("clears when a later load succeeds", async () => { client.listAllTools.mockRejectedValueOnce(boom); await expect(state.refresh()).rejects.toThrow(boom); - const { result } = renderHook(() => useManagedListError(state)); + const { result } = renderHook(() => useListError(state)); expect(result.current).toBe(boom); await act(async () => { @@ -65,7 +65,7 @@ describe("useManagedListError", () => { await expect(state.refresh()).rejects.toThrow(boom); const { result, rerender } = renderHook( - ({ s }: { s: ManagedToolsState | null }) => useManagedListError(s), + ({ s }: { s: ManagedToolsState | null }) => useListError(s), { initialProps: { s: state as ManagedToolsState | null } }, ); expect(result.current).toBe(boom); @@ -75,7 +75,7 @@ describe("useManagedListError", () => { }); it("unsubscribes on unmount", async () => { - const { unmount } = renderHook(() => useManagedListError(state)); + const { unmount } = renderHook(() => useListError(state)); unmount(); client.listAllTools.mockRejectedValueOnce(boom); @@ -98,7 +98,7 @@ describe("useManagedListError", () => { const other = new ManagedToolsState(client, 0); const { result, rerender } = renderHook( - ({ s }: { s: ManagedToolsState }) => useManagedListError(s), + ({ s }: { s: ManagedToolsState }) => useListError(s), { initialProps: { s: state } }, ); expect(result.current).toBe(boom); diff --git a/clients/web/src/test/core/react/usePagedPrompts.test.tsx b/clients/web/src/test/core/react/usePagedPrompts.test.tsx index 1a3bca4a3..c50283c99 100644 --- a/clients/web/src/test/core/react/usePagedPrompts.test.tsx +++ b/clients/web/src/test/core/react/usePagedPrompts.test.tsx @@ -118,4 +118,31 @@ describe("usePagedPrompts", () => { await state.loadPage(); expect(result.current.prompts).toEqual([]); }); + + // #1998: paginated mode renders this store, so its load failure is what the + // panel's alert + Retry key off. + it("exposes the state's last load error and clears it on success", async () => { + const { result } = renderHook(() => usePagedPrompts(client, state)); + expect(result.current.error).toBeNull(); + const boom = new Error("list failed"); + client.listPrompts.mockRejectedValueOnce(boom); + await act(async () => { + await expect(result.current.loadPage()).rejects.toThrow("list failed"); + }); + await waitFor(() => { + expect(result.current.error).toBe(boom); + }); + client.queuePromptPages({ prompts: [prompt("a")] }); + await act(async () => { + await result.current.loadPage(); + }); + await waitFor(() => { + expect(result.current.error).toBeNull(); + }); + }); + + it("reports no error when state is null", () => { + const { result } = renderHook(() => usePagedPrompts(client, null)); + expect(result.current.error).toBeNull(); + }); }); diff --git a/clients/web/src/test/core/react/usePagedResources.test.tsx b/clients/web/src/test/core/react/usePagedResources.test.tsx index b5b6be44a..9ee0dd2d3 100644 --- a/clients/web/src/test/core/react/usePagedResources.test.tsx +++ b/clients/web/src/test/core/react/usePagedResources.test.tsx @@ -125,4 +125,31 @@ describe("usePagedResources", () => { await state.loadPage(); expect(result.current.resources).toEqual([]); }); + + // #1998: paginated mode renders this store, so its load failure is what the + // panel's alert + Retry key off. + it("exposes the state's last load error and clears it on success", async () => { + const { result } = renderHook(() => usePagedResources(client, state)); + expect(result.current.error).toBeNull(); + const boom = new Error("list failed"); + client.listResources.mockRejectedValueOnce(boom); + await act(async () => { + await expect(result.current.loadPage()).rejects.toThrow("list failed"); + }); + await waitFor(() => { + expect(result.current.error).toBe(boom); + }); + client.queueResourcePages({ resources: [resource("a://1")] }); + await act(async () => { + await result.current.loadPage(); + }); + await waitFor(() => { + expect(result.current.error).toBeNull(); + }); + }); + + it("reports no error when state is null", () => { + const { result } = renderHook(() => usePagedResources(client, null)); + expect(result.current.error).toBeNull(); + }); }); diff --git a/clients/web/src/test/core/react/usePagedTools.test.tsx b/clients/web/src/test/core/react/usePagedTools.test.tsx index c5bc5f85c..f6342de0a 100644 --- a/clients/web/src/test/core/react/usePagedTools.test.tsx +++ b/clients/web/src/test/core/react/usePagedTools.test.tsx @@ -125,4 +125,31 @@ describe("usePagedTools", () => { await state.loadPage(); expect(result.current.tools).toEqual([]); }); + + // #1998: paginated mode renders this store, so its load failure is what the + // panel's alert + Retry key off. + it("exposes the state's last load error and clears it on success", async () => { + const { result } = renderHook(() => usePagedTools(client, state)); + expect(result.current.error).toBeNull(); + const boom = new Error("list failed"); + client.listTools.mockRejectedValueOnce(boom); + await act(async () => { + await expect(result.current.loadPage()).rejects.toThrow("list failed"); + }); + await waitFor(() => { + expect(result.current.error).toBe(boom); + }); + client.queueToolPages({ tools: [tool("a")] }); + await act(async () => { + await result.current.loadPage(); + }); + await waitFor(() => { + expect(result.current.error).toBeNull(); + }); + }); + + it("reports no error when state is null", () => { + const { result } = renderHook(() => usePagedTools(client, null)); + expect(result.current.error).toBeNull(); + }); }); diff --git a/core/mcp/state/pagedPromptsState.ts b/core/mcp/state/pagedPromptsState.ts index ed649711c..80fbe6caa 100644 --- a/core/mcp/state/pagedPromptsState.ts +++ b/core/mcp/state/pagedPromptsState.ts @@ -21,6 +21,8 @@ import type { PagePaginationState } from "./pagedToolsState.js"; export interface PagedPromptsStateEventMap { promptsChange: Prompt[]; paginationChange: PagePaginationState; + /** The last page load's failure, or `null` once a load succeeds. */ + errorChange: Error | null; } export interface LoadPageResult { @@ -35,6 +37,11 @@ export class PagedPromptsState extends TypedEventTarget void) | null = null; @@ -43,7 +50,11 @@ export class PagedPromptsState extends TypedEventTarget { if (this.client?.getServerSettings()?.paginatedLists) { - void this.loadPage(undefined); + // No caller to await this one, so its rejection is caught here rather + // than left to become an unhandled rejection. Not a swallow: + // `loadPage` has already recorded the failure via `setError`, and the + // list panel renders it (#1998). + void this.loadPage(undefined).catch(() => {}); } }; const onStatusChange = (): void => { @@ -70,6 +81,20 @@ export class PagedPromptsState extends TypedEventTarget void) | null = null; @@ -44,7 +51,11 @@ export class PagedResourcesState extends TypedEventTarget { if (this.client?.getServerSettings()?.paginatedLists) { - void this.loadPage(undefined); + // No caller to await this one, so its rejection is caught here rather + // than left to become an unhandled rejection. Not a swallow: + // `loadPage` has already recorded the failure via `setError`, and the + // list panel renders it (#1998). + void this.loadPage(undefined).catch(() => {}); } }; const onStatusChange = (): void => { @@ -71,6 +82,20 @@ export class PagedResourcesState extends TypedEventTarget { // page"): both calls would otherwise read the same cursor and append the // same page twice. A load in flight makes the next `loadPage` a no-op (#1721). private loading = false; + // The last page load's failure, kept as observable state so a failing page + // renders an alert with a Retry instead of an empty sidebar. The aggregate + // stores carry the same state, but they are not the display source in + // paginated mode, so their error never fires there (#1998). + private error: Error | null = null; private client: InspectorClientProtocol | null = null; private unsubscribe: (() => void) | null = null; @@ -55,7 +62,11 @@ export class PagedToolsState extends TypedEventTarget { // Auto-load page 1 only in paginated mode — otherwise the managed // (aggregate) state is the display source and this stays idle (#1721). if (this.client?.getServerSettings()?.paginatedLists) { - void this.loadPage(undefined); + // No caller to await this one, so its rejection is caught here rather + // than left to become an unhandled rejection. Not a swallow: + // `loadPage` has already recorded the failure via `setError`, and the + // list panel renders it (#1998). + void this.loadPage(undefined).catch(() => {}); } }; const onStatusChange = (): void => { @@ -82,6 +93,20 @@ export class PagedToolsState extends TypedEventTarget { return { nextCursor: this.nextCursor, pageCount: this.pageCount }; } + /** The last page load's failure, or `null` when the last load succeeded. */ + getError(): Error | null { + return this.error; + } + + // Compared by identity rather than message: two distinct failures with the + // same text are still two events, and a re-render on a repeat failure is + // cheap next to silently coalescing them. + private setError(value: Error | null): void { + if (this.error === value) return; + this.error = value; + this.dispatchTypedEvent("errorChange", value); + } + /** Clear the accumulated list and pagination progress. */ clear(): void { this.reset(); @@ -93,6 +118,9 @@ export class PagedToolsState extends TypedEventTarget { this.pageCount = 0; this.dispatchTypedEvent("toolsChange", this.tools); this.dispatchTypedEvent("paginationChange", this.getPagination()); + // A disconnect ends the session the error belonged to — a stale + // "couldn't load" must not outlive it into the next connect. + this.setError(null); } async loadPage(cursor?: string): Promise { @@ -116,9 +144,17 @@ export class PagedToolsState extends TypedEventTarget { : [...this.tools, ...result.tools]; this.pageCount = cursor === undefined ? 1 : this.pageCount + 1; this.nextCursor = result.nextCursor; + this.setError(null); this.dispatchTypedEvent("toolsChange", this.tools); this.dispatchTypedEvent("paginationChange", this.getPagination()); return { tools: result.tools, nextCursor: result.nextCursor }; + } catch (err) { + // Recorded as observable state AND re-thrown: the state drives the + // panel's alert, while the rejection is what a caller's auth-recovery + // wrapper keys off to detect a 401 and start a re-authorization. The + // connect-time load, which has no such caller, catches it above. + this.setError(err instanceof Error ? err : new Error(String(err))); + throw err; } finally { this.loading = false; } @@ -130,5 +166,6 @@ export class PagedToolsState extends TypedEventTarget { this.tools = []; this.nextCursor = undefined; this.pageCount = 0; + this.error = null; } } diff --git a/core/react/useManagedListError.ts b/core/react/useListError.ts similarity index 58% rename from core/react/useManagedListError.ts rename to core/react/useListError.ts index 64438e523..01da48db9 100644 --- a/core/react/useManagedListError.ts +++ b/core/react/useListError.ts @@ -1,35 +1,46 @@ import { useCallback, useSyncExternalStore } from "react"; -import type { ManagedListEventMap } from "../mcp/state/managedListState.js"; import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; /** - * The slice of a managed list state this hook needs. Declared structurally - * rather than as `ManagedListState` so the four list hooks can share it - * without threading their item type through — the error is the same shape for - * all of them. + * The single event both list-state families expose for this. Declared here + * rather than imported from either so the hook depends on neither: the managed + * (aggregate) states and the paged states each carry an `errorChange` of this + * shape, and this is the whole contract the hook needs. */ -export interface ManagedListErrorSource { +interface ListErrorEventMap { + errorChange: Error | null; +} + +/** + * The slice of a list state this hook needs. Declared structurally rather than + * as a concrete state class so every list hook can share it without threading + * its item type through — the error is the same shape for all of them. + */ +export interface ListErrorSource { getError(): Error | null; addEventListener( type: "errorChange", listener: ( - event: TypedEventGeneric, + event: TypedEventGeneric, ) => void, ): void; removeEventListener( type: "errorChange", listener: ( - event: TypedEventGeneric, + event: TypedEventGeneric, ) => void, ): void; } /** - * Subscribe to a managed list state's last-fetch error (#1953). + * Subscribe to a list state's last-fetch error (#1953, #1998). * - * Shared by the four `useManaged*` hooks so a list load that fails — including - * the connect-time one, which has no caller to await it — reaches the UI - * instead of only the console. `null` means the last fetch succeeded. + * Shared by the four `useManaged*` hooks and the three `usePaged*` hooks so a + * list load that fails — including the connect-time one, which has no caller + * to await it — reaches the UI instead of only the console. `null` means the + * last fetch succeeded. Both families need it because the paged stores are the + * display source in paginated mode, where the managed stores deliberately + * never fetch (#1998). * * Built on `useSyncExternalStore` rather than the `useState` + `useEffect` * subscribe pattern the sibling hooks use. Re-syncing state from the `state` @@ -44,9 +55,7 @@ export interface ManagedListErrorSource { * which it is: it returns the stored `Error` instance itself (or `null`), never * a fresh object. */ -export function useManagedListError( - state: ManagedListErrorSource | null, -): Error | null { +export function useListError(state: ListErrorSource | null): Error | null { const subscribe = useCallback( (onStoreChange: () => void) => { if (!state) return () => {}; diff --git a/core/react/useManagedPrompts.ts b/core/react/useManagedPrompts.ts index e0de0bb13..8a14d0df1 100644 --- a/core/react/useManagedPrompts.ts +++ b/core/react/useManagedPrompts.ts @@ -6,7 +6,7 @@ import type { } from "../mcp/state/managedPromptsState.js"; import type { Prompt } from "@modelcontextprotocol/client"; import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; -import { useManagedListError } from "./useManagedListError.js"; +import { useListError } from "./useListError.js"; export interface UseManagedPromptsResult { /** @@ -72,7 +72,7 @@ export function useManagedPrompts( }; }, [managedPromptsState]); - const error = useManagedListError(managedPromptsState); + const error = useListError(managedPromptsState); const refresh = useCallback(async (): Promise => { if (!managedPromptsState || !client) return []; diff --git a/core/react/useManagedResourceTemplates.ts b/core/react/useManagedResourceTemplates.ts index 40a6b994f..61a4c382c 100644 --- a/core/react/useManagedResourceTemplates.ts +++ b/core/react/useManagedResourceTemplates.ts @@ -6,7 +6,7 @@ import type { } from "../mcp/state/managedResourceTemplatesState.js"; import type { ResourceTemplateType as ResourceTemplate } from "@modelcontextprotocol/client"; import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; -import { useManagedListError } from "./useManagedListError.js"; +import { useListError } from "./useListError.js"; export interface UseManagedResourceTemplatesResult { /** @@ -57,7 +57,7 @@ export function useManagedResourceTemplates( }; }, [managedResourceTemplatesState]); - const error = useManagedListError(managedResourceTemplatesState); + const error = useListError(managedResourceTemplatesState); const refresh = useCallback(async (): Promise => { if (!managedResourceTemplatesState || !client) return []; diff --git a/core/react/useManagedResources.ts b/core/react/useManagedResources.ts index 127860622..ad9b1eb57 100644 --- a/core/react/useManagedResources.ts +++ b/core/react/useManagedResources.ts @@ -6,7 +6,7 @@ import type { } from "../mcp/state/managedResourcesState.js"; import type { Resource } from "@modelcontextprotocol/client"; import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; -import { useManagedListError } from "./useManagedListError.js"; +import { useListError } from "./useListError.js"; export interface UseManagedResourcesResult { /** @@ -83,7 +83,7 @@ export function useManagedResources( }; }, [managedResourcesState]); - const error = useManagedListError(managedResourcesState); + const error = useListError(managedResourcesState); const refresh = useCallback(async (): Promise => { if (!managedResourcesState || !client) return []; diff --git a/core/react/useManagedTools.ts b/core/react/useManagedTools.ts index d7ed1c223..3b8050386 100644 --- a/core/react/useManagedTools.ts +++ b/core/react/useManagedTools.ts @@ -4,7 +4,7 @@ import type { ManagedToolsState } from "../mcp/state/managedToolsState.js"; import type { ManagedToolsStateEventMap } from "../mcp/state/managedToolsState.js"; import type { Tool } from "@modelcontextprotocol/client"; import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; -import { useManagedListError } from "./useManagedListError.js"; +import { useListError } from "./useListError.js"; export interface UseManagedToolsResult { /** @@ -72,7 +72,7 @@ export function useManagedTools( }; }, [managedToolsState]); - const error = useManagedListError(managedToolsState); + const error = useListError(managedToolsState); const refresh = useCallback(async (): Promise => { if (!managedToolsState || !client) return []; diff --git a/core/react/usePagedPrompts.ts b/core/react/usePagedPrompts.ts index 94650c42f..3e3efd44c 100644 --- a/core/react/usePagedPrompts.ts +++ b/core/react/usePagedPrompts.ts @@ -7,6 +7,7 @@ import type { } from "../mcp/state/pagedPromptsState.js"; import type { Prompt } from "@modelcontextprotocol/client"; import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import { useListError } from "./useListError.js"; export interface UsePagedPromptsResult { prompts: Prompt[]; @@ -14,6 +15,12 @@ export interface UsePagedPromptsResult { nextCursor?: string; /** Pages loaded since the last reset (page 1 = 1). */ pageCount: number; + /** + * The last page load's failure, or `null` when it succeeded. In paginated + * mode this store is the display source, so this — not the managed + * store's error — is what the panel renders (#1998). + */ + error: Error | null; loadPage: ( cursor?: string, metadata?: Record, @@ -72,6 +79,8 @@ export function usePagedPrompts( }; }, [pagedPromptsState]); + const error = useListError(pagedPromptsState); + const loadPage = useCallback( async ( cursor?: string, @@ -89,5 +98,5 @@ export function usePagedPrompts( pagedPromptsState?.clear(); }, [pagedPromptsState]); - return { prompts, nextCursor, pageCount, loadPage, clear }; + return { prompts, nextCursor, pageCount, error, loadPage, clear }; } diff --git a/core/react/usePagedResources.ts b/core/react/usePagedResources.ts index 25336a5d6..3e5e3f8a4 100644 --- a/core/react/usePagedResources.ts +++ b/core/react/usePagedResources.ts @@ -7,6 +7,7 @@ import type { } from "../mcp/state/pagedResourcesState.js"; import type { Resource } from "@modelcontextprotocol/client"; import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import { useListError } from "./useListError.js"; export interface UsePagedResourcesResult { resources: Resource[]; @@ -14,6 +15,12 @@ export interface UsePagedResourcesResult { nextCursor?: string; /** Pages loaded since the last reset (page 1 = 1). */ pageCount: number; + /** + * The last page load's failure, or `null` when it succeeded. In paginated + * mode this store is the display source, so this — not the managed + * store's error — is what the panel renders (#1998). + */ + error: Error | null; loadPage: ( cursor?: string, metadata?: Record, @@ -78,6 +85,8 @@ export function usePagedResources( }; }, [pagedResourcesState]); + const error = useListError(pagedResourcesState); + const loadPage = useCallback( async ( cursor?: string, @@ -95,5 +104,5 @@ export function usePagedResources( pagedResourcesState?.clear(); }, [pagedResourcesState]); - return { resources, nextCursor, pageCount, loadPage, clear }; + return { resources, nextCursor, pageCount, error, loadPage, clear }; } diff --git a/core/react/usePagedTools.ts b/core/react/usePagedTools.ts index 026b5831c..844a862da 100644 --- a/core/react/usePagedTools.ts +++ b/core/react/usePagedTools.ts @@ -7,6 +7,7 @@ import type { } from "../mcp/state/pagedToolsState.js"; import type { Tool } from "@modelcontextprotocol/client"; import type { TypedEventGeneric } from "../mcp/typedEventTarget.js"; +import { useListError } from "./useListError.js"; export interface UsePagedToolsResult { tools: Tool[]; @@ -14,6 +15,12 @@ export interface UsePagedToolsResult { nextCursor?: string; /** Pages loaded since the last reset (page 1 = 1). */ pageCount: number; + /** + * The last page load's failure, or `null` when it succeeded. In paginated + * mode this store is the display source, so this — not the managed + * store's error — is what the panel renders (#1998). + */ + error: Error | null; loadPage: (cursor?: string) => Promise; clear: () => void; } @@ -68,6 +75,8 @@ export function usePagedTools( }; }, [pagedToolsState]); + const error = useListError(pagedToolsState); + const loadPage = useCallback( async (cursor?: string): Promise => { if (!pagedToolsState || !client) { @@ -82,5 +91,5 @@ export function usePagedTools( pagedToolsState?.clear(); }, [pagedToolsState]); - return { tools, nextCursor, pageCount, loadPage, clear }; + return { tools, nextCursor, pageCount, error, loadPage, clear }; }