-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix: It's not possible to select browser executable on macOS #576
Conversation
<Callout.Text> | ||
The selected executable doesn't appear to be compatible. | ||
Please select the correct executable for Chrome or Chromium. | ||
</Callout.Text> |
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.
This message does not really detect that the selected browser is correct. In practice you could rename the Firefox executable to Chrome
and it would bypass it, but it handles the most common scenarios and warns the user about incompatibilities.
const executableName = getExecutableNameFromPlist(plistPath) | ||
if (executableName) { | ||
return path.join(filePath, 'Contents', 'MacOS', executableName) | ||
} | ||
return filePath |
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.
Here we read the executable name from Info.plist
, if available.
if (typeof plistData.CFBundleExecutable === 'string') { | ||
return plistData.CFBundleExecutable |
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.
CFBundleExecutable is the default location for the executable name
'chromium.exe', | ||
'/chrome', | ||
'/chromium', | ||
] |
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.
We should probably include chromium derived browsers, for example Edge 🤔
I wonder if there's a better way to detect a working browser, maybe like doing a test run in the background and checking that a request is actually recorded, might be too laborious thou 😄
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.
We should probably include chromium derived browsers, for example Edge 🤔
I wouldn't do it at this stage. If the user wants to try another Chromium-based browser, they should do it at their own risk.
As far as I understand, we don't prevent the user from selecting a different browser, so they can still try to use other browsers.
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.
The message is discouraging thou, so maybe we can iterate on it in the future when we confirm also other chromium based browsers so that we can add them to the list 🤔
After all we are officially supporting only Chrome at this time
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.
We haven't really tested in other Chrome-based browsers and potential ramifications they introduce so I'd say it's safer to just go with Chrome and Chromium for now.
|
||
<Callout.Text> | ||
The selected executable doesn't appear to be compatible. | ||
Please select the correct executable for Chrome or Chromium. |
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.
The hint only mentions Google Chrome. If we release this without the Chromium PR, it might be confusing
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.
I can make that change in the Chromium support PR once we fully validated it's working for all plaforms #535
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!
Description
This PR resolves an issue that prevents selecting the correct executable browser path on macOS from the settings page.
How to Test
Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Screen.Recording.2025-03-18.at.4.14.56.PM.mov
Related PR(s)/Issue(s)
Resolves #562 #501