sadpandajoe commented on code in PR #45495: URL: https://github.com/apache/superset/pull/45495#discussion_r4404298127
########## superset-frontend/src/dashboard/hooks/useSyncActiveTabsToUrl.ts: ########## @@ -0,0 +1,120 @@ +/** + * 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; + } + + // `navigateWithState` updates the address bar without the router, so after + // an in-session save the router can hold a stale pathname (e.g. an old + // slug). Writing it back would undo that navigation. + // The router decodes its pathname, so compare against the decoded address + // bar to keep non-ASCII slugs from reading as a divergence. + let addressBarPath = window.location.pathname; + try { + addressBarPath = decodeURI(addressBarPath); + } catch { + // malformed escapes: compare the raw path + } + if ( + addressBarPath !== + history.createHref({ pathname: history.location.pathname }) + ) { + 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 = Review Comment: A hash deep link is dropped before the tabs reconcile. Open `/dashboard/7/?permalink_key=<key>#CHART-A` where the permalink saved tab B (or revisit the dashboard while Redux still holds tab B): hydration seeds `activeTabs=[B]` while `directPathToChild` points into tab A. The first run of this effect sees a tab ancestor of the hash outside `activeTabs`, writes `hash: ''`, and only afterwards does `Tabs` switch to A, so the chart's `AnchorLink` mounts with no hash, never scrolls to it, and the shared link loses its deep link. Should the stale-hash clearing apply only after a tab change made on the page, rather than on the initial reconciliation? ########## superset-frontend/src/dashboard/containers/DashboardPage.tsx: ########## @@ -224,7 +235,14 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => { dispatch(setDatasetsStatus(status)); }, [dispatch, status]); + // The marker belongs to a single load, so a new `idOrSlug` (even one that + // resolves to the same dashboard id) starts without it. + useEffect(() => { + setHydratedLoadId(undefined); Review Comment: This `[idOrSlug]` reset is what keeps a finished load from letting the builder render and `useSyncActiveTabsToUrl` write while a second lookup for the same dashboard id is still pending, but no test exercises it: every added case starts with `hydratedLoadId` unset, and the superseded-lookup test never completes a first load before starting another. Deleting this effect leaves `DashboardPage.test.tsx` green. Could a case finish loading `idOrSlug="1"`, rerender with a slug that resolves to the same id and the store retained, keep the second permalink lookup pending, and assert `DashboardBuilder` stays hidden and `active_tabs` is untouched until it resolves? ########## superset-frontend/src/dashboard/hooks/useSyncActiveTabsToUrl.ts: ########## @@ -0,0 +1,120 @@ +/** + * 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; + } + + // `navigateWithState` updates the address bar without the router, so after + // an in-session save the router can hold a stale pathname (e.g. an old + // slug). Writing it back would undo that navigation. + // The router decodes its pathname, so compare against the decoded address + // bar to keep non-ASCII slugs from reading as a divergence. + let addressBarPath = window.location.pathname; + try { + addressBarPath = decodeURI(addressBarPath); + } catch { + // malformed escapes: compare the raw path + } + if ( + addressBarPath !== Review Comment: Tab sync stops for the rest of the visit after a save that changes the slug. `navigateWithState` moves the address bar to `/dashboard/new-slug/` while the router keeps `/dashboard/old-slug/`, so this comparison fails on every later run, not just the first stale one: rename the slug in dashboard properties, save, then select the second tab and `active_tabs` is never written, so copying or reloading the URL opens the default tab. Should this skip only the write that would undo the navigation (or build the URL from the address bar's current path) instead of disabling sync until the next router navigation? -- 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]
