gabotorresruiz commented on code in PR #45764:
URL: https://github.com/apache/superset/pull/45764#discussion_r4415526424
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx:
##########
@@ -220,11 +224,21 @@ const DashboardContainer: FC<DashboardContainerProps> =
({ topLevelTabs }) => {
tabsInScope: scope.tabsInScope,
}));
- if (!isEqual(scopes, prevFilterScopesRef.current)) {
+ // Hydration can replace the store's scopes after the last calculation.
+ const storeIsStale = scopes.some(scope => {
+ const stored = nativeFilters[scope.filterId];
+ return (
+ stored &&
+ (!isEqual(stored.chartsInScope, scope.chartsInScope) ||
+ !isEqual(stored.tabsInScope, scope.tabsInScope))
+ );
+ });
+
+ if (storeIsStale || !isEqual(scopes, prevFilterScopesRef.current)) {
prevFilterScopesRef.current = scopes;
dispatch(setInScopeStatusOfFilters(scopes));
}
- }, [chartIds, filterItems, chartLayoutItemMap, dispatch]);
+ }, [chartIds, filterItems, nativeFilters, chartLayoutItemMap, dispatch]);
useEffect(() => {
Review Comment:
Not a blocker. Chart customizations share the `nativeFilters.filters` map
and the same hydrate merge, but this effect still only compares against
`prevCustomizationScopesRef`. When I hydrate a stale `chartsInScope` for a
customization, the store keeps it on this branch, and
`getFormDataWithExtraFilters` gates customizations on that field too. Should
this effect get the same store check, here or in a follow-up?
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx:
##########
@@ -220,11 +224,21 @@ const DashboardContainer: FC<DashboardContainerProps> =
({ topLevelTabs }) => {
tabsInScope: scope.tabsInScope,
}));
- if (!isEqual(scopes, prevFilterScopesRef.current)) {
+ // Hydration can replace the store's scopes after the last calculation.
+ const storeIsStale = scopes.some(scope => {
Review Comment:
I think this can loop when `native_filter_configuration` has two entries
with the same `id` but different scopes. `calculateScopes` returns both, the
reducer keeps only the last one, so the first entry always looks stale, and
each dispatch replaces `nativeFilters`, which reruns this effect. In a Jest
harness it keeps re-dispatching until React logs `Maximum update depth
exceeded`, while the same metadata on master dispatches once. Is that shape
possible in practice? I couldn't find anything on the backend that rejects it,
so it feels like cheap insurance.
Comparing against the last scope per id fixed it for me (the existing tests
still pass), and a test with a duplicated filter id would lock it in:
```suggestion
// Hydration can replace the store's scopes after the last calculation.
// The reducer keeps the last scope per filter id, so compare against
that.
const latestScopes = new Map(
scopes.map(scope => [scope.filterId, scope] as const),
);
const storeIsStale = Array.from(latestScopes.values()).some(scope => {
```
--
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]
