Push: Check existing progress of push on start by epeicher · Pull Request #1943 · Automattic/studio

epeicher · 2025-10-27T18:59:59Z

Related issues

Related to STU-751

Proposed Changes

Check if there is any Push in progress when starting Studio and updates the progress accordingly
Adds a useEffect to sync-sites-context.tsx that only runs once to check if there is any connected site with a push operation in progress
Update the pushStates of the sites if there is any status in progress

CleanShot.2025-10-28.at.19.15.08.mp4

Testing Instructions

Apply these changes and run npm start
Navigate to the Sync tab and connect a site or use any existing connected site
Select Push
Wait for the status to be progressed to Backing up remote site…
Press Cmd+Q to quit Studio
Check that the dialog appears and notify that the operation will continue. Accept and quit Studio
Open Studio again
Navigate to the Sync tab
Check the progress continues

Pre-merge Checklist

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

github-actions · 2025-10-27T19:19:56Z

📊 Performance Test Results

Comparing f3e82e8 vs trunk

site-editor

Metric	trunk	`f3e82e8`	Diff	Change
load	13732.00 ms	17114.00 ms	+3382.00 ms	🔴 24.6%

site-startup

Metric	trunk	`f3e82e8`	Diff	Change
siteCreation	22157.00 ms	28445.00 ms	+6288.00 ms	🔴 28.4%
siteStartup	7019.00 ms	7065.00 ms	+46.00 ms	🔴 0.7%

Results are median values from multiple test runs.

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

Copilot

Pull Request Overview

This PR implements functionality to restore push operations that were in progress when Studio was closed, ensuring users can track ongoing sync operations across app restarts.

Key Changes:

Adds startup check for in-progress push operations via server API query
Maps server import response statuses to internal push state objects
Updates active sync operation tracking when restoring push states

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/lib/active-sync-operations.ts	Removes unused `getSyncOperations()` function
src/hooks/sync-sites/use-sync-push.ts	Adds `mapImportResponseToPushState()` helper and guards `addSyncOperation` call
src/hooks/sync-sites/sync-sites-context.tsx	Implements `useEffect` to check and restore in-progress push operations on mount

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sejas

Feature-wise, it works as described. I confirm that it continues polling push information from the backend after restarting the app. ✨

applying.changes.works.as.expected.mp4

sejas · 2025-10-28T20:56:55Z

+				// Query the server for each connected site to check for in-progress operations
+				for ( const connectedSite of connectedSites ) {
+					try {
+						// Find the local site


Let's remove these comments when they are unnecessary.

Sure, I have removed and re-worded some of them here 5b5f0a1 but please suggest if other comments feel also unnecessary, happy to remove them 😄

…ush-progress-on-start

fredrikekelund

This is a great addition 👍 I haven't finished my review, but since I see you're actively working on this PR, @epeicher, I'll submit some of the feedback I have so far.

fredrikekelund · 2025-10-29T09:22:02Z

+		hasInitialized.current = true;
+
+		const initializePushStates = async () => {
+			try {


This try-catch basically only catches exceptions from getIpcApi().getSiteDetails(), right? I'm not sure that's really necessary, but if we keep it, then maybe we can wrap just that row instead of the entire function body to reduce indentation.

This could also catch potential exceptions thrown by this request, for example, being offline, so I think it's good that it covers both

There's already an inner try-catch statement to capture exceptions from that API call, though. That's why I figured we could make this statement wrap fewer lines.

Ah, you're right, I mixed it with the inner one.

I could reduce the indentation by narrowing the scope, but it would require extracting the allSites variable outside of the try-catch and checking if it's null before continuing after the try-catch, which could be less readable. Alternatively, I could remove this try-catch as it could be unnecessary, what option do you prefer?

How about removing this clause and wrapping the initializePushStates call in a try..catch instead?

I have tested it and in that case, because we are not awaiting the promise because of the void operator, an error in any of the await getIpcApi()... will be propagated and we'll get the following error in the UI

You could add a .catch handler to the initializePushStates call and remove the void operator.

Yes, of course, I have tested that and it works. Do you think that is more readable than the try-catch in the initializePushStates? I honestly think we would be mixing promise handling (the .catch) with async-await, but if you have a strong opinion about it, I am happy to change that

Principally, I don't think it's wrong to mix "classic" promise handling with async/await. The root problem here is essentially that useEffect can't handle async callbacks, but we still want some way to run async code inside that function. To that end, it's more about finding the least clunky solution.

However, this specific issue is really mostly cosmetic and not worth getting hung up on, IMO. Feel free to make a call and proceed as you see fit, @epeicher 👍

The root problem here is essentially that useEffect can't handle async callbacks, but we still want some way to run async code inside that function.

💯 I agree, that's the root issue

it's more about finding the least clunky solution.

I didn't find clunky to have the outer try-catch, but you and two different agents think the .catch is less clunky so e5fb0f8 😄

fredrikekelund

I wish we had untangled some of the push/pull React context and moved it to Redux slices, but that's beyond the scope of this PR.

Again, great addition 👍 Functionality-wise, this works as described. The code looks good overall, but I'd prefer we address the comments from my previous review (especially this). Approving now to unblock you later, @epeicher.

…selected site

fredrikekelund

Some final minor comments

fredrikekelund · 2025-10-29T13:12:43Z

+		hasInitialized.current = true;
+
+		const initializePushStates = async () => {
+			try {


How about removing this clause and wrapping the initializePushStates call in a try..catch instead?

sejas

Works as expected. I confirm all ongoing pushes are resumed after restarting Studio.
Thanks for fixing the issue I caught by mistake, and renaming the localSite variable.

… in useEffect

…ush-progress-on-start

wojtekn · 2025-11-05T14:40:39Z

@epeicher This is a great improvement to sync UX 🥳

epeicher force-pushed the add/check-push-progress-on-start branch from 0826961 to 7dad8c9 Compare October 28, 2025 11:45

Base automatically changed from stu-751-studio-investigate-how-to-stop-a-rewind-or-display-a-warning to trunk October 28, 2025 16:17

Push: Check existing progress of push on start

606f6b3

epeicher force-pushed the add/check-push-progress-on-start branch from 7dad8c9 to 606f6b3 Compare October 28, 2025 16:46

epeicher added 2 commits October 28, 2025 18:44

Notify errors

2e106f2

Simplify addSyncOperation logic

a0d62d5

epeicher marked this pull request as ready for review October 28, 2025 18:19

epeicher requested review from a team and Copilot October 28, 2025 18:19

Copilot AI reviewed Oct 28, 2025

View reviewed changes

Comment thread src/hooks/sync-sites/sync-sites-context.tsx Outdated

Comment thread src/hooks/sync-sites/use-sync-push.ts

Comment thread src/hooks/sync-sites/sync-sites-context.tsx Outdated

sejas reviewed Oct 28, 2025

View reviewed changes

sejas mentioned this pull request Oct 28, 2025

Improve Push message when quitting #1919

Merged

1 task

epeicher added 3 commits October 29, 2025 09:53

Simplify map states logic

449d839

Remove unnecessary comments

5b5f0a1

Merge branch 'trunk' of github.com:Automattic/studio into add/check-p…

f616b1b

…ush-progress-on-start

fredrikekelund reviewed Oct 29, 2025

View reviewed changes

fredrikekelund approved these changes Oct 29, 2025

View reviewed changes

wojtekn assigned wojtekn and epeicher and unassigned wojtekn Oct 29, 2025

sejas reviewed Oct 29, 2025

View reviewed changes

Comment thread src/hooks/sync-sites/sync-sites-context.tsx Outdated

epeicher added 4 commits October 29, 2025 11:50

Use generic for the response body in import endpoint

46bda48

Merge existing state when updating pushStates

fd67b23

Iterate over all connected sites, not only the connected ones of the …

be16976

…selected site

Remove condition and unnecessary dependencies

0201f62

fredrikekelund reviewed Oct 29, 2025

View reviewed changes

Remove superfluous comment

5938e04

sejas approved these changes Oct 29, 2025

View reviewed changes

Remove outer try/catch in initializePushStates and handle with .catch…

e5fb0f8

… in useEffect

Merge branch 'trunk' of github.com:Automattic/studio into add/check-p…

13bcd1f

…ush-progress-on-start

epeicher merged commit fbfc634 into trunk Oct 30, 2025
13 checks passed

epeicher deleted the add/check-push-progress-on-start branch October 30, 2025 11:20

This was referenced Oct 30, 2025

Sync: Remove unnecesary ref when initializing Push states #1980

Merged

Sync: Do not check Push state for disconnected sites #1984

Merged

Uh oh!

Conversation

epeicher commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions Bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Performance Test Results

site-editor

site-startup

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

sejas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekelund Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejas left a comment

Choose a reason for hiding this comment

Uh oh!

wojtekn commented Nov 5, 2025

Labels

5 participants

epeicher commented Oct 27, 2025 •

edited

Loading

github-actions Bot commented Oct 27, 2025 •

edited

Loading

fredrikekelund Oct 30, 2025 •

edited

Loading