Skip to content

Conversation

@gavande1
Copy link
Contributor

@gavande1 gavande1 commented Dec 2, 2025

Related issues

Proposed Changes

Testing Instructions

  • Checkout 197246-ghe-Automattic/wpcom and 196922-ghe-Automattic/wpcom on your sandbox. You probably need to use git checkout branch -- filenames for applying changes from two branches.
  • Sandbox public-api.wordpress.com and make it in FULL ACCESS mode.
  • Checkout this branch in Studio
  • run npm install & install start
  • Connect your wordpress.com
  • Add site for sync
  • Run following tests
  1. Do a full sync
  2. Do a partial sync with different options
  3. Start sync, go offline, wait few seconds, come online again. Make sure sync doesn't fail. This is to confirm resumable capability.
  • Checkout to master branch on Studio
  • Run above tests again. Everything should work. The code handles backward compatibility.
CleanShot.2025-12-02.at.18.44.24.mp4

Additional checks:

  • Check for any abnormal behavior from existing setup.
  • Check if you can not upload more than 2 GB of archive.
  • Make sure you receive success email after sync is completed.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

📊 Performance Test Results

Comparing 4823c56 vs trunk

site-editor

Metric trunk 4823c56 Diff Change
load 10362.00 ms 12119.00 ms +1757.00 ms 🔴 17.0%

site-startup

Metric trunk 4823c56 Diff Change
siteCreation 19361.00 ms 23334.00 ms +3973.00 ms 🔴 20.5%
siteStartup 7979.00 ms 9044.00 ms +1065.00 ms 🔴 13.3%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change

@gavande1 gavande1 self-assigned this Dec 2, 2025
@@ -1,8 +1,17 @@
import * as Sentry from '@sentry/electron/renderer';
// @ts-expect-error - Uppy types require newer moduleResolution, but types exist at runtime
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to look into how we can resolve this typescript error. The tests are also failing because of this.

@gavande1 gavande1 requested a review from a team December 2, 2025 13:04
@gavande1 gavande1 force-pushed the stu-939-implement-resumable-uploads-for-studio-sync branch from 2a6fddf to 1a68a50 Compare December 5, 2025 10:13
@wojtekn
Copy link
Contributor

wojtekn commented Dec 5, 2025

@gavande1 thanks for taking the spike into the implementation. I haven't had a chance to test and review PR yet, but I wanted to share some thoughts on the UX.

Currently, it supports automatic resuming when an internet connection is lost. It's a valid use case, and it's the most common. I'm curious how close the Tus brings us to adding more, e.g.:

  • UI control that allows for pausing/unpausing upload
  • Ability to close Studio during upload and resume after relaunch
  • UI affordance that shows upload paused due to the connection issue

Also, should we at least include the last one in this PR? Or would you treat this PR as a technology change that transparently handles temporary broken connections, and then work on UX improvements separately?

@fredrikekelund
Copy link
Contributor

Reverts changes added in #1972 as uploading happens in frontend.

The point of that change was to speed up uploads. Even if we adopt a new technical strategy with this PR that may bring performance benefits in and of itself, it remains true that uploading the archive from the renderer process adds significant overhead since the entire archive must be stored in memory (currently a maximum of 2GB, soon to be 5GB).

I suggest researching ways to keep the upload running in the node process and to read the file as a stream, instead of storing the archive contents in memory.

The uppy events (upload-error, upload-progress, etc.) could be propagated from the node process to the renderer via IPC (see IpcEvents in src/ipc-utils.ts and useIpcListener).

If uppy doesn't support uploading streams directly, then the tus-js-client library (which uppy wraps) appears to do so (see API docs).

@gavande1
Copy link
Contributor Author

gavande1 commented Dec 8, 2025

Thanks @wojtekn and @fredrikekelund for looking at the PR.

UI control that allows for pausing/unpausing upload

It's already there. We just need to implement in our workflows.

Ability to close Studio during upload and resume after relaunch

Again, the capability is already there. We need to keep track of files being uploaded when closing the app and re-upload them after app restarts. We may need to add more logic for validity of archive.

UI affordance that shows upload paused due to the connection issue

It's very simple to add that. I think your suggestion to reflect connection issue in UI makes sense. I will add that.

The point of that change was to speed up uploads. Even if we adopt a new technical strategy with this PR that may bring performance benefits in and of itself, it remains true that uploading the archive from the renderer process adds significant overhead since the entire archive must be stored in memory (currently a maximum of 2GB, soon to be 5GB).

I agree that uploading the archive from the renderer process adds significant overhead. However, I see that overhead as temporary. You are correct that Uppy does not support streaming uploads, and I explored using tus-js-client in the Node process before starting any implementation. After evaluating the trade-offs, I chose the frontend approach because the temporary overhead felt acceptable, and file uploads have traditionally happened on the frontend. We only recently moved them from the renderer process to Node. Another important factor is time and effort. Uppy gives us many features out of the box on the frontend that we would need to recreate if we used the tus-js-client directly in Node.

That said, I am absolutely open to switching to the Node process if you feel these reasons aren’t strong enough. Just let me know and I am happy to adjust the approach.

@fredrikekelund
Copy link
Contributor

In my previous testing (#1972 (comment)), uploading from the node process yielded more than 7x improvement in the upload time. The bigger the archive, the bigger the impact.

Resumable uploads are a great way to increase reliability (and a great feature in and of itself). Still, users won't be happier if it comes at the price of slower uploads or a big increase in memory consumption.

@gavande1
Copy link
Contributor Author

gavande1 commented Dec 8, 2025

Thanks @fredrikekelund for sharing the improvements history. I am digging deeper into implementing it using file streams in the Node process.

@gavande1
Copy link
Contributor Author

gavande1 commented Dec 8, 2025

@fredrikekelund I have opened a new PR: #2212 to handle file uploads in the Node process. I will close this PR in favor of the new one. The updated PR also includes the suggestion from @wojtekn to display an indication when the upload is paused due to a network error.

@gavande1 gavande1 closed this Dec 8, 2025
@wojtekn wojtekn deleted the stu-939-implement-resumable-uploads-for-studio-sync branch December 15, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants