-
Notifications
You must be signed in to change notification settings - Fork 52
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
Implement RTK Query approach for fetching wpcom sites #2132
Conversation
📊 Performance Test ResultsComparing e04581a 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.
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?
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.
Sounds reasonable to tackle this in a follow-up. For context, my main goal here was to consolidate "business logic" in the Redux slices as much as possible, and make custom hooks and components passive consumers.
In theory, the only reason this needs to be a hook rather than an event listener in the Redux slice is that selectedTab and selectedSiteId state live in React contexts, so we can't access them from the Redux slice. If we could, it would make more sense to have components that simply say "connect this site ID", and the Redux mutation would take care of everything behind the scenes.
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.
FYI, I've created STU-1086 for that
| 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 👀 |
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.
LGTM 👍 This tests well, and the code looks good. I left a couple of small suggestions, but nothing blocking. My biggest concern was about the tooltipText prop for ConnectButton.
|
|
||
| export const PublishSiteButton = () => { | ||
| export const PublishSiteButton = ( { | ||
| redirectToSync = true, |
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.
Just curious: it looks like the only place we pass false for this prop is on the sync tab. Do we really need it if that's the case?
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 need it for these changes as we have two buttons that behave differently (one redirects and the Sync one does not redirect), but that prop will be redundant once this PR #2171 lands, so we can remove that prop after the merge of the PR.
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.
| tooltipText={ | ||
| isAnySiteSyncing | ||
| ? __( | ||
| 'Another site is syncing. Please wait for the sync to finish before you publish your site.' | ||
| ) | ||
| : __( 'Publishing your site requires an internet connection.' ) | ||
| } |
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.
| tooltipText={ | |
| isAnySiteSyncing | |
| ? __( | |
| 'Another site is syncing. Please wait for the sync to finish before you publish your site.' | |
| ) | |
| : __( 'Publishing your site requires an internet connection.' ) | |
| } | |
| disabledTooltipText={ __( | |
| 'Another site is syncing. Please wait for the sync to finish before you publish your site.' | |
| ) } | |
| offlineTooltipText={ __( 'Publishing your site requires an internet connection.' ) } |
This is mostly a suggestion, but I think this would clarify how and when which tooltip text is displayed in ConnectButton.
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 like this suggestion but I would delay adding changes to this until #2171 is merged, at that point we can also refactor the publish-site-button.tsx as suggested here
| { reduxModalMode === 'connect' ? ( | ||
| <SyncSitesModalSelector | ||
| mode="connect" | ||
| isLoading={ isFetchingSyncSites } | ||
| onRequestClose={ () => { | ||
| dispatch( connectedSitesActions.closeModal() ); | ||
| } } | ||
| syncSites={ syncSites } | ||
| onInitialRender={ refetchSites } | ||
| onConnect={ async ( siteId: number ) => { | ||
| await handleSiteSelection( siteId, reduxModalMode ); | ||
| } } | ||
| selectedSite={ selectedSite } | ||
| /> | ||
| ) : syncSites.length === 0 && ! isFetchingSyncSites ? ( | ||
| <NoWpcomSitesModal | ||
| onRequestClose={ () => { | ||
| dispatch( connectedSitesActions.closeModal() ); | ||
| } } | ||
| selectedSite={ selectedSite } | ||
| /> | ||
| ) : ( | ||
| <SyncSitesModalSelector | ||
| mode={ reduxModalMode || 'connect' } | ||
| isLoading={ isFetchingSyncSites } | ||
| onRequestClose={ () => { | ||
| dispatch( connectedSitesActions.closeModal() ); | ||
| } } | ||
| syncSites={ syncSites } | ||
| onInitialRender={ refetchSites } | ||
| onConnect={ async ( siteId: number ) => { | ||
| await handleSiteSelection( siteId, reduxModalMode ); | ||
| } } | ||
| selectedSite={ selectedSite } | ||
| /> | ||
| ) } | ||
| </> | ||
| <SyncSitesModalSelector | ||
| mode={ reduxModalMode || 'connect' } | ||
| onRequestClose={ () => { | ||
| dispatch( connectedSitesActions.closeModal() ); | ||
| } } | ||
| onConnect={ async ( siteId: number ) => { | ||
| await handleSiteSelection( siteId ); | ||
| } } | ||
| selectedSite={ selectedSite } | ||
| /> |
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 great 👍
| isLoading: false, | ||
| isFetching: false, | ||
| refetchSites: jest.fn(), | ||
| refetch: jest.fn().mockResolvedValue( { data: syncSites } ), |
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.
Nit, we could remove these lines, since we only consume data in the component
| isLoading: false, | ||
| isFetching: false, | ||
| refetchSites: jest.fn(), | ||
| refetch: jest.fn().mockResolvedValue( { data: [] } ), |
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.
Same thing here about removing
src/stores/sync/wpcom-sites.ts
Outdated
| } | ||
| }, | ||
| providesTags: ( _result, _error, arg ) => [ { type: 'WpComSites', userId: arg.userId } ], | ||
| keepUnusedDataFor: 60, |
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.
| keepUnusedDataFor: 60, |
60 is already the default, so we could remove 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.
That's right, removing it 😄
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.
Works great. I confirm that it loads the connected sites correctly. I also tested it by logging in and logging out. And changing between accounts with and without sites.
I confirm the list of sites load correctly, they get refreshed and if the user doesn't have sites a CTA modal replaces the connect sites.
I didn't see any warning or error in the console.
Great job!!
…a-for-users-without-sites-displays-before-sites-are-loaded
| import MainSidebar from './page-objects/main-sidebar'; | ||
| import Onboarding from './page-objects/onboarding'; | ||
| import SiteContent from './page-objects/site-content'; | ||
| import WhatsNewModal from './page-objects/whats-new-modal'; |
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 to fix a lint error, it not used
| import { E2ESession } from './e2e-helpers'; | ||
| import Onboarding from './page-objects/onboarding'; | ||
| import SiteContent from './page-objects/site-content'; | ||
| import WhatsNewModal from './page-objects/whats-new-modal'; |
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 to fix a lint error, it not used
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