sadpandajoe commented on code in PR #43821:
URL: https://github.com/apache/superset/pull/43821#discussion_r4162050972
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts:
##########
@@ -293,34 +294,32 @@ export const findFilterScope = (
}
});
- // Get arrays of parents for selected charts
- 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]));
- });
- // Sort arrays of parents to get first shortest array of parents,
- // that means on it's level of parents located common parent, from this
place parents start be different
- checkedItemParents.sort((p1, p2) => p1.length - p2.length);
- const rootPath = checkedItemParents.map(
- parents => parents[checkedItemParents[0].length - 1],
+ // Anchor the scope at the dashboard root and let `excluded` carry the whole
+ // selection. Deriving `rootPath` from the closest common ancestor is lossy
on
+ // dashboards with tabs: its depth was decided by the shallowest checked
+ // chart, so unchecking a single chart could move the anchor from ROOT down
to
+ // the tab level. Every tab holding no checked chart then fell out of
+ // `rootPath`, and its charts never reached `excluded` either (that loop only
+ // visited charts whose parent is in `rootPath`), so they left the scope
+ // unreported and the filter bar showed the filter as out of scope on tabs
the
+ // user never edited.
+ const checkedChartIds = new Set(
+ chartKeys
+ .filter(item => layout[item]?.type === CHART_TYPE)
+ .map(item => layout[item]?.meta?.chartId)
+ .filter((chartId): chartId is number => chartId != null),
);
- const excluded: number[] = [];
- const isExcluded = (parent: string, item: string) =>
- rootPath.includes(parent) && !chartKeys.includes(item);
- // looking for charts to be excluded: iterate over all charts
- // and looking for charts that have one of their parents in `rootPath` and
not in selected items
- Object.entries(layout).forEach(([key, value]) => {
- const parents = value.parents || [];
- if (
- value.type === CHART_TYPE &&
- [DASHBOARD_ROOT_ID, ...parents]?.find(parent => isExcluded(parent, key))
- ) {
- excluded.push(value.meta.chartId as number);
- }
- });
+ const rootPath = [DASHBOARD_ROOT_ID];
+ // Sorted so the saved scope does not depend on the layout's key order
+ const excluded = Object.values(layout)
+ .filter(item => item.type === CHART_TYPE)
+ .map(item => item.meta?.chartId)
+ .filter(
+ (chartId): chartId is number =>
+ chartId != null && !checkedChartIds.has(chartId),
Review Comment:
Selecting a deck.gl layer in one tab now adds the other tabs’ charts to
`excluded`, but `getAllActiveFilters` copies those exclusions onto filters
without layer selections. A separate time filter scoped to the whole dashboard
consequently stops applying to those charts; could that consumer preserve each
filter’s own exclusions before this representation changes?
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts:
##########
@@ -293,34 +294,32 @@ export const findFilterScope = (
}
});
- // Get arrays of parents for selected charts
- 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]));
- });
- // Sort arrays of parents to get first shortest array of parents,
- // that means on it's level of parents located common parent, from this
place parents start be different
- checkedItemParents.sort((p1, p2) => p1.length - p2.length);
- const rootPath = checkedItemParents.map(
- parents => parents[checkedItemParents[0].length - 1],
+ // Anchor the scope at the dashboard root and let `excluded` carry the whole
+ // selection. Deriving `rootPath` from the closest common ancestor is lossy
on
+ // dashboards with tabs: its depth was decided by the shallowest checked
+ // chart, so unchecking a single chart could move the anchor from ROOT down
to
+ // the tab level. Every tab holding no checked chart then fell out of
+ // `rootPath`, and its charts never reached `excluded` either (that loop only
+ // visited charts whose parent is in `rootPath`), so they left the scope
+ // unreported and the filter bar showed the filter as out of scope on tabs
the
+ // user never edited.
+ const checkedChartIds = new Set(
+ chartKeys
+ .filter(item => layout[item]?.type === CHART_TYPE)
+ .map(item => layout[item]?.meta?.chartId)
+ .filter((chartId): chartId is number => chartId != null),
);
- const excluded: number[] = [];
- const isExcluded = (parent: string, item: string) =>
- rootPath.includes(parent) && !chartKeys.includes(item);
- // looking for charts to be excluded: iterate over all charts
- // and looking for charts that have one of their parents in `rootPath` and
not in selected items
- Object.entries(layout).forEach(([key, value]) => {
- const parents = value.parents || [];
- if (
- value.type === CHART_TYPE &&
- [DASHBOARD_ROOT_ID, ...parents]?.find(parent => isExcluded(parent, key))
- ) {
- excluded.push(value.meta.chartId as number);
- }
- });
+ const rootPath = [DASHBOARD_ROOT_ID];
+ // Sorted so the saved scope does not depend on the layout's key order
+ const excluded = Object.values(layout)
Review Comment:
Scoping a filter to one tab now records the other tabs’ chart IDs in
`excluded`, but “Save as” with duplicated charts does not remap native-filter
exclusions, and export/import does not remap display-control exclusions. Those
exclusions no longer match the new chart IDs, so the copied/imported scope
expands to other tabs; could these ID-remapping boundaries be updated alongside
the new representation?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterCard/useFilterScope.ts:
##########
@@ -94,8 +94,15 @@ export const useFilterScope = (filter: FilterElement) => {
// top level tabs, charts excluded
// returns "TAB1, TAB2, CHART1"
if (topLevelTabs) {
- // We start assuming that all charts are in scope for all tabs in the
root path
- const topLevelTabsInFullScope = [...filter.scope.rootPath];
+ // We start assuming that all charts are in scope for all tabs in the
root path.
+ // A scope anchored at the dashboard root covers every top level tab that
+ // holds a chart.
+ const topLevelTabsInFullScope =
+ filter.scope.rootPath[0] === DASHBOARD_ROOT_ID
+ ? topLevelTabs.filter(tabId =>
+ layoutCharts.some(chart => chart.parents?.includes(tabId)),
Review Comment:
When the same chart appears in two top-level tabs and is excluded, this
branch removes only the tab containing its first layout holder, so the other
tab is still advertised as fully in scope. Could the exclusion pass remove
every tab containing an excluded holder?
--
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]
