Copilot commented on code in PR #38762:
URL: https://github.com/apache/superset/pull/38762#discussion_r2967100525
##########
superset-frontend/src/dashboard/util/crossFilters.ts:
##########
@@ -98,17 +98,18 @@ export const getCrossFiltersConfiguration = (
},
};
}
- chartConfiguration[chartId].crossFilters.chartsInScope =
-
isCrossFilterScopeGlobal(chartConfiguration[chartId].crossFilters.scope)
- ? globalChartConfiguration.chartsInScope.filter(
- id => id !== Number(chartId),
- )
- : getChartIdsInFilterScope(
- chartConfiguration[chartId].crossFilters
- .scope as NativeFilterScope,
- Object.values(charts).map(chart => chart.id),
- chartLayoutItems,
- );
+ const scope = chartConfiguration[chartId].crossFilters.scope;
+
+ const effectiveScope: NativeFilterScope =
isCrossFilterScopeGlobal(scope)
+ ? globalChartConfiguration.scope
+ : scope;
+
+ chartConfiguration[chartId].crossFilters.chartsInScope =
+ getChartIdsInFilterScope(
+ effectiveScope,
+ Object.values(charts).map(chart => chart.id),
+ chartLayoutItems,
+ ).filter(id => id !== Number(chartId));
Review Comment:
`getCrossFiltersConfiguration` recomputes `getChartIdsInFilterScope(...)`
for every interactive chart, including when `scope` is the global pointer. This
is redundant with `globalChartConfiguration.chartsInScope` and can become
expensive on large dashboards (nested loop via `layoutItems.find`). Consider
reusing `globalChartConfiguration.chartsInScope` for the global case (as
before) and/or caching the computed chart ids per unique scope, and also
computing `Object.values(charts).map(chart => chart.id)` once outside the loop.
##########
superset-frontend/src/dashboard/util/crossFilters.ts:
##########
@@ -98,17 +98,18 @@ export const getCrossFiltersConfiguration = (
},
};
}
- chartConfiguration[chartId].crossFilters.chartsInScope =
-
isCrossFilterScopeGlobal(chartConfiguration[chartId].crossFilters.scope)
- ? globalChartConfiguration.chartsInScope.filter(
- id => id !== Number(chartId),
- )
- : getChartIdsInFilterScope(
- chartConfiguration[chartId].crossFilters
- .scope as NativeFilterScope,
- Object.values(charts).map(chart => chart.id),
- chartLayoutItems,
- );
+ const scope = chartConfiguration[chartId].crossFilters.scope;
+
+ const effectiveScope: NativeFilterScope =
isCrossFilterScopeGlobal(scope)
+ ? globalChartConfiguration.scope
+ : scope;
+
+ chartConfiguration[chartId].crossFilters.chartsInScope =
+ getChartIdsInFilterScope(
+ effectiveScope,
+ Object.values(charts).map(chart => chart.id),
+ chartLayoutItems,
+ ).filter(id => id !== Number(chartId));
Review Comment:
There’s a whitespace-only line and indentation looks off in this new block;
this will likely be flagged by prettier/eslint. Please remove the trailing
whitespace and run the formatter so the `const scope` / `effectiveScope` block
aligns with surrounding indentation.
```suggestion
const scope = chartConfiguration[chartId].crossFilters.scope;
const effectiveScope: NativeFilterScope =
isCrossFilterScopeGlobal(scope)
? globalChartConfiguration.scope
: scope;
chartConfiguration[chartId].crossFilters.chartsInScope =
getChartIdsInFilterScope(
effectiveScope,
Object.values(charts).map(chart => chart.id),
chartLayoutItems,
).filter(id => id !== Number(chartId));
```
##########
superset-frontend/src/dashboard/util/crossFilters.ts:
##########
@@ -98,17 +98,18 @@ export const getCrossFiltersConfiguration = (
},
};
}
- chartConfiguration[chartId].crossFilters.chartsInScope =
-
isCrossFilterScopeGlobal(chartConfiguration[chartId].crossFilters.scope)
- ? globalChartConfiguration.chartsInScope.filter(
- id => id !== Number(chartId),
- )
- : getChartIdsInFilterScope(
- chartConfiguration[chartId].crossFilters
- .scope as NativeFilterScope,
- Object.values(charts).map(chart => chart.id),
- chartLayoutItems,
- );
+ const scope = chartConfiguration[chartId].crossFilters.scope;
+
+ const effectiveScope: NativeFilterScope =
isCrossFilterScopeGlobal(scope)
+ ? globalChartConfiguration.scope
+ : scope;
+
+ chartConfiguration[chartId].crossFilters.chartsInScope =
+ getChartIdsInFilterScope(
+ effectiveScope,
+ Object.values(charts).map(chart => chart.id),
+ chartLayoutItems,
+ ).filter(id => id !== Number(chartId));
Review Comment:
This change is intended to make cross-filter scoping work across tabs, but
there’s no unit test exercising a tabbed dashboard layout (e.g., charts under
different `TAB-*` parents) to prevent regressions. Consider adding a test case
(in the existing `crossFilters.test.ts`) that builds a layout with multiple
tabs and verifies `chartsInScope` includes charts from inactive tabs when
global scoping is used.
--
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]
