You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2026/05/04 16:41:26 UTC
[PR] fix(embedded): prevent duplicate React root on rehandshake [superset]
msyavuz opened a new pull request, #39860:
URL: https://github.com/apache/superset/pull/39860
### SUMMARY
Follow-up to #38563. The `started` flag and the inline `createRoot(appMountPoint).render(...)` call both lived inside the message handler closure, so each `'port transfer'` MessageEvent reset `started=false` and could call `createRoot` a second time on the same DOM node. React 18 logs *"You are calling ReactDOMClient.createRoot() on a container that has already been passed to createRoot() before"* and behavior is undefined.
Hoist `started` and a lazily-created `root` to module scope, render via the existing root on rehandshake, and unmount it in the existing `beforeunload` handler. The root is created lazily so the no-iframe failure path can still write to `appMountPoint.innerHTML` without conflicting with React.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no UI change; defensive fix for a console warning + undefined behavior on rehandshake.
### TESTING INSTRUCTIONS
1. Embed a Superset dashboard in an iframe, set up Switchboard.
2. Trigger a second `'port transfer'` handshake from the parent (e.g. recreate the iframe channel).
3. Confirm no `createRoot()` warning in console; the embedded app continues to render correctly.
### ADDITIONAL INFORMATION
- Has associated issue: No
- Required feature flags: None
- Changes UI: No — defensive fix in module bootstrap
- Includes DB Migration: No
- Introduces new feature or API: No
- Removes existing feature or API: No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): prevent duplicate React root on rehandshake [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39860:
URL: https://github.com/apache/superset/pull/39860#issuecomment-4372759036
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): prevent duplicate React root on rehandshake [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #39860:
URL: https://github.com/apache/superset/pull/39860#discussion_r3183275678
##########
superset-frontend/src/embedded/index.tsx:
##########
@@ -150,6 +150,8 @@ if (!window.parent || window.parent === window) {
// }
let displayedUnauthorizedToast = false;
+let root: Root | null = null;
+let started = false;
Review Comment:
@msyavuz this looks like it's worth addressing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): prevent duplicate React root on rehandshake [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39860:
URL: https://github.com/apache/superset/pull/39860#issuecomment-4373004939
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39860?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `8 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.36%. Comparing base ([`41a22d7`](https://app.codecov.io/gh/apache/superset/commit/41a22d79184dfce79b54b991dcdb2feb5e4b95cb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`88bedeb`](https://app.codecov.io/gh/apache/superset/commit/88bedebd0971f8739f667e8856d919d7a7b901be?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39860?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset-frontend/src/embedded/index.tsx](https://app.codecov.io/gh/apache/superset/pull/39860?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fembedded%2Findex.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2VtYmVkZGVkL2luZGV4LnRzeA==) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39860?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39860 +/- ##
=======================================
Coverage 64.36% 64.36%
=======================================
Files 2569 2569
Lines 134745 134751 +6
Branches 31278 31280 +2
=======================================
+ Hits 86732 86738 +6
Misses 46515 46515
Partials 1498 1498
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39860/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/39860/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.62% <0.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39860?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): prevent duplicate React root on rehandshake [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39860:
URL: https://github.com/apache/superset/pull/39860#issuecomment-4372809095
<!-- Bito Reply -->
The flagged logic error is correct: hoisting `started` to module scope causes it to persist across rehandshakes, preventing retries if the first startup fails. To resolve, reset `started` to false in the error path of the `start` function, allowing subsequent `guestToken` calls to retry startup.
**superset-frontend/src/embedded/index.tsx**
```
err => {
// something is most likely wrong with the guest token
logging.error('Failed to load user data:', err);
showFailureMessage();
started = false;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): prevent duplicate React root on rehandshake [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39860:
URL: https://github.com/apache/superset/pull/39860#discussion_r3183038404
##########
superset-frontend/src/embedded/index.tsx:
##########
@@ -150,6 +150,8 @@ if (!window.parent || window.parent === window) {
// }
let displayedUnauthorizedToast = false;
+let root: Root | null = null;
+let started = false;
Review Comment:
**Suggestion:** Hoisting `started` to module scope introduces a retry regression: if the first startup attempt fails (for example a transient `/api/v1/me/roles/` failure), the flag remains true and later `guestToken` calls on rehandshake can no longer trigger startup. This can leave the embed permanently stuck on the failure message even after a valid token arrives. Set the startup guard only after a successful render, or reset it in the error path so rehandshakes can recover. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Embedded dashboards cannot recover after transient auth error.
- ⚠️ Embedded Switchboard rehandshake fails to restart app.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load the embedded dashboard entrypoint implemented in
`superset-frontend/src/embedded/index.tsx`, which initializes `let started = false;` at
line 154 and registers the `message` event listener at lines 233–326.
2. From the embedding parent (using Switchboard), send a `'port transfer'` message so the
listener at lines 241–245 obtains a `MessagePort`, calls `Switchboard.init`, and defines
the `guestToken` method at lines 251–258.
3. Have the parent call the `guestToken` method with a guest token that causes
`/api/v1/me/roles/` to fail (e.g. transient auth or network issue): the handler at lines
251–258 calls `setupGuestClient` and, because `started` is false, calls `start()` (line
179) and immediately sets `started = true`. The `start()` function invokes `makeApi(...
'/api/v1/me/roles/' ...)` at lines 180–184; the rejected promise takes the error branch at
lines 199–207, which logs the error and calls `showFailureMessage` (lines 129–131) to
write the failure text into `appMountPoint.innerHTML`. The success branch at lines 185–197
is never executed, so `root` remains `null` and no React tree is mounted.
4. Without reloading the page, perform a rehandshake: the parent sends another `'port
transfer'` `MessageEvent`, re-entering the listener at lines 233–326, which again defines
`guestToken`. On subsequent `guestToken` invocations, the guard at lines 253–258 sees
`started` (line 154) is already `true`, so `start()` is not called again,
`/api/v1/me/roles/` is never retried, `root` stays `null`, and the embedded dashboard
remains stuck showing the failure message from step 3 even if the new guest token is
valid.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fembedded%2Findex.tsx%0A%2A%2ALine%3A%2A%2A%20154%3A154%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Hoisting%20%60started%60%20to%20module%20scope%20introduces%20a%20retry%20regression%3A%20if%20the%20first%20startup%20attempt%20fails%20%28for%20example%20a%20transient%20%60%2Fapi%2Fv1%2Fme%2Froles%2F%60%20failure%29%2C%20the%20flag%20remains%20true%20and%20later%20%60guestToken%60%20calls%20on%20rehandshake%20can%20no%20longer%20trigger%20startup.%20This%20can%20leave%20the%20embed%20permanently%20stuck%20on%20the%20failure%20message%20even%20after%20a%20valid%20token%20arrives.%20Set%20the%20startup%20guard%20only%20after%20a%20successful%20render%2C%20or%20reset%20it%20in%20the%20error%20path%20so%20rehandshakes%20can%20recover.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.
%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fsrc%2Fembedded%2Findex.tsx%0A%2A%2ALine%3A%2A%2A%20154%3A154%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Hoisting%20%60started%60%20to%20module%20scope%20introduces%20a%20retry%20regression%3A%20if%20the%20first%20startup%20attempt%20fails%20%28for%20example%20a%20transient%20%60%2Fapi%2Fv1%2Fme%2Froles%2F%60%20failure%
29%2C%20the%20flag%20remains%20true%20and%20later%20%60guestToken%60%20calls%20on%20rehandshake%20can%20no%20longer%20trigger%20startup.%20This%20can%20leave%20the%20embed%20permanently%20stuck%20on%20the%20failure%20message%20even%20after%20a%20valid%20token%20arrives.%20Set%20the%20startup%20guard%20only%20after%20a%20successful%20render%2C%20or%20reset%20it%20in%20the%20error%20path%20so%20rehandshakes%20can%20recover.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/embedded/index.tsx
**Line:** 154:154
**Comment:**
*Logic Error: Hoisting `started` to module scope introduces a retry regression: if the first startup attempt fails (for example a transient `/api/v1/me/roles/` failure), the flag remains true and later `guestToken` calls on rehandshake can no longer trigger startup. This can leave the embed permanently stuck on the failure message even after a valid token arrives. Set the startup guard only after a successful render, or reset it in the error path so rehandshakes can recover.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39860&comment_hash=1beb346d4e7f53a17ca6655b8fae8edb51e84ebf01e8bb7699922701e86c1139&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39860&comment_hash=1beb346d4e7f53a17ca6655b8fae8edb51e84ebf01e8bb7699922701e86c1139&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): prevent duplicate React root on rehandshake [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39860:
URL: https://github.com/apache/superset/pull/39860#issuecomment-4373486065
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): prevent duplicate React root on rehandshake [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz merged PR #39860:
URL: https://github.com/apache/superset/pull/39860
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org