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: [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]
