fix(browser-tab): paste into webview form fields via guest.paste() on cmd/ctrl+v#1088
Conversation
|
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)
📝 WalkthroughWalkthroughThis PR adds clipboard paste support for form fields inside Maestro browser-tab guests. The implementation defines a ChangesPaste Support for Browser-Tab Guests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
Greptile SummaryThis PR fixes silent paste failures (Cmd/Ctrl+V) inside Electron
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to the Cmd/Ctrl+V chord in the webview before-input-event handler and does not touch any auth, data persistence, or security-boundary code. The fix is minimal and self-contained: one new guard, one explicit privileged call, and two consistent passthrough-list removals across the main-process and renderer injections. The clipboard-read permission denial is preserved; guest.paste() fires only on a real user keystroke. The new tests exercise both the happy path (Cmd+V, Ctrl+V) and the non-paste cases (plain v, Cmd+Shift+V), and vi.clearAllMocks() in beforeEach ensures clean mock state between the two new tests. No regressions observed. No files require special attention — the renderer-side keyboardInjection string change in BrowserTabView.tsx is not covered by the new unit tests, but it is a straightforward one-character removal that mirrors the already-tested main-process change. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Electron as Electron (Main)
participant Guest as Guest WebContents (webview)
participant Page as Page JS (injected listeners)
participant Renderer as App Renderer
User->>Electron: Cmd/Ctrl+V keydown
Electron->>Electron: before-input-event fires
Note over Electron: isPaste = true, event.preventDefault()
Electron->>Guest: guest.paste()
Note over Guest: Privileged paste — no clipboard-read needed
Note over Page: Event never reaches page (preventDefault consumed it)
User->>Electron: Cmd+A keydown
Electron->>Electron: before-input-event fires
Note over Electron: isTextEditing = true, return (no preventDefault)
Electron->>Page: Event dispatched to page
Note over Page: acxz.indexOf(a) !== -1, return (native select-all proceeds)
User->>Electron: Cmd+Shift+V keydown
Electron->>Electron: before-input-event fires
Note over Electron: isPaste=false (shift=true), isTextEditing=false, event.preventDefault()
Electron->>Renderer: "IPC browser-tab:shortcutKey {key:v, shift:true}"
Reviews (2): Last reviewed commit: "fix(browser-tab): align capture-phase ke..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Cmd+Shift+V (paste-and-match-style etc.) is not the plain paste chord. | ||
| const shiftEvent = { preventDefault: vi.fn() }; | ||
| beforeInputHandler?.(shiftEvent, { | ||
| type: 'keyDown', | ||
| key: 'v', | ||
| code: 'KeyV', | ||
| meta: true, | ||
| control: false, | ||
| alt: false, | ||
| shift: true, | ||
| }); | ||
|
|
||
| expect(mockGuestWebContents.paste).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
The isPaste guard explicitly requires !input.alt, so Ctrl+Alt+V (which on Windows/Linux AltGr keyboards produces @ or other characters, not a paste) correctly passes through. There is no test for this combination, however, so a future change to the modifier logic that accidentally drops the !alt guard would not be caught. Adding a Ctrl+Alt+V case to the "does not hijack" test would make this invariant explicit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/__tests__/main/app-lifecycle/window-manager.test.ts (1)
1337-1385: ⚡ Quick winThis test does not prove Cmd/Ctrl+Shift+V is not hijacked.
Line 1337 says non-paste edit chords should pass through, but the only assertion is that
guest.paste()stays untouched. The current handler still callspreventDefault()and forwardsbrowser-tab:shortcutKeyfor Shift+V, so this test passes even when the page loses the chord. Either assert nopreventDefault/send, or rename the test to the narrowerguest.paste()contract.🤖 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/__tests__/main/app-lifecycle/window-manager.test.ts` around lines 1337 - 1385, The test currently only asserts that mockGuestWebContents.paste() is not called but doesn't verify that the handler doesn't intercept the chord (calls preventDefault or forwards shortcut); update the test for beforeInputHandler (retrieved from guestWebContentsEventHandlers) for the Cmd/Ctrl+Shift+V case to also assert the event.preventDefault was NOT called and that mockGuestWebContents.send was NOT called with 'browser-tab:shortcutKey' (or, if you prefer the narrower intent, rename the test to indicate it only asserts guest.paste() is not invoked). Ensure you reference the same beforeInputHandler, mockGuestWebContents, and preventDefault/send checks used elsewhere in this test file.
🤖 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.
Inline comments:
In `@src/main/app-lifecycle/window-manager.ts`:
- Around line 373-377: The renderer-side guest shortcut filter in
BrowserTabView.tsx still includes 'v' in the allowlist ('acvxz'), which
mismatches the window-manager filter that uses 'acxz'; update the allowlist
string in BrowserTabView (the shortcut filter logic around the guest interceptor
in BrowserTabView.tsx lines ~376-416) to remove 'v' so it becomes 'acxz', and
update the surrounding comment to match the main-process comment about letting
standard text-editing shortcuts pass through (explicitly noting that Cmd/Ctrl+F
and Cmd/Ctrl+V are handled differently as in window-manager.ts).
---
Nitpick comments:
In `@src/__tests__/main/app-lifecycle/window-manager.test.ts`:
- Around line 1337-1385: The test currently only asserts that
mockGuestWebContents.paste() is not called but doesn't verify that the handler
doesn't intercept the chord (calls preventDefault or forwards shortcut); update
the test for beforeInputHandler (retrieved from guestWebContentsEventHandlers)
for the Cmd/Ctrl+Shift+V case to also assert the event.preventDefault was NOT
called and that mockGuestWebContents.send was NOT called with
'browser-tab:shortcutKey' (or, if you prefer the narrower intent, rename the
test to indicate it only asserts guest.paste() is not invoked). Ensure you
reference the same beforeInputHandler, mockGuestWebContents, and
preventDefault/send checks used elsewhere in this test file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a53050a-e724-44b8-ad6a-398c446ff298
📒 Files selected for processing (2)
src/__tests__/main/app-lifecycle/window-manager.test.tssrc/main/app-lifecycle/window-manager.ts
|
@jSydorowicz21 Thanks for the contribution, and for the thorough writeup and tests! The core approach is solid: routing Cmd/Ctrl+V through the privileged One change before this is ready to merge: Align the third allowlist in This PR removes
That one is the renderer-injected capture-phase keydown listener on the same guest webview (the comment at lines 376-384 references "the main-process-injected bubble-phase one"). It still treats Cmd/Ctrl+V as page-native, which now disagrees with the native handler and the bubble-phase fallback you did update. This is the exact inconsistency your own test guards against in the bubble-phase script ( Could you update Minor / optional (not blocking):
No merge conflicts, so nothing to rebase. Once the |
|
Thanks @pedramamini - good catch on the third allowlist. Pushed
tsc clean on touched files; |
|
@greptile re-review |
2 participants
Fixes #1063
Problem
Pasting (Cmd/Ctrl+V) clipboard text into a focused input/textarea/contenteditable inside an embedded browser tab (Electron
<webview>) did nothing. The webview permission handler deniesclipboard-readto webviews as a security boundary, so Chromium's native paste silently fails inside browser-tab form fields.Fix
src/main/app-lifecycle/window-manager.ts, in the webviewbefore-input-eventhandler:(meta || control) && !alt && !shift && key === 'v', callevent.preventDefault(), and invokeguest.paste()- a privileged main-process Electron call that performs the paste without granting the page web-facing clipboard-read access.vfrom the residual text-editing passthrough list (now handled explicitly) in both the native handler and the injected in-page capture listener, keeping the two layers consistent.Security
The
clipboard-readpermission denial is left intact.guest.paste()fires only in response to a real user keystroke and does not grant the page clipboard-read, so a malicious page cannot read the clipboard on demand. Mirrors how the right-click Paste menu item works.Tests
guest.paste()once withpreventDefault, and that plainv/ Cmd+Shift+V do not.rcbaseline (zero regression);window-manager.test.tspasses.Notes
BrowserTabGuestContents/before-input-eventwebview infrastructure already exists onrc, so this change is self-contained and merges independently (no dependency on the Right-click context menu does not work inside embedded browser tabs #1065/fix(browser-tab): right-click context menu inside embedded webview tabs #1066 context-menu PR).Summary by CodeRabbit
Bug Fixes
Tests