codeant-ai-for-open-source[bot] commented on code in PR #40870:
URL: https://github.com/apache/superset/pull/40870#discussion_r3375872991
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -248,20 +267,33 @@ export async function embedDashboard({
}
const [guestToken, ourPort]: [string, Switchboard] = await Promise.all([
- fetchGuestToken(),
+ fetchGuestTokenWithTimeout(),
mountIframe(),
]);
Review Comment:
**Suggestion:** If the token promise times out/rejects, `Promise.all`
rejects after `mountIframe()` has already mutated the DOM and opened iframe
comms, but no cleanup runs because the function exits before `unmount` is
returned. Add cleanup on the error path (e.g., catch around the initial await)
to remove the iframe and dispose resources before rethrowing. [missing cleanup]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Failed initial token fetch leaves iframe mounted without cleanup.
- ⚠️ Host receives error yet keeps orphaned embedded dashboard iframe.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A host app (or the embedded test app described in
`superset-frontend/playwright/pages/EmbeddedPage.ts:24-29`) calls
`embedDashboard` from
`superset-embedded-sdk/src/index.ts:126-139`, providing a `fetchGuestToken`
that can
reject (e.g., backend error) or take longer than the configured timeout in
`withTimeout.ts:25-42`.
2. Inside `embedDashboard`, the initial setup concurrently starts
`fetchGuestTokenWithTimeout()` and `mountIframe()` via `Promise.all` at
`index.ts:269-272`. `mountIframe` at `index.ts:184-267` creates an iframe,
sets its `src`,
and inserts it into the DOM with `mountPoint.replaceChildren(iframe)` at
line 264, as well
as wiring a `load` listener that establishes a `MessageChannel` and
constructs a
`Switchboard` instance at lines 226-250.
3. If `fetchGuestTokenWithTimeout()` rejects (from the host callback or
timeout),
`Promise.all` at `index.ts:269-272` rejects after `mountIframe()` has
already mutated the
DOM and created the `Switchboard`. Execution jumps out of `embedDashboard`
before the code
that emits the initial guest token (`index.ts:274-275`), starts the
Switchboard
(`ourPort.start()` at 300), defines iframe methods (`index.ts:301-314`),
sets up the
refresh timer (`index.ts:293-296`), or creates the `unmount` function
(`index.ts:316-325`).
4. The caller receives a rejected Promise from `embedDashboard` but the
iframe appended at
`index.ts:264` remains mounted, along with its message ports and event
listeners, and the
host has no `EmbeddedDashboard.unmount` handle to clean it up. Until the
host explicitly
manipulates `mountPoint` or reloads the page, the orphaned iframe and
associated resources
persist despite the failed initialization, matching the suggestion that the
rejection path
lacks cleanup for partially initialized state.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=175dd144e4cf4603ba1c70130ed8afd9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=175dd144e4cf4603ba1c70130ed8afd9&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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-embedded-sdk/src/index.ts
**Line:** 269:272
**Comment:**
*Missing Cleanup: If the token promise times out/rejects, `Promise.all`
rejects after `mountIframe()` has already mutated the DOM and opened iframe
comms, but no cleanup runs because the function exits before `unmount` is
returned. Add cleanup on the error path (e.g., catch around the initial await)
to remove the iframe and dispose resources before rethrowing.
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%2F40870&comment_hash=2b085243f8260447eb3aea47688e88848ce3473f3fceeb112ef32b3099f65c8c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40870&comment_hash=2b085243f8260447eb3aea47688e88848ce3473f3fceeb112ef32b3099f65c8c&reaction=dislike'>👎</a>
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -248,20 +267,33 @@ export async function embedDashboard({
}
const [guestToken, ourPort]: [string, Switchboard] = await Promise.all([
- fetchGuestToken(),
+ fetchGuestTokenWithTimeout(),
mountIframe(),
]);
ourPort.emit('guestToken', { guestToken });
log('sent guest token');
+ // Track the pending refresh timer so it can be cancelled on unmount, and
+ // stop the cycle once unmounted so it cannot leak across mount/unmount
cycles.
+ let refreshTimer: ReturnType<typeof setTimeout> | undefined;
+ let unmounted = false;
+
async function refreshGuestToken() {
- const newGuestToken = await fetchGuestToken();
+ if (unmounted) return;
+ const newGuestToken = await fetchGuestTokenWithTimeout();
+ if (unmounted) return;
ourPort.emit('guestToken', { guestToken: newGuestToken });
- setTimeout(refreshGuestToken, getGuestTokenRefreshTiming(newGuestToken));
+ refreshTimer = setTimeout(
+ refreshGuestToken,
+ getGuestTokenRefreshTiming(newGuestToken),
+ );
}
Review Comment:
**Suggestion:** The refresh callback awaits token fetch without any error
handling, so a timeout/rejection will throw out of the `setTimeout` callback as
an unhandled promise rejection and permanently stop further token refreshes.
Wrap this flow in `try/catch` and always schedule the next retry (or controlled
backoff) so one transient failure does not break auth refresh for the rest of
the session. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Embedded dashboards stop refreshing guest tokens on fetch errors.
- ⚠️ Expired guest tokens break iframe API requests until reload.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a host app or test harness, call `embedDashboard` exported from
`superset-embedded-sdk/src/index.ts:126-139`, passing a valid `mountPoint`
and a
`fetchGuestToken` implementation that can reject (e.g., network error or
backend 500).
2. `embedDashboard` completes the initial token fetch via `Promise.all` at
`index.ts:269-272`, then schedules the first refresh with `refreshTimer =
setTimeout(refreshGuestToken, getGuestTokenRefreshTiming(guestToken))` at
`index.ts:293-296`, where the delay is computed from the token expiry in
`getGuestTokenRefreshTiming` at
`superset-embedded-sdk/src/guestTokenRefresh.ts:26-32`.
3. When the refresh timer fires, `refreshGuestToken` at `index.ts:282-291`
runs; since
`unmounted` is still `false`, it hits `const newGuestToken = await
fetchGuestTokenWithTimeout();` at line 284. `fetchGuestTokenWithTimeout`
wraps the host
callback in `withTimeout` from
`superset-embedded-sdk/src/withTimeout.ts:25-42`, which
rejects if the underlying promise rejects or the timeout elapses.
4. On such a rejection, the `await` at `index.ts:284` throws inside the async
`refreshGuestToken` function, so the subsequent lines 285-290 (emitting the
new token and
scheduling the next `setTimeout`) never execute. Because `setTimeout`
ignores the returned
Promise from an async callback, this rejection becomes an unhandled promise
rejection and
no new `refreshTimer` is scheduled. The refresh loop stops after this single
failure, and
once the original token expires (per `guestTokenRefresh.ts:26-32`), the
embedded iframe
continues running with an expired guest token and no further refresh
attempts until the
host reloads the page or re-invokes `embedDashboard`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=33c67e3a9ed841fca3c0f9a610b8a29c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=33c67e3a9ed841fca3c0f9a610b8a29c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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-embedded-sdk/src/index.ts
**Line:** 282:291
**Comment:**
*Logic Error: The refresh callback awaits token fetch without any error
handling, so a timeout/rejection will throw out of the `setTimeout` callback as
an unhandled promise rejection and permanently stop further token refreshes.
Wrap this flow in `try/catch` and always schedule the next retry (or controlled
backoff) so one transient failure does not break auth refresh for the rest of
the session.
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%2F40870&comment_hash=1fcdbe313a1819c35f2eb830d67f3c9ca3fb2889fb67dc9f23fc9c8973c094ae&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40870&comment_hash=1fcdbe313a1819c35f2eb830d67f3c9ca3fb2889fb67dc9f23fc9c8973c094ae&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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
