-
Notifications
You must be signed in to change notification settings - Fork 16.6k
fix(embedded-sdk): clear fetchGuestToken timers on unmount #35143 #37280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(embedded-sdk): clear fetchGuestToken timers on unmount #35143 #37280
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #31b020Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
CodeAnt AI finished reviewing your PR. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #c29043Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
There was a problem hiding this 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 fixes a memory leak in the @superset-ui/embedded-sdk package where guest token refresh timers continued running after calling unmount(). The fix introduces timer tracking and cleanup to ensure proper resource disposal.
Changes:
- Added timer ID tracking for the
fetchGuestTokenrefresh mechanism - Implemented timer cleanup in the
unmount()function - Bumped package version from 0.3.0 to 0.3.1
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| superset-embedded-sdk/src/index.ts | Added refreshTokenTimerId variable to track setTimeout timers and clear them in unmount() |
| superset-embedded-sdk/package.json | Version bump to 0.3.1 |
| superset-embedded-sdk/package-lock.json | Version bump to 0.3.1 (lockfile update) |
Files not reviewed (1)
- superset-embedded-sdk/package-lock.json: Language not supported
| async function refreshGuestToken() { | ||
| const newGuestToken = await fetchGuestToken(); | ||
| ourPort.emit('guestToken', { guestToken: newGuestToken }); | ||
| setTimeout(refreshGuestToken, getGuestTokenRefreshTiming(newGuestToken)); | ||
| refreshTokenTimerId = setTimeout(refreshGuestToken, getGuestTokenRefreshTiming(newGuestToken)); | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a race condition in the timer cleanup logic. If unmount() is called while refreshGuestToken() is executing (after the timer has fired but before the new timer is set at line 256), the new timer won't be cleared.
To fix this, consider adding a flag to prevent setting new timers after unmount is called, or setting refreshTokenTimerId to null immediately when the timer fires, or clearing it at the start of refreshGuestToken before the async operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
superset-embedded-sdk/src/index.ts
Outdated
| if (refreshTokenTimerId) { | ||
| clearTimeout(refreshTokenTimerId); | ||
| } |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new timer cleanup functionality in the unmount method lacks test coverage. Consider adding tests to verify that clearTimeout is called when unmount is invoked, and that no timers continue running after unmount.
df6a8a8 to
28387f5
Compare
| clearTimeout(refreshTokenTimerId); | ||
| refreshTokenTimerId = 'UNMOUNTED'; | ||
|
|
||
| mountPoint.replaceChildren(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: unmount() clears the refresh timer but does not stop or close the Switchboard/message port, so the message channel and any defined methods/listeners can remain active after unmount, causing memory leaks and continued message handling. Stop the switchboard and close its underlying port (if present) during unmount. [resource leak]
Severity Level: Critical 🚨
- ❌ Message ports/listeners may leak after unmount.
- ⚠️ Long-running host pages accumulate memory leaks.
- ⚠️ Embedded comms may still receive messages unexpectedly.| mountPoint.replaceChildren(); | |
| // Stop the switchboard and close the underlying MessagePort if available to avoid | |
| // leaving listeners/ports open after unmount. | |
| try { | |
| (ourPort as any).stop?.(); | |
| } catch (e) { | |
| // ignore stop errors | |
| } | |
| try { | |
| (ourPort as any).port?.close?.(); | |
| } catch (e) { | |
| // ignore close errors | |
| } | |
Steps of Reproduction ✅
1. Call embedDashboard() (superset-embedded-sdk/src/index.ts:118) which eventually calls
ourPort.start() at line 266 to begin iframe comms.
2. Interact with the embedded dashboard; ourPort.defineMethod(...) is used (lines 267-279)
and communication channel remains active.
3. Call unmount() (lines 282-289). Current unmount clears the refresh timer (lines
285-286) and removes iframe (line 288) but does not call any Switchboard stop/port close
methods.
4. Because ourPort.was started at line 266 and no stop/close is invoked on unmount, the
underlying MessageChannel/handlers defined by Switchboard can remain active, causing
retained listeners/ports and potential memory leaks.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-embedded-sdk/src/index.ts
**Line:** 288:288
**Comment:**
*Resource Leak: `unmount()` clears the refresh timer but does not stop or close the Switchboard/message port, so the message channel and any defined methods/listeners can remain active after unmount, causing memory leaks and continued message handling. Stop the switchboard and close its underlying port (if present) during unmount.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Code Review Agent Run #f5eb83Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
| ourPort.emit('guestToken', { guestToken }); | ||
| log('sent guest token'); | ||
|
|
||
| let refreshTokenTimerId: ReturnType<typeof setTimeout> | 'INITIAL' | 'UNMOUNTED' = 'INITIAL'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit off. Do we need this and UNMOUNTED check under refreshGuestToken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to fix race condition problem from this comment #37280 (comment)
It will happen when call unmount function during waiting for new guest token response. So unmount function will clear the old setTimeout and refreshGuestToken will create a new setTimeout.
SUMMARY
Fixes memory leak in @superset-ui/embedded-sdk where fetchGuestToken refresh timers continue running after unmount().
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION