-
Notifications
You must be signed in to change notification settings - Fork 53
Move pull and push states from SyncSitesProvider to a new Redux slice #2037
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?
Move pull and push states from SyncSitesProvider to a new Redux slice #2037
Conversation
📊 Performance Test ResultsComparing c108a94 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
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.
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! |
nightnei
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.
Thanks for addressing feedback, works well now, no regressions 👍
Related issues
Fixes STU-711
Proposed Changes
SyncSitesProvidercompletelysyncOperationsRedux sliceuse-sync-pullanduse-sync-pushhooks to Redux thunkssyncOperationsRedux sliceTesting Instructions
npm startPre-merge Checklist