Sync site list when CLI creates sites while app is running#2313
Conversation
There was a problem hiding this comment.
Test STU-1164 (Site creation)
Worked as expected
- Run node dist/cli/main.js site set-domain hello.wp.local --path PATH_TO_SITE
The changes are not reflected in Studio, tested a few times and reinstalled node_moduels and rebuild
set-php-version
Also changes are not reflected in Studio
set-wp-version
Stucks endlessly on Changing WordPress version… and can't be termninated with controll+c
UPDATE: I got error and again it stucks:

|
thanks for the review @nightnei I couldn't replicate any of the issues, can you run |
sejas
left a comment
There was a problem hiding this comment.
We removed and introduced the useIpcListener( 'user-data-updated' listener multiple times in the past. See:
I understand this is a good way to update the Studio UI from external changes, such as the CLI creating a new site, although I wonder why we removed it in the first place.
sejas
left a comment
There was a problem hiding this comment.
It works as expected. Great work! I confirmed that the site appears automatically in the left sidebar when creating a new site with the Studio CLI. I also tested node dist/cli/main.js site set-domain hello.wp.local --path /tmp/test-site-cli and node dist/cli/main.js site set-php-version 8.4 --path /tmp/test-site-cli.
fredrikekelund
left a comment
There was a problem hiding this comment.
In #2172, we decided that Studio should rely on studio site list --watch to subscribe to updates to the list of sites. The logic in src/modules/cli/lib/execute-site-watch-command.ts already supports parsing new sites, AFAICT. The reason new sites don't show up on the front-end is most likely that the site-status-changed IPC event handler only supports sites that already exists.
Instead of implementing this second, competing approach for subscribing to changes, we should look into fixing the site-status-changed IPC event handler.
There was a problem hiding this comment.
This is a concern for a later PR, but I think we should reconsider storing a global list of SiteServer instances in the node process. The CLI (or pm2, really) will soon be the source of truth for whether a server is running. It might make more sense to treat server instances as ephemeral variables in the node process, always reading from disk or the CLI whenever the client process queries them.
fredrikekelund
left a comment
There was a problem hiding this comment.
@bcotrim and I just huddled about this change and concluded that while we are both a little concerned about the relationship between the appdata watcher and the site list watcher being confusing, we still think it's a good idea to land this change for now, to resolve the concrete issues at hand and keep up the momentum.
@bcotrim will add a pretty exhaustive comment for now as a first step towards clarifying the relationship between the newly added watcher callback and the site list watcher (src/modules/cli/lib/execute-site-watch-command.ts). He'll also add a Linear issue detailing potential future refactorings in this area, to ensure the architecture remains clear and quality doesn't deteriorate with future changes
|
This is up to you, @bcotrim, but I still also think we could consider updating the |
I considered this, but I think it's better to leave it for the refactor:
For now, keeping clear separation (status handler = status only, data handler = everything else) seems cleaner until we do the proper refactor. |
|
I believe I just ran into a regression caused by this PR. While creating a site, the site list seems to get overwritten by the cc @bcotrim |
* Abort async operations on SIGTERM/SIGINT * Also send abort message to child server * Only create AbortController when topic is not `abort` * Fix * More fixes * Tweaks * Fix unit tests * Fix types * Remove `this.sessionPath` files individually To help us diagnose which specific files or directories are causing trouble * Stop running servers in a detached process * Revert "Remove `this.sessionPath` files individually" This reverts commit 8786163. * Retry * Increase timeouts * Fix deprecated blueprint syntax * Increase timeout * Try adding a small delay * Kill `site list --watch` on SIGINT * Create main window after creating site watcher * Try using async fs method for cleanup * Try rimraf (which has advanced retry strategies) * New approach: don't remove `sessionPath` dir * Unused import * Force close app * disconnect from pm2 in response to SIGTERM * Revert user data watcher changes from #2313 * Wait for running button * Use Electron's will-quit event in `execute-command.ts` * SIGINT and SIGTERM listeners in `wp` command * More SIGINT and SIGTERM listeners in `wp` command * Temporarily skip blueprints test * Revert "Temporarily skip blueprints test" This reverts commit 0d16ae5. * Try with force kill again * Logging * New approach to waiting for app close * Logging again * Try to make all child processes detached * Experiment * Revert "Experiment" This reverts commit 3596bac. * Revert "Try to make all child processes detached" This reverts commit f99f4b0. * Try a 5s timeout for closing the app * Experiment with removing stopAllServersOnQuit * shutdown message * Revert "shutdown message" This reverts commit 9d56d0c. * Revert E2ESession::closeApp implementation * Temporarily skip app.test.ts * Temporarily skip blueprints.test.ts * Revert "Temporarily skip app.test.ts" This reverts commit 8307750. * Revert "Temporarily skip blueprints.test.ts" This reverts commit 9d78492. * Add logging to Playwright source code * pidtree * More playwright logging * More logging and await pidtree * pidtree after close * Remove stdio listeners * Fix pidtree logging after close * destroy stdio streams on exit * Disconnect IPC * Try teardown workaround * Log pid and result in will-quit handler * Unregister will-quit handlers * Bring back logging * Stop all servers on quit * Undo all Windows hacks in E2ESession * Bring back tree-kill * Log stop-all pid and pidtree * Catch errors from tree-kill * Remove pidtree * No IPC channel in stopAllServersOnQuit * Update playwright-core patch to test theory * Fix patch * `spawn` over `fork` * Temporarily remove `stopAllServersOnQuit` * Never call `electronApp.close` on Windows * Restore `e2e/e2e-helpers.ts` * Reinstate `stopAllServersOnQuit` * Revert `spawn` and IPC channel theory * Fix unit tests * Remove playwright-core patch * Remove tree-kill dependency * Remove `started` event * More lenient child process cleanup * Revert "Remove `started` event" This reverts commit b42fa0b. * Destroy stdio streams again * Explicitly disconnect IPC * Clean up `E2ESession` * Always call `killRemainingProcesses` * Reset test timeouts * No `detached` option * Reset more test timings * Only prevent `will-quit` when applicable * fix e2e test * Children connect to pm2 daemon in sequence * Fix unit tests * Kill pm2 daemon in `site stop --all` command * Fix stop command unit test, async disconnect, `process.exit` * Remove stray `console.info` calls * Undo all hacks in `E2ESession` * Restore `E2ESession`, but don't kill children * Increase timeout * Don't log child pids * Experimental: detach `site stop --all` again * Clean up session files again * Let's give rimraf one more try * Experiment with removing `child.disconnect()` call * Don't destroy stdio streams on exit * Always run `site stop --all` command if pending update * Notify Studio if user runs `site stop --all` * Fix `package-lock.json` diff * Fix and document "stop sites on exit" logic * Speed up `site stop --all` * Fix logic * Revert "Experimental: detach `site stop --all` again" This reverts commit 01ae1c4. * String tweaks --------- Co-authored-by: bcotrim <bernardo.cotrim@a8c.com>







Related issues
Proposed Changes
useIpcListenerforuser-data-updatedevent inuse-site-details.tsxto refresh the site list when the appdata file changes externally (e.g., when CLI creates a site or modifies site settings)Root cause: The file watcher (
user-data-watcher.ts) already broadcastsuser-data-updatedIPC events when the appdata file changes, butuse-site-details.tsxwas not listening to this event. The snapshot slice listened to it for preview sites, but the main site list did not.Testing Instructions
Test STU-1164 (Site creation)
npm run cli:buildnode dist/cli/main.js site create --path /tmp/test-site-cliTest STU-1163 (Site settings changes)
node dist/cli/main.js site set-domain hello.wp.local --path PATH_TO_SITEset-php-version,set-wp-version,set-httpsPre-merge Checklist