Skip to content

Commit 35f4dc8

Browse files
committed
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`.
1 parent 9658678 commit 35f4dc8

3 files changed

Lines changed: 99 additions & 12 deletions

File tree

src/app-bridge.test.ts

Lines changed: 66 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2021,6 +2021,45 @@ describe("App <-> AppBridge integration", () => {
20212021
expect(warnings()).toEqual([]);
20222022
});
20232023

2024+
it("does not warn on re-registration when a handler existed pre-connect", async () => {
2025+
const h = () => {};
2026+
app.addEventListener("toolresult", h);
2027+
await bridge.connect(bridgeTransport);
2028+
await app.connect(appTransport);
2029+
2030+
// React-style: useEffect cleanup removes, dep change re-adds.
2031+
app.removeEventListener("toolresult", h);
2032+
app.addEventListener("toolresult", () => {});
2033+
2034+
expect(warnings().filter((m) => lateMsg.test(m))).toEqual([]);
2035+
});
2036+
2037+
it("warns once for the first late registration, not on subsequent ones", async () => {
2038+
await bridge.connect(bridgeTransport);
2039+
await app.connect(appTransport);
2040+
2041+
app.addEventListener("toolinput", () => {});
2042+
app.addEventListener("toolinput", () => {});
2043+
app.addEventListener("toolinput", () => {});
2044+
2045+
const late = warnings().filter((m) => lateMsg.test(m));
2046+
expect(late).toHaveLength(1);
2047+
expect(late[0]).toContain('"toolinput"');
2048+
});
2049+
2050+
it("tracks first-handler per event independently", async () => {
2051+
app.addEventListener("toolinput", () => {});
2052+
await bridge.connect(bridgeTransport);
2053+
await app.connect(appTransport);
2054+
2055+
app.addEventListener("toolinput", () => {}); // had pre-connect handler → silent
2056+
app.addEventListener("toolresult", () => {}); // first reg, late → warns
2057+
2058+
const late = warnings().filter((m) => lateMsg.test(m));
2059+
expect(late).toHaveLength(1);
2060+
expect(late[0]).toContain('"toolresult"');
2061+
});
2062+
20242063
it("throws instead of warning when strict: true", async () => {
20252064
const [strictAppT, strictBridgeT] =
20262065
InMemoryTransport.createLinkedPair();
@@ -2034,6 +2073,9 @@ describe("App <-> AppBridge integration", () => {
20342073
{},
20352074
{ autoResize: false, strict: true },
20362075
);
2076+
// Pre-connect registration for toolcancelled — later swap must not throw.
2077+
strictApp.addEventListener("toolcancelled", () => {});
2078+
20372079
await strictBridge.connect(strictBridgeT);
20382080
await strictApp.connect(strictAppT);
20392081

@@ -2043,10 +2085,33 @@ describe("App <-> AppBridge integration", () => {
20432085
expect(() => {
20442086
strictApp.addEventListener("toolinput", () => {});
20452087
}).toThrow(lateMsg);
2046-
expect(warnings()).toEqual([]);
2088+
// Swapping a handler that existed pre-connect is allowed under strict.
2089+
expect(() => {
2090+
strictApp.addEventListener("toolcancelled", () => {});
2091+
}).not.toThrow();
2092+
expect(warnings().filter((m) => lateMsg.test(m))).toEqual([]);
20472093
});
20482094
});
20492095

2096+
it("close() stops further notification delivery (StrictMode cleanup relies on this)", async () => {
2097+
const received: unknown[] = [];
2098+
app.addEventListener("toolresult", (r) => received.push(r));
2099+
await bridge.connect(bridgeTransport);
2100+
await app.connect(appTransport);
2101+
2102+
await bridge.sendToolResult({
2103+
content: [{ type: "text", text: "before" }],
2104+
});
2105+
expect(received).toHaveLength(1);
2106+
2107+
await app.close();
2108+
2109+
await bridge
2110+
.sendToolResult({ content: [{ type: "text", text: "after" }] })
2111+
.catch(() => {});
2112+
expect(received).toHaveLength(1);
2113+
});
2114+
20502115
it("AppBridge warns on tools/call from a View that skipped the handshake", async () => {
20512116
bridge.oncalltool = async () => ({ content: [] });
20522117
await bridge.connect(bridgeTransport);

src/app.ts

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -395,14 +395,28 @@ export class App extends ProtocolWithEvents<
395395
new Set(["toolinput", "toolinputpartial", "toolresult", "toolcancelled"]);
396396

397397
/**
398-
* Warn (or throw under `strict`) when a one-shot event handler is registered
399-
* after the `ui/initialize` → `ui/notifications/initialized` handshake has
400-
* completed. The host may have already fired the notification by then.
398+
* One-shot events that have had at least one handler registered (via `on*`
399+
* setter or `addEventListener`) at any point. Once an event is in this set,
400+
* subsequent late registrations are not flagged — only the *first* handler
401+
* matters for the missed-notification race, and re-registration (e.g. React
402+
* `useEffect` cleanup → re-add on dep change) is a legitimate pattern.
403+
*/
404+
private readonly _everHadListener = new Set<keyof AppEventMap>();
405+
406+
/**
407+
* Warn (or throw under `strict`) when the *first* handler for a one-shot
408+
* event is registered after the `ui/initialize` → `ui/notifications/initialized`
409+
* handshake has completed. The host may have already fired the notification
410+
* by then. Subsequent registrations for the same event are not flagged.
401411
*
402412
* Mirrors {@link _assertInitialized `_assertInitialized`} (the outbound-side guard).
403413
*/
404414
private _assertHandlerTiming(event: keyof AppEventMap): void {
405-
if (!this._initializedSent || !App.ONE_SHOT_EVENTS.has(event)) return;
415+
if (!App.ONE_SHOT_EVENTS.has(event) || this._everHadListener.has(event)) {
416+
return;
417+
}
418+
this._everHadListener.add(event);
419+
if (!this._initializedSent) return;
406420
const msg =
407421
`[ext-apps] "${String(event)}" handler registered after connect() ` +
408422
`completed the ui/initialize handshake. The host may have already sent ` +

src/react/useApp.tsx

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -138,25 +138,32 @@ export function useApp({
138138

139139
useEffect(() => {
140140
let mounted = true;
141+
let appInstance: App | undefined;
141142

142143
async function connect() {
143144
try {
144145
const transport = new PostMessageTransport(
145146
window.parent,
146147
window.parent,
147148
);
148-
const app = new App(appInfo, capabilities, { autoResize, strict });
149+
appInstance = new App(appInfo, capabilities, { autoResize, strict });
149150

150151
// Register handlers BEFORE connecting
151-
onAppCreated?.(app);
152+
onAppCreated?.(appInstance);
152153

153-
await app.connect(transport);
154+
await appInstance.connect(transport);
154155

155-
if (mounted) {
156-
setApp(app);
157-
setIsConnected(true);
158-
setError(null);
156+
if (!mounted) {
157+
// Unmounted during the handshake (e.g. React StrictMode dev
158+
// double-invoke). Close so the abandoned transport's window
159+
// `message` listener doesn't keep receiving host postMessages
160+
// alongside the second mount's instance.
161+
void appInstance.close();
162+
return;
159163
}
164+
setApp(appInstance);
165+
setIsConnected(true);
166+
setError(null);
160167
} catch (error) {
161168
if (mounted) {
162169
setApp(null);
@@ -172,6 +179,7 @@ export function useApp({
172179

173180
return () => {
174181
mounted = false;
182+
void appInstance?.close();
175183
};
176184
}, []); // Intentionally not including options to avoid reconnection
177185

0 commit comments

Comments
 (0)