sadpandajoe commented on code in PR #45495:
URL: https://github.com/apache/superset/pull/45495#discussion_r4386797282
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -339,6 +367,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
} as unknown as Parameters<typeof hydrateDashboard>[0]),
);
dispatch(clearDashboardHistory());
+ setHydratedLoadId(id);
Review Comment:
`hydratedLoadId` is keyed by the dashboard id rather than by the load, so it
stays set when this page instance loads the same dashboard again (for example
SPA navigation from `/dashboard/1/` to `/dashboard/my-slug/?permalink_key=K`,
which reuses `DashboardPage`). Once the dashboard and charts requests finish,
`hydratedLoadId === id` and `hydratedDashboardId === id` already hold while
`getPermalinkValue` is still pending, so `useSyncActiveTabsToUrl` writes the
previous tabs into `active_tabs`; the continuation then reads that back as an
explicit selection and overrides the permalink's saved tabs. The same unscoped
marker can also strand the page: if a slower load for dashboard 1 finishes
after the user has moved on to dashboard 2, its `setHydratedLoadId(1)` lands
after dashboard 2's, `hydratedLoadId === id` stays false, and the loader at the
render gate never clears. Could the marker be reset when `idOrSlug` changes and
only set by the load that is still current (a cancelled flag i
n the effect, as `useApiResourceFullBody` does)? A test with retained Redux
state and an unresolved permalink that asserts the builder is absent until
hydration resolves would also pin the gate, which no added test exercises.
##########
superset-frontend/src/dashboard/hooks/useSyncActiveTabsToUrl.ts:
##########
@@ -0,0 +1,102 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useEffect } from 'react';
+import { matchPath, useHistory } from 'react-router-dom';
+import { useSelector } from 'react-redux';
+import { URL_PARAMS } from 'src/constants';
+import { RoutePaths } from 'src/views/routePaths';
+import { TAB_TYPE } from 'src/dashboard/util/componentTypes';
+import type { RootState } from 'src/dashboard/types';
+
+const TAB_ID_SEPARATOR = ',';
+
+/**
+ * Parses the `active_tabs` URL param into a list of tab ids.
+ */
+export function parseActiveTabsParam(value: string | null): string[] {
+ return (value ?? '')
+ .split(TAB_ID_SEPARATOR)
+ .map(tabId => tabId.trim())
+ .filter(Boolean);
+}
+
+/**
+ * Keeps the `active_tabs` URL param in line with the dashboard's active tabs,
+ * so that a link to the dashboard opens on the same tabs. Follows the redux
+ * state rather than tab clicks so that tabs restored from a permalink or from
+ * the URL itself are reflected as well.
+ *
+ * @param enabled Whether the dashboard has been hydrated
+ */
+export default function useSyncActiveTabsToUrl(enabled: boolean): void {
+ const history = useHistory();
+ const activeTabs = useSelector(
+ (state: RootState) => state.dashboardState.activeTabs,
+ );
+ const layout = useSelector(
+ (state: RootState) => state.dashboardLayout?.present ?? {},
+ );
+ const editMode = useSelector(
+ (state: RootState) => state.dashboardState.editMode,
+ );
+
+ useEffect(() => {
+ // The pathname check avoids racing with navigation to Explore
+ if (
+ !enabled ||
+ editMode ||
+ matchPath(history.location.pathname, {
+ path: [RoutePaths.DASHBOARD_LIST, RoutePaths.DASHBOARD],
+ exact: true,
+ })?.path !== RoutePaths.DASHBOARD
+ ) {
+ return;
+ }
+
+ const params = new URLSearchParams(history.location.search);
+ const current = params.get(URL_PARAMS.activeTabs.name);
+ const next = activeTabs?.length ? [...activeTabs].sort().join(',') : null;
+ const currentNormalized = current
+ ? parseActiveTabsParam(current).sort().join(',')
+ : null;
+ if (currentNormalized === next) {
+ return;
+ }
+
+ if (next) {
+ params.set(URL_PARAMS.activeTabs.name, next);
+ } else {
+ params.delete(URL_PARAMS.activeTabs.name);
+ }
+ // A deep link hash outranks `active_tabs` on load, so it is dropped once
+ // the selected tabs no longer include a tab the hash targets. The path is
+ // resolved from the layout, as a tab click replaces `directPathToChild`.
+ const hashId = history.location.hash.slice(1);
+ const hashIsStale =
+ Boolean(hashId) &&
+ [...(layout[hashId]?.parents ?? []), hashId].some(
+ id => layout[id]?.type === TAB_TYPE && !activeTabs?.includes(id),
+ );
+ history.replace({
Review Comment:
Rebuilding the URL from `...history.location` uses React Router's location,
which `navigateWithState` does not update (it calls `window.history.pushState`
after a dashboard save). This hook is skipped in edit mode, so tabs changed
while editing are written when edit mode ends; if the slug was changed and
saved in that session, the router still holds the old slug path, and this
replace puts `/dashboard/<old-slug>/` back in the address bar and re-renders
the route with a slug that no longer resolves. Could the pathname be taken from
`window.location`, or the write skipped when it differs from
`history.location`, so it can't undo the post-save navigation?
##########
superset-frontend/src/constants.ts:
##########
@@ -130,6 +134,7 @@ export const RESERVED_DASHBOARD_URL_PARAMS: string[] = [
URL_PARAMS.nativeFilters.name,
URL_PARAMS.nativeFiltersKey.name,
URL_PARAMS.permalinkKey.name,
+ URL_PARAMS.activeTabs.name,
Review Comment:
`constants.test.ts` already pins the dashboard reserved list (it asserts
`permalinkKey` is in `RESERVED_DASHBOARD_URL_PARAMS`), but nothing covers this
new entry, and `extractUrlParams.test.ts` exercises a different reserved set.
Deleting this line would leave every test green while `getDashboardUrlParams()`
starts copying `active_tabs` into permalink `urlParams`, where it can later
outrank the permalink's own saved `activeTabs`. Could `constants.test.ts`
assert that `RESERVED_DASHBOARD_URL_PARAMS` contains
`URL_PARAMS.activeTabs.name`?
--
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]
