-
Notifications
You must be signed in to change notification settings - Fork 50
Fix connected sites when logging in #2083
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 connected sites when logging in #2083
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where connected sites were not displayed after logging in. The fix adds an event listener that automatically reloads connected sites when user authentication data is updated.
- Adds subscription to
user-data-updatedevent to detect login state changes - Dispatches
loadAllConnectedSites()when a user is logged in
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| window.ipcListener.subscribe( 'user-data-updated', async ( _, userData ) => { | ||
| const currentUserId = userData.authToken?.id; | ||
|
|
||
| if ( currentUserId ) { | ||
| void store.dispatch( loadAllConnectedSites() ); | ||
| } |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subscription is created at module load time but never unsubscribed, which could lead to memory leaks if the module is ever reloaded. While examining similar patterns in new-sites-slice.ts and snapshot-slice.ts, I found they also follow this pattern, which suggests this is an intentional design choice. However, unlike those slices which check additional conditions (e.g., payload.newSites?.length in new-sites-slice), this implementation only checks for currentUserId existence. Consider whether the subscription should fire on every user data update (even when just refreshing the same user's token) or only when the user actually changes from logged out to logged in. If this causes excessive reloading, you may want to track the previous user ID and only dispatch when transitioning from no user to a logged-in user.
| window.ipcListener.subscribe( 'user-data-updated', async ( _, userData ) => { | |
| const currentUserId = userData.authToken?.id; | |
| if ( currentUserId ) { | |
| void store.dispatch( loadAllConnectedSites() ); | |
| } | |
| // Track the previous user ID to avoid unnecessary reloads | |
| let previousUserId: string | undefined; | |
| const unsubscribeUserDataUpdated = window.ipcListener.subscribe( 'user-data-updated', async ( _, userData ) => { | |
| const currentUserId = userData.authToken?.id; | |
| // Only dispatch if transitioning from no user to a logged-in user, or if the user ID has changed | |
| if ( currentUserId && currentUserId !== previousUserId ) { | |
| void store.dispatch( loadAllConnectedSites() ); | |
| } | |
| previousUserId = currentUserId; |
📊 Performance Test ResultsComparing 505d826 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
katinthehatsite
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so bizarre, I was able to reproduce this on trunk.
The fix worked for me but perhaps we can consider the suggestion from Copilot.
Yes, I already considered that, and discarded it because it seemed unnecessary, but I am going to reconsider to avoid excessive reloadings as suggested |
…dio-fix-connected-sites-when-logging-in
nightnei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and I confirm that in trunk we had the issue and it's fixed with the changes 👍
|
I think we should take a different approach here. The user config watcher was implemented to notify the Studio app of changes made by the CLI. The Studio app already has all state in memory, so unless we are truly looking for changes made by another program (the CLI) to the file on disk, resorting to the user config watcher is an indication we need to rethink the state structure, IMO. The ideal solution here would be to implement |
Thanks @fredrikekelund for the additional context. The changes followed similar patterns used by other components like new-sites and snapshots, but I didn't realize they had some other reasons for that. Your approach make sense to me, I will review your changes with interest when they’re ready 😄 |
Related issues
Proposed Changes
user-data-updatedevent.Testing Instructions
npm startcmd + rif you are in dev modePre-merge Checklist