fix(browser-tab): right-click context menu inside embedded webview tabs#1066
fix(browser-tab): right-click context menu inside embedded webview tabs#1066pedramamini wants to merge 3 commits into
Conversation
…w tabs Electron renders no default context menu for guest <webview> content, so right-clicking inside a browser tab produced nothing. The only context-menu handler lived on mainWindow.webContents (the app window) and never reached the guest. Wire a context-menu handler onto the guest webContents in did-attach-webview that builds a native menu whose actions target the guest: - editable fields: Cut / Copy / Paste / Select All (+ spellcheck suggestions) - selected text: Copy - links: Copy Link, Open Link in Browser (http/https/mailto only) - images: Copy Image, Copy Image Address - page background: Back / Forward / Reload Actions call guest webContents methods directly (edit roles would target the host window), and shell.openExternal is restricted to safe schemes so a malicious page can't smuggle a dangerous URL through the menu. closes #1065
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR wires a guest-targeted context menu for embedded browser-tab webviews, adding a context-menu template builder with editable, non-editable, image, link, navigation, and spellcheck actions (with a protocol allowlist for external opens) and comprehensive tests/mocks covering those scenarios. ChangesBrowser-tab context menu implementation and testing
sequenceDiagram
participant WebView as Browser Tab Webview
participant Guest as Guest webContents
participant Main as Main Process (window-manager)
participant Menu as Electron Menu
WebView->>Guest: user right-clicks in page
Guest->>Main: emits 'context-menu' with params
Main->>Main: build template from params and guest navigation/session state
Main->>Menu: Menu.buildFromTemplate(template)
Menu->>Guest: popup({window: guest})
Guest->>Main: menu item click executes guest or main actions (editing, navigation, clipboard, shell)
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/app-lifecycle/window-manager.ts (1)
195-204: 💤 Low valueConsider adding error handling for
shell.openExternal.The
void shell.openExternal(linkURL)pattern ignores promise rejections. If opening the URL fails (e.g., no default browser configured, system error), the failure is silently lost. Per coding guidelines, consider at least logging the error.Proposed fix
if (isExternallyOpenableLink(linkURL)) { linkSection.push({ label: 'Open Link in Browser', click: () => { - void shell.openExternal(linkURL); + shell.openExternal(linkURL).catch((err) => { + logger.warn(`Failed to open external link: ${err.message}`, 'Window', { url: linkURL }); + }); }, }); }As per coding guidelines: "Do not silently swallow errors. Handle expected/recoverable errors explicitly."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/app-lifecycle/window-manager.ts` around lines 195 - 204, The click handler for the "Open Link in Browser" menu item currently calls void shell.openExternal(linkURL) which swallows promise rejections; update the click in the linkSection (inside window-manager.ts) to handle the returned promise and log failures (e.g., shell.openExternal(linkURL).catch(err => processLogger.error(...) or console.error(...))) so any error opening the URL is not silently lost; ensure the log includes linkURL and the error object for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/main/app-lifecycle/window-manager.ts`:
- Around line 195-204: The click handler for the "Open Link in Browser" menu
item currently calls void shell.openExternal(linkURL) which swallows promise
rejections; update the click in the linkSection (inside window-manager.ts) to
handle the returned promise and log failures (e.g.,
shell.openExternal(linkURL).catch(err => processLogger.error(...) or
console.error(...))) so any error opening the URL is not silently lost; ensure
the log includes linkURL and the error object for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 604aa6c8-32eb-4a3c-879f-b564c312be42
📒 Files selected for processing (2)
src/__tests__/main/app-lifecycle/window-manager.test.tssrc/main/app-lifecycle/window-manager.ts
Greptile SummaryThis PR wires a native right-click context menu onto the guest
Confidence Score: 4/5Safe to merge; the scheme allowlist and direct guest-webContents targeting are correctly implemented, and the 11 new tests cover the critical branches. The feature is well-scoped and the security-sensitive path (scheme validation before shell.openExternal) is correctly implemented and tested. Two minor issues were found: an unreachable guard after template construction, and a fire-and-forget shell.openExternal call that drops rejections without logging them. Neither affects correctness of the happy path. src/main/app-lifecycle/window-manager.ts — the void shell.openExternal call at the Open Link in Browser click handler. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[context-menu event on guest webContents] --> B{params.isEditable?}
B -- Yes --> C[Editable path]
C --> D{misspelledWord?}
D -- Yes --> E[Spellcheck suggestions + Add to Dictionary + separator]
D -- No --> F[Cut / Copy / Paste + separator + Select All]
E --> F
F --> Z[Menu.buildFromTemplate → popup]
B -- No --> G[Non-editable path: collect sections]
G --> H{linkURL?}
H -- Yes --> I[Copy Link + isExternallyOpenableLink? Open Link in Browser]
H -- No --> J{mediaType === image && srcURL?}
I --> J
J -- Yes --> K[Copy Image / Copy Image Address]
J -- No --> L{selectionText.trim?}
K --> L
L -- Yes --> M[Copy]
L -- No --> N[Always: Back / Forward / Reload]
M --> N
N --> O[Join sections with separators]
O --> Z
Reviews (1): Last reviewed commit: "fix(browser-tab): add right-click contex..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| click: () => { | ||
| void shell.openExternal(linkURL); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
shell.openExternal
void discards the promise, so if shell.openExternal rejects (e.g. the OS has no default handler for mailto: or the call is rate-limited), the error disappears with no log entry or user feedback. Per the project's error-handling guideline, failures should surface to Sentry via the logger rather than vanishing. Adding a .catch that logs with logger.warn would make failures visible.
| const template = buildBrowserTabContextMenuTemplate(guest, params); | ||
| if (template.length === 0) return; | ||
| Menu.buildFromTemplate(template).popup({ window: mainWindow }); |
There was a problem hiding this comment.
template is never empty
The navigation section (Back / Forward / Reload) is pushed unconditionally at the end of the non-editable path, and the editable path always returns at least four items (Cut / Copy / Paste / Select All). So template.length === 0 can never be true and this early-return is dead code. Removing it avoids misleading future readers into thinking there is a scenario where no menu should be shown.
| const template = buildBrowserTabContextMenuTemplate(guest, params); | |
| if (template.length === 0) return; | |
| Menu.buildFromTemplate(template).popup({ window: mainWindow }); | |
| const template = buildBrowserTabContextMenuTemplate(guest, params); | |
| Menu.buildFromTemplate(template).popup({ window: mainWindow }); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
chr1syy
left a comment
chr1syy
left a comment
There was a problem hiding this comment.
Nice focused fix — the security thinking (protocol allowlist before shell.openExternal, guest-targeted actions instead of edit roles, non-deprecated navigationHistory API) and the 11 tests are solid. Confirmed rc is the correct base since did-attach-webview doesn't exist on main yet (browser-tab webview feature still lives only on rc).
One blocking issue and one optional nit, both already surfaced by the bots — I'm raising the first because it conflicts with the codebase's own pattern + CLAUDE.md's error-handling guidance.
Blocking: silent rejection from shell.openExternal
src/main/app-lifecycle/window-manager.ts:198-200:
click: () => {
void shell.openExternal(linkURL);
},void drops the promise on the floor. If shell.openExternal rejects (no default browser, Launch Services failure, etc.) the user clicks "Open Link in Browser" and gets nothing — no toast, no log, no Sentry. CLAUDE.md's Error Handling & Sentry section is explicit: "DO NOT silently swallow errors."
The codebase already has the right pattern for this exact API in src/main/ipc/handlers/system.ts:230-240:
try {
await shell.openExternal(url);
} catch (err) {
const message = err instanceof Error ? err.message : String(err);
if (message.includes('Launch Services') || message.includes('No application')) {
logger.warn(`No application found to open "${url}"`, 'Shell', { error: message });
return;
}
throw err;
}A minimal fix here would be:
click: () => {
shell.openExternal(linkURL).catch((err) => {
logger.warn(`Failed to open external link: ${err instanceof Error ? err.message : String(err)}`, 'Window', { url: linkURL });
});
},Worth a one-line test ("logs a warning when shell.openExternal rejects") so future refactors don't quietly regress this back to void.
Optional nit: dead-code guard
src/main/app-lifecycle/window-manager.ts:244:
if (template.length === 0) return;Greptile flagged this as unreachable, and it is — the non-editable path unconditionally pushes the navigation section (Back/Forward/Reload), and the editable path always returns ≥5 items. I'd lean toward keeping it as a defensive belt-and-suspenders at the popup boundary (cheap insurance if the section logic ever changes), but happy either way.
Out of scope / observations
- Author's note that this also helps the paste case from #1063 is correct — paste lands in the editable path, so #1063 should be fixed transitively. Worth mentioning in the closing comment when this merges.
- Targeting
rcis correct; this can't land onmainuntil the browser-tab webview feature does.
Otherwise LGTM once the .catch() lands.
Address review feedback on #1066: - shell.openExternal rejections were silently dropped; now logged via logger.warn - document the template.length === 0 popup guard as intentional defense
…ntext-menu # Conflicts: # src/__tests__/main/app-lifecycle/window-manager.test.ts # src/main/app-lifecycle/window-manager.ts
2 participants
Closes #1065
Problem
Right-clicking inside an embedded Maestro browser tab did nothing. Electron renders no default context menu for guest
<webview>content, and the onlycontext-menuhandler inwindow-manager.tswas registered onmainWindow.webContents(the app window). The guest webContents never had one, so right-click was dead inside browser-tab pages.Fix
Wire a
context-menuhandler onto the guest webContents in the existingdid-attach-webviewblock. It builds a native menu whose actions operate on the guest webContents (edit roles would target the host window, so each item calls the guest's edit/navigation methods directly):http/https/mailtoso a malicious page can't smuggle a dangerous URL intoshell.openExternal).navigationHistory, the non-deprecated API in Electron 41).Navigation actions are always offered, so right-clicking blank background still produces a useful menu.
Notes
rc, notmain: the browser-tab webview feature (Port browser tabs into the unified rc tab strip #785) currently lives only onrc, so the fix has to land there.clipboard.writeText; existingwill-navigate/will-redirecthardening still gates any navigation the guest performs.Tests
Added 11 tests in
window-manager.test.tscovering handler registration, editable-field menu +editFlagsenable/disable, spellcheck suggestions, link copy/open + scheme guard, image copy, selection copy, navigation state, and separator hygiene. Full file (48 tests) passes;window-manager.tstype-checks and lints clean.Acceptance criteria
Summary by CodeRabbit