You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "gerbermichi (via GitHub)" <gi...@apache.org> on 2025/10/21 06:56:36 UTC
[PR] feat(embedded): Add setDataMask API method [superset]
gerbermichi opened a new pull request, #35754:
URL: https://github.com/apache/superset/pull/35754
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-4074042758
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] feat(embedded): Add setDataMask API method [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 #35754:
URL: https://github.com/apache/superset/pull/35754#discussion_r3672429722
##########
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 only dispatches updates for entries present in the new object, while the public `setDataMask` API accepts a complete mask. Filters omitted from a later call remain in Redux, and calling `setDataMask({})` leaves all existing filters active instead of clearing or replacing the current mask. Replace the state atomically or explicitly remove/reset filters absent from the supplied mask. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Filters omitted from later calls remain applied.
- ❌ Empty masks cannot clear embedded dashboard filters.
- ⚠️ `getDataMask` returns stale omitted filter entries.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5042783ff8804747b9dd8b4139294205&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=5042783ff8804747b9dd8b4139294205&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-frontend/src/embedded/api.tsx
**Line:** 91:93
**Comment:**
*Api Mismatch: The method only dispatches updates for entries present in the new object, while the public `setDataMask` API accepts a complete mask. Filters omitted from a later call remain in Redux, and calling `setDataMask({})` leaves all existing filters active instead of clearing or replacing the current mask. Replace the state atomically or explicitly remove/reset filters absent from the supplied mask.
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%2F35754&comment_hash=8601becf8b57259520449d36a6e2027f4d113fc5df19a0df53b705a52d4a7b75&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=8601becf8b57259520449d36a6e2027f4d113fc5df19a0df53b705a52d4a7b75&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] feat(embedded): Add setDataMask API method [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-3583557932
Thanks for updating! Pinging @villebro and adding a couple more reviewers for good measure.
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-5117747899
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] feat(embedded): Add setDataMask API method [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 #35754:
URL: https://github.com/apache/superset/pull/35754#discussion_r3788559045
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag setDataMask for retaining omitted filters or treating an empty mask as a no-op; it intentionally follows the application's per-filter merge semantics and documented read-modify-write usage.
**Applied to:**
- `superset-frontend/src/embedded/api.tsx`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-3428060970
@gerbermichi mind adding tests? or do you think the existing tests already cover all the use cases?
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-5115508062
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/35754?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 65.27%. Comparing base ([`dac69f9`](https://app.codecov.io/gh/apache/superset/commit/dac69f9bcde078c5baa7d495bbad4c447294bf68?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ce54e3a`](https://app.codecov.io/gh/apache/superset/commit/ce54e3abda306b4ef64ee5eec83629d84b0a1446?dropdown=coverage&el=desc&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 #35754 +/- ##
=======================================
Coverage 65.27% 65.27%
=======================================
Files 2797 2797
Lines 157994 158005 +11
Branches 36104 36105 +1
=======================================
+ Hits 103125 103138 +13
+ Misses 52874 52872 -2
Partials 1995 1995
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/35754/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/35754/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `71.40% <100.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 Harness](https://app.codecov.io/gh/apache/superset/pull/35754?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] feat(embedded): Add setDataMask API method [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-3425015829
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] feat(embedded): Add setDataMask API method [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #35754:
URL: https://github.com/apache/superset/pull/35754#discussion_r3788558177
##########
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:
This one's moot now. The SDK dropped Jest for Vitest entirely (`vitest --run --dir src` in `package.json`, no `jest.config.js` or `ts-jest` left in the tree). CI's `embedded-sdk-test` job runs green on the current setup.
##########
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:
Fixed on the SDK side. `setDataMask` in `superset-embedded-sdk/src/index.ts` now filters entries to `typeof mask === 'object' && mask !== null` before sending, so the `crossFiltersChanged`/`nativeFiltersChanged` booleans from `observeDataMask` never get forwarded. Covered by the change-trigger-flag test in both `index.test.ts` and `api.test.ts`.
##########
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:
Fixed. `applyDataMask` in `api.tsx` now builds a `knownFilterIds` allowlist from the current `dataMask` and drops anything not in it, with a `logging.warn`, so unknown ids never reach `updateDataMask`. Covered by the 'ignores filter ids the dashboard does not know' test.
##########
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:
Fixed, and handled better than the suggested fix here would've been (that one just throws). `setDataMask` now queues the mask via `store.subscribe` and replays it once `dashboardInfo.id` shows up, instead of dropping it or racing hydration. There's a dedicated test for this ('queues the mask until the dashboard hydrates').
##########
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:
This one's intentional, not a bug. `setDataMask` matches `updateDataMask`'s existing per-filter merge semantics everywhere else in the app (see `UPDATE_DATA_MASK` in the reducer), it's not meant to be a full replace. Matches the documented usage pattern too: read the full mask via `getDataMask`, mutate, pass it back. A `setDataMask({})` no-op is consistent with that.
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -355,6 +356,18 @@ export async function embedDashboard({
ourPort.get<string>("getDashboardPermalink", { anchor });
const getActiveTabs = () => ourPort.get<string[]>("getActiveTabs");
const getDataMask = () => ourPort.get<Record<string, any>>("getDataMask");
+ // `observeDataMask` hands the host a mask with the change-trigger booleans
+ // mixed in, so feeding that payload straight back into `setDataMask` is a
+ // natural thing for a host to do. Keep only the entries that look like a
+ // filter's mask, so those flags never reach the dashboard as filter ids.
+ const setDataMask = (dataMask: Record<string, any>) =>
+ ourPort.emit("setDataMask", {
+ dataMask: Object.fromEntries(
+ Object.entries(dataMask).filter(
+ ([, mask]) => typeof mask === "object" && mask !== null,
+ ),
+ ),
+ });
Review Comment:
Fixed. The SDK switched from `emit` to `ourPort.get`, specifically so a host gets a rejected promise instead of a silent no-op against an older embedded page. There's a direct test for it ('rejects when the embedded page does not support it').
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -83,6 +88,32 @@ const getActiveTabs = () => store?.getState()?.dashboardState?.activeTabs || [];
const getDataMask = () => store?.getState()?.dataMask || {};
+const setDataMask = ({ dataMask }: { dataMask: DataMaskStateWithId }) => {
+ // The dashboard's own data mask holds an entry for every native filter and
+ // every cross-filter-capable chart, so it doubles as the set of filter ids
+ // this dashboard can accept. Anything else — a filter id from a different
+ // dashboard, or the change-trigger flags that `observeDataMask` emits
+ // alongside the mask — would otherwise be inserted as a bogus filter and
+ // treated as a globally scoped filter by the active-filter derivation.
+ const knownFilterIds = new Set(Object.keys(getDataMask()));
+ const [applicable, ignored] = partition(Object.entries(dataMask), ([id]) =>
+ knownFilterIds.has(id),
+ );
Review Comment:
Same fix as the hydration-race thread on `index.tsx`, the queue-and-replay in `setDataMask` covers this too. Nothing's lost, it's held until hydration and applied then.
--
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] feat(embedded): Add setDataMask API method [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 #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: 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] feat(embedded): Add setDataMask API method [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #35754:
URL: https://github.com/apache/superset/pull/35754
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-3425047168
<details open>
<summary><strong>Interaction Diagram by <a href="https://bito.ai#sequence_diagram">Bito</a></strong></summary>
```mermaid
sequenceDiagram
participant EXT as External App
participant SWB as Switchboard
participant API as EmbeddedApi<br/>🔄 Updated | ●●○ Medium
participant ACT as updateDataMask<br/>🔄 Updated | ●●○ Medium
participant STR as Redux Store
participant DSH as Dashboard
Note over API: New setDataMask method<br/>enables external filter control
EXT->>SWB: setDataMask(dataMask)
SWB->>API: Call setDataMask function
API->>ACT: Dispatch updateDataMask actions
ACT->>STR: Update dataMask state
STR->>DSH: Trigger dashboard re-render
DSH-->>EXT: Updated dashboard with filters
```
Critical path: External App->Switchboard->EmbeddedApi->updateDataMask->Redux Store->Dashboard
> **Note:** The embedded API now supports programmatic data mask setting from external applications. The new setDataMask method iterates through provided filters and dispatches Redux actions to update the dashboard state, enabling external control over embedded dashboard filters.
</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] feat(embedded): Add setDataMask API method [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-5168884351
Let us know if you have time to look into the latest bot review comments @gerbermichi, or if you want a hand.
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "gerbermichi (via GitHub)" <gi...@apache.org>.
gerbermichi commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-5115224881
Thanks for the nudge @rusackas — rebased onto master, and the `embedded-sdk-test` job is green again.
**What the red job actually was:** master migrated `superset-embedded-sdk` to vitest while this branch was adding a Jest setup, so the job's `npm test` (now `vitest --run --dir src`) never ran the new tests, and the Jest devDeps weren't in the committed lockfile either. I dropped `jest.config.js` along with the `jest`/`ts-jest`/`jest-environment-jsdom` deps and ported `src/index.test.ts` to vitest instead, which is consistent with the sibling tests in that package.
**While I was in here, I also worked through the review feedback:**
- @korbit-ai's batching point was already covered by `61b5042` (`batch` from react-redux).
- The note about the SDK forwarding its payload unchanged was a real bug. `observeDataMask` emits `crossFiltersChanged`/`nativeFiltersChanged` alongside the mask (`src/embedded/index.tsx`), so the natural "observe on one dashboard, set on another" sync pattern pushed those flags through as if they were filter ids. `setDataMask` now keeps only entries whose value is a non-null object before emitting.
- Same class of problem for unknown filter ids on the embedded side: `embeddedApi.setDataMask` now dispatches only for ids the dashboard already has in its own data mask — which covers every native filter and every cross-filter-capable chart — and `logging.warn`s about the rest instead of inserting them, where the active-filter derivation would treat them as globally scoped and apply them to charts.
- The `allowJs` suggestion on `jest.config.js` is moot now that the file is gone.
- I did **not** gate `setDataMask` behind dashboard hydration (the race-condition suggestion). Every other embedded setter has the same exposure, so a one-off guard on just this method seemed inconsistent — happy to add it if you'd rather, and the new warning at least makes that window visible instead of silent.
Tests: 4 new tests in `superset-frontend/src/embedded/api.test.ts` covering the filtering behavior, plus 4 in `superset-embedded-sdk/src/index.test.ts`. `pre-commit run` is clean on the changed files, including frontend type-checking.
Ready for another look — @villebro whenever you have a moment.
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35754:
URL: https://github.com/apache/superset/pull/35754#discussion_r2446915411
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -65,9 +67,16 @@ const getActiveTabs = () => store?.getState()?.dashboardState?.activeTabs || [];
const getDataMask = () => store?.getState()?.dataMask || {};
+const setDataMask = (dataMask: DataMaskStateWithId) => {
+ Object.entries(dataMask).forEach(([filterId, mask]) => {
+ store?.dispatch(updateDataMask(filterId, mask));
+ });
+};
Review Comment:
### Lack of batched updates for data mask <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
setDataMask dispatches an update action per filter entry instead of batching.
###### Why this matters
Dispatching many actions sequentially can trigger multiple state updates and re-renders, increasing CPU work and UI latency when updating large masks.
###### Suggested change ∙ *Feature Preview*
Wrap dispatches in a batch to apply all updates in a single render cycle, or implement a single action that updates the entire mask. Example using react-redux batch:
```ts
import { batch } from 'react-redux';
const setDataMask = (dataMask: DataMaskStateWithId) => {
batch(() => {
Object.entries(dataMask).forEach(([filterId, mask]) => {
store?.dispatch(updateDataMask(filterId, mask));
});
});
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4110eef-81fc-4f26-881c-2843966178c7/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4110eef-81fc-4f26-881c-2843966178c7?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4110eef-81fc-4f26-881c-2843966178c7?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4110eef-81fc-4f26-881c-2843966178c7?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d4110eef-81fc-4f26-881c-2843966178c7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4c06f21d-e99a-4a8c-9a02-7aadae69a8d2 -->
[](4c06f21d-e99a-4a8c-9a02-7aadae69a8d2)
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-4748921863
@gerbermichi this looks like a nice complement to `getDataMask`, but the `embedded-sdk-test` job is red. Mind getting that green so we can take a proper look? A rebase MIGHT help.
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-3544238948
looks like this needs some conflicts resolved. Holler when those are cleared, and we'll go from there! Thanks!
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-4073629341
## **Sequence Diagram**
This PR adds a new setDataMask method to the embedded dashboard API so parent applications can programmatically update native filter states. The flow sends data mask updates from the SDK into the embedded page, batches Redux updates, and applies filter changes to the dashboard.
```mermaid
sequenceDiagram
participant ParentApp
participant EmbeddedSDK
participant EmbeddedPage
participant ReduxStore
participant Dashboard
ParentApp->>EmbeddedSDK: Call setDataMask with filter values
EmbeddedSDK->>EmbeddedPage: Send setDataMask message
EmbeddedPage->>ReduxStore: Batch update data mask for each filter
ReduxStore->>Dashboard: Apply updated filter states
Dashboard-->>ParentApp: Filters and charts reflect new mask
```
---
*Generated by [CodeAnt AI](https://codeant.ai)*
--
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] feat(embedded): Add setDataMask API method [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 #35754:
URL: https://github.com/apache/superset/pull/35754#discussion_r3680643418
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -355,6 +356,18 @@ export async function embedDashboard({
ourPort.get<string>("getDashboardPermalink", { anchor });
const getActiveTabs = () => ourPort.get<string[]>("getActiveTabs");
const getDataMask = () => ourPort.get<Record<string, any>>("getDataMask");
+ // `observeDataMask` hands the host a mask with the change-trigger booleans
+ // mixed in, so feeding that payload straight back into `setDataMask` is a
+ // natural thing for a host to do. Keep only the entries that look like a
+ // filter's mask, so those flags never reach the dashboard as filter ids.
+ const setDataMask = (dataMask: Record<string, any>) =>
+ ourPort.emit("setDataMask", {
+ dataMask: Object.fromEntries(
+ Object.entries(dataMask).filter(
+ ([, mask]) => typeof mask === "object" && mask !== null,
+ ),
+ ),
+ });
Review Comment:
**Suggestion:** The SDK sends this new method using `emit`, and Switchboard's emit path does not return an acknowledgement or error when the iframe does not define the method. If the SDK and embedded page are deployed at different versions, `setDataMask` appears to succeed but silently does nothing. Add a capability/acknowledgement mechanism or expose a failure when the peer does not support the method. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Filter updates silently fail across SDK/page version skew.
- ⚠️ Host applications cannot detect unsupported embedded API versions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=48c9d27901d94c4c910b06376d4a3eff&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=48c9d27901d94c4c910b06376d4a3eff&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:** 363:370
**Comment:**
*Api Mismatch: The SDK sends this new method using `emit`, and Switchboard's emit path does not return an acknowledgement or error when the iframe does not define the method. If the SDK and embedded page are deployed at different versions, `setDataMask` appears to succeed but silently does nothing. Add a capability/acknowledgement mechanism or expose a failure when the peer does not support the method.
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%2F35754&comment_hash=ce9907801d3b6c9787fa052e9f4d69a6829fd0e2f38a294097a1ac499653e030&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=ce9907801d3b6c9787fa052e9f4d69a6829fd0e2f38a294097a1ac499653e030&reaction=dislike'>👎</a>
##########
superset-frontend/src/embedded/api.tsx:
##########
@@ -83,6 +88,32 @@ const getActiveTabs = () => store?.getState()?.dashboardState?.activeTabs || [];
const getDataMask = () => store?.getState()?.dataMask || {};
+const setDataMask = ({ dataMask }: { dataMask: DataMaskStateWithId }) => {
+ // The dashboard's own data mask holds an entry for every native filter and
+ // every cross-filter-capable chart, so it doubles as the set of filter ids
+ // this dashboard can accept. Anything else — a filter id from a different
+ // dashboard, or the change-trigger flags that `observeDataMask` emits
+ // alongside the mask — would otherwise be inserted as a bogus filter and
+ // treated as a globally scoped filter by the active-filter derivation.
+ const knownFilterIds = new Set(Object.keys(getDataMask()));
+ const [applicable, ignored] = partition(Object.entries(dataMask), ([id]) =>
+ knownFilterIds.has(id),
+ );
Review Comment:
**Suggestion:** Because the allowlist is taken from the current Redux `dataMask`, calls made immediately after `embedDashboard()` resolves can be ignored before dashboard hydration has populated the filter entries. Hydration then replaces the temporary state, so the requested values are permanently lost and there is no retry. Queue the requested mask until hydration completes, or avoid rejecting valid filter IDs solely because the store is not initialized yet. [stale reference]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Immediate post-embed filter updates are permanently lost.
- ⚠️ Embedded dashboard filters may show default values unexpectedly.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=556080ae20b74f368edfb7bed89c86e6&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=556080ae20b74f368edfb7bed89c86e6&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-frontend/src/embedded/api.tsx
**Line:** 98:101
**Comment:**
*Stale Reference: Because the allowlist is taken from the current Redux `dataMask`, calls made immediately after `embedDashboard()` resolves can be ignored before dashboard hydration has populated the filter entries. Hydration then replaces the temporary state, so the requested values are permanently lost and there is no retry. Queue the requested mask until hydration completes, or avoid rejecting valid filter IDs solely because the store is not initialized yet.
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%2F35754&comment_hash=f3467155f433c35f303947166fb84668e67a7b272fa9e7ee4571b0380735ed36&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35754&comment_hash=f3467155f433c35f303947166fb84668e67a7b272fa9e7ee4571b0380735ed36&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] feat(embedded): Add setDataMask API method [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-5239498669
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] feat(embedded): Add setDataMask API method [superset]
Posted by "gerbermichi (via GitHub)" <gi...@apache.org>.
gerbermichi commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-5237683359
@rusackas Thanks — already on it. I pushed fixes for two of them just now (aad2835fd0):
--
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] feat(embedded): Add setDataMask API method [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35754:
URL: https://github.com/apache/superset/pull/35754#issuecomment-5300662608
Went back through this before merging. All the CodeAnt threads that were still open turned out to already be fixed in code, the pre-hydration race, unknown-filter-id handling, change-trigger-flag stripping, and the emit vs get version-skew gap, so I replied and resolved them. Each has its own test on both the SDK and frontend side, plus the jest-config thread is moot now that the SDK's on Vitest. The one open design question (partial-update vs full-replace on `setDataMask`) is intentional, matches `updateDataMask`'s merge semantics everywhere else.
LGTM, merging.
--
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