fix(react): StrictMode cleanup + relax late-handler guard for re-reg (#631)
* fix(react): close App on useApp unmount; relax late-handler guard for re-registration useApp's effect cleanup now closes the App (and its transport) instead of only flipping a `mounted` flag. Under React StrictMode dev double-invoke, the abandoned first instance's PostMessageTransport otherwise keeps a window `message` listener and receives every host postMessage alongside the second instance. The late-handler guard from #629 now tracks `_everHadListener` per event and only flags the *first* registration of a one-shot event post-handshake. Re-registration (React useEffect cleanup + re-add on dep change) of an event that had a pre-connect handler is silent, including under `strict: true`. * feat(app-bridge): warn on second ui/initialize If a View double-mounts (e.g. React StrictMode in dev) without closing the previous App, AppBridge now logs a console.warn when it receives a second ui/initialize. Behavior is unchanged — it still responds and the latest appInfo/appCapabilities replace the previous values — this is purely a diagnostic for host implementers.
This commit is contained in:
parent
9658678eaa
commit
cc02e17e21
4 changed files with 133 additions and 12 deletions
|
|
@ -15,6 +15,7 @@ import {
|
||||||
import { z } from "zod/v4";
|
import { z } from "zod/v4";
|
||||||
|
|
||||||
import { App } from "./app";
|
import { App } from "./app";
|
||||||
|
import { LATEST_PROTOCOL_VERSION } from "./types";
|
||||||
import {
|
import {
|
||||||
AppBridge,
|
AppBridge,
|
||||||
getToolUiResourceUri,
|
getToolUiResourceUri,
|
||||||
|
|
@ -2021,6 +2022,45 @@ describe("App <-> AppBridge integration", () => {
|
||||||
expect(warnings()).toEqual([]);
|
expect(warnings()).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not warn on re-registration when a handler existed pre-connect", async () => {
|
||||||
|
const h = () => {};
|
||||||
|
app.addEventListener("toolresult", h);
|
||||||
|
await bridge.connect(bridgeTransport);
|
||||||
|
await app.connect(appTransport);
|
||||||
|
|
||||||
|
// React-style: useEffect cleanup removes, dep change re-adds.
|
||||||
|
app.removeEventListener("toolresult", h);
|
||||||
|
app.addEventListener("toolresult", () => {});
|
||||||
|
|
||||||
|
expect(warnings().filter((m) => lateMsg.test(m))).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("warns once for the first late registration, not on subsequent ones", async () => {
|
||||||
|
await bridge.connect(bridgeTransport);
|
||||||
|
await app.connect(appTransport);
|
||||||
|
|
||||||
|
app.addEventListener("toolinput", () => {});
|
||||||
|
app.addEventListener("toolinput", () => {});
|
||||||
|
app.addEventListener("toolinput", () => {});
|
||||||
|
|
||||||
|
const late = warnings().filter((m) => lateMsg.test(m));
|
||||||
|
expect(late).toHaveLength(1);
|
||||||
|
expect(late[0]).toContain('"toolinput"');
|
||||||
|
});
|
||||||
|
|
||||||
|
it("tracks first-handler per event independently", async () => {
|
||||||
|
app.addEventListener("toolinput", () => {});
|
||||||
|
await bridge.connect(bridgeTransport);
|
||||||
|
await app.connect(appTransport);
|
||||||
|
|
||||||
|
app.addEventListener("toolinput", () => {}); // had pre-connect handler → silent
|
||||||
|
app.addEventListener("toolresult", () => {}); // first reg, late → warns
|
||||||
|
|
||||||
|
const late = warnings().filter((m) => lateMsg.test(m));
|
||||||
|
expect(late).toHaveLength(1);
|
||||||
|
expect(late[0]).toContain('"toolresult"');
|
||||||
|
});
|
||||||
|
|
||||||
it("throws instead of warning when strict: true", async () => {
|
it("throws instead of warning when strict: true", async () => {
|
||||||
const [strictAppT, strictBridgeT] =
|
const [strictAppT, strictBridgeT] =
|
||||||
InMemoryTransport.createLinkedPair();
|
InMemoryTransport.createLinkedPair();
|
||||||
|
|
@ -2034,6 +2074,9 @@ describe("App <-> AppBridge integration", () => {
|
||||||
{},
|
{},
|
||||||
{ autoResize: false, strict: true },
|
{ autoResize: false, strict: true },
|
||||||
);
|
);
|
||||||
|
// Pre-connect registration for toolcancelled — later swap must not throw.
|
||||||
|
strictApp.addEventListener("toolcancelled", () => {});
|
||||||
|
|
||||||
await strictBridge.connect(strictBridgeT);
|
await strictBridge.connect(strictBridgeT);
|
||||||
await strictApp.connect(strictAppT);
|
await strictApp.connect(strictAppT);
|
||||||
|
|
||||||
|
|
@ -2043,10 +2086,57 @@ describe("App <-> AppBridge integration", () => {
|
||||||
expect(() => {
|
expect(() => {
|
||||||
strictApp.addEventListener("toolinput", () => {});
|
strictApp.addEventListener("toolinput", () => {});
|
||||||
}).toThrow(lateMsg);
|
}).toThrow(lateMsg);
|
||||||
expect(warnings()).toEqual([]);
|
// Swapping a handler that existed pre-connect is allowed under strict.
|
||||||
|
expect(() => {
|
||||||
|
strictApp.addEventListener("toolcancelled", () => {});
|
||||||
|
}).not.toThrow();
|
||||||
|
expect(warnings().filter((m) => lateMsg.test(m))).toEqual([]);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("AppBridge warns on a second ui/initialize (View double-mount)", async () => {
|
||||||
|
await bridge.connect(bridgeTransport);
|
||||||
|
await app.connect(appTransport);
|
||||||
|
expect(warnings()).toEqual([]);
|
||||||
|
|
||||||
|
// Simulate a second View instance re-running the handshake.
|
||||||
|
appTransport.send({
|
||||||
|
jsonrpc: "2.0",
|
||||||
|
id: 99,
|
||||||
|
method: "ui/initialize",
|
||||||
|
params: {
|
||||||
|
protocolVersion: LATEST_PROTOCOL_VERSION,
|
||||||
|
appInfo: testAppInfo,
|
||||||
|
appCapabilities: {},
|
||||||
|
},
|
||||||
|
});
|
||||||
|
await flush();
|
||||||
|
|
||||||
|
const doubleInit = warnings().filter((m) =>
|
||||||
|
/second ui\/initialize/.test(m),
|
||||||
|
);
|
||||||
|
expect(doubleInit).toHaveLength(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("close() stops further notification delivery (StrictMode cleanup relies on this)", async () => {
|
||||||
|
const received: unknown[] = [];
|
||||||
|
app.addEventListener("toolresult", (r) => received.push(r));
|
||||||
|
await bridge.connect(bridgeTransport);
|
||||||
|
await app.connect(appTransport);
|
||||||
|
|
||||||
|
await bridge.sendToolResult({
|
||||||
|
content: [{ type: "text", text: "before" }],
|
||||||
|
});
|
||||||
|
expect(received).toHaveLength(1);
|
||||||
|
|
||||||
|
await app.close();
|
||||||
|
|
||||||
|
await bridge
|
||||||
|
.sendToolResult({ content: [{ type: "text", text: "after" }] })
|
||||||
|
.catch(() => {});
|
||||||
|
expect(received).toHaveLength(1);
|
||||||
|
});
|
||||||
|
|
||||||
it("AppBridge warns on tools/call from a View that skipped the handshake", async () => {
|
it("AppBridge warns on tools/call from a View that skipped the handshake", async () => {
|
||||||
bridge.oncalltool = async () => ({ content: [] });
|
bridge.oncalltool = async () => ({ content: [] });
|
||||||
await bridge.connect(bridgeTransport);
|
await bridge.connect(bridgeTransport);
|
||||||
|
|
|
||||||
|
|
@ -1463,6 +1463,15 @@ export class AppBridge extends ProtocolWithEvents<
|
||||||
): Promise<McpUiInitializeResult> {
|
): Promise<McpUiInitializeResult> {
|
||||||
const requestedVersion = request.params.protocolVersion;
|
const requestedVersion = request.params.protocolVersion;
|
||||||
|
|
||||||
|
if (this._appInfo !== undefined) {
|
||||||
|
console.warn(
|
||||||
|
"[ext-apps] AppBridge received a second ui/initialize. The View may " +
|
||||||
|
"be double-mounting (e.g. React StrictMode in dev) without closing " +
|
||||||
|
"the previous App instance. Responding normally; the latest " +
|
||||||
|
"appInfo/appCapabilities replace the previous values.",
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
this._appCapabilities = request.params.appCapabilities;
|
this._appCapabilities = request.params.appCapabilities;
|
||||||
this._appInfo = request.params.appInfo;
|
this._appInfo = request.params.appInfo;
|
||||||
|
|
||||||
|
|
|
||||||
22
src/app.ts
22
src/app.ts
|
|
@ -395,14 +395,28 @@ export class App extends ProtocolWithEvents<
|
||||||
new Set(["toolinput", "toolinputpartial", "toolresult", "toolcancelled"]);
|
new Set(["toolinput", "toolinputpartial", "toolresult", "toolcancelled"]);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Warn (or throw under `strict`) when a one-shot event handler is registered
|
* One-shot events that have had at least one handler registered (via `on*`
|
||||||
* after the `ui/initialize` → `ui/notifications/initialized` handshake has
|
* setter or `addEventListener`) at any point. Once an event is in this set,
|
||||||
* completed. The host may have already fired the notification by then.
|
* subsequent late registrations are not flagged — only the *first* handler
|
||||||
|
* matters for the missed-notification race, and re-registration (e.g. React
|
||||||
|
* `useEffect` cleanup → re-add on dep change) is a legitimate pattern.
|
||||||
|
*/
|
||||||
|
private readonly _everHadListener = new Set<keyof AppEventMap>();
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Warn (or throw under `strict`) when the *first* handler for a one-shot
|
||||||
|
* event is registered after the `ui/initialize` → `ui/notifications/initialized`
|
||||||
|
* handshake has completed. The host may have already fired the notification
|
||||||
|
* by then. Subsequent registrations for the same event are not flagged.
|
||||||
*
|
*
|
||||||
* Mirrors {@link _assertInitialized `_assertInitialized`} (the outbound-side guard).
|
* Mirrors {@link _assertInitialized `_assertInitialized`} (the outbound-side guard).
|
||||||
*/
|
*/
|
||||||
private _assertHandlerTiming(event: keyof AppEventMap): void {
|
private _assertHandlerTiming(event: keyof AppEventMap): void {
|
||||||
if (!this._initializedSent || !App.ONE_SHOT_EVENTS.has(event)) return;
|
if (!App.ONE_SHOT_EVENTS.has(event) || this._everHadListener.has(event)) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
this._everHadListener.add(event);
|
||||||
|
if (!this._initializedSent) return;
|
||||||
const msg =
|
const msg =
|
||||||
`[ext-apps] "${String(event)}" handler registered after connect() ` +
|
`[ext-apps] "${String(event)}" handler registered after connect() ` +
|
||||||
`completed the ui/initialize handshake. The host may have already sent ` +
|
`completed the ui/initialize handshake. The host may have already sent ` +
|
||||||
|
|
|
||||||
|
|
@ -138,6 +138,7 @@ export function useApp({
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
let mounted = true;
|
let mounted = true;
|
||||||
|
let appInstance: App | undefined;
|
||||||
|
|
||||||
async function connect() {
|
async function connect() {
|
||||||
try {
|
try {
|
||||||
|
|
@ -145,18 +146,24 @@ export function useApp({
|
||||||
window.parent,
|
window.parent,
|
||||||
window.parent,
|
window.parent,
|
||||||
);
|
);
|
||||||
const app = new App(appInfo, capabilities, { autoResize, strict });
|
appInstance = new App(appInfo, capabilities, { autoResize, strict });
|
||||||
|
|
||||||
// Register handlers BEFORE connecting
|
// Register handlers BEFORE connecting
|
||||||
onAppCreated?.(app);
|
onAppCreated?.(appInstance);
|
||||||
|
|
||||||
await app.connect(transport);
|
await appInstance.connect(transport);
|
||||||
|
|
||||||
if (mounted) {
|
if (!mounted) {
|
||||||
setApp(app);
|
// Unmounted during the handshake (e.g. React StrictMode dev
|
||||||
setIsConnected(true);
|
// double-invoke). Close so the abandoned transport's window
|
||||||
setError(null);
|
// `message` listener doesn't keep receiving host postMessages
|
||||||
|
// alongside the second mount's instance.
|
||||||
|
void appInstance.close();
|
||||||
|
return;
|
||||||
}
|
}
|
||||||
|
setApp(appInstance);
|
||||||
|
setIsConnected(true);
|
||||||
|
setError(null);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
if (mounted) {
|
if (mounted) {
|
||||||
setApp(null);
|
setApp(null);
|
||||||
|
|
@ -172,6 +179,7 @@ export function useApp({
|
||||||
|
|
||||||
return () => {
|
return () => {
|
||||||
mounted = false;
|
mounted = false;
|
||||||
|
void appInstance?.close();
|
||||||
};
|
};
|
||||||
}, []); // Intentionally not including options to avoid reconnection
|
}, []); // Intentionally not including options to avoid reconnection
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue