Update SyncSitesProvider types
#723
Conversation
| const isPushError = pushState.isError; | ||
| const isPushing = pushState && isKeyPushing( pushState.status.key ); | ||
| const isPushError = pushState && isKeyFailed( pushState.status.key ); | ||
| const hasPushFinished = pushState && isKeyFinished( pushState.status.key ); | ||
|
|
There was a problem hiding this comment.
This change ensures that getPullState and getPushState can have the same return type. Previously, getPushState would add a few attributes to its return value to include these details. I don't see any good reason for treating getPullState and getPushState differently, though.
| @@ -147,7 +141,7 @@ const SyncConnectedSitesSection = ( { | |||
| variant="link" | |||
| className="!ml-auto !text-a8c-gray-70 hover:!text-a8c-red-50 " | |||
| onClick={ handleDisconnectSite } | |||
| disabled={ isPulling || isPushing } | |||
| disabled={ isSiteIdPulling( selectedSite.id ) || isSiteIdPushing( selectedSite.id ) } | |||
There was a problem hiding this comment.
I believe this should be the equivalent of the previous isPulling and isPushing definitions.
There was a problem hiding this comment.
I think I spotted a small regression with this one - the disconnect button now is disabled for all connected sites on a particular selected site. The expected behaviour is that it should be disabled only for the Disconnect button on a site that is syncing. I think this screenshot illustrates the issue:
There was a problem hiding this comment.
Thanks, great catch 👍 Fixed now
|
I looked through changes and tested the flow. It seemed to work as expected besides one small regression I noticed. However, I am not sure if the regression is related to the changes in this PR or to the differences with trunk. Perhaps we could resolve the conflicts with trunk and give it another review? 🙇 |
|
Conflicts fixed and your comment addressed, @katinthehatsite. This should be good for a second look now |
| return sitePushState?.isInProgress; | ||
| } ); | ||
| const isPulling = connectedSites.some( ( site ) => isSiteIdPulling( selectedSite.id, site.id ) ); | ||
| const isPushing = connectedSites.some( ( site ) => isSiteIdPushing( selectedSite.id, site.id ) ); | ||
| const isThisSiteSyncing = isPulling || isPushing; |
There was a problem hiding this comment.
I see that we are using this logic in a couple of different files. I am wondering if we might want to create a hook to be able to reuse this check. This is not directly related to this PR though as this was already present before. We can create a separate issue to consider this.
Related issues
Proposed Changes
The type definitions in
SyncSitesProviderand the related "sub-hooks" are unnecessarily complex. They cross-reference in ways that make them difficult to reason about.This PR adds explicit return type definitions for
useSyncPull,useSyncPush, anduseSiteSyncManagement. I've also cleaned upSyncSitesContextTypeand excluded state and functions that weren't being used anywhere in the codebase.Testing Instructions
Code review and smoke testing the sync flow should suffice
Pre-merge Checklist