Add browser detection and "Open Link in Browser" context menu#3732
Open
svelle wants to merge 11 commits intomattermost:masterfrom
Open
Add browser detection and "Open Link in Browser" context menu#3732svelle wants to merge 11 commits intomattermost:masterfrom
svelle wants to merge 11 commits intomattermost:masterfrom
Conversation
When right-clicking an external link on macOS, a new submenu "Open Link in Browser" appears listing all detected installed browsers (Safari, Chrome, Firefox, Edge, Brave, Arc, Opera, Vivaldi, Chromium, Orion, Zen). This allows opening links in a browser other than the system default. Browser detection uses mdfind with bundle identifiers and results are cached for the session lifetime. Links are opened via `open -b` with fallback to shell.openExternal. https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
Extends browser detection to all platforms: - Windows: queries the registry (HKLM\SOFTWARE\Clients\StartMenuInternet) to find installed browsers and their executable paths - Linux: uses `which` for known browser commands, then discovers additional browsers from .desktop files that handle x-scheme-handler/https - Removes the macOS-only guard from the context menu so the submenu appears on all platforms https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
- Fix command injection: replace exec() with execFile() in openLinkInBrowser so URLs are passed as arguments, not shell-interpolated - Restructure BrowserInfo to store executable + args separately instead of a shell command string - Remove dead identical if/else branches in openLinkInBrowser - Validate .desktop file exec paths are absolute and exist before adding - Add HKCU registry fallback for per-user browser installs on Windows - Search ~/.local/share/applications/ in addition to /usr/share/applications/ - Fix import paths to use project aliases instead of relative paths - Add tests: existsSync false case, HKCU fallback, shell injection safety https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a cross-platform BrowserManager (discover, cache, and open browsers), integrates an "Open Link in Browser" submenu into MattermostWebContentsView for external links, adds an i18n key, and includes comprehensive Jest tests for discovery and opening behavior. Changes
Sequence DiagramsequenceDiagram
participant User
participant WebView as MattermostWebContentsView
participant BrowserMgr as BrowserManager
participant OS as Platform
participant Browser as Browser Process
User->>WebView: Right-click external link
WebView->>WebView: Detect external URL
WebView->>BrowserMgr: getInstalledBrowsers()
alt cached
BrowserMgr-->>WebView: BrowserInfo[]
else
BrowserMgr->>OS: Probe installed browsers (mdfind / registry / which + .desktop)
OS-->>BrowserMgr: BrowserInfo[]
BrowserMgr->>BrowserMgr: Cache results
BrowserMgr-->>WebView: BrowserInfo[]
end
WebView->>User: Render context menu with browser options
User->>WebView: Select browser
WebView->>BrowserMgr: openLinkInBrowser(url, browser)
BrowserMgr->>Browser: execFile or shell.openExternal
Browser-->>User: Browser opens link
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/views/MattermostWebContentsView.ts (1)
502-524:⚠️ Potential issue | 🟠 MajorTreat other configured server URLs as internal before showing the browser submenu.
This branch only matches the current server URL. If a user right-clicks a link to another Mattermost server that is already configured in the desktop app, it falls into "Open Link in Browser" instead of preserving the in-app open-in-tab/window path.
Based on learnings, "External navigation links to non-configured servers should open in the user's default browser via
will-navigate,did-start-navigation, andsetWindowOpenHandler."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/views/MattermostWebContentsView.ts` around lines 502 - 524, The current branch only treats links matching the active server (isInternalURL(parsedURL, server.url)) as internal, so links to other configured Mattermost servers fall through to generateOpenInBrowserMenuItems; update the condition to detect any configured server URL (e.g., iterate configured servers or add a helper like isURLForConfiguredServer(parsedURL)) and return the same in-app menu (separator + Open in new tab/window with ViewManager and NavigationManager calls) when the link belongs to any configured server rather than only server.url. Ensure you reference the existing isInternalURL logic or reuse it against each configured server URL to avoid duplicating navigation behavior.
🧹 Nitpick comments (1)
src/main/browserManager.test.js (1)
94-97: Keep this test file on the repo's ES-module path.This file still drops back to CommonJS inside
src/. Useawait import()in the asyncbeforeEach/ test body so it stays aligned with the repo’s module rule.As per coding guidelines, "Always use ES module `import`/`export`, never `require()` in source files".♻️ Suggested change
- beforeEach(() => { + beforeEach(async () => { jest.clearAllMocks(); jest.resetModules(); mockExistsSync.mockReturnValue(true); - const browserManager = require('./browserManager'); - getInstalledBrowsers = browserManager.getInstalledBrowsers; - openLinkInBrowser = browserManager.openLinkInBrowser; - clearBrowserCache = browserManager.clearBrowserCache; + ({getInstalledBrowsers, openLinkInBrowser, clearBrowserCache} = await import('./browserManager')); clearBrowserCache(); }); ... - const {shell} = require('electron'); + const {shell} = await import('electron');Also applies to: 297-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/browserManager.test.js` around lines 94 - 97, Replace the CommonJS require() usage with dynamic ES-module imports inside the async test lifecycle so the test file stays on the repo's ESM path: remove the top-level require('./browserManager') and instead use await import('./browserManager') inside an async beforeEach or the individual test, then destructure the exported functions (getInstalledBrowsers, openLinkInBrowser, clearBrowserCache) from the imported module; make the same change for the other occurrence around the later block (near line 297) so no require() calls remain in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/browserManager.ts`:
- Around line 107-121: The registry "command" string (captured in stdout and
assigned to browserPath from match[1]) can include launcher flags and a %1
placeholder, so do not treat the entire match as the executable; instead parse
the command token: trim the string, if it starts with a quote extract up to the
matching closing quote as the executable path, otherwise take the first
whitespace-delimited token as the executable; remove any %1 or similar
placeholders and surrounding quotes before calling fs.existsSync and before
returning executable/args (adjust args to include any remaining tokens if
needed). Update the code around the exec result handling (the stdout match,
browserPath extraction, fs.existsSync check, and the returned executable/args)
to implement this parsing logic.
- Around line 247-253: The debug log in browserManager.ts currently outputs the
raw URL and shell.openExternal() is called without awaiting its Promise; update
the code so log.debug('Opening link in browser', ...) does not include the full
URL (log only browser.name and either a masked URL or omit url entirely) and
change the fallback to await shell.openExternal(url) inside a try/catch so any
rejection is caught and logged (use the same log.error pattern). Adjust the
catch block around execFile to call await shell.openExternal(url) in its own
try/catch and log the caught error with identifying context (referencing
execFile, log.debug, log.error, and shell.openExternal).
---
Outside diff comments:
In `@src/app/views/MattermostWebContentsView.ts`:
- Around line 502-524: The current branch only treats links matching the active
server (isInternalURL(parsedURL, server.url)) as internal, so links to other
configured Mattermost servers fall through to generateOpenInBrowserMenuItems;
update the condition to detect any configured server URL (e.g., iterate
configured servers or add a helper like isURLForConfiguredServer(parsedURL)) and
return the same in-app menu (separator + Open in new tab/window with ViewManager
and NavigationManager calls) when the link belongs to any configured server
rather than only server.url. Ensure you reference the existing isInternalURL
logic or reuse it against each configured server URL to avoid duplicating
navigation behavior.
---
Nitpick comments:
In `@src/main/browserManager.test.js`:
- Around line 94-97: Replace the CommonJS require() usage with dynamic ES-module
imports inside the async test lifecycle so the test file stays on the repo's ESM
path: remove the top-level require('./browserManager') and instead use await
import('./browserManager') inside an async beforeEach or the individual test,
then destructure the exported functions (getInstalledBrowsers,
openLinkInBrowser, clearBrowserCache) from the imported module; make the same
change for the other occurrence around the later block (near line 297) so no
require() calls remain in the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 12b8d085-0329-45c2-a548-93de7b2e1b9a
📒 Files selected for processing (4)
i18n/en.jsonsrc/app/views/MattermostWebContentsView.tssrc/main/browserManager.test.jssrc/main/browserManager.ts
- Parse Windows registry command strings properly: extract quoted executable paths, preserve launcher flags, strip %1/%* placeholders - Remove raw URL from debug log in openLinkInBrowser for privacy - Await shell.openExternal fallback and catch its rejection - Detect links to any configured server (not just active) for the "Open in new tab/window" context menu items - Skip require->import nitpick: require() with jest.resetModules() is the established pattern across all test files in this codebase https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/browserManager.ts (1)
131-132: Quoted arguments in registry commands may be split incorrectly.The
rest.split(/\s+/)approach doesn't handle quoted arguments with spaces (e.g.,--profile "My Profile"). While uncommon in browser launcher commands, this could drop or corrupt such arguments.♻️ Optional: Handle quoted arguments
- // Filter out placeholders like %1, %*, etc. and keep real arguments - const args = rest.split(/\s+/).filter((token) => token && !(/(%\d|%\*)/).test(token)); + // Filter out placeholders like %1, %*, etc. and keep real arguments + const args = (rest.match(/"[^"]*"|\S+/g) || []) + .filter((token) => !(/^"?%(\d|\*)"?$/).test(token));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/browserManager.ts` around lines 131 - 132, The current tokenization uses rest.split(/\s+/) and will break quoted arguments (e.g., --profile "My Profile"); change the tokenization used to build const args so it respects quoted strings: parse rest into tokens that match either double-quoted, single-quoted, or unquoted sequences (then strip surrounding quotes) and then filter out placeholders as before; update the code around the args declaration in browserManager.ts (look for the variables rest and args) to use this quoted-aware splitting logic before the placeholder filter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/browserManager.ts`:
- Around line 131-132: The current tokenization uses rest.split(/\s+/) and will
break quoted arguments (e.g., --profile "My Profile"); change the tokenization
used to build const args so it respects quoted strings: parse rest into tokens
that match either double-quoted, single-quoted, or unquoted sequences (then
strip surrounding quotes) and then filter out placeholders as before; update the
code around the args declaration in browserManager.ts (look for the variables
rest and args) to use this quoted-aware splitting logic before the placeholder
filter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c855777a-5623-4da1-85c9-547c0d3118d5
📒 Files selected for processing (3)
src/app/views/MattermostWebContentsView.tssrc/main/browserManager.test.jssrc/main/browserManager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/browserManager.test.js
Replace exec() with execFile() for the grep call that searches .desktop files. The directory path is derived from process.env.HOME, which is untrusted input — using exec() would interpolate it into a shell command string, allowing injection. execFile() passes it as an argument without shell interpretation. https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
- Replace parameter reassignment (no-param-reassign) with a resolveExecCallback helper that returns the correct callback - Remove unnecessary quotes on HKLM property key (quote-props) - Add blank line before inline comment (lines-around-comment) https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/browserManager.test.js (1)
211-266: Consider adding a test for.desktopfile parsing.The Linux tests cover
which-based detection but don't test the.desktopfile parsing path when grep finds matches. Adding a test that mocksreadFileSyncreturning valid.desktopcontent would improve coverage.Example test case
it('should discover browsers from .desktop files', async () => { const fs = require('fs'); mockExec.mockImplementation((cmd, opts, callback) => { const cb = resolveExecCallback(opts, callback); if (typeof cb === 'function') { cb(new Error('not found')); // no which matches } }); mockExecFile.mockImplementation((file, args, opts, callback) => { const cb = resolveExecCallback(opts, callback); if (typeof cb === 'function') { cb(null, {stdout: '/usr/share/applications/custom-browser.desktop', stderr: ''}); } }); fs.readFileSync.mockReturnValue( '[Desktop Entry]\nName=Custom Browser\nExec=/usr/bin/custom-browser %u\n' ); const browsers = await getInstalledBrowsers(); expect(browsers.length).toBe(1); expect(browsers[0].name).toBe('Custom Browser'); expect(browsers[0].executable).toBe('/usr/bin/custom-browser'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/browserManager.test.js` around lines 211 - 266, Add a unit test that exercises the .desktop parsing branch of getInstalledBrowsers by mocking mockExec to return “not found” for which lookups and mocking mockExecFile to return one or more .desktop file paths, then mock fs.readFileSync to return a valid .desktop content string (including Name= and Exec= lines) and assert that getInstalledBrowsers returns the parsed browser(s) with correct name and executable; reference the existing test helpers mockExec, mockExecFile and fs.readFileSync and add the new it('should discover browsers from .desktop files', ...) case to the Linux describe block so the .desktop parsing code path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/browserManager.ts`:
- Around line 131-132: The filtering misses quoted placeholders (e.g. "%1")
because the filter tests token directly; update the filter used when computing
args (the rest.split(...).filter(...) callback) to strip surrounding quotes from
token before testing or adjust the regex to accept optional surrounding quotes —
e.g., inside the filter normalize token (remove leading/trailing single or
double quotes) then test that normalized token against /(%\d|%\*)/ and exclude
it; ensure you still ignore empty tokens and preserve other logic.
---
Nitpick comments:
In `@src/main/browserManager.test.js`:
- Around line 211-266: Add a unit test that exercises the .desktop parsing
branch of getInstalledBrowsers by mocking mockExec to return “not found” for
which lookups and mocking mockExecFile to return one or more .desktop file
paths, then mock fs.readFileSync to return a valid .desktop content string
(including Name= and Exec= lines) and assert that getInstalledBrowsers returns
the parsed browser(s) with correct name and executable; reference the existing
test helpers mockExec, mockExecFile and fs.readFileSync and add the new
it('should discover browsers from .desktop files', ...) case to the Linux
describe block so the .desktop parsing code path is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f583f4aa-2797-49f1-b018-7a01374edf8d
📒 Files selected for processing (2)
src/main/browserManager.test.jssrc/main/browserManager.ts
Strip surrounding quotes from tokens before testing against the placeholder regex so that "%1" and '%1' are filtered out alongside unquoted %1. Add a test case for this scenario. https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/browserManager.ts`:
- Around line 238-247: The Exec parsing currently captures only the first token
and drops launcher arguments; change the logic that creates execMatch (from
content.match(/^Exec=(\S+)/m)) to capture the full Exec= line, then parse it
into an executable token and remaining args (respecting quoted tokens), strip
out placeholders like %u %U %f %F %F etc from the args, set executable to the
first token (handle quoted paths) and args to the remaining cleaned tokens, and
then perform the existing path.isAbsolute(execPath) && fs.existsSync(execPath)
check against that executable before pushing the found entry (referencing
execMatch, execPath, nameMatch, seenNames, and the found object with executable
and args).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5a71c24-0653-409c-881d-65333a35f2da
📒 Files selected for processing (2)
src/main/browserManager.test.jssrc/main/browserManager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/browserManager.test.js
The previous regex only captured the first token of the Exec= line, dropping launcher arguments (e.g. flatpak run org.example.browser). Now captures the full line, splits into executable + args, and strips .desktop field codes (%u, %U, %f, %F, etc.) including quoted variants. https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
Reject non-http(s) schemes (file://, ms-msdt://, etc.) in generateOpenInBrowserMenuItems to prevent attacker-controlled content from triggering dangerous URL schemes via the context menu. https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
devinbinnie
requested changes
Mar 23, 2026
Member
devinbinnie
left a comment
devinbinnie
left a comment
There was a problem hiding this comment.
Cool feature design, just needs a few adjustments :)
| KNOWN_WINDOWS_BROWSERS.map(async (browser) => { | ||
| try { | ||
| const {stdout} = await exec( | ||
| `reg query "${root}\\${browser.registryKey}\\shell\\open\\command" /ve`, |
Member
There was a problem hiding this comment.
We can avoid all of this code by instead using the registry-js module that is already built in to our Desktop App. See https://github.com/mattermost/desktop/blob/master/src/common/config/policyConfigLoader.ts for an example.
My approach would be to try and read the registry at start-up as well and pre-load all of the browsers (can't imagine hot-reloading needs to matter here) and store them here. One thing to try and make sure of is that the registry operations don't interfere with each other (I don't think that happens with the new module, but would be worth verifying)
| } | ||
| } | ||
|
|
||
| // Also discover browsers from .desktop files that handle http(s) |
Member
There was a problem hiding this comment.
I know this is also a way to capture Linux browsers, but is which not enough here?
Reason I ask is that I'd like to avoid the advanced matching and parsing of desktop files here, this is quite a bit more complicated that I would expect this to be. The regex pieces have me a bit worried that something could be exploited here.
| } | ||
| } | ||
|
|
||
| export class BrowserManager { |
Member
There was a problem hiding this comment.
I think this class is missing the above state cachedBrowsers. Anything that doesn't require state doesn't need a manager, but this does. We should encapsulate the state within the class and put the functions in here, same as the other examples.
This also makes it more testable.
Member
There was a problem hiding this comment.
Also, let's call this something more descript to the problem, maybe ExternalBrowserManager? Would probably be worth having a comment on it as to what it does too.
|
|
||
| export class BrowserManager { | ||
| init = async () => { | ||
| await getInstalledBrowsers(); |
Member
There was a problem hiding this comment.
This should not be awaited, I/O operations can take a while and we don't want to hold up starting the application on this functionality. Once the browsers are populated then the menu items should work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds functionality to detect installed browsers on macOS, Windows, and Linux, and provides a context menu option to open external links in the user's preferred browser. The implementation includes:
browserManagermodule that detects installed browsers per platform using platform-specific methods (mdfind on macOS, registry on Windows, which/desktop files on Linux)Changes
getInstalledBrowsers(),openLinkInBrowser(), andclearBrowserCache()functions with platform-specific browser detection logicshell.openExternalChecklist
Release Note
https://claude.ai/code/session_01WWHsdZzBXhrUtXo9WYEwY4
Change Impact: 🟡 Medium
Reasoning: Adds a new cross-platform BrowserManager module and context-menu integration into MattermostWebContentsView, spanning main and renderer layers with platform-specific system calls; unit tests cover the manager extensively but integration/UI timing remains untested.
Regression Risk: Moderate — Browser detection logic is well-covered by unit tests, but changes touch a widely-instantiated UI component (MattermostWebContentsView) and introduce asynchronous initialization (cachedBrowsers) that can cause timing-dependent context-menu inconsistencies early after startup; fallback behavior reduces severity but integration paths lack tests.
** QA Recommendation:** Perform manual QA on macOS, Windows, and Linux to verify the submenu appears consistently after app start and page loads, that detected browsers launch correctly, and that failures fall back to the system default; add integration/unit tests for MattermostWebContentsView menu population and startup timing to reduce regressions.
Generated by CodeRabbitAI