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