sadpandajoe commented on code in PR #45495: URL: https://github.com/apache/superset/pull/45495#discussion_r4319613742
########## superset-frontend/src/dashboard/hooks/useSyncActiveTabsToUrl.ts: ########## @@ -0,0 +1,81 @@ +/** + * 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 { useHistory } from 'react-router-dom'; +import { useSelector } from 'react-redux'; +import { URL_PARAMS } from 'src/constants'; +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 || + !window.location.pathname.includes('/superset/dashboard') Review Comment: This guard only passes for `/superset/dashboard/...`, but the dashboard route is `/dashboard/:idOrSlug/` under the configured application root (FilterBar's `publishDataMask` checks `${applicationRoot()}/dashboard` for this reason). On a root deployment (`/dashboard/7/`) or a custom prefix (`/analytics/dashboard/7/`) the hook always returns early, so switching tabs never updates `active_tabs` and a copied link reopens the wrong tab. Could this use the same `applicationRoot()`-based check as FilterBar? ########## superset-frontend/src/dashboard/containers/DashboardPage.tsx: ########## @@ -247,6 +250,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) { Review Comment: The URL-over-permalink precedence here isn't exercised by any added test: the hook tests start from a populated store and the `DashboardContainer` cases inject `activeTabs` into Redux directly, so deleting this block would leave every new test green while a reload ignores the selected tabs. Could `DashboardPage` get a case with `active_tabs=TAB-second` plus a resolved permalink holding `activeTabs: ['TAB-first']` that asserts hydration receives `['TAB-second']`, and one with an absent/blank `active_tabs` asserting the permalink tabs survive? ########## superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx: ########## @@ -160,6 +160,15 @@ const publishDataMask = debounce( if (appRoot !== '/' && replacementPathname.startsWith(appRoot)) { replacementPathname = replacementPathname.substring(appRoot.length); } + // the tabs may have changed while the filter key was being created + const latestActiveTabs = new URLSearchParams(history.location.search).get( + URL_PARAMS.activeTabs.name, + ); + if (latestActiveTabs) { Review Comment: The re-read of `active_tabs` after `createFilterKey` resolves has no regression coverage; existing FilterBar tests only assert filter-key publication, and the hook test has no competing async writer. If this block were removed, a tab change made while the key request is in flight would be overwritten by the stale value, or a removed tab resurrected. Could a FilterBar test hold the key response, change `active_tabs` from `TAB-a` to `TAB-b` (and separately remove it) before resolving, and assert the final URL has the new key plus the latest tab state? ########## 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) { Review Comment: When the URL carries both a deep-link hash and `active_tabs`, the two tab consumers disagree. Here `directPathToChild` wins whenever it resolves, but `Tabs.tsx` (`initTabIndex`) falls back to `activeTabs` whenever the deep-link index is 0. With `?active_tabs=TAB-second#TAB-first` the second tab header is selected while the first tab's content renders, and clicking the already-selected header can't switch content. The sync hook preserves the hash, so opening a first-tab anchor, selecting the second tab and refreshing produces exactly this URL. Should both consumers apply the same precedence? ########## superset-frontend/src/constants.ts: ########## @@ -83,6 +83,10 @@ export const URL_PARAMS = { name: 'permalink_key', type: 'string', }, + activeTabs: { + name: 'active_tabs', + type: 'string', + }, Review Comment: Agreed—`RESERVED_DASHBOARD_URL_PARAMS` isn't what `extractUrlParams('regular')` consults; it uses its own set (`standalone`, `edit`), so `active_tabs` still flows into chart `url_params` and can alter queries/cache keys. Should `active_tabs` be excluded there as well? -- 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]
