fix(apps): close the three gaps the review flagged as previously missed
**The task-augmented inbound branch bypassed the routing.** A server→client `elicitation/create` carrying `params.task` answers immediately with a `CreateTaskResult` and settles the TASK when the user answers, so it cannot ride `enqueuePendingElicitation` — and it was therefore never offered to an app, even with a valid `_meta.ui.resourceUri`. It now makes the same attempt, completing the task with the app's result and falling back to the native queue otherwise; an abort fails the task rather than reopening it natively. The two task-settling closures are extracted so both answer routes settle it identically. **The web client advertised the capability without a sandbox.** `useInitialConfig` leaves `sandboxUrl` undefined while `/api/config` is in flight and when the sandbox controller could not start, but the renderer was supplied unconditionally — so the client claimed it could host an app in exactly the window where it could not. The option is now gated on a resolved sandbox URL: such a connection behaves like the CLI/TUI, native queue and no claim made. **Concurrent modals each owned the keyboard.** Every open `Modal.Root` installed its own focus trap, Escape handler and overlay, so the traps fought and one Escape could dismiss more than one pending request. Only the topmost owns them now; the rest stay mounted, keeping their bridges and handshakes, but inert to the keyboard. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
98d1fe752d
commit
d90c51a04c
4 changed files with 206 additions and 32 deletions
|
|
@ -2483,11 +2483,16 @@ function App() {
|
|||
// Sampling / elicitation are on by default; keep the parameterized
|
||||
// options off until the UI grows the surface to render them.
|
||||
elicit: { form: true, url: true },
|
||||
// Web only: hands form elicitations that name a `ui://` resource to the
|
||||
// MCP App the server chose, and advertises the nested MCP Apps
|
||||
// `elicitation` capability that makes a server willing to send one
|
||||
// (#1854). The native elicitation queue stays the fallback.
|
||||
appElicitation: appElicitationController.render,
|
||||
// Web only, and only when the sandbox renderer is actually available:
|
||||
// supplying this advertises the nested MCP Apps `elicitation`
|
||||
// capability, and a client that cannot host an app must not claim it
|
||||
// (#1854). `sandboxUrl` is undefined while `/api/config` is in flight
|
||||
// and when the sandbox controller could not start, so this connection
|
||||
// simply behaves like the CLI/TUI: the native elicitation queue, with
|
||||
// no claim made to the server.
|
||||
...(sandboxUrl && {
|
||||
appElicitation: appElicitationController.render,
|
||||
}),
|
||||
// Always advertise the roots capability (even with no configured
|
||||
// roots) so the server can issue roots/list and receive
|
||||
// roots/list_changed; the configured roots are the answer to
|
||||
|
|
@ -2590,6 +2595,7 @@ function App() {
|
|||
onBeforeOAuthRedirect,
|
||||
clientConfig,
|
||||
appElicitationController.render,
|
||||
sandboxUrl,
|
||||
],
|
||||
);
|
||||
|
||||
|
|
|
|||
|
|
@ -193,6 +193,35 @@ describe("AppElicitationHost (#1854)", () => {
|
|||
expect(onSettle.mock.calls[0][0]).toBe("req-2");
|
||||
});
|
||||
|
||||
it("gives the focus trap, Escape and the overlay to the top modal only", async () => {
|
||||
// Every entry stays mounted (each app keeps its own bridge and handshake),
|
||||
// but only one may own the keyboard — otherwise the traps fight and a
|
||||
// single Escape can dismiss more than one pending request.
|
||||
const first = createMockBridge({});
|
||||
const second = createMockBridge({});
|
||||
const bridges = [first.bridge, second.bridge];
|
||||
const factory = vi.fn(() => bridges.shift()) as unknown as BridgeFactory;
|
||||
const user = userEvent.setup();
|
||||
|
||||
renderWithMantine(
|
||||
<AppElicitationHost
|
||||
entries={[makeEntry("req-1"), makeEntry("req-2")]}
|
||||
sandboxPath="/sandbox.html"
|
||||
bridgeFactory={factory}
|
||||
onSettle={onSettle}
|
||||
onFail={onFail}
|
||||
/>,
|
||||
);
|
||||
await flushAsync();
|
||||
expect(screen.getAllByTestId("app-elicitation")).toHaveLength(2);
|
||||
|
||||
await user.keyboard("{Escape}");
|
||||
// Exactly one request is dismissed — the topmost, which is the last one.
|
||||
expect(onFail).toHaveBeenCalledTimes(1);
|
||||
expect(onFail.mock.calls[0][0]).toBe("req-2");
|
||||
expect(onFail.mock.calls[0][1].message).toMatch(/dismissed/);
|
||||
});
|
||||
|
||||
it("falls back when the app does not advertise elicitation", async () => {
|
||||
const mock = createMockBridge({ elicitation: false });
|
||||
renderWithMantine(
|
||||
|
|
|
|||
|
|
@ -118,10 +118,13 @@ export function AppElicitationHost({
|
|||
|
||||
return (
|
||||
<>
|
||||
{entries.map((entry) => (
|
||||
{entries.map((entry, index) => (
|
||||
<AppElicitationFrame
|
||||
key={entry.requestId}
|
||||
entry={entry}
|
||||
// Only the last-rendered modal is on top, so only it may own the
|
||||
// focus trap, the Escape key and the overlay — see the prop's doc.
|
||||
isTop={index === entries.length - 1}
|
||||
sandboxPath={sandboxPath}
|
||||
bridgeFactory={bridgeFactory}
|
||||
onSettle={onSettle}
|
||||
|
|
@ -134,6 +137,16 @@ export function AppElicitationHost({
|
|||
|
||||
interface AppElicitationFrameProps {
|
||||
entry: AppElicitationEntry;
|
||||
/**
|
||||
* Whether this modal is the topmost of the open set.
|
||||
*
|
||||
* Concurrent elicitations mean several are open at once, and each would
|
||||
* otherwise install its own focus trap, Escape handler and overlay — which
|
||||
* fight over the keyboard and let one Escape dismiss more than one pending
|
||||
* request. Only the top one owns those; the rest stay mounted (their apps
|
||||
* keep their bridges and their handshakes) but inert to the keyboard.
|
||||
*/
|
||||
isTop: boolean;
|
||||
sandboxPath: string;
|
||||
bridgeFactory: BridgeFactory;
|
||||
onSettle: (requestId: string, result: ElicitResult) => void;
|
||||
|
|
@ -150,6 +163,7 @@ interface AppElicitationFrameProps {
|
|||
*/
|
||||
function AppElicitationFrame({
|
||||
entry,
|
||||
isTop,
|
||||
sandboxPath,
|
||||
bridgeFactory,
|
||||
onSettle,
|
||||
|
|
@ -221,8 +235,13 @@ function AppElicitationFrame({
|
|||
);
|
||||
|
||||
return (
|
||||
<ElicitationModal opened onClose={dismiss}>
|
||||
<ElicitationOverlay />
|
||||
<ElicitationModal
|
||||
opened
|
||||
onClose={dismiss}
|
||||
trapFocus={isTop}
|
||||
closeOnEscape={isTop}
|
||||
>
|
||||
{isTop && <ElicitationOverlay />}
|
||||
{/* `aria-label` and the `data-*` attributes go on the CONTENT (the
|
||||
role="dialog" element), not the Root — the Root is only a portal
|
||||
wrapper, so a name placed there never reaches the dialog. Named per
|
||||
|
|
|
|||
|
|
@ -94,11 +94,24 @@ class ElicitTransport implements Transport {
|
|||
id: number,
|
||||
params: Record<string, unknown>,
|
||||
timeoutMs = 2000,
|
||||
): Promise<JSONRPCMessage> {
|
||||
return this.sendRequest(id, "elicitation/create", params, timeoutMs);
|
||||
}
|
||||
|
||||
/**
|
||||
* Deliver any server→client request and resolve with the client's reply.
|
||||
* NOT named `send` — that is the Transport method the client calls.
|
||||
*/
|
||||
sendRequest(
|
||||
id: number,
|
||||
method: string,
|
||||
params?: Record<string, unknown>,
|
||||
timeoutMs = 2000,
|
||||
): Promise<JSONRPCMessage> {
|
||||
const reply = new Promise<JSONRPCMessage>((resolve) => {
|
||||
this.waiters.set(id, resolve);
|
||||
});
|
||||
this.deliver({ jsonrpc: "2.0", id, method: "elicitation/create", params });
|
||||
this.deliver({ jsonrpc: "2.0", id, method, ...(params && { params }) });
|
||||
let timer: ReturnType<typeof setTimeout>;
|
||||
return Promise.race([
|
||||
reply,
|
||||
|
|
@ -125,6 +138,17 @@ function appParams(overrides: Record<string, unknown> = {}) {
|
|||
};
|
||||
}
|
||||
|
||||
/** The receiver tasks the client reports over `tasks/list`, by status. */
|
||||
async function taskStatuses(
|
||||
transport: ElicitTransport,
|
||||
id: number,
|
||||
): Promise<string[]> {
|
||||
const reply = (await transport.sendRequest(id, "tasks/list")) as {
|
||||
result?: { tasks?: { status?: string }[] };
|
||||
};
|
||||
return (reply.result?.tasks ?? []).map((t) => t.status ?? "");
|
||||
}
|
||||
|
||||
async function connectClient(options: {
|
||||
transport: ElicitTransport;
|
||||
appElicitation?: AppElicitationRenderer;
|
||||
|
|
@ -405,6 +429,78 @@ describe("app-rendered elicitation routing (#1854)", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("task-augmented inbound requests", () => {
|
||||
/**
|
||||
* A `params.task` elicitation answers the server immediately with a
|
||||
* `CreateTaskResult` and settles the TASK when the user answers, so it
|
||||
* cannot ride `enqueuePendingElicitation`. It is still an
|
||||
* `elicitation/create`, so the app-rendering contract applies to it too.
|
||||
*/
|
||||
async function connectWithTasks(
|
||||
transport: ElicitTransport,
|
||||
renderer?: AppElicitationRenderer,
|
||||
) {
|
||||
const client = new InspectorClient(
|
||||
{ type: "stdio", command: "noop", args: [] },
|
||||
{
|
||||
environment: { transport: () => ({ transport }) },
|
||||
elicit: { form: true },
|
||||
receiverTasks: true,
|
||||
...(renderer && { appElicitation: renderer }),
|
||||
},
|
||||
);
|
||||
await client.connect();
|
||||
return client;
|
||||
}
|
||||
|
||||
const taskParams = () => appParams({ task: { ttl: 60_000 } });
|
||||
|
||||
it("renders the app and completes the task with its result", async () => {
|
||||
const transport = new ElicitTransport();
|
||||
const seen: AppElicitationRequest[] = [];
|
||||
const client = await connectWithTasks(transport, async (request) => {
|
||||
seen.push(request);
|
||||
return { action: "accept", content: { choice: "option-a" } };
|
||||
});
|
||||
|
||||
// The immediate response is the task handle, not the answer.
|
||||
const reply = await transport.elicit(50, taskParams());
|
||||
expect(reply).toMatchObject({
|
||||
id: 50,
|
||||
result: { task: { taskId: expect.any(String) } },
|
||||
});
|
||||
|
||||
await vi.waitFor(() => expect(seen).toHaveLength(1));
|
||||
expect(seen[0].resourceUri).toBe(APP_URI);
|
||||
// The app answered, so the native queue never opened and the task
|
||||
// completed on its own — read back the way a server reads it, over
|
||||
// `tasks/list`.
|
||||
expect(client.getPendingElicitations()).toHaveLength(0);
|
||||
await vi.waitFor(async () =>
|
||||
expect(await taskStatuses(transport, 60)).toContain("completed"),
|
||||
);
|
||||
await client.disconnect();
|
||||
});
|
||||
|
||||
it("falls back to the native queue when the app cannot answer", async () => {
|
||||
const transport = new ElicitTransport();
|
||||
const client = await connectWithTasks(transport, async () => {
|
||||
throw new Error("sandbox unavailable");
|
||||
});
|
||||
await transport.elicit(51, taskParams());
|
||||
await vi.waitFor(() =>
|
||||
expect(client.getPendingElicitations()).toHaveLength(1),
|
||||
);
|
||||
await client
|
||||
.getPendingElicitations()[0]
|
||||
.respond({ action: "accept", content: { choice: "native" } });
|
||||
await vi.waitFor(async () =>
|
||||
expect(await taskStatuses(transport, 61)).toContain("completed"),
|
||||
);
|
||||
await client.disconnect();
|
||||
});
|
||||
});
|
||||
|
||||
describe("request-scoped ids", () => {
|
||||
it("mints ids that are unique across client instances, not just within one", async () => {
|
||||
// A host may hold ONE renderer across replacement clients (the web client
|
||||
|
|
|
|||
Loading…
Reference in a new issue