Preview Sites: Avoid removing snapshots when switching between users #937

ivan-ottinger · 2025-02-14T10:13:21Z

Related issues

Fixes https://github.com/Automattic/dotcom-forge/issues/7516.

Proposed Changes

add userId to the snapshot object as the new preview site is created
automatically add current userId to all snapshots that are missing when appdata is getting loaded
add filter that will ensure only snapshots linked with the logged-in user get loaded on the Previews tab
remove clearFloatingSnapshots that is no longer needed

Testing Instructions

To test the changes in this PR, we will need two WPCOM accounts.

While logged-in to one of your WPCOM account in Studio, review your current preview sites.
Create a new preview site.
Take a look into appdata-v1.json. The newly created preview site should have a new userId value that equals to your WPCOM user ID:

Log out from your WPCOM account in Studio and log in to your another WPCOM account.
When you review the existing preview sites in Studio, the preview site that you created in the step 2 should not load in the Preview tab of the related local site.
Create another preview site while still logged-in to your second WPCOM account.
Take a look into appdata-v1.json. The newly created preview site should have a new userId value - but this time it should equal to your second WPCOM user ID (that you are currently logged into).
Log out from your second WPCOM account in Studio and log in to your first WPCOM account.
When you review the existing preview sites in Studio, you should see the preview site that you have created in the step 2. The preview site that you created with your second account in the step 6 should not load in the Preview tab of the related local site.
If you manually remove userId value from any of your existing preview sites in the appdata-v1.json, the value should get added automatically on the next appdata-v1.json load, e.g. when you restart the app. The userId of a currently-logged-in user will be used.
You can try other cases / variations as well.

The snapshot sequence number should be calculated for each user and site separately.

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

src/hooks/use-archive-site.ts

…-in user

katinthehatsite · 2025-02-14T15:20:13Z

The switching between accounts worked for me! I noticed one other small thing - if I start creating a site with the user account A and then switch to user account B, I would still see the loading state. I have not investigated in detail though, it might be a different issue. I can give it another test on Monday.

ivan-ottinger · 2025-02-14T16:52:00Z

The switching between accounts worked for me! I noticed one other small thing - if I start creating a site with the user account A and then switch to user account B, I would still see the loading state. I have not investigated in detail though, it might be a different issue. I can give it another test on Monday.

Great observation!

I can see why this is happening.

When the first user initiates the preview site creation and then quickly logs out and then logs in with another user before the preview site creation is finished, the progress bar will still display itself until the site is created.

This is because the isUploading gets its value from isUploadingSiteId callback that is tied with the local site ID instead of the snapshot being created.

I would say this edge-case is expected behavior because when the preview site is being created isUploading should return true for any user that is logged in at the moment - since we want to block creating new preview sites at that time. If the user switches their accounts so quickly before the preview site creation is finished, it should be clear that the process is still finishing - since it is tied to that same local site.

Let's also see what Antonio thinks about this as well.

katinthehatsite · 2025-02-17T09:18:21Z

I would say this edge-case is expected behavior because when the preview site is being created isUploading should return true for any user that is logged in at the moment - since we want to block creating new preview sites at that time. If the user switches their accounts so quickly before the preview site creation is finished, it should be clear that the process is still finishing - since it is tied to that same local site.

Yeah, makes sense. Also, I do not think the user would be switching when the process is running as this would not be a logical user behaviour so it would indeed be more of an edge case 👍

sejas

@ivan-ottinger , thanks for creating the PR. It works as expected and now I can see the preview sites for my user.
The expired sites are not automatically removed.
The sequence number takes into account the userId ✨ .

My only concern is something I suggested in the issue.

If snapshots don't have userId because were created before this PR, then I think it's ok to include them in the list of previews, as they'll eventually expire.

After testing the PR, I think it would be better if we assign the current userId to old snapshots in appData when Studio starts. What do you think?

sejas · 2025-02-17T14:46:04Z

src/hooks/use-archive-site.ts

@@ -158,6 +164,7 @@ export function useArchiveSite() {
 							nextSequence
 						),
 						sequence: nextSequence,
+						userId: user.id ?? undefined,


Let's make sure user.id always exists when archiving a site.

Suggested change

userId: user.id ?? undefined,

userId: user.id,

Related to #937 (comment).

sejas · 2025-02-17T14:48:33Z

src/hooks/use-archive-site.ts

@@ -74,7 +80,7 @@ export function useArchiveSite() {
 	const errorMessages = useArchiveErrorMessages();
 	const archiveSite = useCallback(
 		async ( siteId: string ) => {
-			if ( ! client ) {
+			if ( ! client || ! user ) {


Let's check if user?.id has a value and if not, we can display a generic error to the user, suggesting to re-authenticate.

Suggested change

if ( ! client || ! user ) {

if ( ! client ) {

Nice idea! Thanks! What do you think about a change like this? ff18ea9

src/hooks/use-snapshots.tsx

ivan-ottinger · 2025-02-17T16:52:49Z

Thank you for your reviews Kat and Antonio!

After testing the PR, I think it would be better if we assign the current userId to old snapshots in appData when Studio starts. What do you think?

Sounds good. 👍🏼 I will make the change tomorrow, alongside the other changes you have suggested. 🙂

…more

ivan-ottinger · 2025-02-18T12:38:36Z

After testing the PR, I think it would be better if we assign the current userId to old snapshots in appData when Studio starts. What do you think?

I have made the change in 5ff1f6f and 68855c3. When the appdata loads, it will populate the userId value for all snapshots that are missing it.

I have also removed checks that accepted snapshots without userId: 9b31ef9.

The PR is now ready for another review round. 🙂

sejas

@ivan-ottinger , great work!
I've tested it and I confirm that now the preview sites appear under the user who created it.
We've removed clearFloatingSnapshots, so no preview sites are removed from appdata unless the user delete the preview site or they expire and the user clears the row.
For former demo sites without userId assigned we run a function when Studio starts that populates that value with the current authenticated user.

sejas · 2025-02-18T12:56:52Z

src/storage/user-data.ts

+function populateSnapshotUserIds( data: UserData ): void {
+	const userId = data.authToken?.id;
+
+	if ( userId && data.snapshots ) {
+		data.snapshots = data.snapshots.map( ( snapshot ) => {
+			if ( ! snapshot.userId ) {
+				return { ...snapshot, userId };
+			}
+			return snapshot;
+		} );
+	}
+}


We are adding this function that assigns a userId to pre-existing snapshots so they appear to in the previews tab.
We can consider removing this code after a few weeks/months as those preview sites would expire.
Before this PR, if a user logged out the snapshots disappeared from Studio.

→ I have created a new task so we don't forget about the removal: https://github.com/Automattic/dotcom-forge/issues/10484.

Wojtek suggested that we could rename the function and add a little bit of context instead of leaving the task hanging, so I updated the task and created related PR: #954.

sejas · 2025-02-18T12:57:14Z

src/storage/user-data.ts

@@ -55,6 +68,8 @@ export async function loadUserData(): Promise< UserData > {
 		try {
 			const parsed = JSON.parse( asString );
 			const data = fromDiskFormat( parsed );
+
+			populateSnapshotUserIds( data );


Thanks for isolating the code into this function. ✨

My pleasure! 🙂

ivan-ottinger added 5 commits February 14, 2025 10:56

Add userId to snapshot object

e4ca9f3

Avoid clearing snapshots

5d5e47a

Filter out snapshots that do not belong to logged-in user

fb5703a

Fix test

98bbd07

Handle userId as number instead of string

2957ba0

ivan-ottinger self-assigned this Feb 14, 2025

Fall back userId to undefined if user.id is not available

d500736

ivan-ottinger requested a review from a team February 14, 2025 10:47

katinthehatsite reviewed Feb 14, 2025

View reviewed changes

src/hooks/use-archive-site.ts Outdated Show resolved Hide resolved

Calculate next snapshot sequence number based on the currently logged…

ea79d11

…-in user

katinthehatsite approved these changes Feb 17, 2025

View reviewed changes

sejas reviewed Feb 17, 2025

View reviewed changes

ivan-ottinger added 6 commits February 18, 2025 12:59

Display error message if client or user?.id are not available

ff18ea9

Add userId to all snapshots that are missing it

5ff1f6f

Remove definition of clearFloatingSnapshots as it is not needed any…

773722c

…more

Create separate populateSnapshotUserIds function

68855c3

Remove unnecessary getIpcApi import

8553342

Consider all snapshots have their userId value

9b31ef9

sejas approved these changes Feb 18, 2025

View reviewed changes

sejas merged commit 77e11a2 into trunk Feb 18, 2025
10 checks passed

sejas deleted the update/keep-snapshots-between-users branch February 18, 2025 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preview Sites: Avoid removing snapshots when switching between users #937

Preview Sites: Avoid removing snapshots when switching between users #937

ivan-ottinger commented Feb 14, 2025 •

edited

Loading

katinthehatsite commented Feb 14, 2025

ivan-ottinger commented Feb 14, 2025

katinthehatsite commented Feb 17, 2025

sejas left a comment

sejas Feb 17, 2025

ivan-ottinger Feb 18, 2025

sejas Feb 17, 2025

ivan-ottinger Feb 18, 2025

ivan-ottinger commented Feb 17, 2025

ivan-ottinger commented Feb 18, 2025

sejas left a comment •

edited

Loading

sejas Feb 18, 2025

ivan-ottinger Feb 18, 2025

ivan-ottinger Feb 19, 2025

sejas Feb 18, 2025

ivan-ottinger Feb 18, 2025

Preview Sites: Avoid removing snapshots when switching between users #937

Preview Sites: Avoid removing snapshots when switching between users #937

Conversation

ivan-ottinger commented Feb 14, 2025 • edited Loading

Related issues

Proposed Changes

Testing Instructions

Pre-merge Checklist

katinthehatsite commented Feb 14, 2025

ivan-ottinger commented Feb 14, 2025

katinthehatsite commented Feb 17, 2025

sejas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-ottinger commented Feb 17, 2025

ivan-ottinger commented Feb 18, 2025

sejas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-ottinger commented Feb 14, 2025 •

edited

Loading

sejas left a comment •

edited

Loading