Add the ability to copy an existing site - #2107
Conversation
📊 Performance Test ResultsComparing 60ddd15 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
| setAddingSiteIds( ( prev ) => { | ||
| prev.push( newSite.id ); | ||
| return prev; | ||
| } ); |
There was a problem hiding this comment.
| setAddingSiteIds( ( prev ) => { | |
| prev.push( newSite.id ); | |
| return prev; | |
| } ); | |
| setAddingSiteIds( ( prev ) => [ ...prev, newSite.id ] ); |
| <MenuItem | ||
| aria-disabled={ isCopyDisabled } | ||
| onClick={ () => { | ||
| if ( isCopyDisabled || ! selectedSite ) { |
There was a problem hiding this comment.
isCopyDisabled is defined as ! selectedSite (line 15), so this condition checks the same thing twice
There was a problem hiding this comment.
Unrelated to this PR, but I noticed the DeleteSite component has a similar issue.
It would be a nice time to address it in my opinion.
| import { moreVertical } from '@wordpress/icons'; | ||
| import { useI18n } from '@wordpress/react-i18n'; | ||
| import { PropsWithChildren } from 'react'; | ||
| import CopySite from 'src/components/copy-site'; |
There was a problem hiding this comment.
Could we move CopySite and DeleteSite to src/modules/site-settings.
AFAIK these aren't being used anywhere else and src/components should be used for generic, reusable components.
| fs.mkdirSync( newPath, { recursive: true } ); | ||
| } | ||
|
|
||
| if ( ! ( await isEmptyDir( newPath ) ) ) { |
There was a problem hiding this comment.
This is a good validation, but it should happen in the Copy Site form, preventing users from submitting an operation we know will fail.
The use-add-site hook already has similar checks that validate directory when the path is selected. Extend this pattern to the Copy Site flow to provide immediate feedback.
Keep the backend validation as a safety net.
| sendProgress( 'preparing', __( 'Preparing to copy site...' ), 0 ); | ||
|
|
||
| const wasSourceRunning = sourceSite.details.running; | ||
| if ( wasSourceRunning ) { |
There was a problem hiding this comment.
tempServer and newServer are both created with identical parameters (details and targetWpVersion). I think we can create the server once at the beginning and reuse it
| await fsPromises.mkdir( nodePath.join( wpContentDest, 'themes' ), { recursive: true } ); | ||
| } | ||
|
|
||
| if ( copyOptions.uploads ) { |
There was a problem hiding this comment.
To reduce duplication let's extract this to a helper function for example copyWpContentDirectory.
We could then do something like:
const directories = [
{ name: 'plugins', enabled: copyOptions.plugins, step: 'copying-plugins', percentage: 30 },
{ name: 'themes', enabled: copyOptions.themes, step: 'copying-themes', percentage: 50 },
{ name: 'uploads', enabled: copyOptions.uploads, step: 'copying-uploads', percentage: 60 },
...
] as const;
for ( const dir of directories ) {
await copyWpContentDirectory( ... );
}
| await installSqliteIntegration( newPath ); | ||
| } | ||
|
|
||
| const newServer = SiteServer.create( details, { |
There was a problem hiding this comment.
I think we could do this early on in the process and avoid creating the tempServer on the needsDifferentWpVersion check.
|
|
||
| sendProgress( 'finalizing', __( 'Finalizing copy...' ), 90 ); | ||
|
|
||
| const parentWindow = BrowserWindow.fromWebContents( event.sender ); |
There was a problem hiding this comment.
We could probably only get the parentWindow once at the start and re-use here and in the sendProgress function
|
Thanks for the feedback @bcotrim 🙏 I'll pick this up after the holiday break and implement these changes. I'll also simplify this to just copying a full site, and we can consider implementing more granular options in future iterations. |
|
I set this PR as draft until is ready for another review. |
|
Hello @ndiego, are you planning to complete this task? If not, I can take over and bring it to completion so you can concentrate on more important matters. |
|
I'll be happy to bring it over the finish line next week. 👌 |
|
Closing this PR in favor of a simpler alternative, see pfHvTO-Nd-p2#comment-721 |
|
New PR #2528 |
Related issues
Proposed Changes
This PR adds the ability to copy (clone, duplicate, etc.) an existing Studio site from both the context menu and from the "More options" menu on the Settings tab. Users have granular control over what gets copied (database, plugins, themes, uploads). Users can also configure different PHP/WordPress versions (and all the other normal settings) for the copied site.
Features
Implementation
Frontend:
copy-site.tsx(menu trigger) andmodules/add-site/components/copy-site.tsx(form)useCloneSite()hook for managing clone stateuseSiteDetailshook withcopySite()method/copyroutecopySiteProgressIPC eventsBackend (
copySitehandler in ipc-handlers.ts:308-565):mu-plugins(SQLite integration)plugins/,themes/,uploads/,database/based on optionsTelemetry: Added
SITE_COPIEDmetricHow Copy Site Works
Key features:
Testing Instructions
Pre-merge Checklist