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

fix(sync): refetch sites on opening popup #654

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

nightnei
Copy link
Contributor

@nightnei nightnei commented Nov 11, 2024

Related issues

Proposed Changes

Previously opening Sync popup didn't trigger refetching of sites. So if a user opened "Sync" tab, then created wordpress.com site and then clicked "Connect" button in Sync tab - they didn't see recently created website, so it was necessary to reload Studio.
With this PR we are refetching sites each time the popup is opening .

Testing Instructions

  1. Run Studio with STUDIO_SITE_SYNC=true npm start
  2. Select any site in left sidebar and open Sync tab
  3. Go to wordpress.com and purchase a website
  4. Come back to Studio and click on "Connect site"
  5. Assert that you see just created site in the list (previously it would not be there)
    Actually, I think that now the code looks even more straightforward, w/o extra props drilling and handleConnect is now isolated inside the dialog. Moreover, we removed 30% of lines - +29 −45

@nightnei nightnei requested a review from a team November 12, 2024 13:35
@nightnei nightnei self-assigned this Nov 12, 2024
@katinthehatsite
Copy link
Contributor

katinthehatsite commented Nov 13, 2024

I have not looked at the code yet but it seems that I have encountered a small edge case. This is how I got to it:

  • Create a site on WPcom and make it Atomic
  • Open a modal to Connect a site
  • Observe that the site is present in Studio modal and is available to connect. KEEP THE MODAL OPEN along with the app being open
  • Go back to WP.com
  • Create a staging site from the Atomic site
  • Focus on the app again where the modal is open
  • Observe that the site is still showing as production and the staging one is not displaying right by
  • In that state, try to connect the site in question
  • Observe that it is not displayed under the Sync tab
  • Observe when you open the modal with the Connect button again, the site in question shows as already connected
  • If you hit cmd + r, the app refreshes and the site starts displaying under the Sync tab:
Screenshot 2024-11-13 at 8 52 53 AM

If the testing steps are not clear, I am happy to record a screencast with how I ended up with this!

There is probably not a straightforward fix but I was checking network requests and it seems to me that the API requests from the Welcome Component work in a way that could solve the issue for this case. When I focused on the app with the modal being open after creating a staging site on WPCOM, I saw that we are making requests for Welcome Component but not list of sites. What do you think about possibly implementing a similar strategy?

It also seems that this PR addresses the issue specifically when the modal is opening so happy to open another issue to address this case.

@katinthehatsite
Copy link
Contributor

Actually, it seems that there is now an issue with updating the state on the sync tab when connecting a site (see below):

Screen.Recording.2024-11-13.at.9.07.43.AM.mov

@nightnei nightnei force-pushed the nightnei/refetchSyncSitesOnOpeningTheDialog branch from fb4ba08 to 23bcbc6 Compare November 13, 2024 11:05
@nightnei
Copy link
Contributor Author

@katinthehatsite fixed

Copy link

@stephanethomas stephanethomas left a comment

Choose a reason for hiding this comment

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

The code looks good to me, and I can confirm that it also works in all the cases I tested except the one Kateryna mentioned. I'm going to approve those changes anyway, as it's already an improvement but maybe we should look at fixing that edge case in another pull request?

@nightnei
Copy link
Contributor Author

nightnei commented Nov 14, 2024

@stephanethomas - I will fix menthioned bug by @katinthehatsite separately, it's anotehr issue with up-to-dating data in Studio.
I have follow up PR for url and for syncSupport properties. After it I will fix staging sites, when I figure out details, how we work with them.

@nightnei nightnei merged commit 9fd8e33 into trunk Nov 14, 2024
15 checks passed
@nightnei nightnei deleted the nightnei/refetchSyncSitesOnOpeningTheDialog branch November 14, 2024 12:56
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