-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for Chromium on all platforms #535
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@going-confetti I've made some changes in the browser process to catch the exception and used a listener to show a toast in the frontend. I'll pick up #525 next so we can make the settings dialog more accessible.

const browserProc = launch({ | ||
executablePath: path, | ||
args: args, | ||
onExit: sendBrowserClosedEvent, | ||
}) | ||
browserProc.nodeProcess.on('error', sendBrowserLaunchFailedEvent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
launch
uses spawn
under the hood, which handles errors asynchronously, so try/catch doesn't work here and we need to listen on error
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with chromium on Windows - had to specify browser executable manually, but otherwise worked as expected 👌
Would be interesting to understand why it fails since we are trying to autodetect it there as well 🤔 |
Description
This PR extends the Linux PR #513 and makes Chromium the second default browser for all platforms.
How to Test
browser.ts
to make sure Chromium is detected$PATH
for it to be detected. Once it's added, restart your terminal and relaunch the applicationnpm start
Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Related PR(s)/Issue(s)
Resolves #533