Add site to Studio when creating preview from CLI by bcotrim · Pull Request #1272 · Automattic/studio

bcotrim · 2025-04-25T12:42:05Z

Related issues

Fixes STU-395

Proposed Changes

Added newSites field to user data schema to track sites pending creation
Implemented handling for the case when a preview is created for a site that doesn't exist in Studio
Created a Redux slice to manage new site processing
Added IPC handlers and listeners to automatically create sites when detected
Updated relevant tests to cover new functionality

Testing Instructions

Run npm run start and create a new site in Studio
Go to settings, copy the path of the site and delete it from Studio with the option to keep files in the system
Run npm run cli:build
Run node dist/cli/main.js preview create <STUDIO SITE FOLDER>
Wait for preview creation success
Go back to Studio
There should be a new site, with the name = pathname
That site should have the newly created preview

Pre-merge Checklist

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

fredrikekelund

I haven't tested the changes yet, but judging by the code, this looks like a solid solution 👍

fredrikekelund · 2025-04-25T12:44:55Z

src/stores/new-sites-slice.ts

+	const state = store.getState() as RootState & { newSites?: NewSitesState };
+
+	const newSites = payload.newSites;


Suggested change

const state = store.getState() as RootState & { newSites?: NewSitesState };

const newSites = payload.newSites;

const state = store.getState();

const newSites = payload.newSites;

No need for as

fredrikekelund · 2025-04-25T12:49:32Z

src/stores/new-sites-slice.ts

+	if ( ! state.newSites?.isProcessing && newSites && newSites.length > 0 ) {
+		store.dispatch( newSitesSlice.actions.setIsProcessing( true ) );
+
+		Promise.all(
+			newSites.map( async ( site: NewSiteDetails ) => {
+				try {
+					await getIpcApi().handleNewSite( site );
+				} catch ( error ) {
+					console.error(
+						`[New Sites Slice] Failed to create site for folder: ${ site.path }`,
+						error
+					);
+				}
+			} )
+		).finally( () => {
+			store.dispatch( newSitesSlice.actions.setIsProcessing( false ) );
+		} );
+	}


Suggested change

if ( ! state.newSites?.isProcessing && newSites && newSites.length > 0 ) {

store.dispatch( newSitesSlice.actions.setIsProcessing( true ) );

Promise.all(

newSites.map( async ( site: NewSiteDetails ) => {

try {

await getIpcApi().handleNewSite( site );

} catch ( error ) {

console.error(

`[New Sites Slice] Failed to create site for folder: ${ site.path }`,

error

);

}

} )

).finally( () => {

store.dispatch( newSitesSlice.actions.setIsProcessing( false ) );

} );

}

if ( ! state.newSites.isProcessing && newSites ) {

store.dispatch( handleNewSite( newSites ) );

}

I would make the event handler smaller and add a handleNewSite thunk like this:

const handleNewSite = createAsyncThunk( 'newSites/handleNewSite', ( sites: NewSiteDetails[] ) => { return Promise.all( sites.map( async ( site ) => { try { await getIpcApi().handleNewSite( site ); } catch ( error ) { console.error( `[New Sites Slice] Failed to create site for folder: ${ site.path }`, error ); } } ) ); } );

Lastly, we should update the isProcessing state depending on the thunk state:

extraReducers: ( builder ) => { builder .addCase( handleNewSite.pending, ( state ) => { state.isProcessing = true; } ) .addCase( handleNewSite.fulfilled, ( state ) => { state.isProcessing = false; } ) .addCase( handleNewSite.rejected, ( state ) => { state.isProcessing = false; } ); },

fredrikekelund · 2025-04-25T12:52:16Z

cli/lib/appdata.ts

-		throw new LoggerError( __( 'The specified folder is not added to Studio.' ) );
+		return null;


I guess this is ultimately subjective, but I would rather continue throwing an error from this function and instead wrap the call in saveSnapshotToAppdata in a try .. catch

fredrikekelund · 2025-04-25T12:54:14Z

cli/lib/appdata.ts

 	return site;
 }
+
+export function getNewSiteAppData( siteFolder: string ): z.infer< typeof siteSchema > {


Suggested change

export function getNewSiteAppData( siteFolder: string ): z.infer< typeof siteSchema > {

export function getNewSitePartial( siteFolder: string ) {

I feel like we could clarify the name here. I would also let TS automatically intuit the return type.

fredrikekelund · 2025-04-25T12:56:18Z

cli/lib/appdata.ts

+const newSiteSchema = z
+	.object( {
+		id: z.string(),
+		path: z.string(),
+	} )
+	.passthrough();
+
 const userDataSchema = z
 	.object( {
+		newSites: z.array( newSiteSchema ).optional(),


Suggested change

const newSiteSchema = z

.object( {

id: z.string(),

path: z.string(),

} )

.passthrough();

const userDataSchema = z

.object( {

newSites: z.array( newSiteSchema ).optional(),

const userDataSchema = z

.object( {

newSites: z.array( siteSchema ).optional(),

In my quick testing, it looked like we could use siteSchema here, too. Maybe the current code is a leftover from a previous iteration?

fredrikekelund

I've tested the changes now, and everything works well apart from the fact that the list of sites isn't automatically refreshed. Maybe we could export a refreshSitesList function from src/hooks/use-site-details.tsx to resolve that?

fredrikekelund

Tested again, and it works nicely with auto-refreshing the sites list 👍 Impressively small diff for a relatively complex change, too. Nice job!

It would be good to iron out the question about whether the isProcessing state is truly needed, but other than that, this LGTM.

fredrikekelund · 2025-04-25T14:10:16Z

src/ipc-types.d.ts

+type NewSiteDetails = {
+	id: string;
+	path: string;
+	name: string;
+};


Suggested change

type NewSiteDetails = {

id: string;

path: string;

name: string;

};

type NewSiteDetails = Pick< SiteDetails, 'id' | 'path' | 'name' >;

Nit, but this allows us to continue using a canonical type definition for the sites objects.

fredrikekelund · 2025-04-25T14:11:55Z

src/stores/new-sites-slice.ts

+	const state = store.getState();
+	const newSites = payload.newSites;
+
+	if ( ! state.newSites.isProcessing && newSites ) {


I'm curious, what would happen if we didn't have the ! state.newSites.isProcessing check here? It's not immediately clear to me why there might be multiple events in such short succession and why we shouldn't process all of them if that happens.

Since this event subscription handles all user data changes, and not only newSites ones, I think it is crucial to have some tracking mechanism to avoid site duplication. However I see your point, and issue, that a single flag would probably lead to some unwanted behaviors, like new sites not being picked up if some are currently processing, and staying pending, until another user data event (probably unrelated) gets triggered.

I've updated the implementation to track individual site IDs instead of a single processing flag. Now we filter out only sites already being processed while allowing new ones to proceed immediately.

Let me know what you think about this approach.

Makes sense. If a single user-data-updated event carried multiple newSites, then this logic will be required to avoid adding duplicate site entries.

The updated solution looks great to me 👍

fredrikekelund

LGTM 🚀

fredrikekelund · 2025-04-28T06:59:16Z

src/stores/new-sites-slice.ts

+	},
+} );
+
+export const { addProcessingSites, removeProcessingSites } = newSitesSlice.actions;


Suggested change

export const { addProcessingSites, removeProcessingSites } = newSitesSlice.actions;

export const newSitesActions = newSitesSlice.actions;

Nit, but it'd be nice to stay consistent with other slices and export the actions inside a "namespace".

fredrikekelund · 2025-04-28T06:59:49Z

src/stores/tests/new-sites-slice.test.ts

+			const action = {
+				type: 'newSites/addProcessingSites',
+				payload: [ 'site-1', 'site-2' ],
+			};


Suggested change

const action = {

type: 'newSites/addProcessingSites',

payload: [ 'site-1', 'site-2' ],

};

const action = newSitesActions.addProcessingSites( [ 'site-1', 'site-2' ] );

If we export the actions in a single namespace, we can create the action in this way.

* add site to studio when create preview from cli * pr comments changes * fix unit tests * fix lint * improve new sites tracking mechanism * address pr feedback

fredrikekelund reviewed Apr 25, 2025

View reviewed changes

bcotrim added 3 commits April 25, 2025 15:26

add site to studio when create preview from cli

ea2fcc1

pr comments changes

d38ad87

fix unit tests

2c8afdb

bcotrim force-pushed the stu-395-the-studio-preview-create-command-should-add-a-new-site-to branch from f13a37c to 2c8afdb Compare April 25, 2025 14:26

bcotrim requested a review from fredrikekelund April 25, 2025 14:27

fredrikekelund reviewed Apr 25, 2025

View reviewed changes

bcotrim marked this pull request as ready for review April 25, 2025 14:29

bcotrim added 2 commits April 25, 2025 15:30

fix lint

5e127e6

improve new sites tracking mechanism

fb2afbf

bcotrim changed the title ~~add site to studio when create preview from cli~~ Add site to Studio when creating preview from CLI Apr 25, 2025

fredrikekelund approved these changes Apr 28, 2025

View reviewed changes

address pr feedback

60772e9

bcotrim merged commit e9d504d into trunk Apr 28, 2025
8 checks passed

bcotrim deleted the stu-395-the-studio-preview-create-command-should-add-a-new-site-to branch April 28, 2025 07:44

fredrikekelund mentioned this pull request May 1, 2025

Make preview update --overwrite command support adding new sites #1308

Merged

1 task

fredrikekelund mentioned this pull request Dec 1, 2025

Studio: Refactor studio preview create to adjust new site logic #2148

Merged

1 task

		const state = store.getState() as RootState & { newSites?: NewSitesState };

		const newSites = payload.newSites;

		throw new LoggerError( __( 'The specified folder is not added to Studio.' ) );
		return null;

	export function getNewSiteAppData( siteFolder: string ): z.infer< typeof siteSchema > {
	export function getNewSitePartial( siteFolder: string ) {

	export const { addProcessingSites, removeProcessingSites } = newSitesSlice.actions;
	export const newSitesActions = newSitesSlice.actions;

Conversation

bcotrim commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

Pre-merge Checklist

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bcotrim commented Apr 25, 2025 •

edited

Loading