chore(web): address Copilot's second review on #2130
Two findings, both valid. `onCallTool`, `onGetPrompt`, `onReadResource` and `onOpenApp` await `handleCommandScopedAuthRecovery` from inside their catch blocks, and a rejection thrown from a catch is not caught by that same catch — so it escapes the handler. That helper awaits `checkAuthChallengeSatisfied` and `pushRemoteAuthState`, both of which reach the backend and can reject, so the bare `void` on those four could still leak. Rather than patch the four and keep a comment enumerating which handlers are safe, all seven wrappers now terminate the same way: a reporting `.catch` via `reportDispatchFailure`. Whether a handler "owns its failures" turned out not to be reliably knowable by reading — the previous two attempts at that judgement were both wrong — so the code no longer depends on getting it right. A handler that does surface its own failure resolves and never reaches the catch, so nothing double-toasts. The reorder callback lifted out of the JSX had no App-level coverage. The `InspectorView` double grows a reorder control, with tests for both forwarding the ordered ids and reporting a rejected reorder. Adds the auth-recovery escape test the review asked for. All four new rejection tests were confirmed to fail against the bare-`void` code, so they pin the fixes rather than merely passing beside them. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
4e0f4e5460
commit
0139fd74f8
2 changed files with 170 additions and 36 deletions
|
|
@ -594,6 +594,11 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
|
|||
edit-server
|
||||
</button>
|
||||
<button onClick={() => props.servers.onServerAdd()}>add-server</button>
|
||||
{/* The real drag-and-drop reorder lives inside this mocked view, so the
|
||||
callback is only reachable through a control like this one. */}
|
||||
<button onClick={() => props.servers.onServerReorder(["B", "A"])}>
|
||||
reorder-servers
|
||||
</button>
|
||||
<span data-testid="highlighted-servers">
|
||||
{(props.servers.highlightedServerIds ?? []).join(",") || "none"}
|
||||
</span>
|
||||
|
|
@ -3806,6 +3811,30 @@ describe("App background command rejections (#2049)", () => {
|
|||
return user;
|
||||
};
|
||||
|
||||
// A complete `useServers` return with one injected mutator, so the mock stays
|
||||
// type-checked against the hook's contract rather than spread from a call.
|
||||
const serversWithReorder = (
|
||||
reorderServers: ReturnType<typeof useServers>["reorderServers"],
|
||||
): ReturnType<typeof useServers> => ({
|
||||
servers: [
|
||||
{
|
||||
id: "A",
|
||||
name: "PlotRocket",
|
||||
config: { type: "streamable-http", url: "https://api.example.com/mcp" },
|
||||
connection: { status: "disconnected" },
|
||||
},
|
||||
],
|
||||
loading: false,
|
||||
error: undefined,
|
||||
refresh: vi.fn().mockResolvedValue(undefined),
|
||||
addServer: vi.fn().mockResolvedValue(undefined),
|
||||
updateServer: vi.fn().mockResolvedValue(undefined),
|
||||
updateServerSettings: vi.fn().mockResolvedValue(undefined),
|
||||
removeServer: vi.fn().mockResolvedValue(undefined),
|
||||
reorderServers,
|
||||
importSource: vi.fn().mockResolvedValue({ servers: {} }),
|
||||
});
|
||||
|
||||
it("does not leak an unhandled rejection when an all-pages Refresh fails", async () => {
|
||||
vi.mocked(useManagedTools).mockReturnValue({
|
||||
tools: [],
|
||||
|
|
@ -3921,6 +3950,92 @@ describe("App background command rejections (#2049)", () => {
|
|||
}
|
||||
});
|
||||
|
||||
// The four command handlers await `handleCommandScopedAuthRecovery` from
|
||||
// *inside* their catch blocks, and a rejection thrown from a catch is not
|
||||
// caught by that same catch — so it escapes the handler entirely. That helper
|
||||
// awaits `checkAuthChallengeSatisfied`, which reaches the backend and can
|
||||
// reject. Before #2130's review this escaped a bare `void` as a global
|
||||
// unhandled rejection with nothing shown to the user.
|
||||
it("toasts an auth-recovery failure that escapes a tool call instead of leaking it", async () => {
|
||||
const rejections = captureUnhandledRejections();
|
||||
try {
|
||||
const user = await connect();
|
||||
const client = clientInstances[0] as EventTarget & {
|
||||
callTool: ReturnType<typeof vi.fn>;
|
||||
};
|
||||
client.callTool.mockRejectedValueOnce(
|
||||
new AuthRecoveryRequiredError(
|
||||
new URL("https://as.example.com/authorize"),
|
||||
{ reason: "unauthorized" },
|
||||
),
|
||||
);
|
||||
// The recovery helper's first await is the one that breaks.
|
||||
rejectNextChallengeCheck(new Error("challenge check failed"));
|
||||
|
||||
await user.click(screen.getByText("call"));
|
||||
|
||||
await waitFor(() =>
|
||||
expect(notificationsMock.show).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
title: "Tool call failed",
|
||||
message: "challenge check failed",
|
||||
color: "red",
|
||||
}),
|
||||
),
|
||||
);
|
||||
await settleRejections();
|
||||
expect(rejections.seen).toEqual([]);
|
||||
} finally {
|
||||
rejections.stop();
|
||||
}
|
||||
});
|
||||
|
||||
// The reorder handler was lifted out of the JSX into a named callback with a
|
||||
// `.catch` that toasts (#2130). `reorderServers` reverts its own optimistic
|
||||
// ordering and re-throws — on a 409 from a racing external edit, or a network
|
||||
// error — so without the toast the drag just silently bounces back.
|
||||
it("forwards a reorder to the server list", async () => {
|
||||
const reorderServers = vi.fn().mockResolvedValue(undefined);
|
||||
vi.mocked(useServers).mockReturnValue(serversWithReorder(reorderServers));
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<App />);
|
||||
|
||||
await user.click(screen.getByText("reorder-servers"));
|
||||
|
||||
await waitFor(() =>
|
||||
expect(reorderServers).toHaveBeenCalledWith(["B", "A"]),
|
||||
);
|
||||
expect(notificationsMock.show).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("toasts a failed reorder instead of leaking it", async () => {
|
||||
const reorderServers = vi
|
||||
.fn()
|
||||
.mockRejectedValue(new Error("stale server list"));
|
||||
vi.mocked(useServers).mockReturnValue(serversWithReorder(reorderServers));
|
||||
const rejections = captureUnhandledRejections();
|
||||
try {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<App />);
|
||||
|
||||
await user.click(screen.getByText("reorder-servers"));
|
||||
|
||||
await waitFor(() =>
|
||||
expect(notificationsMock.show).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
title: "Failed to reorder servers",
|
||||
message: "stale server list",
|
||||
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 {
|
||||
|
|
|
|||
|
|
@ -122,6 +122,21 @@ import { FetchBodyDroppedToastMessage } from "./components/elements/Toasts/Fetch
|
|||
import { OutputValidationToastMessage } from "./components/elements/Toasts/OutputValidationToastMessage";
|
||||
import { ReAuthBannerBar } from "./components/groups/ReAuthBanner/ReAuthBannerBar";
|
||||
|
||||
/**
|
||||
* Terminates a dispatched handler's promise by reporting the failure, for the
|
||||
* `InspectorView` action wrappers below. Module scope because it closes over
|
||||
* nothing — the title is the only thing that varies per action.
|
||||
*/
|
||||
function reportDispatchFailure(title: string) {
|
||||
return (err: unknown) => {
|
||||
notifications.show({
|
||||
title,
|
||||
message: err instanceof Error ? err.message : String(err),
|
||||
color: "red",
|
||||
});
|
||||
};
|
||||
}
|
||||
|
||||
function App() {
|
||||
const { onToggleTheme } = useThemeToggle();
|
||||
|
||||
|
|
@ -1550,71 +1565,75 @@ function App() {
|
|||
[],
|
||||
);
|
||||
|
||||
// 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",
|
||||
});
|
||||
};
|
||||
}, []);
|
||||
|
||||
// Every `InspectorView` action prop is synchronous while the handler behind
|
||||
// it is async, so each wrapper below has to terminate its promise somehow.
|
||||
// They all do it the same way: a reporting `.catch`, never a bare `void`.
|
||||
//
|
||||
// The uniformity is the point. Deciding per handler whether it "owns its
|
||||
// failures" is a judgement this PR got wrong twice (#2130 review) — the
|
||||
// handlers really do catch their ordinary errors, but a rejection can still
|
||||
// escape by a route the reading misses:
|
||||
//
|
||||
// - `onToggleConnection` awaits `initialConfigSettledRef` and constructs
|
||||
// the client via `setupClientForServer` outside its own try/catch, and
|
||||
// its already-connected branch is a `try/finally` that lets a failed
|
||||
// transport close propagate. `onDisconnect` is that `try/finally` alone.
|
||||
// - `onCallTool` / `onGetPrompt` / `onReadResource` / `onOpenApp` await
|
||||
// `handleCommandScopedAuthRecovery` from *inside* their catch blocks, and
|
||||
// a rejection thrown from a catch is not caught by that same catch. That
|
||||
// helper awaits `checkAuthChallengeSatisfied` and `pushRemoteAuthState`,
|
||||
// both of which reach the backend and can reject.
|
||||
//
|
||||
// A handler that does surface its own failure resolves, so it never reaches
|
||||
// this `.catch` and there is no double-toast; what lands here is only the
|
||||
// escape, which would otherwise be a global unhandled rejection with nothing
|
||||
// shown to the user.
|
||||
const dispatchToggleConnection = useCallback(
|
||||
(id: string) => {
|
||||
onToggleConnection(id).catch(
|
||||
reportConnectionFailure("Failed to change the connection"),
|
||||
reportDispatchFailure("Failed to change the connection"),
|
||||
);
|
||||
},
|
||||
[onToggleConnection, reportConnectionFailure],
|
||||
[onToggleConnection],
|
||||
);
|
||||
const dispatchDisconnect = useCallback(() => {
|
||||
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.
|
||||
onDisconnect().catch(reportDispatchFailure("Failed to disconnect"));
|
||||
}, [onDisconnect]);
|
||||
const dispatchCallTool = useCallback(
|
||||
(name: string, args: Record<string, unknown>, runAsTask?: boolean) => {
|
||||
void onCallTool(name, args, runAsTask);
|
||||
onCallTool(name, args, runAsTask).catch(
|
||||
reportDispatchFailure("Tool call failed"),
|
||||
);
|
||||
},
|
||||
[onCallTool],
|
||||
);
|
||||
const dispatchGetPrompt = useCallback(
|
||||
(name: string, args: Record<string, string>) => {
|
||||
void onGetPrompt(name, args);
|
||||
onGetPrompt(name, args).catch(
|
||||
reportDispatchFailure("Failed to get prompt"),
|
||||
);
|
||||
},
|
||||
[onGetPrompt],
|
||||
);
|
||||
const dispatchReadResource = useCallback(
|
||||
(uri: string) => {
|
||||
void onReadResource(uri);
|
||||
onReadResource(uri).catch(
|
||||
reportDispatchFailure("Failed to read resource"),
|
||||
);
|
||||
},
|
||||
[onReadResource],
|
||||
);
|
||||
const dispatchCancelTask = useCallback(
|
||||
(taskId: string) => {
|
||||
void onCancelTask(taskId);
|
||||
onCancelTask(taskId).catch(
|
||||
reportDispatchFailure("Failed to cancel task"),
|
||||
);
|
||||
},
|
||||
[onCancelTask],
|
||||
);
|
||||
const dispatchOpenApp = useCallback(
|
||||
(name: string, args: Record<string, unknown>) => {
|
||||
void onOpenApp(name, args);
|
||||
onOpenApp(name, args).catch(reportDispatchFailure("Failed to open app"));
|
||||
},
|
||||
[onOpenApp],
|
||||
);
|
||||
|
|
|
|||
Loading…
Reference in a new issue