codeant-ai-for-open-source[bot] commented on code in PR #35754:
URL: https://github.com/apache/superset/pull/35754#discussion_r2945593160
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -83,6 +86,14 @@ const getActiveTabs = () =>
store?.getState()?.dashboardState?.activeTabs || [];
const getDataMask = () => store?.getState()?.dataMask || {};
+const setDataMask = ({ dataMask }: { dataMask: DataMaskStateWithId }) => {
+ batch(() => {
+ Object.entries(dataMask).forEach(([filterId, mask]) => {
+ store?.dispatch(updateDataMask(filterId, mask));
+ });
Review Comment:
**Suggestion:** The method dispatches updates for every incoming key without
validating that the filter exists in current dashboard state. Unknown IDs are
still inserted into `dataMask`, and downstream active-filter derivation treats
such entries as globally scoped, which can apply stale or unintended filters to
charts. Ignore unknown filter IDs before dispatching. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unknown IDs become active filters across all charts.
- ❌ Chart queries may include unintended extra_form_data filters.
- ⚠️ Embedded integrations break when filter IDs are mistyped.
```
</details>
```suggestion
const currentDataMask = store?.getState()?.dataMask || {};
Object.entries(dataMask).forEach(([filterId, mask]) => {
if (!(filterId in currentDataMask)) {
return;
}
store?.dispatch(updateDataMask(filterId, mask));
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Parent app calls `dashboard.setDataMask(...)` from SDK
(`superset-embedded-sdk/src/index.ts:32`) with a typo/unknown key, e.g. `{
"NATIVE_FILTER-typo": { extraFormData: {...} } }`.
2. Embedded RPC wiring at `superset-frontend/src/embedded/index.tsx:48`
invokes
`embeddedApi.setDataMask`, which dispatches `updateDataMask(filterId, mask)`
for every key
(`superset-frontend/src/embedded/api.tsx:91-92`).
3. DataMask reducer handles `UPDATE_DATA_MASK` by creating
`draft[action.filterId]`
unconditionally (`superset-frontend/src/dataMask/reducer.ts:29-34`), so
unknown IDs are
inserted into Redux state.
4. Active-filter derivation iterates all `dataMask` entries; for unknown
IDs, scope falls
back to `allSliceIds`
(`superset-frontend/src/dashboard/util/activeAllDashboardFilters.ts:113-122`),
effectively
treating them as dashboard-wide filters.
5. Chart query construction uses those active filters and applies matching
IDs to each
chart
(`superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts:33-44`),
so
unintended extraFormData can affect chart queries broadly.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/embedded/api.tsx
**Line:** 91:93
**Comment:**
*Logic Error: The method dispatches updates for every incoming key
without validating that the filter exists in current dashboard state. Unknown
IDs are still inserted into `dataMask`, and downstream active-filter derivation
treats such entries as globally scoped, which can apply stale or unintended
filters to charts. Ignore unknown filter IDs before dispatching.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=aae0414056644dc295ab1868b7af8a2ae9bf3e5d07d5e6d2aacc653b6ba2cf8d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=aae0414056644dc295ab1868b7af8a2ae9bf3e5d07d5e6d2aacc653b6ba2cf8d&reaction=dislike'>👎</a>
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -287,6 +288,7 @@ export async function embedDashboard({
ourPort.get<string>('getDashboardPermalink', { anchor });
const getActiveTabs = () => ourPort.get<string[]>('getActiveTabs');
const getDataMask = () => ourPort.get<Record<string, any>>('getDataMask');
+ const setDataMask = (dataMask: Record<string, any>) =>
ourPort.emit('setDataMask', {dataMask});
Review Comment:
**Suggestion:** `setDataMask` forwards the payload unchanged, so if callers
pass the object from `observeDataMask` (which includes non-filter keys like
change-trigger booleans), those keys are sent as filter IDs and can create
invalid data-mask entries in the embedded Redux state. Filter out
non-object/non-filter entries before emitting. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ getDataMask includes synthetic non-filter entries.
- ⚠️ observeDataMask forwarding pollutes Redux dataMask keys.
- ❌ Cross-dashboard filter sync can apply unintended updates.
```
</details>
```suggestion
const setDataMask = (dataMask: Record<string, any>) =>
ourPort.emit('setDataMask', {
dataMask: Object.fromEntries(
Object.entries(dataMask).filter(
([filterId, mask]) =>
filterId !== 'crossFiltersChanged' &&
filterId !== 'nativeFiltersChanged' &&
mask !== null &&
typeof mask === 'object',
),
),
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable embedded emission (`dashboardUiConfig.emitDataMasks`) so the
iframe publishes
data-mask events; in `superset-frontend/src/embedded/index.tsx:73-87`,
`Switchboard.emit('observeDataMask', { ...currentDataMask,
...getDataMaskChangeTrigger(...) })` adds `crossFiltersChanged` and
`nativeFiltersChanged`.
2. Register parent callback through SDK `observeDataMask`
(`superset-embedded-sdk/src/index.ts:295-298`); callback type at
`superset-embedded-sdk/src/index.ts:83-86` explicitly includes those boolean
keys.
3. In host app, forward that callback payload into
`dashboard.setDataMask(payload)`; SDK
`setDataMask` at `superset-embedded-sdk/src/index.ts:291` emits payload
unchanged,
including boolean trigger keys.
4. Iframe receives `setDataMask` via `Switchboard.defineMethod('setDataMask',
embeddedApi.setDataMask)` (`superset-frontend/src/embedded/index.tsx:267`);
`embeddedApi.setDataMask` (`superset-frontend/src/embedded/api.tsx:89-93`)
iterates all
entries and dispatches `updateDataMask` for each key; reducer
(`superset-frontend/src/dataMask/reducer.ts:208-213`) creates entries for
those synthetic
keys, polluting `state.dataMask` and affecting subsequent `getDataMask`
(`superset-frontend/src/embedded/api.tsx:87`).
```
</details>
<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:** 291:291
**Comment:**
*Logic Error: `setDataMask` forwards the payload unchanged, so if
callers pass the object from `observeDataMask` (which includes non-filter keys
like change-trigger booleans), those keys are sent as filter IDs and can create
invalid data-mask entries in the embedded Redux state. Filter out
non-object/non-filter entries before emitting.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=6389bb084ebde1fc21301465244e9d6c28d6a2ba4a3fcf11258e9f3beb9bc982&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=6389bb084ebde1fc21301465244e9d6c28d6a2ba4a3fcf11258e9f3beb9bc982&reaction=dislike'>👎</a>
##########
superset-embedded-sdk/jest.config.js:
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+module.exports = {
+ preset: 'ts-jest',
+ testEnvironment: 'jsdom',
+ transform: {
+ '^.+\\.(ts|tsx|js|jsx)$': ['ts-jest', {
+ tsconfig: {
+ jsx: 'react',
+ esModuleInterop: true,
+ allowSyntheticDefaultImports: true,
+ },
Review Comment:
**Suggestion:** The transformer is configured to run `ts-jest` on `.js/.jsx`
files, but the inline TypeScript config does not enable JavaScript compilation.
This mismatch can make Jest fail when it needs to transform JavaScript files
(for example from allowed `node_modules` packages). Enable JavaScript
transpilation in the same `tsconfig` block. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Embedded SDK PR checks can fail on npm test.
- ⚠️ Local SDK contributors hit avoidable Jest transform errors.
- ⚠️ Dependency upgrades increase breakage risk in test pipeline.
```
</details>
```suggestion
tsconfig: {
jsx: 'react',
esModuleInterop: true,
allowSyntheticDefaultImports: true,
allowJs: true,
},
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the Embedded SDK CI entrypoint at
`.github/workflows/embedded-sdk-test.yml:15-27`;
PRs touching `superset-embedded-sdk/**` run `npm ci` then `npm test` in
`superset-embedded-sdk`.
2. During `npm test`, Jest loads
`superset-embedded-sdk/jest.config.js:23-33`, where
transform includes `.(ts|tsx|js|jsx)` and `transformIgnorePatterns`
explicitly whitelists
`@superset-ui` from `node_modules` ignore.
3. Tests import `@superset-ui/switchboard` in
`superset-embedded-sdk/src/index.test.ts:21`
and SDK code imports it in `superset-embedded-sdk/src/index.ts:26`, so Jest
can route
JavaScript files from that package through `ts-jest`.
4. `ts-jest` is configured with inline `tsconfig` at
`superset-embedded-sdk/jest.config.js:24-28` without `allowJs`; when JS
input is
transformed, test execution can fail with TypeScript/ts-jest JS-compilation
errors.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-embedded-sdk/jest.config.js
**Line:** 24:28
**Comment:**
*Possible Bug: The transformer is configured to run `ts-jest` on
`.js/.jsx` files, but the inline TypeScript config does not enable JavaScript
compilation. This mismatch can make Jest fail when it needs to transform
JavaScript files (for example from allowed `node_modules` packages). Enable
JavaScript transpilation in the same `tsconfig` block.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=c4b34f3eff3846837383ae2964e7f5f412da5c5c41249f70ea0cd6413dc339f2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=c4b34f3eff3846837383ae2964e7f5f412da5c5c41249f70ea0cd6413dc339f2&reaction=dislike'>👎</a>
##########
superset-frontend/src/embedded/index.tsx:
##########
@@ -264,6 +264,7 @@ window.addEventListener('message', function
embeddedPageInitializer(event) {
Switchboard.defineMethod('getActiveTabs', embeddedApi.getActiveTabs);
Switchboard.defineMethod('getDataMask', embeddedApi.getDataMask);
Switchboard.defineMethod('getChartStates', embeddedApi.getChartStates);
+ Switchboard.defineMethod('setDataMask', embeddedApi.setDataMask);
Review Comment:
**Suggestion:** `setDataMask` is exposed immediately after the message port
handshake, so a host can call it before the dashboard has finished hydrating.
In that window, dispatched data mask updates can be overwritten by the later
hydrate flow, causing silent loss of filter updates. Guard this method until
dashboard state is initialized and return an explicit error so callers can
retry. [race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ⚠️ Initial embedded filter presets can be silently dropped.
- ❌ Parent-to-iframe filter synchronization becomes unreliable.
- ⚠️ `getDataMask` may not reflect earlier `setDataMask` call.
```
</details>
```suggestion
Switchboard.defineMethod(
'setDataMask',
(payload: Parameters<typeof embeddedApi.setDataMask>[0]) => {
if (!store.getState()?.dashboardInfo?.id) {
throw new Error(
'Dashboard is not initialized yet. Call setDataMask after the
dashboard is ready.',
);
}
return embeddedApi.setDataMask(payload);
},
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a host app, call `embedDashboard(...)` and immediately call returned
`setDataMask(...)`; SDK exposes this at
`superset-embedded-sdk/src/index.ts:291` and
returns API object at `:325`.
2. This sends fire-and-forget `ourPort.emit('setDataMask', {dataMask})`
(`superset-embedded-sdk/src/index.ts:291`) before dashboard hydration
finishes.
3. Iframe already registered handler `Switchboard.defineMethod('setDataMask',
embeddedApi.setDataMask)` at `superset-frontend/src/embedded/index.tsx:267`;
handler
dispatches `updateDataMask` per filter in
`superset-frontend/src/embedded/api.tsx:89-93`.
4. Later, dashboard load effect dispatches `hydrateDashboard(...)`
(`superset-frontend/src/dashboard/containers/DashboardPage.tsx:202-210`),
and reducer
`HYDRATE_DASHBOARD` rebuilds/returns `cleanState`
(`superset-frontend/src/dataMask/reducer.ts:215-291`), overwriting earlier
native-filter
updates.
```
</details>
<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:** 267:267
**Comment:**
*Race Condition: `setDataMask` is exposed immediately after the message
port handshake, so a host can call it before the dashboard has finished
hydrating. In that window, dispatched data mask updates can be overwritten by
the later hydrate flow, causing silent loss of filter updates. Guard this
method until dashboard state is initialized and return an explicit error so
callers can retry.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=1ccf9354029a4908ab1c2cfd97a1fb2d40d4ad1ce3617c009f07c1f7423cfac7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=1ccf9354029a4908ab1c2cfd97a1fb2d40d4ad1ce3617c009f07c1f7423cfac7&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]
