Skip to content

MSD: read origin_site_id param and set as current omnibar site #109856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fushar wants to merge 3 commits into from
+56 −19
Closed

MSD: read origin_site_id param and set as current omnibar site #109856

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf20200
MSD: read origin_site_id param and set as current omnibar site
fushar Apr 7, 2026
d80b469
if origin_site_id > 0
fushar Apr 8, 2026
78c7425
Refactor current site logic
fushar Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions client/dashboard/app/interim-omnibar/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import {
userPreferenceQuery,
} from '@automattic/api-queries';
import { QueryObserver } from '@tanstack/react-query';
import { removeQueryArgs } from '@wordpress/url';
import { hydrateRoot } from 'react-dom/client';
import { setCurrentOmnibarSite } from '../../utils/omnibar';
import { AUTH_QUERY_KEY, initializeCurrentUser } from '../auth';
import type { OmnibarEvents } from './click-handlers';
import type { UserPreferences } from '@automattic/api-core';
Expand Down Expand Up @@ -56,7 +58,9 @@ export default async function loadOmnibar( events: OmnibarEvents ) {
);

async function renderWithSiteId( siteId: number | undefined ) {
const site = siteId ? await queryClient.ensureQueryData( siteByIdQuery( siteId ) ) : null;
const site = siteId
? await queryClient.ensureQueryData( siteByIdQuery( siteId ) ).catch( () => null )
Copy link
Copy Markdown
Member

@p-jackson p-jackson Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops! Good catch 😄

: null;
root.render(
<InterimOmnibar
user={ user }
Expand All @@ -68,11 +72,24 @@ export default async function loadOmnibar( events: OmnibarEvents ) {
);
}

// Render with the initial recent site (or primary blog as fallback).
const originSiteId = Number(
new URLSearchParams( window.location.search ).get( 'origin_site_id' )
);
if ( originSiteId ) {
Copy link
Copy Markdown
Member

@p-jackson p-jackson Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: If someone tricked the user include clicking a link with ?origin_site_id=-1 then it would pollute their recentSite preference.

Suggested change
if ( originSiteId ) {
if ( originSiteId > 0 ) {

setCurrentOmnibarSite( originSiteId );
Copy link
Copy Markdown
Member

@p-jackson p-jackson Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting 🤔 This updates the recentSites, which initially didn't seem right—passing a query param doesn't seem like an authoritative reason to update the recentSites pref. But I can see why we need it. Is this something the v1 HD does?

The other thing that strikes me is that this isn't really interim work, we will want this logic even when the interim solution goes away. So I propose we move this to the MSD router. In the same site router remembers the selected site ID, the root router should be in charge of remembering the ?origin_site_id.

Copy link
Copy Markdown
Contributor Author

@fushar fushar Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting 🤔 This updates the recentSites, which initially didn't seem right—passing a query param doesn't seem like an authoritative reason to update the recentSites pref. But I can see why we need it. Is this something the v1 HD does?

Yeah, I just realized, v1 HD only sets the "selected site" redux slice, but does not actually update the recentSites preferences. But we don't have such store in dashboard. Let me think a better way...

Copy link
Copy Markdown
Contributor Author

@fushar fushar Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it seems to make sense either way. If a user visits an MSD link with an origin_site_id param, it means they just came from that site's wp-admin. By that logic, that site must be their "most recent" site. So I don't see any drawbacks 😬 what do you think?

Copy link
Copy Markdown
Member

@p-jackson p-jackson Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you're right. I can't think of any, just naturally cautious about expanding the meaning of this preference.

I had thought maybe, with the OmniBar coming, that MSD would just need to implement it's own concept of "selected site". But perhaps "recent sites" is the most natural thing to use. "Selected" sounds like something transitive, like a keyboard focus, not something that needs persisting. But "recent sites", now that sounds like something that you would naturally persist.

Perhaps recent sites is best.

I still prefer the idea of updating it in the root router since this isn't interim behaviour.

Copy link
Copy Markdown
Contributor Author

@fushar fushar Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other thing that strikes me is that this isn't really interim work, we will want this logic even when the interim solution goes away. So I propose we move this to the MSD router.

I tried this but it seems we will run into a problem. Similar reason to the one discussed above, it means we need to wait until the recentSite update call finishes. This is because we immediately clear the origin_site_id param from the URL. So, the omnibar component will never see it, and there can be a race condition where it still renders the old most recent site.

Copy link
Copy Markdown
Contributor Author

@fushar fushar Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm wrong. The query is updated optimistically! Let me test 😅

Copy link
Copy Markdown
Member

@p-jackson p-jackson Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block the page

It's not quite blocking the page either, it's more that the server rendered components aren't hydrated until the recent site preference is up to date. So it shouldn't really feel like a slow down for the user.

We're already depending on the route optimistically updating the recentSites preference when you navigate directly to /sites/$siteSlug and it seems to work 🤞 so I was hoping it would "just work" in this instance too. But I mean I can understand if it turns out there does happen to be some complicated race condition in this particular case. I felt quite lucky that it just happened to work for the site route :)

Copy link
Copy Markdown
Contributor Author

@fushar fushar Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so there's still ugly flash because we are doing await queryClient.ensureQueryData( rawUserPreferencesQuery() ) first before optimistically update the recentSites. There's no way around it. 😅 I think I'm just going with the current approach for now.

Copy link
Copy Markdown
Contributor Author

@fushar fushar Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're already depending on the route optimistically updating the recentSites preference when you navigate directly to /sites/$siteSlug and it seems to work

Yes, but in this case, the old recent site is already visible in the omnibar before we navigate to another site. So it's not "flashing" in that sense.

Copy link
Copy Markdown
Contributor Author

@fushar fushar Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're already depending on the route optimistically updating the recentSites preference when you navigate directly to /sites/$siteSlug and it seems to work

Yes, but in this case, the old recent site is already visible in the omnibar before we navigate to another site. So it's not "flashing" in that sense.

I was wrong. It's still flashing if we directly visit a site overview URL of another site in another tab for example.

I've spent hours to try to prevent this. But I just realized that it's almost impossible to do because the TanStack router and the omnibar live in different React roots, so there will be race condition 😅 Maybe one way is for the omnibar component itself to parse the URL and extracts siteSlug and converts to siteId. But I don't think it's worth it. Let's just live with what we have now.

window.history.replaceState(
null,
'',
removeQueryArgs( window.location.pathname + window.location.search, 'origin_site_id' )
);
}

const recentSites = queryClient.getQueryData< UserPreferences >(
rawUserPreferencesQuery().queryKey
)?.recentSites;
const initialSiteId = recentSites?.[ 0 ] || user.primary_blog;

// Render with the origin site, or the most recent site, or primary blog as fallback.
const initialSiteId = originSiteId || recentSites?.[ 0 ] || user.primary_blog;
renderWithSiteId( initialSiteId );

// Re-render whenever recentSites changes (e.g. user navigates to a different site).
Expand Down
15 changes: 3 additions & 12 deletions client/dashboard/app/router/sites.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,11 @@ import {
siteSshAccessStatusQuery,
siteStaticFile404SettingQuery,
siteWordPressVersionQuery,
userPreferenceOptimisticMutation,
queryClient,
wpOrgCoreVersionQuery,
} from '@automattic/api-queries';
import { isEnabled } from '@automattic/calypso-config';
import { isSupportSession } from '@automattic/calypso-support-session';
import { MutationObserver } from '@tanstack/react-query';
import {
createLazyRoute,
createRoute,
Expand All @@ -58,6 +56,7 @@ import {
canViewWordPressSettings,
} from '../../sites/features';
import { shouldLoadWpVersionNotice } from '../../sites/overview/wp-version-notice';
import { setCurrentOmnibarSite } from '../../utils/omnibar';
import {
getActivityLogHiddenGroups,
hasHostingFeature,
Expand Down Expand Up @@ -173,16 +172,8 @@ export const siteRoute = createRoute( {
},
onEnter: async ( { loaderData } ) => {
const siteId = loaderData?.site?.ID;
if ( ! siteId ) {
return;
}
const prefs = await queryClient.ensureQueryData( rawUserPreferencesQuery() );
const recentSites = prefs?.recentSites ?? [];
if ( siteId !== recentSites[ 0 ] ) {
const updated = [ ...new Set( [ siteId, ...recentSites ] ) ].slice( 0, 5 );
new MutationObserver( queryClient, userPreferenceOptimisticMutation( 'recentSites' ) ).mutate(
updated
);
if ( siteId ) {
setCurrentOmnibarSite( siteId );
}
},
errorComponent: lazyRouteComponent( () => import( '../../sites/site/error' ) ),
Expand Down
22 changes: 22 additions & 0 deletions client/dashboard/utils/omnibar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import {
queryClient,
rawUserPreferencesQuery,
userPreferenceOptimisticMutation,
} from '@automattic/api-queries';
import { MutationObserver } from '@tanstack/react-query';

/**
* Sets the current site to be displayed in the omnibar,
* by pushing the site ID to the front of the recentSites user preferences.
*/
export async function setCurrentOmnibarSite( siteId: number ) {
const prefs = await queryClient.ensureQueryData( rawUserPreferencesQuery() );
const recentSites = prefs?.recentSites ?? [];
if ( siteId === recentSites[ 0 ] ) {
return;
}
const updated = [ ...new Set( [ siteId, ...recentSites ] ) ].slice( 0, 5 );
new MutationObserver( queryClient, userPreferenceOptimisticMutation( 'recentSites' ) ).mutate(
updated
);
}
Loading