Skip to content

Fix PostMessageTransport listener accumulation in AppRenderer/AppFrame#178

Open
Copilot wants to merge 3 commits intomainfrom
copilot/fix-duplicate-message-listeners
Open

Fix PostMessageTransport listener accumulation in AppRenderer/AppFrame#178
Copilot wants to merge 3 commits intomainfrom
copilot/fix-duplicate-message-listeners

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 6, 2026

PostMessageTransport instances register window.addEventListener("message") but were never cleaned up, causing listeners to accumulate on component re-renders or prop changes. This resulted in duplicate message handling and console spam.

Changes

AppRenderer.tsx

  • Track AppBridge instance in effect scope for cleanup
  • Call bridge.close() in effect cleanup to remove listeners when component unmounts or client changes

AppFrame.tsx

  • Close previous bridge when switching to new appBridge or sandbox.url
  • Remove await on bridge close during setup to avoid blocking cleanup
  • Unconditionally close bridge in effect cleanup (removed ref equality check that could prevent cleanup during rapid prop changes)

Tests

  • Add close() method to mock AppBridge
  • Verify cleanup behavior on unmount and prop changes

Example

Before:

useEffect(() => {
  const bridge = new AppBridge(...);
  setAppBridge(bridge);
  return () => { mounted = false; }; // Listener never removed
}, [client]);

After:

useEffect(() => {
  let bridge: AppBridge | null = null;
  bridge = new AppBridge(...);
  setAppBridge(bridge);
  return () => {
    mounted = false;
    bridge?.close(); // Removes window message listener
  };
}, [client]);
Original prompt

This section details on the original issue you should resolve

<issue_title>Multiple message listeners accumulate causing duplicate event handling</issue_title>
<issue_description>## Description

When using AppRenderer, multiple message event listeners accumulate on window, causing each postMessage to be received multiple times. This results in:

  • Console spam with "Ignoring message from unknown source" errors
  • Performance degradation from redundant event processing

Steps to Reproduce

  1. Render an AppRenderer component with callback props (onCallTool, onMessage, etc.) instead of a client
  2. Trigger a message from the guest iframe (e.g., click a button that calls tools/call)
  3. Observe multiple console messages for a single action

Expected Behavior

One message listener should handle each postMessage event.

Actual Behavior

Multiple listeners fire for the same event. Checking getEventListeners(window).message in DevTools shows 5+ listeners accumulating:

getEventListeners(window).message
// Returns array of 5 listeners when there should be 1

Console output for a single button click:

Ignoring message from unknown source  MessageEvent {...}
Ignoring message from unknown source  MessageEvent {...}
Ignoring message from unknown source  MessageEvent {...}
Parsed message {jsonrpc: '2.0', id: 17, method: 'tools/call', params: {...}}
Ignoring message from unknown source  MessageEvent {...}

Root Cause Analysis

The issue appears to be in the lifecycle management of PostMessageTransport instances:

  1. AppRenderer creates a new AppBridge in a useEffect
  2. AppFrame calls appBridge.connect(new PostMessageTransport(...)) which adds a window.addEventListener("message", ...)
  3. When props change or re-renders occur, new instances are created but transport.close() is not called to remove old listeners

The PostMessageTransport.close() method exists in @modelcontextprotocol/ext-apps and correctly removes the listener, but it doesn't appear to be called during component cleanup.

Environment

  • @mcp-ui/client: 6.0.0
  • @modelcontextprotocol/ext-apps: 0.3.1
  • React: 19.2.4
  • Platform: Electron desktop app

Suggested Fix

Ensure useEffect cleanup functions call transport.close() or appBridge.close() before creating new instances:

useEffect(() => {
  const bridge = new AppBridge(...);
  // ... setup ...
  
  return () => {
    bridge.close();  // Clean up old transport/listeners
  };
}, [dependencies]);
```</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Fix accumulation of multiple message listeners in AppRenderer Fix PostMessageTransport listener accumulation in AppRenderer/AppFrame Feb 6, 2026
Copilot AI requested a review from idosal February 6, 2026 21:26
@idosal idosal marked this pull request as ready for review February 6, 2026 23:43
Copilot AI review requested due to automatic review settings February 6, 2026 23:43
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses window.message listener accumulation caused by PostMessageTransport / AppBridge instances not being closed during React effect cleanup in the TypeScript client SDK’s AppRenderer/AppFrame components.

Changes:

  • Add bridge.close() / appBridge.close() calls in useEffect cleanups to remove message listeners on unmount and dependency changes.
  • Add logic in AppFrame to close a previous bridge when switching iframe/sandbox context.
  • Update unit tests to mock and assert close() behavior on unmount and prop changes.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
sdks/typescript/client/src/components/AppRenderer.tsx Tracks the created AppBridge in effect scope and closes it in cleanup to prevent leaked listeners.
sdks/typescript/client/src/components/AppFrame.tsx Closes bridges during iframe/bridge switches and on effect cleanup to remove message listeners.
sdks/typescript/client/src/components/__tests__/AppRenderer.test.tsx Extends AppBridge mock with close() and adds cleanup assertions on unmount and client changes.
sdks/typescript/client/src/components/__tests__/AppFrame.test.tsx Extends AppBridge mock with close() and adds cleanup assertions for unmount and appBridge changes.

Comment on lines +248 to +252
// Close the current appBridge being cleaned up regardless of ref equality
// to ensure proper cleanup even if refs haven't been updated yet
appBridge.close().catch((err) => {
console.error('[AppFrame] Error closing bridge on unmount:', err);
});
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The effect cleanup unconditionally calls appBridge.close(), but this effect can re-run even when you intend to reuse the existing iframe/bridge (e.g. React StrictMode mount double-invokes effects, or when sandbox.url / sandbox.csp change identity but produce the same sandboxUrlString). In those cases the previous cleanup will close the bridge, and the next effect run may hit the early-return and skip re-connecting, leaving a closed bridge with a preserved iframe. Consider (a) stabilizing dependencies to primitives (e.g. sandbox.url.href and a stable CSP key) so the effect doesn’t tear down on identity-only changes, and/or (b) clearing iframeRef/current*Ref in cleanup so the next run performs setup again instead of returning early.

Copilot uses AI. Check for mistakes.
Comment on lines +174 to +179
// Close the previous bridge if it exists (don't await to avoid blocking)
if (currentAppBridgeRef.current) {
currentAppBridgeRef.current.close().catch((err) => {
console.error('[AppFrame] Error closing previous bridge:', err);
});
}
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currentAppBridgeRef.current.close() is invoked when removing the old iframe, but the previous effect’s cleanup also closes the previous appBridge on dependency changes. This can lead to double-close / concurrent close calls for the same bridge when switching sandboxes/bridges. Consider centralizing bridge teardown in the effect cleanup (and in setup only remove DOM/refs), or add a guard so you only close once per bridge instance.

Copilot uses AI. Check for mistakes.
Comment on lines +427 to +458
it('should call previous appBridge.close() when appBridge changes', async () => {
const { rerender } = render(<AppFrame {...getPropsWithBridge()} />);

await act(() => {
onReadyResolve();
});

await act(() => {
registeredOninitialized?.();
});

// First bridge should be connected
expect(mockAppBridge.connect).toHaveBeenCalledTimes(1);

// Create new mock for second bridge
const secondMockAppBridge = createMockAppBridge();
const secondOnReadyPromise = new Promise<void>((resolve) => {
onReadyResolve = resolve;
});
vi.mocked(appHostUtils.setupSandboxProxyIframe).mockResolvedValue({
iframe: mockIframe as HTMLIFrameElement,
onReady: secondOnReadyPromise,
});

// Re-render with DIFFERENT appBridge
rerender(<AppFrame {...getPropsWithBridge({ appBridge: secondMockAppBridge as unknown as AppBridge })} />);

// Verify the first bridge was closed
await waitFor(() => {
expect(mockAppBridge.close).toHaveBeenCalled();
});
});
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s no test covering the case where sandbox.url (or sandbox.csp) changes by reference but represents the same effective sandbox URL/CSP (common if callers inline new URL(...) or {...csp} in JSX). Given the effect’s early-return optimization and the new unconditional close() in cleanup, adding a regression test for this scenario would help prevent disconnects (and would have caught the StrictMode/identity-change case).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple message listeners accumulate causing duplicate event handling

3 participants