Implement RTK Query approach for connectedSites #2095
Conversation
epeicher
left a comment
epeicher
left a comment
There was a problem hiding this comment.
Thanks for working on this @fredrikekelund! I really like this refactoring, as the api hooks should "do their work" and refresh the data, so this is 💯
I have tested it, and it works as expected. I have tested the connected sites and the disabling of the import/export buttons, and I have not found any issues.
I have left one question and one minor comment, but accepting this as they are non-blocking. Please remember to merge it with trunk as there has been some changes in the Sync tab. LGTM! 
src/index.html
Outdated
There was a problem hiding this comment.
Just curious, this was not required originally, right?
There was a problem hiding this comment.
Precisely 👍 I should have left a comment about this straight away, but yes, this is more of a follow-up to #2076, because I realized that the file we actually use is index.html in the root directory
src/modules/sync/index.tsx
Outdated
| const [ isModalOpen, setIsModalOpen ] = useState( false ); | ||
| const [ modalMode, setModalMode ] = useState< SyncModalMode | null >( null ); |
There was a problem hiding this comment.
Nit: Have you considered using an object for the two states, since they are directly related?
There was a problem hiding this comment.
Adding here that this local state was added to Redux to allow for these changes #2025, please let me know if you have any questions about it after the merge
There was a problem hiding this comment.
The changes to this file are unrelated to the core purpose of this PR. I just realized that this test calls testActions.resetState in a beforeEach function, and this action has no effect unless we also apply testReducer
| } | ||
|
|
||
| if ( mode === 'push' || mode === 'pull' ) { | ||
| dispatch( connectedSitesActions.setModalMode( mode ) ); |
There was a problem hiding this comment.
This is the only place I could identify where setModalMode is called with a non-null argument. I went ahead and replaced it with openModal, because I believe that will have the same effect. Would appreciate if you could confirm, @epeicher
| @@ -1,5 +1,4 @@ | |||
| import { resolve } from 'path'; | |||
| import { pathToFileURL } from 'url'; | |||
There was a problem hiding this comment.
Unrelated to the core changes in this PR. This function was unused.
|
Thanks for the review, @epeicher 🙏 I've merged trunk, resolved the conflicts, and opted to move the modal open state back to Redux to support your changes in #2025. I'm relatively confident that these changes shouldn't have an impact on the flows you're currently working on, but I would appreciate if you could take it for another test run to confirm. |
📊 Performance Test ResultsComparing 62396ac vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
epeicher
left a comment
epeicher
left a comment
There was a problem hiding this comment.
Thanks @fredrikekelund for merging with the latest changes. I have added the commit 0931b37 to fix a small issue with the mode being reset. I have also logged a task, STU-1035, that I discovered while testing this, but it is unrelated to these changes. All in all, I have tested it by enabling the feature flag, and it works as expected, nice refactor. LGTM!
Publish site header button
CleanShot.2025-11-21.at.18.24.22.mp4
Publish site on Sync tab
CleanShot.2025-11-21.at.18.25.04.mp4
There was a problem hiding this comment.
Great refactor, thanks for simplifying the logic and also removing the user-data-updated listener.
I confirm the connected sites work correctly when logging out and logging in.
restore-connected-sites-logout.mp4
The connected sites also work
Related issues
Proposed Changes
In #2083, we implemented a manual refetch of the list of connected sites when users log in to Studio. This prompted me to look into refactoring the state tree to use RTK Query instead, because with RTK Query, queries are automatically re-evaluated whenever a dependency changes (e.g., the currently logged-in user).
This PR does the following:
src/stores/sync/connected-sites.tswith an RTK Query API (query hooks and mutation hooks)src/stores/sync/connected-sites-hooks.tsandsrc/stores/sync/connected-sites-slice.tsTesting Instructions
Do a full smoke test of the sync tab, with and without the "Streamline Onboarding" feature flag enabled.
Also try logging out and then logging back in to WordPress.com and ensure that the list of connected sites refreshes as expected.
Pre-merge Checklist