-
Notifications
You must be signed in to change notification settings - Fork 54
Implement selective syncing in the pull dialog #1596
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 selective syncing in the pull dialog #1596
Conversation
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 👍
It works perfectly, I tested that rewind_id is used correctly with manually adding file to fonts folder, pushign it then opening Pull dialog and checking that I don't see it, but after updating backup it appeared. Also tested pulling separately plugins, fonts, and all files, and every time it worked as expected. Also tested importing backups: locals, wpress, playground and jetpack - everything works well 👍


Regarding code I have some feedback and things to discuss.
| @@ -0,0 +1,131 @@ | |||
| import { createSlice } from '@reduxjs/toolkit'; | |||
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 haven't finished figuring out all the code around sync store, but I am thinking - would it be less code and woudl it be more clear and simple code if we use react-query here? I have feeling that redux here is redundant.
WDYT?
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 addressed all the feedback above, thanks for the great suggestions.
About react-query vs redux, I think we shouldn't introduce a new library in this PR, and we should discuss it as a team.
I like TansStack Query, but since we decided to use RTK Queries I think we should keep using them.
p1736435348402909/1736434430.061079-slack-C04GESRBWKW
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 all the feedback, the changes LGTM and I tested one more time all scenarious and everything works well 👍
Related issues
Proposed Changes
Testing Instructions
Test the new flow pulling the latest rewind_id
npm start.Try pulling a site without backups
Test Studio other backups
2025-partial-pull.mp4
Pre-merge Checklist