sadpandajoe commented on code in PR #45495:
URL: https://github.com/apache/superset/pull/45495#discussion_r4320951697
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -438,6 +449,8 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
const relevantDataMask = useSelector(selectRelevantDatamask);
const activeFilters = useSelector(selectActiveFilters);
+ useSyncActiveTabsToUrl(readyToRender && hydratedDashboardId === id);
Review Comment:
`hydratedDashboardId === id` doesn't say the new dashboard is hydrated:
`dashboardInfo.id` stays in Redux after you leave a dashboard (the comment
above `hydratedDashboardId` notes the previous layout lingers). If you open
dashboard 1 on `TAB-a`, go to Explore, then come back through a dashboard-1
permalink that saved `TAB-b`, the ids match as soon as the APIs load while
`getPermalinkValue` is still pending. The hook then writes `active_tabs=TAB-a`
from the stale `activeTabs`, and the `parseActiveTabsParam(getUrlParam(...))`
block at lines 255-259 reads that injected value as an explicit selection and
overrides the saved `TAB-b`. Should the sync wait on something tied to this
load's hydration (e.g. `isDashboardHydrated`, or clearing `dashboardInfo` on
unmount) instead of the id alone?
##########
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({}, '', `/superset/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: 2, metadata: {} },
+ dashboardState: { sliceIds: [], activeTabs: ['TAB-previous'] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ },
+ },
+ );
+
+ try {
+ await waitFor(() => expect(getPermalinkValue).toHaveBeenCalled());
+ expect(
Review Comment:
This test can't catch the stale-write case it is named for. It starts at
`/superset/dashboard/1/` with the helper's basename-less router, so
`useSyncActiveTabsToUrl`'s `matchPath` against `RoutePaths.DASHBOARD` rejects
the route and the URL stays untouched regardless of the hydration guard; the
seeded `dashboardInfo.id` is also `2`, so `hydratedDashboardId === id` is
already false. If the `readyToRender && hydratedDashboardId === id` condition
were weakened, this still passes. Could it use a route the hook accepts and
seed `dashboardInfo.id: 1` with `activeTabs: ['TAB-previous']`, then assert
`active_tabs` is still absent/explicit while the permalink is pending?
--
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]
