Refactor Studio to depend on CLI for preview site creation #1201

fredrikekelund · 2025-04-14T14:58:13Z

Related issues

Fixes https://linear.app/a8c/issue/STU-240

Proposed Changes

Add a Redux slice to hold state related to preview site CRUD operations.
Refactor the preview site creation functionality to depend on the CLI.
Add a getSnapshotUsage query to src/stores/wpcom-api.ts and update the UI to use this hook in the relevant places.
Remove the useArchiveSite hook.

Note

Snapshot state is now split between the new Redux reducer and the old SnapshotProvider. This means updating and deleting snapshots does not update the UI as expected. We will address this in separate PRs (potentially branching off this PR instead of merging this to trunk first).

Testing Instructions

Ensure that creating a preview site works as expected.
Ensure that the Preview sites counter in the settings modal works as expected.

Pre-merge Checklist

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

cli/commands/preview/create.ts

bcotrim

I don't really like the progress bar behavior for this scenario.
What happens on all my tests is that the bar progresses quickly until the last step, so it just gets stuck on this state for a while:

In my opinion, we could either use an indeterminate progress indicator (like a continuous progress bar) or display the same information we show on the CLI (Creating archive, Uploading archive, etc...) which seems far more valuable for our users compared to a stuck progress bar.

Code-wise I like this approach, looks clean and it's easy to read.
One minor comment would be to clear up the naming a bit. At least for me, the distinction between previews and snapshots seem confusing. Is there a benefit for keeping the 2, could we pick one?

Another thing on my mind is how we are currently cleaning up operations. Operations that fail, timeout, or are hanging for other reasons could potentially go on existing forever in our state, correct? I don't think this alone will be an issue, but it's something we should be mindful of, if this is going to be our approach for operations in Studio moving forward.

Is it possible to use a feature flag for this during the development?

bcotrim · 2025-04-15T13:08:13Z

cli/commands/preview/logger-actions.ts

@@ -0,0 +1,28 @@
+// Store the actions in a separate file to avoid Webpack issues when importing them in the Studio


At some point I believe it would be better for us to start building a common folder for types and functions that are used in both CLI and Studio. I don't think it will make any difference behavior wise, but will make it more intentional and easier to read.

Good idea 👍

fredrikekelund · 2025-04-15T14:55:23Z

Is it possible to use a feature flag for this during the development?

This PR touches so many parts of the code, so I was thinking it might be easier to branch off of this PR for the related tasks in https://linear.app/a8c/issue/STU-235

youknowriad · 2025-04-15T21:05:22Z

src/ipc-handlers.ts

@@ -17,12 +17,15 @@ import nodePath from 'path';
 import * as Sentry from '@sentry/electron/main';
 import { __, LocaleData, defaultI18n } from '@wordpress/i18n';
 import archiver from 'archiver';
+import { z } from 'zod';
+import { CreateLoggerAction } from 'cli/commands/preview/logger-actions';


If you're importing code from the "cli" app, why not import the command and run it as a function instead of a subprocess. I don't have a fully formed opinion at the moment but I think right now we're doing a bit of both which is kind of weird.

I suggested this approach of keeping the business logic squarely in the CLI when planning the project. It definitely adds complexity, but the upside is that it lets us avoid double bundling of PHP-WASM dependencies (once we get to moving that functionality from Studio to the CLI), which would increase the bundle size by ~700MB in the short term and ~400MB in the long term.

As for code sharing, the idea is to only share modules without side effects. @bcotrim suggested moving those to a common folder for added clarity, which I think is a good idea.

Sounds good to me ;)

fredrikekelund · 2025-04-16T08:07:51Z

progress bar

That's fair. I guess the current behavior in this PR would easily be perceived as a UX regression. I'll look into improving this.

clear up the naming

That's fair. We could probably stick with "snapshots", since that's what we use in the code already.

how we are currently cleaning up operations. Operations that fail, timeout, or are hanging for other reasons could potentially go on existing forever in our state, correct?

They could indeed. That's not unique for this architecture, though, even if I realize that I still need to add some error handling to this PR.

fredrikekelund · 2025-04-16T10:01:05Z

cli/lib/appdata.ts

-	name: z.string().optional(),
+	name: z.string(),
 	userId: z.number().optional(),
 } );

 const siteSchema = z
 	.object( {
 		id: z.string(),
 		path: z.string(),
-		name: z.string().optional(),
+		name: z.string(),


I don't believe name props are actually optional for sites or snapshots. I made this change to accommodate the reportKeyValuePair logging of the snapshot name.

fredrikekelund · 2025-04-16T10:01:32Z

cli/lib/snapshots.ts

-			return;
-		}
-		const snapshot: z.infer< typeof snapshotSchema > = {
+		const site = await getSiteFromFolder( siteFolder );


This change means that this function now throws if the site isn't found in the appdata file.

bcotrim

LGTM 👍
Code and UI are looking great!

nightnei

I was testing and:

Counter looks broken for me
Deleting also broken (maybe it's expected for this PR and will be solved in taht separate PR for handling deletion of previews, but wanted to flag in case if it's not expected)

@bcotrim coukd you please test it one more time, to understand is it my local issue or it's indeed exist.

Recording of the issues: https://drive.google.com/file/d/1xd71MGjY8bU0ZFdEcbzrRKjNqounppAp/view?usp=sharing

bcotrim · 2025-04-18T10:26:12Z

@nightnei I see that same issue but I assumed it was expected based on

Note

Snapshot state is now split between the new Redux reducer and the old SnapshotProvider. This means updating and deleting snapshots does not update the UI as expected. We will address this in separate PRs (potentially branching off this PR instead of merging this to trunk first).

If you reload the app after creating a preview site, the count is updated as expected.

nightnei · 2025-04-18T13:43:04Z

If you reload the app after creating a preview site, the count is updated as expected.

It wasn't menthioned in testing instructions, I see only "... works es expected". But it doesn't work as expected.

Also, here (p1744904341127939-slack-C04GESRBWKW) he menthioned "I opted for this multi-PR approach to not break the preview site functionality on trunk while still making the PR size relatively manageable.", so looks liek it should work well even after merging just this one PR, w/o other PRs.

Let's wait for Fredrik's response to clarify things.

UPDATE:
I tested with the help of "Refactor preview site deleting to depend on CLI #1230" (it's chain of PRs) - we indeed have bugs:

Delete previeew site - counter isn't updated
Delete all preview sites - doesn't have any effect to UI, but under teh hood looks liek everything works well, so necessary to fix UI handling.

nightnei · 2025-04-18T14:07:11Z

src/components/progress-bar.tsx

+		if ( value > animatedValue ) {
+			setAnimatedValue( value );
+		}
+		// eslint-disable-next-line react-hooks/exhaustive-deps


Let's add some description, otherwise anotehr A12N can make mistake with adding it animatedValue. And it will save time for A12S in the future with debugging to understand teh reason and logic.

nightnei · 2025-04-18T14:11:34Z

src/components/progress-bar.tsx

+
+	useEffect( () => {
+		const maximumAutoIncrementValue = maxValue * 0.95;
+
 		const interval = setInterval( () => {


It seems it should be setTimeout, since we run this effect every time when we update animatedValue.
Theoreticaly, it works even now, since we clear interval every time when we rerun the effect, but a safer and correct way would be to use setTimeout.
Right or am I missing something?

Good call 👍

nightnei · 2025-04-18T14:55:02Z

src/stores/snapshot-slice.ts

+				.filter( ( operation ) => operation.status === 'pending' )
+				.find( ( operation ) => operation.siteId === siteId ),


Suggested change

.filter( ( operation ) => operation.status === 'pending' )

.find( ( operation ) => operation.siteId === siteId ),

.filter( ( operation ) => operation.status === 'pending' && operation.siteId === siteId )

I see that you already did it in the follow up PR https://github.com/Automattic/studio/pull/1223/files#diff-e7f3116a80918052c6f2b0b4cc2e22b2f8dc46e44d97249c6d49522bc2bf3b2cR127-R131

nightnei · 2025-04-18T14:59:10Z

src/stores/snapshot-slice.ts

+	},
+	selectors: {
+		selectSnapshots: ( state ) => state.snapshots,
+		selectSnapshotsCount: ( state ) => state.snapshots.length,


We use it only in 1 place.
Also it looks a bit redundant to create extra selector for length if we already have selector for teh list.
Also, it even simlifies using it and readign code:

BEFORE: const allSnapshots = useRootSelector( snapshotSelectors.selectSnapshots ); const snapshotsCount = useRootSelector( snapshotSelectors.selectSnapshotsCount ); AFTER: const allSnapshots = useRootSelector( snapshotSelectors.selectSnapshots ); const snapshotsCount = allSnapshots.length; // But actually, I think we can even avoid creating this extra variable and use "allSnapshots.length" directly in code

Suggested change

selectSnapshotsCount: ( state ) => state.snapshots.length,

I agree. I actually removed this in #1230.

nightnei · 2025-04-18T15:11:37Z

At some point I believe it would be better for us to start building a common folder for types and functions that are used in both CLI and Studio.

It's great idea 👍
But also, when we proceed with it - necessary to take into account that we have 3 things: backend, cli and frontend. We already have mix of frontend and backend and one day I belive we should split it. So if we create kinda common folder, then necessary to think about more specific name and / or location, to avoid bounding all 3 things. Otherwise, it will be very messy.

bcotrim · 2025-04-18T15:50:29Z

At some point I believe it would be better for us to start building a common folder for types and functions that are used in both CLI and Studio.

It's great idea 👍 But also, when we proceed with it - necessary to take into account that we have 3 things: backend, cli and frontend. We already have mix of frontend and backend and one day I belive we should split it. So if we create kinda common folder, then necessary to think about more specific name and / or location, to avoid bounding all 3 things. Otherwise, it will be very messy.

We already merged a change with the initial version of that: #1222
Let me know if you have any feedback, and I can address it in a follow-up

bcotrim · 2025-04-18T18:28:00Z

src/modules/cli/lib/execute-command.ts

+	} );
+
+	child.on( 'exit', ( code: number | null ) => {
+		console.log( 'Preview site creation completed with code', code );


While reviewing #1223 I noticed that we will have a specific execute command for previews. It will likely make sense to remove this

Good point 👍 I've removed it in #1230

fredrikekelund · 2025-04-22T07:33:46Z

Delete previeew site - counter isn't updated

Good catch. We do actually invalidate the query correctly, but it looks like the deletion hasn't taken effect yet on the back-end when we do that, because the API still returns the old site count. Maybe we should delay the query invalidation to solve this…

Delete all preview sites - doesn't have any effect to UI, but under teh hood looks liek everything works well, so necessary to fix UI handling.

Good catch. Looking into this 👍

fredrikekelund · 2025-04-22T08:35:42Z

I've addressed your comments, @nightnei. You should probably re-review #1230, but this PR needs an approval before merging.

nightnei

LGTM 👍

* [WIP] Refactor Studio to depend on CLI for preview site creation * More changes * Fix * Use node's child_process module to fork CLI process * Tests * useGetSnapshotUsage * Build CLI before starting dev version of app * Tweaks * Fixes * preview ➡️ snapshot * Improved progress bar * Display notice when operation completes * Update zod * Remove useArchiveSite hook * Fix test * Modify exitCode in Logger::reportError * Set exit code on error and fix outputFormat parsing * Shorten * Fix outputFormat parsing * Clarify naming * Tweaks

[WIP] Refactor Studio to depend on CLI for preview site creation

e12452a

fredrikekelund requested a review from a team April 14, 2025 14:58

fredrikekelund self-assigned this Apr 14, 2025

fredrikekelund commented Apr 14, 2025

View reviewed changes

cli/commands/preview/create.ts Outdated Show resolved Hide resolved

fredrikekelund added 7 commits April 15, 2025 13:03

More changes

86440a0

Fix

28ea3dd

Use node's child_process module to fork CLI process

67ff103

Tests

d78606d

Merge branch 'trunk' into f26d/depend-on-cli-preview-site-creation

f57a538

useGetSnapshotUsage

476a587

Build CLI before starting dev version of app

fbb1aa9

fredrikekelund requested a review from youknowriad April 15, 2025 12:53

fredrikekelund changed the title ~~[WIP] Refactor Studio to depend on CLI for preview site creation~~ Refactor Studio to depend on CLI for preview site creation Apr 15, 2025

fredrikekelund marked this pull request as ready for review April 15, 2025 12:53

bcotrim reviewed Apr 15, 2025

View reviewed changes

youknowriad reviewed Apr 15, 2025

View reviewed changes

fredrikekelund added 3 commits April 16, 2025 09:38

Merge branch 'trunk' into f26d/depend-on-cli-preview-site-creation

2d0cf49

Tweaks

0a091f2

Fixes

f8134c5

fredrikekelund added 5 commits April 16, 2025 10:15

preview ➡️ snapshot

d91f3ac

Improved progress bar

f9c162d

Display notice when operation completes

0213f83

Update zod

b7e5811

Remove useArchiveSite hook

e34f4a7

fredrikekelund commented Apr 16, 2025

View reviewed changes

Fix test

0ba4f63

fredrikekelund requested a review from bcotrim April 16, 2025 10:03

bcotrim approved these changes Apr 16, 2025

View reviewed changes

fredrikekelund added 6 commits April 17, 2025 08:56

Modify exitCode in Logger::reportError

8ac1eb2

Merge branch 'trunk' into f26d/depend-on-cli-preview-site-creation

93f7650

Set exit code on error and fix outputFormat parsing

4d6657e

Shorten

8b8bc5e

Fix outputFormat parsing

6404aa3

Clarify naming

7fef103

fredrikekelund mentioned this pull request Apr 17, 2025

Add common folder structure & some adjustments #1222

Merged

1 task

fredrikekelund added 2 commits April 17, 2025 16:14

Merge branch 'trunk' into f26d/depend-on-cli-preview-site-creation

8559207

Merge branch 'trunk' into f26d/depend-on-cli-preview-site-creation

5f71f58

nightnei requested changes Apr 18, 2025

View reviewed changes

nightnei reviewed Apr 18, 2025

View reviewed changes

nightnei mentioned this pull request Apr 18, 2025

Refactor preview site deleting to depend on CLI #1230

Merged

1 task

bcotrim reviewed Apr 18, 2025

View reviewed changes

fredrikekelund added 2 commits April 22, 2025 08:59

Merge branch 'trunk' into f26d/depend-on-cli-preview-site-creation

a66f10e

Tweaks

4b961db

fredrikekelund requested a review from nightnei April 22, 2025 08:35

nightnei approved these changes Apr 22, 2025

View reviewed changes

fredrikekelund merged commit f93e22e into trunk Apr 22, 2025
8 checks passed

fredrikekelund deleted the f26d/depend-on-cli-preview-site-creation branch April 22, 2025 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Studio to depend on CLI for preview site creation #1201

Refactor Studio to depend on CLI for preview site creation #1201

fredrikekelund commented Apr 14, 2025 •

edited

Loading

bcotrim left a comment

bcotrim Apr 15, 2025

fredrikekelund Apr 16, 2025

fredrikekelund commented Apr 15, 2025

youknowriad Apr 15, 2025 •

edited

Loading

fredrikekelund Apr 16, 2025

fredrikekelund Apr 16, 2025

youknowriad Apr 16, 2025

fredrikekelund commented Apr 16, 2025

fredrikekelund Apr 16, 2025

fredrikekelund Apr 16, 2025

bcotrim left a comment

nightnei left a comment

bcotrim commented Apr 18, 2025

nightnei commented Apr 18, 2025 •

edited

Loading

nightnei Apr 18, 2025 •

edited

Loading

nightnei Apr 18, 2025 •

edited

Loading

fredrikekelund Apr 22, 2025

nightnei Apr 18, 2025 •

edited

Loading

nightnei Apr 18, 2025

nightnei Apr 18, 2025 •

edited

Loading

fredrikekelund Apr 22, 2025

nightnei commented Apr 18, 2025

bcotrim commented Apr 18, 2025

bcotrim Apr 18, 2025

fredrikekelund Apr 22, 2025

fredrikekelund commented Apr 22, 2025

fredrikekelund commented Apr 22, 2025

nightnei left a comment

		@@ -0,0 +1,28 @@
		// Store the actions in a separate file to avoid Webpack issues when importing them in the Studio

		.filter( ( operation ) => operation.status === 'pending' )
		.find( ( operation ) => operation.siteId === siteId ),

Refactor Studio to depend on CLI for preview site creation #1201

Refactor Studio to depend on CLI for preview site creation #1201

Conversation

fredrikekelund commented Apr 14, 2025 • edited Loading

Related issues

Proposed Changes

Testing Instructions

Pre-merge Checklist

bcotrim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekelund commented Apr 15, 2025

youknowriad Apr 15, 2025 • 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

fredrikekelund commented Apr 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcotrim left a comment

Choose a reason for hiding this comment

nightnei left a comment

Choose a reason for hiding this comment

bcotrim commented Apr 18, 2025

nightnei commented Apr 18, 2025 • edited Loading

nightnei Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

nightnei Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei commented Apr 18, 2025

bcotrim commented Apr 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekelund commented Apr 22, 2025

fredrikekelund commented Apr 22, 2025

nightnei left a comment

Choose a reason for hiding this comment

fredrikekelund commented Apr 14, 2025 •

edited

Loading

youknowriad Apr 15, 2025 •

edited

Loading

nightnei commented Apr 18, 2025 •

edited

Loading

nightnei Apr 18, 2025 •

edited

Loading

nightnei Apr 18, 2025 •

edited

Loading

nightnei Apr 18, 2025 •

edited

Loading

nightnei Apr 18, 2025 •

edited

Loading