Skip to content

Show loading screen while fetching sites in Pull an existing site modal - #2237

Merged
epeicher merged 2 commits into
trunkfrom
stu-1125-add-a-loading-state-to-the-pull-an-existing-site-modal
Dec 12, 2025
Merged

Show loading screen while fetching sites in Pull an existing site modal#2237
epeicher merged 2 commits into
trunkfrom
stu-1125-add-a-loading-state-to-the-pull-an-existing-site-modal

Conversation

@epeicher

@epeicher epeicher commented Dec 11, 2025

Copy link
Copy Markdown
Contributor

Related issues

Proposed Changes

  • Show the original modal while fetching the sites with the Loading… message
CleanShot.2025-12-11.at.14.37.39.mp4

Testing Instructions

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@epeicher epeicher self-assigned this Dec 11, 2025
@epeicher
epeicher requested a review from a team December 11, 2025 13:38
@github-actions

Copy link
Copy Markdown
Contributor

📊 Performance Test Results

Comparing 046d1df vs trunk

site-editor

Metric trunk 046d1df Diff Change
load 11114.00 ms 10195.00 ms -919.00 ms 🟢 -8.3%

site-startup

Metric trunk 046d1df Diff Change
siteCreation 22552.00 ms 18586.00 ms -3966.00 ms 🟢 -17.6%
siteStartup 9026.00 ms 7020.00 ms -2006.00 ms 🟢 -22.2%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change

@katinthehatsite katinthehatsite left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this, this worked as expected for me 👍

@ivan-ottinger ivan-ottinger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix, Roberto! The loading message is displaying as expected and the experience is more clear now. 👍🏼

I have noticed one issue - the scrollbars are displayed on the side like this:

Some local sites already exist:

CleanShot.2025-12-11.at.17.07.59.mp4

No local sites:

CleanShot.2025-12-11.at.17.12.29.mp4
@epeicher

Copy link
Copy Markdown
Contributor Author

Thanks @katinthehatsite and @ivan-ottinger for your reviews!

I have noticed one issue - the scrollbars are displayed on the side like this:

Well spotted! That was an existing issue, but I have fixed it as part of f5594d4. Now the list of sites is no longer rendered while sites are loading. I have also refactored the component a bit for better readability.

@epeicher
epeicher merged commit ebcc518 into trunk Dec 12, 2025
8 of 9 checks passed
@epeicher
epeicher deleted the stu-1125-add-a-loading-state-to-the-pull-an-existing-site-modal branch December 12, 2025 09:50
@ivan-ottinger

Copy link
Copy Markdown
Contributor

Well spotted! That was an existing issue, but I have fixed it as part of f5594d4. Now the list of sites is no longer rendered while sites are loading. I have also refactored the component a bit for better readability.

Thank you for addressing that, Roberto! I can confirm the loading is now working correctly! 👌🏼

@sejas sejas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for bringing back the loading state.
I confirm it works great. I also tested it with a user without no sites, where it appears the loading state and then switches to the Purchase CTA.
For regular users, the loading state only takes a fraction to load, which is great.

Image
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants