Skip to content

Refactor sync-support.ts #2359

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.

Sign up for GitHub

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

Merged
nightnei merged 1 commit into from
Jan 9, 2026
Merged

Refactor sync-support.ts #2359

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions src/modules/sync/lib/sync-support.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,23 @@ export function isPressableSite( site: SitesEndpointSite ): boolean {
return site.hosting_provider_guess === 'pressable';
}

export function isAtomicSite( site: SitesEndpointSite ): boolean {
function isAtomicSite( site: SitesEndpointSite ): boolean {
return site.is_wpcom_atomic;
}

export function hasSupportedPlan( site: SitesEndpointSite ): boolean {
function hasSupportedPlan( site: SitesEndpointSite ): boolean {
return site.plan?.features.active.includes( STUDIO_SYNC_FEATURE_NAME ) ?? false;
}

export function isJetpackSite( site: SitesEndpointSite ): boolean {
return !! site.jetpack && ! isAtomicSite( site ) && ! isPressableSite( site );
Copy link
Contributor Author

@nightnei nightnei Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Why detection of jetpack includes ! isAtomic && ! isPressable? Theoretically !! site.jetpack should be enough.

I took a look at history and now the function directly says that jetpack site, which is not atomic is unsupported, and the example is jurasic.ninja

Copy link
Contributor

@ivan-ottinger ivan-ottinger Jan 8, 2026

Choose a reason for hiding this comment

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

Why detection of jetpack includes ! isAtomic && ! isPressable?

As far as I remember, we used this in the past to filter out self-hosted sites that are not hosted by us (via WPCOM and Pressable).

In that logic, "Jetpack site" referred to "self-hosted WordPress site".

Copy link
Contributor Author

@nightnei nightnei Jan 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The main point here - below we have a condition that Jetpack sites are unsupported:

if ( isJetpackSite( site ) ) {
	return 'unsupported';
}

But it's not true, since all supported sites must have Jetpack. I worked around this code, needed cases to reproduce it, and got stuck.

In that logic, "Jetpack site" referred to "self-hosted WordPress site".

Then it should be either isUnsupported or isSelfHosted, but not isJetpackSite.

// Sites hosted outside wp.com and Pressable (e.g. jurassic.ninja).
Copy link
Contributor

@ivan-ottinger ivan-ottinger Jan 8, 2026

Choose a reason for hiding this comment

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

While reviewing the code I wanted to suggest moving isPressableSite check into the isUnsupported helper function directly, but then I read your other comment below:

In this condition we keep isPressableSite here, outside the hasSupportedPlan and imo it's good idea, to be clear at the upper level that the plan is about only dotcom, and it's not about Pressable.
So I added the same approach to two other cases (isUnsupported and needsTransfer).
Also, it's for consistency.

Sounds good to me. 👍🏼 With that I think we may want to update the comment above:

// Sites hosted outside wp.com and Pressable (e.g. jurassic.ninja).

As now it looks as if isUnsupported also checks for Pressable sites.

function isUnsupported( site: SitesEndpointSite ): boolean {
return !! site.jetpack && ! isAtomicSite( site );
}

export function needsTransfer( site: SitesEndpointSite ): boolean {
return ! isJetpackSite( site ) && ! isPressableSite( site ) && ! isAtomicSite( site );
Copy link
Contributor Author

@nightnei nightnei Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If we read this condition, it says:

  1. NOT !! site.jetpack && ! isAtomicSite( site ) && ! isPressableSite( site )
    &&
  2. && ! isPressableSite( site ) && ! isAtomicSite( site )

It's dificult to undersathnd when it's actually true. I have read the history of comits and simplified to just ! site.jetpack && ! isAtomicSite( site ). And added the comment to how to reproduce the case.

Copy link
Contributor

@ivan-ottinger ivan-ottinger Jan 8, 2026

Choose a reason for hiding this comment

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

I think this was a workaround to check for a Simple site (a site that is not Jetpack, not Atomic and not Pressable).

It was as long time again since I looked at the old logic, but I believe there was some edge case that resulted in this more complex check.

Copy link
Contributor Author

@nightnei nightnei Jan 8, 2026

Choose a reason for hiding this comment

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

The main point here, that the condition is - ! ( !! site.jetpack && ! isAtomicSite( site ) && ! isPressableSite( site ) ) && ! isPressableSite( site ) && ! isAtomicSite( site ), and it's not possible to understand it. And I haven't found any use case for something like this. Also reading commits history - I haven't found any "Testing instruction" or some PR description which says about some unusual use case.

// needsTransfer means that the site was reverted to "simple" via "bulk-delete-test-sites" tool.
// Activating e.g. "VaultPress Backup" or "Hosting features" returns "is_wpcom_atomic === true && jetpack === true".
function needsTransfer( site: SitesEndpointSite ): boolean {
return ! site.jetpack && ! isAtomicSite( site );
}

export function getSyncSupport( site: SitesEndpointSite, connectedSiteIds: number[] ): SyncSupport {
Expand All @@ -30,17 +33,18 @@ export function getSyncSupport( site: SitesEndpointSite, connectedSiteIds: numbe
if ( ! site.capabilities?.manage_options ) {
return 'missing-permissions';
}
if ( isJetpackSite( site ) ) {
Copy link
Contributor Author

@nightnei nightnei Jan 7, 2026

Choose a reason for hiding this comment

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

Why jetpack site is unsupported?
Jetpack is required. So I renamed to isUnsupported.

Copy link
Contributor

@ivan-ottinger ivan-ottinger Jan 8, 2026

Choose a reason for hiding this comment

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

Similar as in my previous comment:

As far as I remember, we used this in the past to filter out self-hosted sites that are not hosted by us (via WPCOM and Pressable).

In that logic, "Jetpack site" referred to "self-hosted WordPress site".

if ( isUnsupported( site ) && ! isPressableSite( site ) ) {
return 'unsupported';
}
if ( ! hasSupportedPlan( site ) && ! isPressableSite( site ) ) {
Copy link
Contributor Author

@nightnei nightnei Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

In this condition we keep isPressableSite here, outside the hasSupportedPlan and imo it's good idea, to be clear at the upper level that the plan is about only dotcom, and it's not about Pressable.
So I added the same approach to two other cases (isUnsupported and needsTransfer).
Also, it's for consistency.

return 'needs-upgrade';
}
if ( needsTransfer( site ) ) {
if ( needsTransfer( site ) && ! isPressableSite( site ) ) {
return 'needs-transfer';
}
if ( connectedSiteIds.some( ( id ) => id === site.ID ) ) {
return 'already-connected';
}

return 'syncable';
}