Skip to content
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

Add Sync Push Site #662

Merged
merged 19 commits into from
Nov 15, 2024
Merged

Add Sync Push Site #662

merged 19 commits into from
Nov 15, 2024

Conversation

sejas
Copy link
Member

@sejas sejas commented Nov 14, 2024

Related issues

Proposed Changes

  • Add push state
  • Add polling for restoring backup progress
  • Display Progress, Success and Error messages
  • Refactor states to re-use pull logic
  • Blocks import/export, disconnect and other pull/push actions. I'm not blocking start/stop.
  • Display a notification after the successful push

Testing Instructions

You can try with a site up to 2GB in size.

  • Run STUDIO_SITE_SYNC=true npm start
  • Click on the Sync tab
  • Connect a site
  • Click on Push
  • Change tabs and sites, observe the process continues correctly
  • Wait until the process finishes
  • Observe your remo site has restored. In fact, the remote site would look the same as before, but it restored a previous backup of the same site.
NERCzq.mp4

Pre-merge Checklist

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

@sejas sejas self-assigned this Nov 14, 2024
Comment on lines +117 to +120
getIpcApi().showErrorMessageBox( {
title: sprintf( __( 'Error pushing to %s' ), connectedSite.name ),
message: __( 'Studio was unable to connect to WordPress.com. Please try again.' ),
} );
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm displaying this generic error. I could also display the error that comes from the backend.

When trying to upload a bakcup bigger than the limit, we cannot grab the actual error message, because we receive a CORS error.

@sejas sejas marked this pull request as ready for review November 15, 2024 07:14
@sejas sejas requested a review from a team November 15, 2024 11:14
Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

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

Looks good. Works correctly for me - it starts and completes the push. Rewind debugger shows successful backup, and then restore. The site is the same, but i's expected as we haven't connected the logic that restores from the actual file.

Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

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

I noticed one missing part - there is not system notification after the finished Push, but we have one for Pull. We can add a separate issue for that, though.

src/hooks/use-sync-states-progress-info.ts Outdated Show resolved Hide resolved
importing: {
key: 'importing',
progress: 80,
message: __( 'Pushing changes…' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what message we could use here. Any thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only message that appears in the design. I think it's good to leave it.
Other options:

  • Applying changes
  • Unpacking changes

Copy link
Member Author

Choose a reason for hiding this comment

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

We could also split this state using the remote states, so the user will get more information initial_backup_started, archive_import_started, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

'Applying changes' sounds good for now.

Copy link
Member Author

@sejas sejas Nov 15, 2024

Choose a reason for hiding this comment

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

there is not system notification after the finished Push, but we have one for Pull. We can add a separate issue for that, though.

I implemented it in this PR 🥳: 8e22d1b

'Applying changes' sounds good for now.

@sejas sejas merged commit 55e314d into trunk Nov 15, 2024
17 checks passed
@sejas sejas deleted the add/sync-push-site branch November 15, 2024 14:39
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.

2 participants