Fix PostMessageTransport listener accumulation in AppRenderer/AppFrame#178
Fix PostMessageTransport listener accumulation in AppRenderer/AppFrame#178
Conversation
Co-authored-by: idosal <[email protected]>
Co-authored-by: idosal <[email protected]>
There was a problem hiding this comment.
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 inuseEffectcleanups to remove message listeners on unmount and dependency changes. - Add logic in
AppFrameto 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. |
| // 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); | ||
| }); |
There was a problem hiding this comment.
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.
| // 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); | ||
| }); | ||
| } |
There was a problem hiding this comment.
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.
| 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(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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).
PostMessageTransportinstances registerwindow.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
AppBridgeinstance in effect scope for cleanupbridge.close()in effect cleanup to remove listeners when component unmounts orclientchangesAppFrame.tsx
appBridgeorsandbox.urlawaiton bridge close during setup to avoid blocking cleanupTests
close()method to mockAppBridgeExample
Before:
After:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.