Copilot commented on code in PR #43821:
URL: https://github.com/apache/superset/pull/43821#discussion_r3924014663
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.test.ts:
##########
@@ -18201,3 +18201,89 @@ describe('Ensure buildTree does not throw runtime
errors when encountering an in
}).not.toThrow();
});
});
+
+// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from
describe blocks
+describe('findFilterScope', () => {
Review Comment:
This adds a new `describe(...)` block (and another `eslint-disable-next-line
no-restricted-globals`) even though the codebase is actively migrating away
from `describe` blocks in favor of flat `test(...)` usage. To avoid increasing
that tech debt, consider rewriting these as top-level `test('findFilterScope:
...', ...)` cases (or `test.each`) so no additional disable is needed.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts:
##########
@@ -293,34 +294,30 @@ 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 as number),
);
- 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];
+ const excluded = Object.values(layout)
+ .filter(
+ item =>
+ item.type === CHART_TYPE &&
+ item.meta?.chartId != null &&
+ !checkedChartIds.has(item.meta.chartId as number),
+ )
+ .map(item => item.meta.chartId as number);
Review Comment:
Since this scope is persisted, returning `excluded` in a deterministic order
helps avoid unnecessary churn in saved `native_filter_configuration` when
layout object key order changes (e.g., exports/copies). Sorting the chart ids
is behavior-preserving because `excluded` is treated as a set.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts:
##########
@@ -293,34 +294,30 @@ 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 as number),
);
Review Comment:
`checkedChartIds` is built using `as number`, but
`layout[item]?.meta?.chartId` is typed as optional and can be `undefined` at
runtime (e.g., invalid/incomplete layout entries). If a checked chart ever
lacks `chartId`, it will be treated as unchecked and end up in `excluded`,
effectively clearing scope unexpectedly. Filter out null/undefined ids when
building the set to keep behavior safe and type-correct.
--
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]
