Studio: Refactor studio preview create to adjust new site logic#2148
Conversation
📊 Performance Test ResultsComparing 72ea508 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
fredrikekelund
left a comment
There was a problem hiding this comment.
Such a relief to clean this upp! I left a couple of comments. The most substantial one relates to the user-data-updated event listener in src/hooks/use-site-details.tsx.
cli/commands/preview/create.ts
Outdated
| @@ -21,6 +21,7 @@ export async function runCommand( siteFolder: string ): Promise< void > { | |||
| try { | |||
| logger.reportStart( LoggerAction.VALIDATE, __( 'Validating…' ) ); | |||
| validateSiteFolder( siteFolder ); | |||
There was a problem hiding this comment.
| validateSiteFolder( siteFolder ); |
I think this can be removed. No need to validate the directory type when creating a site if we also use getSiteByFolder to ensure the current directory is added to Studio.
There was a problem hiding this comment.
Let's remove the validateSiteFolder mocking from here, too.
There was a problem hiding this comment.
We should remove the validateSiteFolder call in runCommand here as well and replace it with a getSiteByFolder call.
There was a problem hiding this comment.
Same thing here about removing the validateSiteFolder mocking.
src/hooks/use-site-details.tsx
Outdated
| @@ -427,7 +426,7 @@ export function SiteDetailsProvider( { children }: SiteDetailsProviderProps ) { | |||
|
|
|||
| useEffect( () => { | |||
| const unsubscribe = window.ipcListener.subscribe( 'user-data-updated', async ( _, payload ) => { | |||
There was a problem hiding this comment.
Could we remove this event listener altogether now..? This might be a useful starting point for picking up latestCliPid changes, but all the logic around addingSiteIds seems to ultimately relate to newSites, which makes me think that we could clean up this file further.
There was a problem hiding this comment.
I think it is related to the add site workflow from Studio, not the newSites workflow.
Since we will need to revist this code when we work on having Studio depending on Studio CLI for site management, I suggest adjusting this file at that time.
What do you think?
There was a problem hiding this comment.
This was originally implemented by you in #1272 (d38ad87 in response to #1272 (review)). @ivan-ottinger then updated this listener in #1710 to fix a bug where sites would disappear from the sidebar while being created.
The current logic has no purpose, really. If I attempt to summarize it, this listener will:
- Trigger any time the appdata file changes
- Load the list of sites from appdata, instantiating each of them as
SiteServer - Proceed if
appdata.sitesis truthy (which it always is, because it's an array) - If there's no site currently being added by Studio, return early
- If there IS a site currently being added to Studio, recreate the
datastate by adding the temporary site to the list of sites from appdata
We should remove this.
7afdc43 to
14a6e98
Compare
14a6e98 to
a1d9e92
Compare
fredrikekelund
left a comment
There was a problem hiding this comment.
Thanks for the changes, @bcotrim, this looks good now 👍
I identified two remaining issues: that we were still using validateSiteFolder in the preview list command, and that the user-data-updated should be removed after all. Since I was planning to land this today (while you're AFK), I went ahead and pushed fixes for both of those.
| useEffect( () => { | ||
| const unsubscribe = window.ipcListener.subscribe( 'user-data-updated', async ( _, payload ) => { | ||
| if ( ! fastDeepEqual( payload.newSites, payload.sites ) ) { | ||
| const updatedSites = await getIpcApi().getSiteDetails(); | ||
| setData( ( prevData ) => { | ||
| const tempSite = prevData.find( ( site ) => addingSiteIds.includes( site.id ) ); | ||
|
|
||
| if ( ! tempSite ) { | ||
| return updatedSites; | ||
| } | ||
|
|
||
| const tempSiteExists = updatedSites.some( ( site ) => site.id === tempSite.id ); | ||
|
|
||
| if ( ! tempSiteExists ) { | ||
| return sortSites( [ ...updatedSites, tempSite ] ); | ||
| } | ||
|
|
||
| return updatedSites; | ||
| } ); | ||
| } | ||
| } ); | ||
|
|
||
| return () => { | ||
| unsubscribe(); | ||
| }; | ||
| }, [ addingSiteIds ] ); |
There was a problem hiding this comment.
I think this listener is necessary. The site creation in Studio app is currently broken without it.
Related issues
Proposed Changes
studio preview createnow requires the target folder to be an existing Studio site, failing with a helpful error if notnewSitesconcept from app and cli entirelyTesting Instructions
npm startnode dist/cli/main.js preview --path <new site folder>studio preview create <existing-site-folder>works correctly for existing Studio sitesPre-merge Checklist