-
Notifications
You must be signed in to change notification settings - Fork 51
Implement RTK Query approach for fetching wpcom sites #2132
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.
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
base: trunk
Are you sure you want to change the base?
Implement RTK Query approach for fetching wpcom sites #2132
Conversation
📊 Performance Test ResultsComparing 6ba4ba7 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
sejas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Thanks for moving the wpcom sites request to RTK. Now it doesn't block the user every time we click on the buttons.
I just noticed that we display a loading state in the push site button every time we change tabs.
loading-push-site-button.mp4
src/modules/sync/index.tsx
Outdated
| dispatch( connectedSitesActions.openModal( pendingModalMode ) ); | ||
| setPendingModalMode( null ); | ||
| if ( isModalOpen && isAuthenticated && ! isUninitializedSyncSites ) { | ||
| refetchWpComSites().catch( ( error ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be possible to refetch wpcom sites inside openModal action or call it at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…a-for-users-without-sites-displays-before-sites-are-loaded
|
|
||
| function pruneCache(): void { | ||
| for ( const [ fn, cachedResults ] of cache ) { | ||
| for ( const [ _, cachedResults ] of cache ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lint fix, unrelated to this PR
sejas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@epeicher, thanks for making the adjustments. The PR looks good. I only found that Studio throws a warning when clicking Publish site from a different tab, like overview.
Failed to refetch sites: Error: Cannot refetch a query that has not been started yet.
| // Schema for WordPress.com sites endpoint | ||
| const sitesEndpointSiteSchema = z.object( { | ||
| ID: z.number(), | ||
| is_wpcom_atomic: z.boolean(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have parts of this type duplicated in sync-support.ts. Can we unify them?
studio/src/modules/sync/lib/sync-support.ts
Lines 3 to 8 in b09206b
| // Schema type for WordPress.com sites endpoint | |
| export type SitesEndpointSite = { | |
| ID: number; | |
| is_wpcom_atomic: boolean; | |
| name: string; | |
| URL: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, this type can be inferred from the schema, I will update this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as part of 427d3ab, duplication removed, great catch!
src/modules/sync/index.tsx
Outdated
| try { | ||
| const result = await refetchWpComSites(); | ||
| return result.data ?? []; | ||
| } catch ( error ) { | ||
| // Query might not be ready to refetch yet (e.g., was skipped due to offline) | ||
| console.warn( 'Failed to refetch sites:', error ); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this pattern of catching the refetchWpComSite error repeated a few times. I wonder if makes sense to move the try catch inside the query and always return an empty array if it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right, the refetchWpComSite is not always ready, so it can throw errors, I will encapsulate this on a function 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been simplified as part of 63cc6b8, as the effect is not required anymore after refetching the sites directly. There are not any errors now, but I have added the void operator to refetch the sites on the background (i.e. do not use await every time) and ignore any potential errors during the refetch, please let me know if you think it would be better to handle and maybe log them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, I have consolidated both buttons into one with a condition that redirects to the Sync tab only when required in the commit 7e1bbdb
…a-for-users-without-sites-displays-before-sites-are-loaded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, nice work 👍 This is a huge PR and lots of work so well done!
I noticed one thing that when the sync modal is already open, I see the loading state on the Publish site button in the background:
I also see it for the case when I click on the Publish site in the top right corner and then close the modal, the loading state persists in that case instead of going away.
| setSelectedTab( 'sync' ); | ||
| } | ||
| if ( isAuthenticated && ! isUninitializedSyncSites ) { | ||
| // Refetch sites on the background but ignore errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Refetch sites on the background but ignore errors | |
| // Refetch sites in the background but ignore errors |
The query is executed in the background so it is not required to indicate that in the button
|
Thanks for the review @katinthehatsite!
Yes, that was expected as I included |
fredrikekelund
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great refactoring, I love to see it 👍
I left a couple of comments, mostly with a focus on how we can make the code more declarative and rely on RTK Query internals more, or move logic from components inside mutations.
I haven't tested extensively yet, but happy to do so in a second review later.
| const handlePublishClick = useCallback( () => { | ||
| if ( redirectToSync ) { | ||
| setSelectedTab( 'sync' ); | ||
| } | ||
| if ( isAuthenticated && ! isUninitializedSyncSites ) { | ||
| // Refetch sites in the background but ignore errors | ||
| void refetchWpComSites(); | ||
| } | ||
| dispatch( connectedSitesActions.openModal( 'push' ) ); | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to call refetchWpComSites here? If the goal is to refetch whenever the sync modal opens, would it not make more sense to call this from within the sync modal?
We could combine it with the refetchOnMountOrArgChange option to refetch the hook data when mounting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that's a matter of preference, I updated that motivated by this comment but I will investigate the result of using it when opening the sync modal and combining it with refetchOnMountOrArgChange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After doing the changes, it's clear that it was better to do it in the component, thanks for the suggestion! I have done it as part of 5079edf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest moving more of this logic to src/stores/sync/connected-sites.ts by updating the connectSite mutation like so:
connectSite: builder.mutation<
SyncSite[],
{ remoteSiteId: number; localSiteId: string; userId?: number }
>( {
queryFn: async ( { remoteSiteId, localSiteId, userId }, api ) => {
const connectedSites = await getIpcApi().getConnectedWpcomSites( localSiteId );
const { data: remoteSites = [] } = await api.dispatch(
wpcomSitesApi.endpoints.getWpComSites.initiate( {
connectedSiteIds: connectedSites.map( ( site ) => site.id ),
userId,
} )
);
const siteToConnect = remoteSites.find( ( site ) => site.id === remoteSiteId );
if ( ! siteToConnect ) {
return {
error: { status: 'CUSTOM_ERROR', error: 'Site not found in WordPress.com sites' },
};
}
await getIpcApi().connectWpcomSites( [
{
sites: [ siteToConnect ],
localSiteId,
},
] );
const actualConnectedSites = await getIpcApi().getConnectedWpcomSites( localSiteId );
return { data: actualConnectedSites };
},This would allow us to shorten this hook significantly:
export function useListenDeepLinkConnection() {
const [ connectSite ] = useConnectSiteMutation();
const { selectedSite, setSelectedSiteId } = useSiteDetails();
const { setSelectedTab, selectedTab } = useContentTabs();
const { user } = useAuth();
useIpcListener( 'sync-connect-site', async ( _event, { remoteSiteId, studioSiteId } ) => {
if ( selectedSite?.id && selectedSite.id !== studioSiteId ) {
// Select studio site that started the sync
setSelectedSiteId( studioSiteId );
}
await connectSite( { remoteSiteId, localSiteId: studioSiteId, userId: user?.id } );
if ( selectedTab !== 'sync' ) {
// Switch to sync tab
setSelectedTab( 'sync' );
}
} );
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I will work on that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been working on this, but I have found that this will trigger an additional call to the /me/sites endpoint when the connectSite mutation is used from the add-site and from the ContentTabSync handleConnect, where they already have the site, so I am not sure we are improving here.
What do you think about tackling this specific refactor as a follow-up and progressing with the current changes?
| const { data: syncSites = [], refetch: refetchSites } = useGetWpComSitesQuery( { | ||
| connectedSiteIds, | ||
| userId: user?.id, | ||
| } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const { data: syncSites = [], refetch: refetchSites } = useGetWpComSitesQuery( { | |
| connectedSiteIds, | |
| userId: user?.id, | |
| } ); | |
| const { data: syncSites = [] } = useGetWpComSitesQuery( | |
| { connectedSiteIds, userId: user?.id }, | |
| { refetchOnMountOrArgChange: true } | |
| ); |
I believe we could remove the refetchSites call by passing the refetchOnMountOrArgChange flag here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will have a look at it, I had that initially, but it was refetching the sites more than expected. I will check it again after latest updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done the changes as part of 61e8a4b, they do the same refetch on mount and we avoid the useEffect 👍
src/modules/sync/index.tsx
Outdated
| if ( isAuthenticated && ! isUninitializedSyncSites ) { | ||
| // Refetch sites on the background but ignore errors | ||
| void refetchWpComSites(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of explicitly refetching in the handleOpenModal handler, I suggest adding another useGetWpComSitesQuery call in SyncSitesModalSelector. See my comment in that file for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a better approach, I have implemented it as part of 5079edf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest making it so that the useGetWpComSitesQuery hook is declaratively refetched by removing the syncSites prop from this component, and adding another call to the useGetWpComSitesQuery hook like so:
const { user } = useAuth();
const { data: connectedSites = [] } = useGetConnectedSitesForLocalSiteQuery( {
localSiteId: selectedSite.id,
userId: user?.id,
} );
const connectedSiteIds = connectedSites.map( ( { id } ) => id );
const { data: syncSites = [] } = useGetWpComSitesQuery(
{ connectedSiteIds, userId: user?.id },
{ refetchOnMountOrArgChange: true }
);The refetchOnMountOrArgChange option should accomplish the same thing as the refetch call in src/modules/sync/index.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 😄 , I have done it as part of 5079edf
src/stores/sync/wpcom-sites.ts
Outdated
| // First transformation using all connected sites (for reconciliation) | ||
| const syncSitesForReconciliation = transformSitesResponse( | ||
| parsedResponse.sites, | ||
| allConnectedSites.map( ( { id } ) => id ) | ||
| ); | ||
|
|
||
| // whenever array of syncSites changes, we need to update connectedSites to keep them updated with wordpress.com | ||
| const { updatedConnectedSites } = reconcileConnectedSites( | ||
| allConnectedSites, | ||
| syncSitesForReconciliation | ||
| ); | ||
| await getIpcApi().updateConnectedWpcomSites( updatedConnectedSites ); | ||
|
|
||
| // Second transformation using connectedSiteIds parameter (for syncSupport calculation for selected site) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // First transformation using all connected sites (for reconciliation) | |
| const syncSitesForReconciliation = transformSitesResponse( | |
| parsedResponse.sites, | |
| allConnectedSites.map( ( { id } ) => id ) | |
| ); | |
| // whenever array of syncSites changes, we need to update connectedSites to keep them updated with wordpress.com | |
| const { updatedConnectedSites } = reconcileConnectedSites( | |
| allConnectedSites, | |
| syncSitesForReconciliation | |
| ); | |
| await getIpcApi().updateConnectedWpcomSites( updatedConnectedSites ); | |
| // Second transformation using connectedSiteIds parameter (for syncSupport calculation for selected site) | |
| const syncSitesForReconciliation = transformSitesResponse( | |
| parsedResponse.sites, | |
| allConnectedSites.map( ( { id } ) => id ) | |
| ); | |
| const { updatedConnectedSites } = reconcileConnectedSites( | |
| allConnectedSites, | |
| syncSitesForReconciliation | |
| ); | |
| await getIpcApi().updateConnectedWpcomSites( updatedConnectedSites ); | |
Nit, but the variable names are clear enough here that I would argue we could skip the line comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as part of 99a6b55
| }; | ||
| } | ||
|
|
||
| function transformSitesResponse( sites: unknown[], connectedSiteIds: number[] ): SyncSite[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this function is the same as before, but I think the connectedSiteIds deserves a short explainer in a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm… ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as part of 99a6b55
…a-for-users-without-sites-displays-before-sites-are-loaded
|
Thanks @fredrikekelund for the review and the suggestions! I have implemented most of them and I still want to look at this one and this one. I will let you know when they are addressed and we can do another review 🙏 |
…he sites" This reverts commit 5cfc0f2.
…a-for-users-without-sites-displays-before-sites-are-loaded
|
Hi @fredrikekelund, I have implemented most of your suggestions, and replied to one of the suggestions here. I have tested it again, but this will benefit from another testing and some review after latest changes. Could you please give another round of review and let me know if you find any issues? 🙏 |
|
Taking a look now 👀 |
Related issues
Before
CleanShot.2025-11-26.at.20.17.26-trimmed.mp4
After
CleanShot.2025-11-26.at.20.18.33.mp4
Proposed Changes
useFetchWpComSiteshook to RTK Query withuseGetWpComSitesQuery. The new query uses the userId to invalidate the cachesrc/hooks/use-fetch-wpcom-sites/tosrc/modules/sync/lib/src/modules/sync/types.tssync-support.tsmodule forgetSyncSupportand helper functionsStreamline Onboardingfeature flagTesting Instructions
npm startPublish sitebutton and observe that the loading experience is fine (there are no multiple loading indicators)Publish siteand check that the loading is minimal as the sites are cachedPull sitebuttonPublish sitebutton, check that the deleted or created site is displayed (there could be a delay)Publish sitebuttonFind a perfect planmodal is displayedPre-merge Checklist