kgabryje commented on code in PR #20743:
URL: https://github.com/apache/superset/pull/20743#discussion_r925436975
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -82,12 +90,95 @@ type PageProps = {
idOrSlug: string;
};
+const getDashboardContextLocalStorage = () => {
+ const dashboardsContexts = getItem(
+ LocalStorageKeys.dashboard__explore_context,
+ {},
+ );
+ return Object.fromEntries(
+ Object.entries(dashboardsContexts).filter(
+ ([, value]) => !value.isRedundant,
+ ),
+ );
+};
+
+const updateDashboardTabLocalStorage = (
+ dashboardTabId: string,
+ payload?: Record<string, any>,
+) => {
+ const dashboardsContexts = getDashboardContextLocalStorage();
+ setItem(LocalStorageKeys.dashboard__explore_context, {
+ ...dashboardsContexts,
+ [dashboardTabId]: payload,
+ });
+};
+
+const useSyncDashboardStateWithLocalStorage = () => {
+ const labelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.label_colors || {},
+ );
+ const sharedLabelColors = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo?.metadata?.shared_label_colors || {},
+ );
+ const colorScheme = useSelector<RootState, string>(
+ state => state.dashboardState?.colorScheme,
+ );
+ const chartConfiguration = useSelector<RootState, JsonObject>(
+ state => state.dashboardInfo.metadata?.chart_configuration,
+ );
+ const nativeFilters = useSelector<RootState, Filters>(
+ state => state.nativeFilters.filters,
+ );
+ const dataMask = useSelector<RootState, DataMaskStateWithId>(
+ state => state.dataMask,
+ );
+ const dashboardId = useSelector<RootState, number>(
+ state => state.dashboardInfo.id,
+ );
+ const filterBoxFilters = useSelector<RootState, Record<string, any>>(() =>
+ getActiveFilters(),
+ );
+ const dashboardTabId = useMemo(() => shortid.generate(), []);
+
+ useEffect(() => {
+ const payload = {
+ labelColors,
+ sharedLabelColors,
+ colorScheme,
+ chartConfiguration,
+ nativeFilters,
+ dataMask,
+ dashboardId,
+ filterBoxFilters,
+ };
+ updateDashboardTabLocalStorage(dashboardTabId, payload);
+ return () => {
+ updateDashboardTabLocalStorage(dashboardTabId, {
+ ...payload,
+ isRedundant: true,
+ });
Review Comment:
That might lead to race conditions between removing local storage key and
consuming it in Explore.
Case 1: You open Explore in a new tab and close the tab with dashboard
before Explore mounts - key doesn't exist anymore in Explore.
Case 2: You open Explore in the same tab as dashboard - Dashboard component
unmounts and removes the key before Explore mounts. That case could be handled
by always removing the key in Explore instead of on Dashboard unmount. However,
that leads us to Case 3: you open Explore in a new tab, key is removed on
Explore mount, then you go back to the tab with dashboard and open a new tab
with Explore - key doesn't exist anymore in this tab.
I think flagging key as redundant and removing it on next dashboard action
is the safest approach here
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
