sadpandajoe commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r4129007004
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,6 +90,68 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'dashboard__native_filters__';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
Review Comment:
This falls back to the unscoped key whenever `userId` is falsy, but the
comments on both call sites below explicitly rely on `!= null` (not a
truthiness check) so that an authenticated `userId` of `0` isn't treated as
anonymous. This function still uses a truthiness check, so a real `userId` of
`0` would read/write the guest-shared key instead of its own.
```suggestion
return userId != null
```
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -383,8 +545,58 @@ 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(() => {
+ // Skip persistence for unauthenticated/guest users: they have no stable
+ // identity to scope the key to, and restoring filter state across
+ // guest-token sessions would leak selections between unrelated sessions.
+ // Use == null (not !userId) so a valid userId of 0 is not treated as
+ // anonymous.
+ // Also skip when a historical version preview is active: the dashboard
+ // rehydrates with the snapshot's filter defaults, and every other guard
+ // would pass, causing those defaults to overwrite the user's live mask.
+ if (
+ !id ||
+ userId == null ||
+ isVersionPreviewActive ||
+ hydratedDashboardId !== id ||
+ !isDashboardHydrated.current
+ )
+ 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);
+ // Do not overwrite a previously saved state with an empty mask.
+ // When the store clears dataMask on unmount (SPA navigation away), the
+ // hydratedDashboardId guard may still pass briefly before the next
+ // dashboard's hydration fires; skipping empty writes prevents that race
+ // from wiping the user's last valid selection.
+ if (nativeFilterIds.length === 0) return;
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ // Also skip when the configuration is non-empty but dataMask has already
+ // been cleared (e.g. during SPA unmount before the next dashboard
hydrates).
+ // The ref stays true across the switch so the hydratedDashboardId guard
+ // alone is not sufficient — a nativeFilterMask of {} would still overwrite
+ // the user's saved selection.
+ if (Object.keys(nativeFilterMask).length === 0) return;
+ saveDashboardFilters(id, userId, nativeFilterMask, nativeFilters);
Review Comment:
Opening this dashboard via a permalink, a `native_filters_key`, or a `?f=`
Rison link hydrates `dataMask` from that link, and this effect then persists
whatever `dataMask` currently holds under the user's own storage key, with no
check for whether the value came from an explicit URL rather than an
interactive choice. The next time the same user opens the dashboard normally
(no link in the URL), they get the shared link's filters instead of their own
last selection. Could this effect skip persisting on the same load that just
applied an explicit permalink/native-filters-key/Rison restore?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,6 +90,68 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'dashboard__native_filters__';
+
+function getStorageKey(dashboardId: number, userId: number | undefined) {
+ // Scope the key to userId to prevent one user's filter state from
+ // leaking into another user's session on the same browser profile.
+ // Guest users (no userId) are not scoped — guest sessions are ephemeral.
+ return userId
+ ? `${DASHBOARD_FILTERS_STORAGE_PREFIX}${userId}__${dashboardId}`
+ : `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`;
+}
+
+function getSavedDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+) {
+ try {
+ const raw = localStorage.getItem(getStorageKey(dashboardId, userId));
+ return raw ? JSON.parse(raw) : null;
+ } catch {
+ return null;
+ }
+}
+
+function saveDashboardFilters(
+ dashboardId: number,
+ userId: number | undefined,
+ nativeFilterMask: Record<string, unknown>,
+ nativeFilters: Record<
+ string,
+ Filter | Divider | ChartCustomization | ChartCustomizationDivider
+ >,
+) {
+ try {
+ const key = getStorageKey(dashboardId, userId);
+
+ // Create a lightweight snapshot of the filter definition (targets and
type)
+ // to detect when a dashboard editor retargets or reconfigures a filter,
+ // so we can invalidate the stale extraFormData.
+ const filterDefinitions = Object.fromEntries(
+ Object.entries(nativeFilters).map(([filterId, filter]) => [
+ filterId,
+ {
+ targets: filter.targets,
+ type: filter.filterType,
+ },
+ ]),
+ );
+
+ const nextValue = JSON.stringify({
+ dataMask: nativeFilterMask,
+ filterDefinitions,
+ });
+ // Skip the write if the value has not changed to avoid unnecessary
+ // synchronous main-thread work on every dataMask state update.
+ if (localStorage.getItem(key) !== nextValue) {
+ localStorage.setItem(key, nextValue);
Review Comment:
This writes filter selections into localStorage keyed by the authenticated
user, but nothing removes that entry when the user signs out. On a shared
machine, the next person to use the browser can still read the previous user's
saved filter values (which can include business-sensitive selections like a
specific customer or account) from localStorage after logout, even though this
app's logout flow already clears its other persisted client-side state for
exactly this reason. Should logout also remove keys under this feature's
storage prefix?
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -630,3 +630,413 @@ test('clears undo history after hydrating the dashboard',
async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+// ---------------------------------------------------------------------------
+// localStorage filter persistence
+// ---------------------------------------------------------------------------
+
+test('restores native filter state from localStorage when no URL key is
present', async () => {
Review Comment:
None of the new tests exercise the two behaviors this
dashboard-filter-persistence feature most recently changed: (1) that the save
effect skips persisting while a historical version preview is active, including
through an interrupted exit, and (2) that a `?f=` Rison link in the URL
suppresses the localStorage restore. A regression on either would currently
pass this suite. Could a test be added for each: one that applies a version
preview with a different mask and asserts the live saved entry is unchanged
after exiting, and one that loads with `?f=` present and asserts the saved
entry is not merged into the resulting `dataMask`?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +410,14 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
}, [addDangerToast, datasets, datasetsApiError, dispatch, isNotFoundError]);
const relevantDataMask = useSelector(selectRelevantDatamask);
+ const fullDataMask = useSelector(selectDataMask);
const activeFilters = useSelector(selectActiveFilters);
+ useEffect(() => {
+ if (!id || hydratedDashboardId !== id) return;
+ saveDashboardFilters(id, fullDataMask);
Review Comment:
The save effect still isn't limited to native filters: the map it reads from
also holds dividers and chart customizations, and it saves any key present
there once that key also appears in `dataMask`, so a chart-customization change
still gets written into this dashboard's storage entry. It's silently dropped
again on restore, since that path checks the dashboard's native-filter
configuration instead of this same map, but the write (and its serialization
cost) still happens on every such change. Could the save side filter against
the native-filter configuration the same way the restore side does, instead of
against the full filters map?
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +523,47 @@ 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(() => {
+ // Skip persistence for unauthenticated/guest users: they have no stable
+ // identity to scope the key to, and restoring filter state across
+ // guest-token sessions would leak selections between unrelated sessions.
+ // Use == null (not !userId) so a valid userId of 0 is not treated as
+ // anonymous.
+ if (
+ !id ||
+ userId == null ||
+ hydratedDashboardId !== id ||
+ !isDashboardHydrated.current
+ )
+ 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);
+ // Do not overwrite a previously saved state with an empty mask.
+ // When the store clears dataMask on unmount (SPA navigation away), the
+ // hydratedDashboardId guard may still pass briefly before the next
+ // dashboard's hydration fires; skipping empty writes prevents that race
+ // from wiping the user's last valid selection.
+ if (nativeFilterIds.length === 0) return;
+ const nativeFilterMask = Object.fromEntries(
+ nativeFilterIds
+ .filter(filterId => filterId in fullDataMask)
+ .map(filterId => [filterId, fullDataMask[filterId]]),
+ );
+ // Also skip when the configuration is non-empty but dataMask has already
+ // been cleared (e.g. during SPA unmount before the next dashboard
hydrates).
+ // The ref stays true across the switch so the hydratedDashboardId guard
+ // alone is not sufficient — a nativeFilterMask of {} would still overwrite
+ // the user's saved selection.
+ if (Object.keys(nativeFilterMask).length === 0) return;
+ saveDashboardFilters(id, userId, nativeFilterMask, nativeFilters);
Review Comment:
Closing an applied preview clears the preview state (and the flag this
effect checks) before the live dashboard data and filter selections are
re-hydrated. That leaves a render where the guard is already off but
`dataMask`/`nativeFilters` still describe the historical snapshot, and this
effect can persist that snapshot under the live key during that window. Could
the guard track whether the live state has actually been restored, rather than
only whether a preview is currently active, so persistence stays suspended
until the exit rehydration completes?
--
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]
