sadpandajoe commented on code in PR #45495:
URL: https://github.com/apache/superset/pull/45495#discussion_r4321327224
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -247,6 +262,14 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
} else if (nativeFilterKeyValue) {
dataMask = await getFilterValue(id, nativeFilterKeyValue);
}
+ // An explicit active_tabs param holds the latest tab state, so it takes
+ // precedence over the one stored in a permalink
+ const activeTabsFromUrl = parseActiveTabsParam(
+ getUrlParam(URL_PARAMS.activeTabs),
+ );
+ if (activeTabsFromUrl.length > 0) {
+ activeTabs = activeTabsFromUrl;
Review Comment:
The explicit `active_tabs` value only replaces `activeTabs`; `hydrate.ts`
still keeps the previous `dashboardState.directPathToChild`, and both tab
consumers rank that path above `activeTabs`. Click tab A (this stores its
path), leave for Explore, then come back via a link to
`/dashboard/1/?active_tabs=TAB-B` with no hash: the dashboard opens on A even
though the URL asked for B. Should a stale click-path be cleared when the URL
supplies tabs and no hash?
##########
superset-frontend/src/dashboard/hooks/useSyncActiveTabsToUrl.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 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 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);
+ }
+ history.replace({ ...history.location, search: params.toString() });
Review Comment:
`{ ...history.location, search }` keeps the existing hash. Open
`/dashboard/7/#TAB-first`, switch to `TAB-second`, and the URL becomes
`?active_tabs=TAB-second#TAB-first`. Reloading or sharing that URL reopens the
first tab, because a resolved deep link takes precedence over `active_tabs` in
both `Tabs` and `DashboardContainer`. That defeats the purpose of preserving
the selection. Should the hash be dropped (or reconciled) once the user changes
tabs?
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx:
##########
@@ -179,11 +182,17 @@ const DashboardContainer: FC<DashboardContainerProps> =
({ topLevelTabs }) => {
});
if (nextTabIndex === -1) {
+ const activeTabIndex = topLevelTabs?.children.findIndex(tabId =>
+ activeTabs?.includes(tabId),
+ );
+ if (activeTabIndex !== undefined && activeTabIndex !== -1) {
+ return activeTabIndex;
Review Comment:
When someone re-enters the same dashboard through an in-app link that
carries `active_tabs=TAB-B`, `DashboardPage` can mount the builder from the
retained Redux state before the permalink/filter-key lookup finishes
(`hasDashboardInfoInitiated` is already true). The header `Tabs` then
initializes its local selection to tab A. When hydration lands, this fallback
moves the content to tab B, but `Tabs` only resets its local selection when
`dashboardId` changes, so the header stays on A above B's content, and clicking
the already-selected A header dispatches nothing. Should the builder wait for
this load's hydration, or should header and content read one shared selection?
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -781,3 +782,63 @@ test('clears undo history after hydrating the dashboard',
async () => {
.invocationCallOrder[0];
expect(clearOrder).toBeGreaterThan(hydrateOrder);
});
+
+test.each([null, '', ' ', 'TAB-explicit'])(
+ 'preserves permalink tabs during SPA navigation with explicit tabs %s',
+ async explicitTab => {
+ const search = new URLSearchParams({ permalink_key: 'saved-dashboard' });
+ if (explicitTab !== null) search.set('active_tabs', explicitTab);
+ window.history.replaceState({}, '', `/dashboard/1/?${search}`);
+ jest
+ .mocked(getUrlParam)
+ .mockImplementation((param: UrlParam) =>
+ new URLSearchParams(window.location.search).get(param.name),
+ );
+ type PermalinkValue = Awaited<ReturnType<typeof getPermalinkValue>>;
+ let resolvePermalink!: (value: PermalinkValue) => void;
+ const permalinkValue: PermalinkValue = {
+ dashboardId: '1',
+ state: { activeTabs: ['TAB-saved'], dataMask: {}, anchor: '' },
+ };
+ jest.mocked(getPermalinkValue).mockReturnValueOnce(
+ new Promise<PermalinkValue>(resolve => {
+ resolvePermalink = resolve;
+ }),
+ );
+
+ const { unmount } = render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [], activeTabs: ['TAB-previous'] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ },
+ },
+ );
+
+ try {
+ await waitFor(() => expect(getPermalinkValue).toHaveBeenCalled());
+ expect(
+ new URLSearchParams(window.location.search).get('active_tabs'),
+ ).toBe(explicitTab);
+ resolvePermalink(permalinkValue);
+ await waitFor(() =>
Review Comment:
This case ends at the `hydrateDashboard` call arguments, and
`hydrateDashboard` is mocked to a no-op, so `TAB-previous` stays in Redux and
nothing checks the URL after hydration. If `setHydratedLoadId(id)` were
removed, URL syncing would stay disabled for the whole session and this test
would still pass. Could you add a case with a state-updating hydration that
resolves the permalink, asserts `active_tabs=TAB-saved`, then dispatches a tab
change and asserts the URL follows it?
--
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]
