-
Notifications
You must be signed in to change notification settings - Fork 55
Sync: Add pause/unpause functionality for sync uploads #2379
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?
Conversation
📊 Performance Test ResultsComparing 9be6d83 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
ivan-ottinger
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.
This a great! Thanks for adding the Pause/Play button to the sync process, Rahul.
The changes look good to me and they work as expected. 👍🏼
I left couple comments on things I noticed.
When clicking the Play/Pause button, the progress jumps a bit. Maybe we could consider adjusting the logic to prevent that behavior.
|
Actually, I noticed one issue: Sometimes, when pausing and resuming the upload, the operation fails with the following error: |
|
I got the same error as Ivan:
|
katinthehatsite
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 adding these changes, this will be a great improvement for the users!
I found one edge case where I seem to be getting errors:
- Start push process on a site
- Pause an upload
- Cancel the upload
- Start again the push process for the same site
- Wait until the upload can be paused
- Pause the upload
- Observe that this throws an error:
This is the details that are captured to Sentry:
Sentry Logger [log]: Captured error event `Error invoking remote method 'pushArchive': reply was never sent`
fc6c530 to
6d93fa7
Compare
|
Thanks @ivan-ottinger and @katinthehatsite for noticing this error and reporting them. I have fixed the issue with the cancel push operation. It should also fix the issues you reported earlier. It would great if you could give another review. 🙂 |
Nice, I am not seeing these errors anymore 👍 Functionality-wise, everything looked good to me. I will review the code a bit closer tomorrow morning with fresh eyes |
Thank you for the updates, Rahul. I am still getting error when pausing and resuming, but different one this time: |
katinthehatsite
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.
It is looking good overall on my end, I left some minor comments for consideration.
|
Feature-wise looks good, but I think we should adjust the design before we merge:
|
Thanks, Wojtek, for raising this. These were my concerns as well. I am waiting for a response from @Marinatsu on design feedback. Edit: I have updated the pause icon to match style of |
Thanks @ivan-ottinger. I couldn't reproduce the error. But I tried to fix the issue in b6b2040. Can you give a quick test? |
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 found one more interesting case while testing:
- Connect two sites to a Studio site
- Start Push process on the connected site A and pause the upload
- Start Push process on the connected site B
- Unpause the uploads
- Observe the Studio site pushing the changes to two sites at once:
I also noticed an error in Sentry for this case:
Backup created at: /var/folders/bj/1qgxs1qd6_z20tw9llkw842h0000gn/T/com.wordpress.studio/site_d354f946-9026-4a52-a2ff-cd3ac7c2d172.tar.gz
Backup created at: /var/folders/bj/1qgxs1qd6_z20tw9llkw842h0000gn/T/com.wordpress.studio/site_d354f946-9026-4a52-a2ff-cd3ac7c2d172.tar.gz
Sentry Logger [log]: Captured error event `ENOENT: no such file or directory, unlink '/var/folders/bj/1qgxs1qd6_z20tw9llkw842h0000gn/T/com.wordpress.studio/site_d354f946-9026-4a52-a2ff-cd3ac7c2d172.tar.gz'`
I know that we are currently blocking the UI for pushing and pulling more than to one site at a time so I am wondering if we might want to stay consistent here as well:
In terms of functinality, with this edge case, the push worked for one site and for the other one, it sort of got stuck and it is still pending for me. I will update this comment if it goes through but I think it is probably related to the error I saw in Sentry.
src/components/icons/pause.tsx
Outdated
| @@ -0,0 +1,6 @@ | |||
| export const PauseIcon = () => ( | |||
| <svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg"> | |||
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 noticed that the sizing of the icons is different and I could see that difference visually, not a huge one. What do you think about making it consistent?
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.
Fixed in 9892f91
I have run multiple tests and spotted that the issue happens only when we pause at 100% of the upload progress. Since by then the upload is finished, I think we can hide the Pause button at that point, which should resolve the issue. CleanShot.2026-01-16.at.11.06.54.mp4 |
9be6d83 to
9892f91
Compare
|
@ivan-ottinger Thanks for your suggestion. I have applied your suggestion in 2ec6e22. |
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.
@ivan-ottinger Thanks for your suggestion. I have applied your suggestion in 2ec6e22.
Thanks for the update, Rahul! The change there looks good and I can confirm it fixes the issue. I did not observe any other edge cases and pausing / resuming works great. 👍🏼
I understand there might still be a design adjustment related to the buttons coming, but I am already approving the PR in case we would like to merge it in its current state.
Btw, I have tested on a rebased branch with latest trunk locally, just did not push it remotely.
Tbh, I am not happy with the layout shift it does when the Play/Pause button appears and hides. It definitely needs some better UX in IHMO. cc @Marinatsu 🙂 |
|
Hi! I reviewed the last video shared. There are two moments where we have odd layout shifts:
I see two potential solutions:
|
|
Thanks for looking into this @crisbusquets
I think this is the simplest path forward and would result in good UX without any animated or non-animated layout shifts. |

Related issues
Proposed Changes
Testing Instructions
npm startCleanShot.2026-01-12.at.17.02.49.mp4