codeant-ai-for-open-source[bot] commented on code in PR #38308:
URL: https://github.com/apache/superset/pull/38308#discussion_r3487342258
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -185,15 +185,15 @@ export async function embedDashboard({
}
async function mountIframe(): Promise<Switchboard> {
- return new Promise(resolve => {
- const iframe = document.createElement('iframe');
+ return new Promise((resolve) => {
+ const iframe = document.createElement("iframe");
const dashboardConfigUrlParams = dashboardUiConfig
? { uiConfig: `${calculateConfig()}` }
: undefined;
const filterConfig = dashboardUiConfig?.filters || {};
const filterConfigKeys = Object.keys(filterConfig);
const filterConfigUrlParams = Object.fromEntries(
- filterConfigKeys.map(key => [
+ filterConfigKeys.map((key) => [
DASHBOARD_UI_FILTER_CONFIG_URL_PARAM_KEY[key],
filterConfig[key],
]),
Review Comment:
**Suggestion:** Mapping all `filters` keys directly to
`DASHBOARD_UI_FILTER_CONFIG_URL_PARAM_KEY` can produce an `undefined` query key
when callers pass an unsupported filter property, resulting in malformed URL
params like `undefined=true`. Filter to known keys (or validate before mapping)
so only supported params are serialized. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Embedded dashboard iframe URL contains undefined query parameters.
- ⚠️ Host filters config silently produces incorrect URL output.
- ⚠️ Future filter extensions risk unexpected undefined mappings.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Supply a `dashboardUiConfig` with an unsupported filters key to
`embedDashboard` in
superset-embedded-sdk/src/index.ts:129-142, for example `dashboardUiConfig =
{ filters: {
foo: true } }`, which is allowed by the loose index signature in
UiConfigType at lines
40-49.
2. When `embedDashboard` calls `mountIframe`
(superset-embedded-sdk/src/index.ts:187-270),
it derives `filterConfig` and `filterConfigKeys` from
`dashboardUiConfig.filters` at lines
193-194, so `filterConfigKeys` includes `"foo"`.
3. At lines 195-200 in the same file, `filterConfigUrlParams` is built via
`Object.fromEntries(filterConfigKeys.map((key) =>
[DASHBOARD_UI_FILTER_CONFIG_URL_PARAM_KEY[key], filterConfig[key]]))`, but
the mapping in
superset-embedded-sdk/src/const.ts:21-26 only defines entries for
`"visible"` and
`"expanded"`, so `DASHBOARD_UI_FILTER_CONFIG_URL_PARAM_KEY["foo"]` is
`undefined`, and
Object.fromEntries creates an entry with key `"undefined"` and value `true`.
4. `urlParams` and `urlParamsString` are then computed at lines 202-210, and
the iframe
src is set at line 255 to
`${supersetDomain}/embedded/${id}${urlParamsString}`, producing
a query string containing `undefined=true`, which yields malformed URL
parameters that do
not correspond to any supported filter config option.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4ee3631639c547bebf2f9c83ebdd9108&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4ee3631639c547bebf2f9c83ebdd9108&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:** 196:199
**Comment:**
*Logic Error: Mapping all `filters` keys directly to
`DASHBOARD_UI_FILTER_CONFIG_URL_PARAM_KEY` can produce an `undefined` query key
when callers pass an unsupported filter property, resulting in malformed URL
params like `undefined=true`. Filter to known keys (or validate before mapping)
so only supported params are serialized.
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%2F38308&comment_hash=793170128f0f427cd7c603cd4c18fbe0623c50c59ea73e3ed8c62c029e80310b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38308&comment_hash=793170128f0f427cd7c603cd4c18fbe0623c50c59ea73e3ed8c62c029e80310b&reaction=dislike'>👎</a>
##########
superset-embedded-sdk/src/guestTokenRefresh.ts:
##########
@@ -18,17 +18,23 @@
*/
import { jwtDecode } from "jwt-decode";
-export const REFRESH_TIMING_BUFFER_MS = 5000 // refresh guest token early to
avoid failed superset requests
-export const MIN_REFRESH_WAIT_MS = 10000 // avoid blasting requests as fast as
the cpu can handle
-export const DEFAULT_TOKEN_EXP_MS = 300000 // (5 min) used only when parsing
guest token exp fails
-export const DEFAULT_TOKEN_REFRESH_RETRY_MS = 10000 // wait before retrying a
failed/timed-out token refresh
+export const REFRESH_TIMING_BUFFER_MS = 5000; // refresh guest token early to
avoid failed superset requests
+export const MIN_REFRESH_WAIT_MS = 10000; // avoid blasting requests as fast
as the cpu can handle
+export const DEFAULT_TOKEN_EXP_MS = 300000; // (5 min) used only when parsing
guest token exp fails
+export const DEFAULT_TOKEN_REFRESH_RETRY_MS = 10000; // wait before retrying a
failed/timed-out token refresh
// when do we refresh the guest token?
export function getGuestTokenRefreshTiming(currentGuestToken: string) {
const parsedJwt = jwtDecode<Record<string, any>>(currentGuestToken);
// if exp is int, it is in seconds, but Date() takes milliseconds
- const exp = new Date(/[^0-9\.]/g.test(parsedJwt.exp) ? parsedJwt.exp :
parseFloat(parsedJwt.exp) * 1000);
- const isValidDate = exp.toString() !== 'Invalid Date';
- const ttl = isValidDate ? Math.max(MIN_REFRESH_WAIT_MS, exp.getTime() -
Date.now()) : DEFAULT_TOKEN_EXP_MS;
+ const exp = new Date(
+ /[^0-9\.]/g.test(parsedJwt.exp)
+ ? parsedJwt.exp
+ : parseFloat(parsedJwt.exp) * 1000,
+ );
+ const isValidDate = exp.toString() !== "Invalid Date";
+ const ttl = isValidDate
+ ? Math.max(MIN_REFRESH_WAIT_MS, exp.getTime() - Date.now())
+ : DEFAULT_TOKEN_EXP_MS;
Review Comment:
**Suggestion:** The new date parsing treats non-string/non-numeric values
like `null` as valid dates (`new Date(null)` becomes epoch), so malformed
tokens can be interpreted as expiring immediately and trigger refresh every ~5
seconds instead of using the safe default TTL. Add a strict type/format check
for `exp` and fall back to `DEFAULT_TOKEN_EXP_MS` for
non-number/non-date-string values. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Guest token refresh runs every five seconds.
- ⚠️ Excessive guest token fetches from embedded dashboards.
- ⚠️ Increased load and rate-limit pressure on host API.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the host app that uses the Embedded SDK, implement `fetchGuestToken`
(referenced in
superset-embedded-sdk/src/index.ts:59-68) so it returns a JWT whose payload
has `exp:
null` instead of a numeric or ISO string expiration.
2. Call `embedDashboard` from superset-embedded-sdk/src/index.ts:129-142
with this
`fetchGuestToken`; inside embedDashboard, the initial refresh timer is
scheduled via
`setTimeout(refreshGuestToken, getGuestTokenRefreshTiming(guestToken))` at
lines 319-321.
3. In superset-embedded-sdk/src/guestTokenRefresh.ts:27-38,
`getGuestTokenRefreshTiming`
decodes the token, and for `exp: null` computes `exp = new Date(null)` at
line 30, which
yields the Unix epoch (a valid Date object) so `isValidDate` at line 35 is
true.
4. Because the epoch time is far in the past, `exp.getTime() - Date.now()`
is negative, so
the TTL at line 36 becomes `MIN_REFRESH_WAIT_MS` (10000 ms), and
`getGuestTokenRefreshTiming` returns `MIN_REFRESH_WAIT_MS -
REFRESH_TIMING_BUFFER_MS`
(about 5000 ms), causing the refresh loop in index.ts:296-321 to re-fetch
the guest token
roughly every five seconds instead of using the safer default TTL
`DEFAULT_TOKEN_EXP_MS`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=83c948492c214055bd609917ecad84ab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=83c948492c214055bd609917ecad84ab&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/guestTokenRefresh.ts
**Line:** 30:38
**Comment:**
*Incorrect Condition Logic: The new date parsing treats
non-string/non-numeric values like `null` as valid dates (`new Date(null)`
becomes epoch), so malformed tokens can be interpreted as expiring immediately
and trigger refresh every ~5 seconds instead of using the safe default TTL. Add
a strict type/format check for `exp` and fall back to `DEFAULT_TOKEN_EXP_MS`
for non-number/non-date-string values.
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%2F38308&comment_hash=ccefe4a860c428a4fc7ecd4b3cceebac3a9a16f2e3de81a8e31ab4ef99bfa6bc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38308&comment_hash=ccefe4a860c428a4fc7ecd4b3cceebac3a9a16f2e3de81a8e31ab4ef99bfa6bc&reaction=dislike'>👎</a>
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -350,24 +350,25 @@ export async function embedDashboard({
mountPoint.replaceChildren();
}
- const getScrollSize = () => ourPort.get<Size>('getScrollSize');
+ const getScrollSize = () => ourPort.get<Size>("getScrollSize");
const getDashboardPermalink = (anchor: string) =>
- ourPort.get<string>('getDashboardPermalink', { anchor });
- const getActiveTabs = () => ourPort.get<string[]>('getActiveTabs');
- const getDataMask = () => ourPort.get<Record<string, any>>('getDataMask');
- const getChartStates = () => ourPort.get<Record<string,
any>>('getChartStates');
+ ourPort.get<string>("getDashboardPermalink", { anchor });
+ const getActiveTabs = () => ourPort.get<string[]>("getActiveTabs");
+ const getDataMask = () => ourPort.get<Record<string, any>>("getDataMask");
+ const getChartStates = () =>
+ ourPort.get<Record<string, any>>("getChartStates");
const getChartDataPayloads = (params?: { chartId?: number }) =>
- ourPort.get<Record<string, any>>('getChartDataPayloads', params);
- const observeDataMask = (
- callbackFn: ObserveDataMaskCallbackFn,
- ) => {
- ourPort.defineMethod('observeDataMask', callbackFn);
+ ourPort.get<Record<string, any>>("getChartDataPayloads", params);
+ const observeDataMask = (callbackFn: ObserveDataMaskCallbackFn) => {
+ ourPort.defineMethod("observeDataMask", callbackFn);
};
// TODO: Add proper types once theming branch is merged
- const setThemeConfig = async (themeConfig: Record<string, any>):
Promise<void> => {
+ const setThemeConfig = async (
Review Comment:
**Suggestion:** `setThemeConfig` is now `async` and returns a `Promise`, but
the public `EmbeddedDashboard` contract declares it as returning `void`; this
changes error behavior from synchronous throws to rejected promises that
callers are unlikely to await, causing uncaught promise rejections. Keep this
method synchronous (or update the public type and all call sites to await it
consistently). [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Embedded SDK theme API contract misrepresents return type.
- ⚠️ Errors during theme updates become unhandled rejections.
- ⚠️ Host apps cannot reliably catch theme config failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A host application calls `embedDashboard` from
superset-embedded-sdk/src/index.ts:129-142 and stores the returned
`EmbeddedDashboard`
object, whose type is declared at lines 111-124 with `setThemeConfig:
(themeConfig:
Record<string, any>) => void`, suggesting a synchronous, void-returning
method.
2. The host then calls `dashboard.setThemeConfig(themeConfig)` with a
configuration that
includes an uncloneable value (for example, a function property), which is
allowed at
runtime despite the intended plain-object contract.
3. Inside the SDK implementation at
superset-embedded-sdk/src/index.ts:366-378,
`setThemeConfig` is defined as `async` and calls
`ourPort.emit("setThemeConfig", {
themeConfig })` within a try block; `emit` ultimately calls
`this.port.postMessage(message)` in
superset-frontend/packages/superset-ui-switchboard/src/switchboard.ts:83-94,
where the
browser can throw a `DataCloneError` when it encounters an uncloneable value.
4. That error is caught by the `setThemeConfig` catch block, logged, and
rethrown at lines
372-376, but because `setThemeConfig` is an async function, the throw
results in a
rejected Promise instead of a synchronous exception, so callers (who expect a
void-returning API and typically do not `await` or `.catch` this call) see
an unhandled
promise rejection rather than a catchable synchronous error, diverging from
the published
`EmbeddedDashboard` contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b558672df0e54ff8a0ff3b5f7c94f6ac&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b558672df0e54ff8a0ff3b5f7c94f6ac&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:** 366:378
**Comment:**
*Api Mismatch: `setThemeConfig` is now `async` and returns a `Promise`,
but the public `EmbeddedDashboard` contract declares it as returning `void`;
this changes error behavior from synchronous throws to rejected promises that
callers are unlikely to await, causing uncaught promise rejections. Keep this
method synchronous (or update the public type and all call sites to await it
consistently).
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%2F38308&comment_hash=2c9822947a14a677e4c0aaf14d1bb31ca57b9113a01640f0e3881b5d2db0f546&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38308&comment_hash=2c9822947a14a677e4c0aaf14d1bb31ca57b9113a01640f0e3881b5d2db0f546&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]
