Copilot commented on code in PR #43108:
URL: https://github.com/apache/superset/pull/43108#discussion_r3777573415
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,44 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_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>,
+) {
+ try {
+ localStorage.setItem(
+ getStorageKey(dashboardId, userId),
+ JSON.stringify(nativeFilterMask),
+ );
+ } catch {
Review Comment:
`saveDashboardFilters` writes to localStorage on every relevant state
change; `localStorage.setItem` is synchronous and can become a noticeable
main-thread cost. You can avoid redundant writes by comparing the serialized
value to the existing stored value before setting it.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,44 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_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}`;
+}
Review Comment:
New localStorage keys are expected to be namespaced using the
`namespace__key` pattern (see `src/utils/localStorageHelpers.ts`), but this new
key uses a single-underscore prefix. Using the shared convention reduces
collision risk and keeps keys consistent across the app.
##########
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:
`getSavedDashboardFilters()` returns `any` from `JSON.parse`, and the result
is assigned directly to `dataMask`. If localStorage is corrupted/tampered (or
contains a non-object), this can break downstream code that assumes `dataMask`
is an object map. Guard the shape before using it (and optionally filter keys
to configured native filters).
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -381,8 +427,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.
Review Comment:
This change introduces new behavior (persisting/restoring native filter
state from localStorage, including per-user scoping and the hydration guard),
but there are no unit tests covering it. Adding tests around the load path (no
URL key => restores) and save path (only native filter ids persisted, userId
scoping) would prevent regressions.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -81,6 +81,44 @@ import {
type NativeFilterConfigEntry = Partial<Filter> & { id: string };
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_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}`
Review Comment:
The PR description lists "No per-user scoping" as a known limitation, but
this implementation *does* scope by `userId` when available. Please update the
PR description (and/or linked issue notes) so behavior and documented
limitations match.
--
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]
