MSD: read origin_site_id param and set as current omnibar site#109856
MSD: read origin_site_id param and set as current omnibar site#109856
Conversation
Jetpack Cloud Live (direct link)
Automattic for Agencies Live (direct link)
Dashboard Live (dotcom) (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
ae4756e to
bf20200
Compare
| 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 ) |
| const originSiteId = Number( | ||
| new URLSearchParams( window.location.search ).get( 'origin_site_id' ) | ||
| ); | ||
| if ( originSiteId ) { |
There was a problem hiding this comment.
Nit: If someone tricked the user include clicking a link with ?origin_site_id=-1 then it would pollute their recentSite preference.
| if ( originSiteId ) { | |
| if ( originSiteId > 0 ) { |
| new URLSearchParams( window.location.search ).get( 'origin_site_id' ) | ||
| ); | ||
| if ( originSiteId ) { | ||
| setCurrentOmnibarSite( originSiteId ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe I'm wrong. The query is updated optimistically! Let me test 😅
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Declaring bankruptcy; it's easier to just rewrite this from scratch rather than trying to rebase the commits. 😅 will open a new PR. |
Fixes DOTMSD-1204
Proposed Changes
The W logo from wp-admin's admin bar points to
wordpress.com/sites?origin_site_id=<blog_id>. This is used to set the current site in the admin bar.This PR applies the same logic to the interim omnibar.
Testing Instructions
dashboard/omnibarflag is true.https://wordpress.comwithmy.localhost:3000.Pre-merge Checklist