fix: surface list-fetch failures in paginated mode (#1998)
#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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
885228699e
commit
de60983ccc
7 changed files with 523 additions and 42 deletions
|
|
@ -28,6 +28,14 @@ function waitForChange(state: PagedPromptsState): Promise<Prompt[]> {
|
|||
});
|
||||
}
|
||||
|
||||
function waitForError(state: PagedPromptsState): Promise<Error | null> {
|
||||
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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -28,6 +28,14 @@ function waitForChange(state: PagedResourcesState): Promise<Resource[]> {
|
|||
});
|
||||
}
|
||||
|
||||
function waitForError(state: PagedResourcesState): Promise<Error | null> {
|
||||
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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -28,6 +28,14 @@ function waitForChange(state: PagedToolsState): Promise<Tool[]> {
|
|||
});
|
||||
}
|
||||
|
||||
function waitForError(state: PagedToolsState): Promise<Error | null> {
|
||||
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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
112
clients/web/src/test/core/react/useListError.test.tsx
Normal file
112
clients/web/src/test/core/react/useListError.test.tsx
Normal file
|
|
@ -0,0 +1,112 @@
|
|||
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 { 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("useListError", () => {
|
||||
let client: FakeInspectorClient;
|
||||
let state: ManagedToolsState;
|
||||
const boom = new Error("Invalid result for tools/list: ttlMs required");
|
||||
|
||||
beforeEach(() => {
|
||||
client = new FakeInspectorClient({
|
||||
status: "connected",
|
||||
capabilities: { tools: {} },
|
||||
});
|
||||
state = new ManagedToolsState(client, 0);
|
||||
});
|
||||
|
||||
it("returns null when there is no state", () => {
|
||||
const { result } = renderHook(() => useListError(null));
|
||||
expect(result.current).toBeNull();
|
||||
});
|
||||
|
||||
it("returns null before any load fails", () => {
|
||||
const { result } = renderHook(() => useListError(state));
|
||||
expect(result.current).toBeNull();
|
||||
});
|
||||
|
||||
it("seeds from an error the state already holds", async () => {
|
||||
client.listAllTools.mockRejectedValueOnce(boom);
|
||||
await expect(state.refresh()).rejects.toThrow(boom);
|
||||
|
||||
const { result } = renderHook(() => useListError(state));
|
||||
expect(result.current).toBe(boom);
|
||||
});
|
||||
|
||||
it("updates when the state dispatches errorChange", async () => {
|
||||
const { result } = renderHook(() => useListError(state));
|
||||
|
||||
client.listAllTools.mockRejectedValueOnce(boom);
|
||||
await act(async () => {
|
||||
await expect(state.refresh()).rejects.toThrow(boom);
|
||||
});
|
||||
expect(result.current).toBe(boom);
|
||||
});
|
||||
|
||||
it("clears when a later load succeeds", async () => {
|
||||
client.listAllTools.mockRejectedValueOnce(boom);
|
||||
await expect(state.refresh()).rejects.toThrow(boom);
|
||||
const { result } = renderHook(() => useListError(state));
|
||||
expect(result.current).toBe(boom);
|
||||
|
||||
await act(async () => {
|
||||
await state.refresh();
|
||||
});
|
||||
expect(result.current).toBeNull();
|
||||
});
|
||||
|
||||
it("resets to null when the state becomes null", async () => {
|
||||
client.listAllTools.mockRejectedValueOnce(boom);
|
||||
await expect(state.refresh()).rejects.toThrow(boom);
|
||||
|
||||
const { result, rerender } = renderHook(
|
||||
({ s }: { s: ManagedToolsState | null }) => useListError(s),
|
||||
{ initialProps: { s: state as ManagedToolsState | null } },
|
||||
);
|
||||
expect(result.current).toBe(boom);
|
||||
|
||||
rerender({ s: null });
|
||||
expect(result.current).toBeNull();
|
||||
});
|
||||
|
||||
it("unsubscribes on unmount", async () => {
|
||||
const { unmount } = renderHook(() => useListError(state));
|
||||
unmount();
|
||||
|
||||
client.listAllTools.mockRejectedValueOnce(boom);
|
||||
// No act() wrapper: a listener still attached would warn about an update
|
||||
// outside act, and the assertion below would be the only other signal.
|
||||
await expect(state.refresh()).rejects.toThrow(boom);
|
||||
expect(state.getError()).toBe(boom);
|
||||
});
|
||||
|
||||
// The useState+useEffect subscribe pattern would render one frame carrying
|
||||
// the PREVIOUS store's error here, before the effect re-synced. With
|
||||
// useSyncExternalStore the snapshot is read during render, so the swap lands
|
||||
// in the same frame — asserted immediately after rerender, with no waitFor
|
||||
// and no act() flush, which is what makes it a regression test rather than a
|
||||
// restatement of eventual consistency.
|
||||
it("reflects a store swap in the same frame", async () => {
|
||||
client.listAllTools.mockRejectedValueOnce(boom);
|
||||
await expect(state.refresh()).rejects.toThrow(boom);
|
||||
|
||||
const other = new ManagedToolsState(client, 0);
|
||||
|
||||
const { result, rerender } = renderHook(
|
||||
({ s }: { s: ManagedToolsState }) => useListError(s),
|
||||
{ initialProps: { s: state } },
|
||||
);
|
||||
expect(result.current).toBe(boom);
|
||||
|
||||
rerender({ s: other });
|
||||
expect(result.current).toBeNull();
|
||||
|
||||
rerender({ s: state });
|
||||
expect(result.current).toBe(boom);
|
||||
});
|
||||
});
|
||||
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue