Skip to content

Always ask before stopping running sites on quit - #3350

Merged
lezama merged 2 commits into
trunkfrom
always-ask-before-stopping-sites-on-quit
May 6, 2026
Merged

Always ask before stopping running sites on quit#3350
lezama merged 2 commits into
trunkfrom
always-ask-before-stopping-sites-on-quit

Conversation

@lezama

@lezama lezama commented May 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Related issues

  • Related to a Slack discussion where a user reported that closing the Studio desktop app silently stopped their running sites.

How AI was used in this PR

  • Used Claude to trace the close-on-quit flow, confirm the daemon's lifecycle is independent of the desktop app, and identify the root cause (the dialog gate). Reviewed the diff and architecture findings before committing.
  • The code change itself is small (a few lines), but Claude also drafted this PR description based on the investigation. I read it through and edited where needed before posting.

Proposed Changes

  • Drop the isStudioCliInstalled() gate around the "running sites on quit" confirmation dialog in apps/studio/src/index.ts. The dialog now always shows when sites are running (unless the user has saved a preference, or when running E2E tests).
  • Change the default button from Stop sites to Leave running, so the safer / less destructive option is the one selected by Enter.
  • Remove the now-unused isStudioCliInstalled import.

Why drop the CLI gate

isStudioCliInstalled() only returns true when the CLI symlink points to the packaged CLI installed via the app. Users who installed studio independently (e.g. via npm) failed this check, so the dialog was silently skipped and shouldStopSitesOnQuit defaulted to true — sites were stopped on quit without any confirmation.

Sites are actually managed by a long-lived detached daemon (apps/cli/lib/daemon-client.ts, spawned with detached: true, stdio: 'ignore', daemonProcess.unref()) that survives the desktop app quit completely independently of CLI install state. Picking "Leave running" works regardless of whether the CLI is installed through the app, via npm, or not at all — so the gate was hiding a useful choice from users.

The gate's original intent (in #2314) was to spare GUI-only users from a new prompt, on the assumption that they wouldn't care about keeping sites running. But the capability is the same for everyone: closing the app shouldn't kill background processes the user started, and the dialog is also how users discover that sites can keep running.

Why default to "Leave running"

Quitting the app is rarely a deliberate "stop everything" action — it's often Cmd+Q to free RAM, declutter, or by accident. Defaulting to the destructive option (Stop sites) means a single Enter keypress can terminate work that took non-trivial time to set up (long-running imports, in-progress dev sessions). Leave running is reversible — the user can always studio site stop --all later — while Stop sites is not.

Testing Instructions

  1. Start one or more sites in Studio.
  2. Quit the app (Cmd+Q on macOS, or close the last window on Linux/Windows).
  3. The "You have running sites" dialog should appear with Stop sites / Leave running / Cancel, with Leave running highlighted as the default. Pressing Enter should leave the sites running and quit.
  4. Verify each path explicitly:
    • Stop sites → app quits, sites stop.
    • Leave running → app quits, sites continue running. Verify via ps/Activity Monitor that the daemon process under ~/.studio/daemon/ is still alive, and that the site URL still responds.
    • Cancel (or Esc) → app stays open.
  5. Tick "Don't ask again", choose either option, then re-quit with running sites — the saved preference should be honored without showing the dialog. (Reset by clearing stopSitesOnQuit from ~/.studio/app.json if needed.)
  6. Confirm the dialog now appears regardless of whether the CLI was installed through the app or independently (e.g. uninstall the packaged CLI / install via npm).

Pre-merge Checklist

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

wpmobilebot commented May 5, 2026

Copy link
Copy Markdown
Collaborator

📊 Performance Test Results

Comparing a64b303 vs trunk

app-size

Metric trunk a64b303 Diff Change
App Size (Mac) 1454.03 MB 1454.03 MB 0.00 MB ⚪ 0.0%

site-editor

Metric trunk a64b303 Diff Change
load 1501 ms 1527 ms +26 ms ⚪ 0.0%

site-startup

Metric trunk a64b303 Diff Change
siteCreation 8096 ms 8090 ms 6 ms ⚪ 0.0%
siteStartup 4955 ms 4938 ms 17 ms ⚪ 0.0%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff)
fredrikekelund
fredrikekelund approved these changes May 6, 2026
View reviewed changes

@fredrikekelund fredrikekelund left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For fixing the use case where the wp-studio CLI has been installed from npm, but the bundled CLI hasn't been installed, this seems like an OK fix to me 👍

How AI was used in this PR

When AI is also used to write the PR description, please acknowledge this.
@lezama
lezama merged commit 0f1e4d2 into trunk May 6, 2026
11 checks passed
@lezama
lezama deleted the always-ask-before-stopping-sites-on-quit branch May 6, 2026 12:21
@ivan-ottinger ivan-ottinger mentioned this pull request May 25, 2026
Merged
3 tasks
fredrikekelund pushed a commit that referenced this pull request May 29, 2026
@ivan-ottinger
Prevent ghost windows when quitting during site startup (#3608)
432e17a 
## Related issues

- Fixes
[STU-1750](https://linear.app/a8c/issue/STU-1750/studio-reopens-multiple-times-when-quitting-during-site-startup)

## How AI was used in this PR

Claude Code investigated the bug, identified the root cause
(create-if-missing branch in `getMainWindow()` racing with async IPC
fan-out during the `will-quit` shutdown window), proposed the minimal
gate at `sendIpcEventToRenderer`, and applied the change. I reviewed the
diff and confirmed both the fix and its placement before committing.

## Proposed Changes

- Add an `isAppQuitting` flag in `apps/studio/src/ipc-utils.ts` with an
exported `markAppQuitting()` setter. While set, `sendIpcEventToRenderer`
returns early without calling `getMainWindow()`.
- Call `markAppQuitting()` at the start of the `will-quit` handler in
`apps/studio/src/index.ts`, before any other cleanup.

### Why this works

When the user confirms "Stop sites" on quit, `will-quit` calls
`event.preventDefault()` and waits up to 6 s for `stopAllServers()` to
drain. During that window, in-flight async callbacks (site-startup
events, snapshot events, sync progress, etc.) keep firing through
`sendIpcEventToRenderer()`. Each call routes through `getMainWindow()` —
and because the original window has typically already been closed by
Electron's shutdown sequence, `getMainWindow()` falls through to
`createMainWindow()` and spawns a fresh `BrowserWindow`. One pending
event = one ghost window. With ~6 sites starting simultaneously on a
Linux VM, you reliably see ~6 windows reopen.

Blocking the fan-out at the `sendIpcEventToRenderer` chokepoint is the
smallest fix: it catches every async-IPC path without changing
`getMainWindow`'s signature or touching its 15+ other call sites (menus,
deeplinks, update dialog, etc.), which are user-triggered and can't fire
post-`will-quit` anyway.

`will-quit` (not `before-quit`) is the right gate point: `before-quit`
can still be cancelled by the sync-in-progress dialog or the "Cancel"
button on the running-sites dialog. Once `will-quit` fires, the app is
committed.

### Regression origin

The mechanic was introduced in #86 (May 2024) for OAuth deep-link
handling. It became today's user-visible bug in #849 (Jan 2025), which
routed `sendIpcEventToRenderer` through `getMainWindow()`. The bug got
more reproducible recently due to Linux VM testing (slower shutdown
widens the race window) plus PR #3350 removing an accidental escape
hatch that bypassed the quit dialog when the CLI wasn't installed.

## Testing Instructions

Reproduce on `trunk` first, then verify the fix on this branch.

**Repro on trunk (Linux/Windows easier; macOS rarer):**

1. Have several Studio sites created (~6 makes it obvious).
2. Quit Studio so all sites stop.
3. Re-launch Studio.
4. Start all sites at once.
5. **Before** they finish booting, quit Studio.
6. Observe: Studio reopens 4–6 times in quick succession.

**Verify on this branch:**

1. Repeat steps 1–5 above.
2. Studio should quit cleanly with no ghost windows.

## Pre-merge Checklist

- [x] Have you checked for TypeScript, React or other console errors?
- [x] Manual repro on Linux (with several sites)
- [x] Manual repro on Windows (with several sites)
@bcotrim bcotrim mentioned this pull request Jun 3, 2026
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@lezama @wpmobilebot @youknowriad @fredrikekelund