gkhnelbstn opened a new pull request, #43821:
URL: https://github.com/apache/superset/pull/43821
### SUMMARY
`findFilterScope`
(`superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts`)
encodes a native filter's scope as the closest common ancestor of the checked
charts instead of `ROOT + excluded`:
```js
const checkedItemParents = chartKeys
.filter(item => layout[item]?.type === CHART_TYPE)
.map(key => {
const parents = [DASHBOARD_ROOT_ID, ...(layout[key]?.parents || [])];
return parents.filter(parent => isShowTypeInTree(layout[parent]));
});
checkedItemParents.sort((p1, p2) => p1.length - p2.length);
const rootPath = checkedItemParents.map(
parents => parents[checkedItemParents[0].length - 1],
);
const excluded = [];
const isExcluded = (parent, item) =>
rootPath.includes(parent) && !chartKeys.includes(item);
```
The depth of `rootPath` is decided by the **shallowest checked chart**. On a
dashboard with tabs, as soon as every checked chart lives inside a tab,
`rootPath` stops being `["ROOT_ID"]` and becomes a list of tab ids, and
`excluded` stays empty — the charts outside those tabs are out of scope
implicitly, because the exclusion loop only visits charts whose parent is in
`rootPath`.
`getChartIdsInFilterScope` derives the same `chartsInScope` from either
representation right after the save (I replayed both implementations over the
same layouts to confirm), so the cost lands on the stored value:
1. **The saved intent is lost.** The scope reads `{"rootPath": ["TAB-1",
"TAB-2"], "excluded": []}` — "nothing is excluded" — while a chart *is*
excluded. A chart added or moved outside those tabs afterwards silently falls
out of scope, with nothing in the metadata explaining why.
2. **Tab ids are layout-local.** Copying a dashboard, or exporting and
re-importing it, regenerates them. `rootPath` then resolves to nothing,
`parents ∩ rootPath` is empty for every chart, `chartsInScope` empties out,
`findTabsWithChartsInScope` derives an empty `tabsInScope`, and
`useIsFilterInScope` (`components/nativeFilters/state.ts`) reports the filter
under "Filters out of scope" on *every* tab.
3. **The modal round trip is unstable.** `getTreeCheckedItems` walks the
tree from `rootPath`, so a scope anchored at tab ids cannot show charts living
elsewhere as checked.
This PR anchors `rootPath` at `DASHBOARD_ROOT_ID` and lets `excluded` carry
the whole selection (every chart in the layout minus the checked ones).
`chartsInScope` is unchanged for every scope the previous code represented, and
the stored value no longer depends on tab ids.
`getTreeCheckedItems` is untouched, so scopes already persisted with
tab-level `rootPath` values keep resolving as they do today.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — no visual change. The difference is in the persisted
`native_filter_configuration`:
Dashboard with chart A directly in the grid (no tab), plus tabs `TAB-1`
(chart B) and `TAB-2` (chart C). Filter scoped to everything, then chart A
unchecked:
| | `scope` |
| --- | --- |
| before | `{"rootPath": ["TAB-1", "TAB-2"], "excluded": []}` |
| after | `{"rootPath": ["ROOT_ID"], "excluded": [1]}` |
### TESTING INSTRUCTIONS
No test covered `findFilterScope`. This PR adds unit tests in
`FilterScope/utils.test.ts` for the empty selection, the all-checked case,
unchecking the only chart outside a tab, a tab holding no checked chart, and
the `getTreeCheckedItems` round trip.
Manually: create a dashboard with one chart outside any tab and two tabs
each holding a chart, add a native filter, open its scope, uncheck the chart
outside the tabs, save, and inspect `native_filter_configuration` in **Edit
properties → Advanced**. The scope stays anchored at `ROOT_ID` and names the
excluded chart. Then copy the dashboard and confirm the filter is still in
scope on both tabs.
### ADDITIONAL INFORMATION
- [ ] 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: [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]
