Skip to content

fix(desktop): cleanly quit macOS packaged app#422

Open
fuyizheng3120 wants to merge 1 commit intonexu-io:mainfrom
fuyizheng3120:fix/macos-quit-cleanup
Open

fix(desktop): cleanly quit macOS packaged app#422
fuyizheng3120 wants to merge 1 commit intonexu-io:mainfrom
fuyizheng3120:fix/macos-quit-cleanup

Conversation

@fuyizheng3120
Copy link
Copy Markdown
Contributor

Problem

PR #270 intentionally makes the macOS red close button hide the window instead of destroying the app, which is the right Dock re-open behavior.

However, the same close handler can also intercept the real app quit path. In a packaged macOS app, using Quit / Cmd-Q can leave the Electron main process plus daemon/web sidecars alive, so the Dock still shows Open Design as running.

Fix

  • Handle Electron before-quit in desktop main.
  • On the first real app quit, prevent the default quit and route through the existing unified shutdown() path.
  • Once shutdown() calls desktop.close(), the runtime stopped flag allows the BrowserWindow close to proceed, and the existing app.quit() completes normally.

This keeps the PR #270 red-X behavior intact while making real Quit / Cmd-Q clean up the app.

Test

  • pnpm install --frozen-lockfile
  • pnpm --filter @open-design/desktop typecheck

Local packaged verification in our macOS build:

  • Build and install packaged app.
  • Launch Open Design.
  • Run osascript -e 'tell application "Open Design" to quit'.
  • Verify pgrep -fl 'Open Design|--od-stamp-namespace=release-stable' returns no Open Design main/helper/sidecar processes.

@lefarcen lefarcen self-requested a review May 4, 2026 05:52
@lefarcen lefarcen added the bug-fix Fixes an existing bug label May 4, 2026
@lefarcen
Copy link
Copy Markdown
Contributor

lefarcen commented May 4, 2026

Hi @fuyizheng3120! 🎉

Thanks for the contribution — this is a precise fix for the Quit path regression from PR #270.

I will run a deep review and get back to you within 24h.

Thanks for making open-design better!
— open-design team

Copy link
Copy Markdown
Contributor

@mrcfps mrcfps left a comment

Choose a reason for hiding this comment

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

@fuyizheng3120 I reviewed the macOS quit lifecycle change and verified that the new before-quit path routes real app quits through the existing unified shutdown flow while preserving the stopped-window behavior used to distinguish Cmd-Q/Quit from the red close button hide behavior. I also checked the changed range for lifecycle re-entrancy and sidecar cleanup concerns; the implementation looks focused and safe. Thanks for tightening up this packaged macOS cleanup path — nice work! 🙌

Generated by Looper 0.5.1 · runner=reviewer · agent=opencode

Copy link
Copy Markdown
Contributor

@lefarcen lefarcen left a comment

Choose a reason for hiding this comment

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

Thanks for this clean fix @fuyizheng3120! The approach is solid, and mrcfps already approved ✅

I'm adding one complementary finding from my deep review — a subtle shutdown race that could affect reliability in edge cases:

attachParentMonitor(shutdown);

app.on("before-quit", (event) => {
if (shuttingDown) return;
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.

P2 Race condition: shuttingDown alone isn't a sufficient gate for before-quit. It currently means "cleanup has started", but the handler treats it as "safe to let Electron quit".

If a second quit event arrives while beforeShutdown() is still closing daemon/web sidecars, this handler returns without preventDefault(), allowing Electron to continue quitting while cleanup is in flight.

This is also vulnerable to double-trigger because other callers attach .finally(() => process.exit(0)) to shutdown(), and shutdown() resolves immediately when shuttingDown is already true.

Concrete fix: Track an in-flight shutdownPromise and a separate allowQuit/readyToQuit flag. before-quit should only return when allowQuit is true; otherwise prevent default and await/share the same shutdown promise. Set allowQuit = true immediately before the internal app.quit() call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix Fixes an existing bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants