codeant-ai-for-open-source[bot] commented on code in PR #43085:
URL: https://github.com/apache/superset/pull/43085#discussion_r3819387821
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +256,11 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id);
+ if (savedFilters) {
+ dataMask = savedFilters;
+ }
}
Review Comment:
Agreed. The hydration effect should be keyed by the dashboard identity, and
the hydration guard must be reset when that identity changes. Otherwise SPA
navigation can leave the component “hydrated” for dashboard A while rendering
dashboard B.
For example:
```tsx
const previousDashboardId = useRef<number>();
useEffect(() => {
if (!readyToRender || !id) return;
if (previousDashboardId.current !== id) {
isDashboardHydrated.current = false;
previousDashboardId.current = id;
}
// Existing hydration logic, including:
// const savedFilters = getSavedDashboardFilters(id)
}, [readyToRender, id]);
```
The saved-filter lookup should therefore run from an effect whose
dependencies include both `readyToRender` and `id`. Any in-flight hydration for
the previous dashboard should also be guarded or cancelled so it cannot
overwrite the new dashboard’s state. This ensures dashboard B reads only
`superset_dashboard_filters_${B}` and does not retain dashboard A’s mask.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -157,20 +181,26 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug
}: PageProps) => {
const isNotFoundError = (error as SupersetApiError | null)?.status === 404;
const readyToRender = Boolean(dashboard && charts);
const { dashboard_title, id = 0 } = dashboard || {};
+ const hydratedDashboardId = useSelector<RootState, number | undefined>(
+ state => state.dashboardInfo?.id,
+ );
+ const fullDataMask = useSelector(selectDataMask);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ saveDashboardFilters(id, fullDataMask);
+ }, [id, hydratedDashboardId, fullDataMask]);
Review Comment:
Yes. The write should be gated by an explicit “initial filter load
completed” state, not only by `dashboardInfo.id`. During SPA navigation, the
Redux `dataMask` can temporarily be cleared while `dashboardInfo.id` still
refers to the old dashboard.
For example:
```tsx
const [initialFiltersLoaded, setInitialFiltersLoaded] = useState(false);
useEffect(() => {
setInitialFiltersLoaded(false);
}, [id]);
useEffect(() => {
let cancelled = false;
async function loadInitialFilters() {
// Resolve permalink, URL filter, or localStorage state here.
// Apply the resulting dataMask to Redux.
if (!cancelled) {
setInitialFiltersLoaded(true);
}
}
if (id) {
loadInitialFilters();
}
return () => {
cancelled = true;
};
}, [id]);
useEffect(() => {
if (
!id ||
hydratedDashboardId !== id ||
!initialFiltersLoaded
) {
return;
}
saveDashboardFilters(id, fullDataMask);
}, [id, hydratedDashboardId, fullDataMask, initialFiltersLoaded]);
```
The load effect should set `initialFiltersLoaded` only after the initial
mask has been selected and dispatched, including the cases where there is no
URL/localStorage value. This prevents the transient `{}` from overwriting the
saved state, while still allowing subsequent user changes to be persisted. The
existing dashboard-ID guard remains necessary to prevent cross-dashboard
leakage.
--
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]
