fix: handle missing notification settings app on Linux#8304
fix: handle missing notification settings app on Linux#8304mvanhorn wants to merge 3 commits intoaaif-goose:mainfrom
Conversation
Replace spawn() try/catch with canSpawn() check using
execFileSync('which', [cmd]). Node.js spawn() emits ENOENT
asynchronously via the error event, so try/catch never catches
it. canSpawn() detects missing binaries before spawning.
The first canSpawn('gnome-control-center') check already covers this case.
If it wasn't found then, it won't be found again without args.
Signed-off-by: Douwe Osinga <douwe@squareup.com>
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
Good fix — spawn() indeed doesn't throw synchronously on ENOENT, so the existing try/catch was dead code. The canSpawn approach using which is clean and correct.
I pushed a small cleanup commit removing the redundant fallback canSpawn('gnome-control-center') check — if it wasn't found the first time, checking again without args won't help.
One thing to fix: your original commit is missing the DCO sign-off. Please amend it with git commit --amend -s and force-push so the DCO check passes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a5d31de56
ℹ️ 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".
| // Linux: Try different desktop environments | ||
| function canSpawn(cmd: string): boolean { | ||
| try { | ||
| execFileSync('which', [cmd], { stdio: 'ignore' }); |
There was a problem hiding this comment.
which for command detection
The new Linux path now relies on execFileSync('which', [cmd]) to decide whether a settings binary exists, but which is not guaranteed to be installed on all Linux desktops. In environments where gnome-control-center/systemsettings5 exists but which is missing, canSpawn returns false for every check and this handler will incorrectly fail instead of opening settings, which is a behavior regression introduced by this change.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes a crash when clicking "Open Settings" in Settings > App > Notifications on Linux desktops that don't have GNOME, KDE, or XFCE installed.
Changes
In
ui/desktop/src/main.ts, theopen-notifications-settingsIPC handler usedspawn()insidetry/catchblocks for each desktop environment. Node.jsspawn()does not throw synchronously on ENOENT; it emits the error asynchronously via the ChildProcess 'error' event. Thetry/catchblocks never fired, so the app crashed withspawn gnome-control-center ENOENT.Fix: added a
canSpawn(cmd)helper that usesexecFileSync('which', [cmd])to check if the binary exists before callingspawn(). Each DE check now usesif (canSpawn(...))instead oftry { spawn(...) } catch. When no settings app is found, returnsfalseinstead of crashing.Testing
Verified the logic by reading the Node.js
child_processdocs confirmingspawn()ENOENT behavior. The fix is a straightforward control flow change with no new dependencies.Fixes #8292
This contribution was developed with AI assistance (Codex).