Move pull and push states from SyncSitesProvider to a new Redux slice - #2037
Conversation
📊 Performance Test ResultsComparing 2179acf vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
There was a problem hiding this comment.
Push: it seems it stucks at "Creating backup..." step. Tried a few times and also tried in trunk - worked quickly for me.
Pull: not related to the changes, since I reproduce the error in trunk too, but if you are working there - take a look please at this error when pull is finished, maybe it's a quck win:
…dio-refactor-pull-and-push-states-from-syncsitesprovider
Thanks for reviewing this @nightnei! I took another look at this, and found that earlier we used a ref to keep track of the state, which let us read it back immediately after updating it. For now, I added the ref to the hooks that were previously in the I'm also testing a simpler approach that updated the state objects directly where needed. If it works reliably, I'll update the PR with this approach. |
|
@gcsecsey, I would challenge us to remove With this change, we are preserving functions like |
That's a good point @fredrikekelund, thanks! I'll try to move as much as possible to the Redux slice, and ping for another review. |
Remove the SyncSitesProvider React context layer and migrate all components to use useSyncPull and useSyncPush hooks directly. Changes: - Extract getLastSyncTimeText to useLastSyncTimeText hook - Extract initialization logic to useInitializeSyncStates hook - Move useListenDeepLinkConnection and initialization to App component - Update all components to use hooks directly instead of context - Remove sync-sites-context.tsx
…dio-refactor-pull-and-push-states-from-syncsitesprovider
…dio-refactor-pull-and-push-states-from-syncsitesprovider
|
I carried out the changes to move most of the logic into thunks within @fredrikekelund could you please take another look when you're back? Thanks! |
…s-from-syncsitesprovider
|
I discovered that the ability to manually pause pushes had been lost in the diff, so I restored it. I also went ahead and added zod schemas for the API responses and restored tests that had been botched or skipped. |
There was a problem hiding this comment.
This change (and all the corresponding changes in the other files) was to appease TS. Apparently, TS interfaces can be implicitly merged, which is why the compiler was complaining in apps/studio/src/stores/index.ts when the state interfaces weren't exported. Silly stuff, but this change appeases TS.
…and to cancel other active listeners
…itesprovider' of github.com:Automattic/studio into stu-711-studio-refactor-pull-and-push-states-from-syncsitesprovider
…dio-refactor-pull-and-push-states-from-syncsitesprovider
|
I know it's not easy to see how, but e041e31 breaks sync polling, @gcsecsey. Listeners need to wait for the dispatched actions to finish, because the signal is aborted when the listener completes. Your change might be taken from my previous suggestion. If so, know that I've already implemented everything I suggested. |
Sorry, yes I was trying to implement what you suggested earlier, I didn't realize that you also updated the listeners. I'll revert e041e31. |
… types, and to cancel other active listeners" This reverts commit e041e31.
If I start a push and cancel it before it starts uploading, then quickly start a new push, the second operation will fail. That's because it generates an identical archive path for every push, and the abortion of the first push call is async, meaning the file isn't deleted when the operation is aborted, but only after the first export finishes.
…s-from-syncsitesprovider
|
@fredrikekelund thank you for iterating on this, I re-tested the pull/push functionality with the most recent changes and found no regressions. Resumable uploads and polling all work well too. I think we're in good shape to proceed with merging this. 👍 |
|
As this PR is pretty big, could we get another reviewer from @Automattic/yolo to take a look? @nightnei, you already reviewed this once, could you give it a second look when you have some bandwidth? We already spent a lot of time with @fredrikekelund on code reviews, so testing/verifying would be the priority. |
|
@gcsecsey I tested and all casses work correctly - push/pull, cancel both, offlane mode and pausing/resuming while pushing. But I didn't review code thoroughly, I just glanced it a bit. |
The updateSiteTimestamp calls were dropped in #2037 when push/pull states moved from SyncSitesProvider to a Redux slice. Wire them back up by dispatching the RTK Query mutation from the thunks.
* Remove updateSingleConnectedWpcomSite IPC handler Replace all usages with the existing updateConnectedWpcomSites handler by wrapping the single site in an array. This eliminates a redundant IPC handler that duplicated logic already present in the bulk variant. * Restore timestamp updates on push/pull success The updateSiteTimestamp calls were dropped in #2037 when push/pull states moved from SyncSitesProvider to a Redux slice. Wire them back up by dispatching the RTK Query mutation from the thunks. * Remove updateSiteTimestamp RTK Query mutation, call IPC directly Replace the RTK Query mutation with a plain async function that calls getIpcApi().updateConnectedWpcomSites() directly from the thunks. Await the timestamp write before invalidating the cache to avoid a race where the re-fetch reads stale data.
3 participants
Related issues
Fixes STU-711
Proposed Changes
Sync operations are currently managed through a React Context (SyncSitesProvider) with two large custom hooks (useSyncPull and useSyncPush). This works, but it has drawbacks: polling is driven by useEffect intervals tied to the component lifecycle, state is only accessible to components wrapped in the provider, and the logic is difficult to test in isolation. The hooks mix business logic (API calls, file operations, error handling) with React state management (useState, useEffect), making them hard to reason about.
This PR replaces the Context + hooks architecture with a single Redux Toolkit slice (sync-operations-slice.ts) and listener middleware:
syncOperationsslice (src/stores/sync/sync-operations-slice.ts): consolidates all pull/push state, async thunks for operations (pushSiteThunk,pullSiteThunk, polling thunks, cancel thunks), 22 selectors, and IPC event listeners into one filesrc/stores/index.ts): replacesuseEffect-based polling intervals. Listeners watch for state changes and automatically dispatch polling thunks after 2s delays, creating self-sustaining loops decoupled from React component lifecycleaddSyncOperation/clearSyncOperation) and error modal display are now handled by listeners reacting to state changes and thunk rejections, rather than being scattered across hooksSyncSitesProviderand hooks (sync-sites-context.tsx,use-sync-pull.ts,use-sync-push.ts): ~1,155 lines removeduseRootSelectorwith selectors anddispatchwith thunks instead ofuseSyncSites()context hookuseLastSyncTimeText(src/hooks/sync-sites/use-last-sync-time-text.ts): small utility previously embedded in the contextconditionfor dedup: polling thunks usecreateAsyncThunk'sconditionoption to skip execution if the operation was cancelled, replacing manual guard checksSyncSitesProviderfromroot.tsx, moved initialization toapp.tsxviainitializeSyncStatesThunkTesting Instructions
This is a pure refactor, no user-facing behavior should change. The goal is to Check that everything still works exactly as before:
Pre-merge Checklist