chore(web): address Copilot review on #2130
Three findings, all valid on verification. `onToggleConnection` and `onDisconnect` do not own every rejection, so a bare `void` on them turns an ordinary Connect/Disconnect click into a global unhandled rejection with nothing shown to the user. The toggle 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. Both are now terminated with a `.catch` that toasts the failure. The blanket comment claiming all seven wrappers own their failures was wrong. The five command handlers (`onCallTool`, `onGetPrompt`, `onReadResource`, `onCancelTask`, `onOpenApp`) do each end in a `catch`, so `void` stays correct for them; the comment now splits the two groups and says which is which. Adds the two tests the review asked for, covering the rejected-close path through both the toggle and the explicit Disconnect. Each fails against the previous `void` code, so they pin the fix rather than merely passing. The `InspectorView` double grows a `disconnect` button, since the standalone `onDisconnect` was otherwise unreachable from it. Also restores the `PropOverrides` doc example, which the scripted regrouping had rewritten inside its own JSDoc into a triple-nested shape that would not type-check if copied. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
aacfef3ee2
commit
37add967cd
3 changed files with 103 additions and 13 deletions
|
|
@ -499,6 +499,11 @@ vi.mock("./components/views/InspectorView/InspectorView", () => ({
|
||||||
<button onClick={() => props.connection.onToggleConnection("B")}>
|
<button onClick={() => props.connection.onToggleConnection("B")}>
|
||||||
connect-b
|
connect-b
|
||||||
</button>
|
</button>
|
||||||
|
{/* The header's explicit Disconnect, which routes to the standalone
|
||||||
|
`onDisconnect` rather than through the toggle. */}
|
||||||
|
<button onClick={() => props.connection.onDisconnect()}>
|
||||||
|
disconnect
|
||||||
|
</button>
|
||||||
<button onClick={() => props.servers.onConnectionInfo("A")}>
|
<button onClick={() => props.servers.onConnectionInfo("A")}>
|
||||||
open-connection-info
|
open-connection-info
|
||||||
</button>
|
</button>
|
||||||
|
|
@ -3878,6 +3883,70 @@ describe("App background command rejections (#2049)", () => {
|
||||||
rejections.stop();
|
rejections.stop();
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// `onToggleConnection` and `onDisconnect` both close a live session with
|
||||||
|
// `try { await disconnect() } finally { finalizeExplicitDisconnect() }` — a
|
||||||
|
// `finally`, not a `catch` — so a transport that fails to close rejects out
|
||||||
|
// of the handler. App discards neither: both are terminated with a `.catch`
|
||||||
|
// that toasts, because a bare `void` would turn an ordinary click into a
|
||||||
|
// global unhandled rejection with nothing shown to the user (#2130 review).
|
||||||
|
const CLOSE_FAILURE = new Error("close boom");
|
||||||
|
|
||||||
|
it("toasts a failed transport close from the connection toggle instead of leaking it", async () => {
|
||||||
|
const rejections = captureUnhandledRejections();
|
||||||
|
try {
|
||||||
|
const user = await connect();
|
||||||
|
const client = clientInstances[0] as EventTarget & {
|
||||||
|
disconnect: ReturnType<typeof vi.fn>;
|
||||||
|
};
|
||||||
|
client.disconnect.mockRejectedValueOnce(CLOSE_FAILURE);
|
||||||
|
|
||||||
|
// Second click on the same, now-connected server takes the disconnect
|
||||||
|
// branch of the toggle.
|
||||||
|
await user.click(screen.getByText("connect"));
|
||||||
|
|
||||||
|
await waitFor(() =>
|
||||||
|
expect(notificationsMock.show).toHaveBeenCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
title: "Failed to change the connection",
|
||||||
|
message: "close boom",
|
||||||
|
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 {
|
||||||
|
const user = await connect();
|
||||||
|
const client = clientInstances[0] as EventTarget & {
|
||||||
|
disconnect: ReturnType<typeof vi.fn>;
|
||||||
|
};
|
||||||
|
client.disconnect.mockRejectedValueOnce(CLOSE_FAILURE);
|
||||||
|
|
||||||
|
await user.click(screen.getByText("disconnect"));
|
||||||
|
|
||||||
|
await waitFor(() =>
|
||||||
|
expect(notificationsMock.show).toHaveBeenCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
title: "Failed to disconnect",
|
||||||
|
message: "close boom",
|
||||||
|
color: "red",
|
||||||
|
}),
|
||||||
|
),
|
||||||
|
);
|
||||||
|
await settleRejections();
|
||||||
|
expect(rejections.seen).toEqual([]);
|
||||||
|
} finally {
|
||||||
|
rejections.stop();
|
||||||
|
}
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
/** A complete `useManagedResources` return, so the mock stays type-checked against the hook's contract. */
|
/** A complete `useManagedResources` return, so the mock stays type-checked against the hook's contract. */
|
||||||
|
|
|
||||||
|
|
@ -1550,19 +1550,44 @@ function App() {
|
||||||
[],
|
[],
|
||||||
);
|
);
|
||||||
|
|
||||||
// The seven wrappers below discard a promise the callee already owns — each
|
// The two connection-lifecycle handlers do NOT own every rejection, so they
|
||||||
// of these handlers ends in its own `catch` that surfaces the failure as a
|
// are terminated with a `.catch` rather than discarded. `onToggleConnection`
|
||||||
// toast or a panel error — and the view's props are synchronous. `void` is
|
// has three escape paths outside its own try/catch — awaiting
|
||||||
// the explicit discard `@typescript-eslint/no-floating-promises` requires.
|
// `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",
|
||||||
|
});
|
||||||
|
};
|
||||||
|
}, []);
|
||||||
|
|
||||||
const dispatchToggleConnection = useCallback(
|
const dispatchToggleConnection = useCallback(
|
||||||
(id: string) => {
|
(id: string) => {
|
||||||
void onToggleConnection(id);
|
onToggleConnection(id).catch(
|
||||||
|
reportConnectionFailure("Failed to change the connection"),
|
||||||
|
);
|
||||||
},
|
},
|
||||||
[onToggleConnection],
|
[onToggleConnection, reportConnectionFailure],
|
||||||
);
|
);
|
||||||
const dispatchDisconnect = useCallback(() => {
|
const dispatchDisconnect = useCallback(() => {
|
||||||
void onDisconnect();
|
onDisconnect().catch(reportConnectionFailure("Failed to disconnect"));
|
||||||
}, [onDisconnect]);
|
}, [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.
|
||||||
const dispatchCallTool = useCallback(
|
const dispatchCallTool = useCallback(
|
||||||
(name: string, args: Record<string, unknown>, runAsTask?: boolean) => {
|
(name: string, args: Record<string, unknown>, runAsTask?: boolean) => {
|
||||||
void onCallTool(name, args, runAsTask);
|
void onCallTool(name, args, runAsTask);
|
||||||
|
|
|
||||||
|
|
@ -61,11 +61,7 @@ const noopBridgeFactory: BridgeFactory = () =>
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Per-bundle overrides. Each key takes a `Partial` of that bundle, so a test
|
* Per-bundle overrides. Each key takes a `Partial` of that bundle, so a test
|
||||||
* names only the field it cares about — `makeProps({
|
* names only the field it cares about — `makeProps({ tools: { tools: [t] } })`
|
||||||
tools: {
|
|
||||||
tools: { tools: [t] },
|
|
||||||
},
|
|
||||||
})`
|
|
||||||
* — and inherits the rest of the bundle's defaults.
|
* — and inherits the rest of the bundle's defaults.
|
||||||
*/
|
*/
|
||||||
type PropOverrides = {
|
type PropOverrides = {
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue