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 context for sync sites #642

Merged
merged 51 commits into from
Nov 14, 2024
Merged

Add context for sync sites #642

merged 51 commits into from
Nov 14, 2024

Conversation

sejas
Copy link
Member

@sejas sejas commented Oct 31, 2024

Proposed Changes

  • Add context and move existing management sync hook inside it.
  • Improves the performance avoiding to read the appData file each time we change the tab to Sync.
  • Adapt tests

Testing Instructions

Apply this diff to better highlight the loading time for reading connected sites.

diff --git a/src/ipc-handlers.ts b/src/ipc-handlers.ts
index 475767c..3b92108 100644
--- a/src/ipc-handlers.ts
+++ b/src/ipc-handlers.ts
@@ -280,6 +280,10 @@ export async function disconnectWpcomSite(
 	return updatedConnections.filter( ( conn ) => conn.localSiteId === localSiteId );
 }
 
+function sleep( ms: number ) {
+	return new Promise( ( resolve ) => setTimeout( resolve, ms ) );
+}
+
 export async function getConnectedWpcomSites(
 	event: IpcMainInvokeEvent,
 	localSiteId: string
@@ -287,6 +291,7 @@ export async function getConnectedWpcomSites(
 	const userData = await loadUserData();
 	const currentUserId = userData.authToken?.id;
 
+	await sleep( 3000 );
 	if ( ! currentUserId ) {
 		return [];
 	}

  • Run STUDIO_SITE_SYNC=true npm start
  • Click on the Sync tab.
  • Connect a site.
  • Switch between tabs.
  • Return to the Sync tab.
  • Confirm that the connected sites are still displayed.
  • Before this PR, the initial screen briefly appeared before the connected sites finished loading.

Pre-merge Checklist

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

@sejas sejas self-assigned this Oct 31, 2024
@sejas sejas requested a review from a team October 31, 2024 18:17
Copy link
Contributor

@katinthehatsite katinthehatsite left a comment

Choose a reason for hiding this comment

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

The functionality works for me as expected. I reviewed the code and it looks good. One thing that I am wondering is whether we should break sync-sites-context into a couple of different files so that its responsibilities are limited to providing state and context for the connected sites and sync states.

For instance, we could move the use-site-sync-management and the hook for pulling operation into two different files and then pass through the context? My concern is that if we also add the push operation to this, that file will be hard to manage and navigate.

@sejas sejas requested review from a team and katinthehatsite November 13, 2024 15:42
@sejas
Copy link
Member Author

sejas commented Nov 13, 2024

For instance, we could move the use-site-sync-management and the hook for pulling operation into two different files and then pass through the context? My concern is that if we also add the push operation to this, that file will be hard to manage and navigate.

@katinthehatsite , great observation. I've refactored the code to be every main hook in a separate file. I've grouped them inside the same folder to indicate they belong to a given context. I also moved the state to the context so that way we reduce the risk of small hooks being used directly from the components.

Let me know what you think. Thank you!

Copy link
Contributor

@katinthehatsite katinthehatsite left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, I think the code is much more readable right now 😍

Base automatically changed from add/sync-pull-remote-site to trunk November 14, 2024 09:52
@sejas sejas changed the base branch from trunk to add/sync-pull-remote-site November 14, 2024 09:56
@sejas sejas changed the base branch from add/sync-pull-remote-site to trunk November 14, 2024 10:10
@wojtekn
Copy link
Contributor

wojtekn commented Nov 14, 2024

@sejas when I use a timeout snippet, and I open a site that has connected sites, and then switch to another site, I see connected site from the previous site for a while. It works well without the snippet, though.

@sejas sejas merged commit 5a7675f into trunk Nov 14, 2024
15 checks passed
@sejas sejas deleted the add/sync-context branch November 14, 2024 10:50
@sejas
Copy link
Member Author

sejas commented Nov 14, 2024

@sejas when I use a timeout snippet, and I open a site that has connected sites, and then switch to another site, I see connected site from the previous site for a while. It works well without the snippet, though.

Thanks for testing it, and finding that "glitch" when changing sites.
I think that's fine. This PR wanted to improve the performance when changing tabs, so the sync tab component unmounts and mounts.
Switching sites will have the same performance as trunk.
The timeout just exaggerates what is happening. I don't think we need to optimize it further. Let's keep an eye on that flow.

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.

3 participants