sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3996244421
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +289,60 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ const isVersioned =
+ 'dataMask' in savedFilters && 'filterDefinitions' in savedFilters;
+ const maskToRestore = isVersioned
+ ? savedFilters.dataMask
+ : savedFilters;
+ const savedDefinitions = isVersioned
+ ? savedFilters.filterDefinitions
+ : {};
+
+ const currentFilters = (dashboard?.metadata
+ ?.native_filter_configuration ?? []) as NativeFilterConfigEntry[];
+
+ // Only restore entries whose filter ID still exists in the current
+ // native filter configuration. If we have a snapshotted definition
+ // (from the newer versioned schema), we also verify that the
filter's
+ // target columns/datasets and type have not changed. This prevents
+ // stale extraFormData from a retargeted filter from being hydrated.
+ const validatedFilters = Object.fromEntries(
+ Object.entries(maskToRestore).filter(([filterId]) => {
+ const currentConfig = currentFilters.find(f => f.id ===
filterId);
Review Comment:
Dashboards with legacy null entries in `native_filter_configuration` now
throw here before `hydrateDashboard`, because `.find` dereferences each entry
while the existing hydration path filters falsey values. Could this filter the
configuration and cover `[null, validFilter]` before comparing IDs?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,30 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
Review Comment:
Agreed—embedded users have no `userId`, so this dashboard-only key persists
selections across guest-token sessions even though `FilterBar` disables
persistence for them. Can save and restore skip users without an authenticated
ID?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +267,11 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ if (savedFilters) {
+ dataMask = savedFilters;
+ }
}
Review Comment:
Agreed—the outer-object check still accepts a versioned value whose
`dataMask` is null, then `Object.entries(maskToRestore)` throws before the
dashboard hydrates. Could the nested schema be validated before iterating it?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +434,24 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
+ const nativeFilters = useSelector(selectNativeFilters);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ // Persist only entries that correspond to configured native filters.
+ // This avoids saving chart customization or other transient dataMask
+ // entries that are not part of the user's filter selections.
+ const nativeFilterIds = Object.keys(nativeFilters);
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ saveDashboardFilters(id, userId, nativeFilterMask);
Review Comment:
The ref stays true while this page switches dashboards, so if the old
Dashboard cleanup clears `dataMask` before the new charts arrive, this effect
can still persist an empty mask. Could the guard track the hydrated dashboard
ID or skip an empty `nativeFilterMask`?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -226,6 +268,17 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
}
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
+ } else {
+ const savedFilters = getSavedDashboardFilters(id, userId);
+ // Guard against corrupted or unexpected localStorage data shapes
+ // (e.g. a JSON array or primitive) before assigning to dataMask.
+ if (
+ savedFilters &&
+ typeof savedFilters === 'object' &&
+ !Array.isArray(savedFilters)
+ ) {
+ dataMask = savedFilters;
Review Comment:
The unversioned fallback still restores state by ID alone, so a retargeted
filter can apply the old `extraFormData` and then be resaved with the new
definition. Since this storage key has not shipped, can unversioned entries be
rejected instead?
--
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]
