Repository navigation
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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
left a comment
There was a problem hiding this comment.
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 sameapp.exit(0). - The remaining benefit — avoiding the
process:allow-exitgrant — unfortunately can't be realized: there's a secondexit()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 togetCurrentWindow().close(): the close handler only force-exits fordb_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.
Summary
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
The macOS app bundle was built and exercised. Updater signing was not performed because no release private key is available locally.