Refactor Studio to depend on CLI for import/export#3129
Refactor Studio to depend on CLI for import/export#3129fredrikekelund merged 40 commits intotrunkfrom
Conversation
| function handleExportIpc( emitter: ImportExportEventEmitter ) { | ||
| emitter.on( ExportEvents.EXPORT_START, () => { | ||
| sendIpcEvent( [ ExportEvents.EXPORT_START, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.BACKUP_CREATE_START, () => { | ||
| sendIpcEvent( [ ExportEvents.BACKUP_CREATE_START, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.WP_CONTENT_EXPORT_START, () => { | ||
| sendIpcEvent( [ ExportEvents.WP_CONTENT_EXPORT_START, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.WP_CONTENT_EXPORT_COMPLETE, () => { | ||
| sendIpcEvent( [ ExportEvents.WP_CONTENT_EXPORT_COMPLETE, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.DATABASE_EXPORT_START, () => { | ||
| sendIpcEvent( [ ExportEvents.DATABASE_EXPORT_START, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.DATABASE_EXPORT_COMPLETE, () => { | ||
| sendIpcEvent( [ ExportEvents.DATABASE_EXPORT_COMPLETE, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.BACKUP_CREATE_PROGRESS, ( progressData ) => { | ||
| sendIpcEvent( [ ExportEvents.BACKUP_CREATE_PROGRESS, progressData ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.BACKUP_CREATE_COMPLETE, () => { | ||
| sendIpcEvent( [ ExportEvents.BACKUP_CREATE_COMPLETE, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.CONFIG_EXPORT_START, () => { | ||
| sendIpcEvent( [ ExportEvents.CONFIG_EXPORT_START, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.CONFIG_EXPORT_COMPLETE, () => { | ||
| sendIpcEvent( [ ExportEvents.CONFIG_EXPORT_COMPLETE, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.EXPORT_COMPLETE, () => { | ||
| sendIpcEvent( [ ExportEvents.EXPORT_COMPLETE, undefined ] ); | ||
| } ); | ||
| emitter.on( ExportEvents.EXPORT_ERROR, ( error ) => { | ||
| sendIpcEvent( [ ExportEvents.EXPORT_ERROR, error ] ); | ||
| } ); | ||
| } |
There was a problem hiding this comment.
This obviously repeats a lot of the handleExportEvents implementation, but iterating over ExportEvents in a type-safe way is not trivial. Unless I'm missing some brilliant solution to this, I think the best approach is to just accept the duplication and potentially do a more cohesive refactor in the future.
Same thing for the import command.
There was a problem hiding this comment.
This is a good place to start reviewing the implementation. This file defines IPC functions that Electron exposes to the renderer process. It triggers the CLI processes by calling executeExportCliCommand and executeImportCliCommand.
…1540-continued-studio-import-export-refactor
| "@wordpress/i18n": "^6.9.0", | ||
| "@wordpress/icons": "^11.4.0", | ||
| "@wordpress/react-i18n": "^4.41.0", | ||
| "archiver": "^7.0.1", |
There was a problem hiding this comment.
archiver was only used by the import/export implementation. I removed the dependency and the associated patch.
📊 Performance Test ResultsComparing c0ddf30 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
|
For the record, I'm looking into the E2E test that's failing on Windows. |
There was a problem hiding this comment.
This PR removed the last SQLite version management operations from apps/studio, so I took the opportunity to remove this file, too 👍
| if ( wasNotRunning && running ) { | ||
| void captureSiteThumbnail( siteId ); | ||
| await server.getThemeDetails(); | ||
| } |
There was a problem hiding this comment.
While going over the smoke testing suite, I noticed that thumbnails and theme details were failing to load when I created a new site by importing an archive. I don't believe that was because of my other changes in this PR. In any case, this change fixes that 👍
There was a problem hiding this comment.
A good chunk of this file became stale due to my other changes in this PR. I could have asked AI to refactor it, but I don't think that provides us with any real assurances, so I opted to just remove it. That's because these tests relied too heavily on mocks or tested things that are now assured by proper Typescript types.
|
For clarity, I've run this through multiple rounds of local review, and I've run the import/export and sync smoke tests against this PR. This PR is definitely ready for review now 👍 |
There was a problem hiding this comment.
Possible regression worth double-checking: cancelling a push/pull doesn't stop the CLI child process.
Repro: inflate a site's wp-content/uploads/ to a decent size, push, then hit Cancel mid-backup. UI shows "Cancelled" instantly, but ps | grep 'main.mjs export' shows the child still running and $TMPDIR/com.wordpress.studio//site_.tar.gz keeps growing until the CLI finishes on its own.
Can you replicate this issue?
|
Good catch, @bcotrim. I've pushed a fix that kills the CLI child process when a push operation is aborted 👍 |
bcotrim
left a comment
There was a problem hiding this comment.
LGTM 👍
Couldn't find any regressions during my tests.
Added a minor comment, but it's not something I would consider a blocker.
| 'No suitable backup handler found for the provided backup file', | ||
| ]; | ||
|
|
||
| function isExpectedImportError( error: unknown ): boolean { |
There was a problem hiding this comment.
Did you consider adding error codes to the payload? The substring matches in isExpectedImportError and the 'Error: absolute path: /' check are locale-fragile and library-fragile. If any parsing is necessary, the CLI should own it and send a structured code over IPC.
There was a problem hiding this comment.
That's a good point. This logic was there before, too, though. I'd consider this a good follow-up
Conflicts in apps/studio/src/hooks/use-import-export.tsx and the deletion of its test file: trunk's #3129 refactored the hook to depend on the CLI for import/export, removing Sentry, useSiteDetails, error formatting, and the BackupArchiveInfo path. Took trunk's simpler structure and re-applied the logout cleanup block (useAuth + useRootSelector + useRef-based pre-logout pull-active snapshot driving the setImportState/setExportState reset). Dropped the isError flag on ImportProgressState and the matching pullImportFailed workaround in sync-connected-sites.tsx: trunk's pull thunk now propagates importSite errors via pullStates.failed, so the "dismiss button stuck disabled" bug those addressed is no longer reachable. Accepted trunk's deletion of use-import-export.test.tsx as part of the legacy-test cleanup.
Related issues
How AI was used in this PR
Codex and Claude were used extensively to review the refactor, focusing on the overall implementation and individual aspects, such as error handling.
Proposed Changes
This PR makes Studio depend on the CLI for import/export, rather than maintaining duplicate import/export implementations in both
apps/cliandapps/studio.We achieve this by using the existing event architecture for importers and exporters and forwarding events from the CLI to Studio over IPC, allowing the importers/exporters and the UI state management to remain largely intact.
A few things to note about the change:
exportcommand:--split-db-dump-by-table, and--include-only. Both options are hidden from the CLI's regular--helpoutput.exportcommand now supports--mode=content(which is useful for selective pushing when only files are selected, and not the database). We also support.zipand.tar.gzextensions for--mode=dbexports (which is useful for selective pushing when the database is selected).--start-serveroption to theimportCLI command.Testing Instructions
node apps/cli/dist/cli/main.mjs export --path PATH_TO_SITEnode apps/cli/dist/cli/main.mjs import --path PATH_TO_SITEPre-merge Checklist