Copilot commented on code in PR #43085:
URL: https://github.com/apache/superset/pull/43085#discussion_r3777535800
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -83,6 +83,30 @@ type NativeFilterConfigEntry = Partial<Filter> & { id:
string };
export const DashboardPageIdContext = createContext('');
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
+
+function getSavedDashboardFilters(dashboardId: number) {
+ try {
+ const raw = localStorage.getItem(
+ `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`,
+ );
+ return raw ? JSON.parse(raw) : null;
+ } catch {
+ return null; // localStorage disabled, quota exceeded, corrupt JSON, etc.
+ }
+}
Review Comment:
`getSavedDashboardFilters` returns whatever `JSON.parse` yields (including
arrays/primitives), which can later be passed into `hydrateDashboard({ dataMask
})` and break assumptions that `dataMask` is an object map. It also assumes
`localStorage` exists. Consider validating the parsed value is a plain object
(and bailing out when `localStorage` is unavailable).
##########
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:
Saving the entire `state.dataMask` to `localStorage` on every `fullDataMask`
change can be very chatty and synchronous (blocking the main thread).
`dataMask` can change for reasons unrelated to native filter selection (e.g.
`ownState` updates), so this can introduce avoidable UI jank; consider
debouncing/throttling writes and/or persisting only the filter-relevant subset.
##########
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:
The new localStorage fallback path for `dataMask` is not covered by unit
tests. Since `DashboardPage.test.tsx` already asserts hydration behavior for
URL-driven filter state, add tests that (1) restore from localStorage only when
`native_filters_key`/`permalink_key` are absent, and (2) ensure navigating
between dashboards does not apply another dashboard’s saved state.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -83,6 +83,30 @@ type NativeFilterConfigEntry = Partial<Filter> & { id:
string };
export const DashboardPageIdContext = createContext('');
+const DASHBOARD_FILTERS_STORAGE_PREFIX = 'superset_dashboard_filters_';
+
+function getSavedDashboardFilters(dashboardId: number) {
+ try {
+ const raw = localStorage.getItem(
+ `${DASHBOARD_FILTERS_STORAGE_PREFIX}${dashboardId}`,
+ );
Review Comment:
This introduces a new global localStorage key prefix
(`superset_dashboard_filters_...`) and accesses `localStorage` directly, while
the codebase standardizes keys via `LocalStorageKeys` and `getItem`/`setItem`
(with namespacing like `dashboard__...`). To avoid key collisions and keep
storage concerns centralized, consider adding a namespaced `LocalStorageKeys`
entry and storing a single map keyed by dashboard id (or otherwise routing
through the shared helpers).
--
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]
