Skip to content

fix(db): make recovery quit button work - #6567

Closed
SaladDay wants to merge 1 commit into
farion1231:mainfrom
SaladDay:fix/database-upgrade-quit
Closed

SaladDay wants to merge 1 commit into
farion1231:mainfrom
SaladDay:fix/database-upgrade-quit

Conversation

@SaladDay

Copy link
Copy Markdown
Collaborator

Summary

  • replace the recovery screens unauthorized process exit call with the already-authorized window close API
  • reuse the existing database-recovery CloseRequested path, which exits instead of minimizing to tray
  • avoid expanding process plugin permissions

Context

The exact missing-permission bug was identified in the original review for PR #4575 but was not addressed before merge: #4575 (comment)

Related reports #2484 and #4515 cover the broader too-new-database recovery experience, but neither reports this Quit-button ACL failure, so this PR does not close them.

Validation

  • pnpm typecheck
  • Prettier and git diff checks
  • pnpm test:unit: 131 files, 987 tests passed
  • two independent blind reviews found no in-scope P0/P1/P2 issues
  • macOS real-machine test with CC_SWITCH_TEST_HOME and an isolated SQLite database at user_version 18: the recovery screen appeared, one click on Quit immediately closed the process, normal exit cleanup completed, no ACL or unhandled-rejection error was logged, and the real CC Switch database hash remained unchanged

The macOS app bundle was built and exercised. Updater signing was not performed because no release private key is available locally.

@SaladDay
SaladDay requested a review from farion1231 as a code owner August 18, 2026 07:41
@github-actions github-actions Bot added the frontend Frontend (React/TypeScript) label Aug 18, 2026
@farion1231

Copy link
Copy Markdown
Owner

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: 054af9b82c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@farion1231 farion1231 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this — and honestly, apologies for the awkward timing: you found and fixed this before I did. This PR went up a couple of hours before I independently committed my own fix for the same bug (4549d2907, granting process:allow-exit in the default capability), which is now on main. I hadn't seen this PR yet when I wrote that commit.

I did verify your approach end to end, and it's correct: DatabaseUpgrade only renders for kind === "db_version_too_new" (src/main.tsx#L94), the CloseRequested handler force-exits on exactly the same predicate (src-tauri/src/lib.rs#L410-L418), and core:window:allow-close was already granted. Digging up the original review thread on #4575 was spot-on, and keeping #2484/#4515 out of scope was the right call. The real-machine validation writeup is appreciated too.

Why I'm closing this rather than merging it on top:

  • With the capability fix already on main, this change becomes a behavioral no-op — both paths end at the same app.exit(0).
  • The remaining benefit — avoiding the process:allow-exit grant — unfortunately can't be realized: there's a second exit() call site in the config-load-failure path (src/main.tsx#L75), which exits with a different error kind. That one can't be converted to getCurrentWindow().close(): the close handler only force-exits for db_version_too_new, so any other kind would fall into the minimize-to-tray branch and the app would hide in the background instead of quitting. The permission has to stay either way.
  • Keeping exit(0) on the button also keeps it self-contained — it doesn't depend on the backend close-predicate staying in sync with the frontend render condition if this screen is ever reused for another error kind.

The fix ships in the next release, and I'll make sure the notes credit that this was independently caught and fixed here first. Sorry the race went this way — your recent PRs have been consistently excellent, and this one was no exception in quality. Hope to see more of them.

@farion1231 farion1231 closed this Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend Frontend (React/TypeScript)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants