fix(terminal): use Electron clipboard IPC for Ctrl+V paste on Windows#2008
Open
octo-patch wants to merge 2 commits intoAndyMik90:developfrom
Open
fix(terminal): use Electron clipboard IPC for Ctrl+V paste on Windows#2008octo-patch wants to merge 2 commits intoAndyMik90:developfrom
octo-patch wants to merge 2 commits intoAndyMik90:developfrom
Conversation
…tion (fixes AndyMik90#1928) When the agent writes files to the worktree but does not call `git commit`, the subsequent `gh pr create` fails with a cryptic GraphQL error: 'Head sha can\'t be blank, Base sha can\'t be blank, No commits between main and <branch>'. Fix: - Auto-stage and commit any uncommitted changes in the worktree before pushing (handles agents that skip the commit step) - After pushing, verify at least one commit exists ahead of the base branch and return a clear, actionable error message if none are found
…ndyMik90#1911) On Windows, navigator.clipboard.readText() in the Electron renderer may be silently denied due to the clipboard-read permission not being granted. This caused Ctrl+V in the agent terminal to briefly show pasted text and then clear it, with no feedback to the user. Add a TERMINAL_READ_CLIPBOARD IPC channel that reads clipboard text from the main process using Electron's clipboard module, which has full clipboard access without renderer permission restrictions. The renderer falls back to navigator.clipboard.readText() if the IPC call fails.
|
|
Contributor
📝 WalkthroughWalkthroughThis pull request adds clipboard text reading via IPC for the renderer process and updates PR creation logic with auto-commit functionality and commit count validation. Changes span the main process IPC handlers, preload API, renderer paste handling, shared IPC constants, and PR creation workflow. Changes
Sequence DiagramsequenceDiagram
participant Renderer as Renderer Process
participant Preload as Preload Bridge
participant Main as Main Process
participant Clipboard as System Clipboard
participant Navigator as Navigator API
Renderer->>Renderer: Paste event triggered
Renderer->>Preload: readClipboardText()
Preload->>Main: IPC invoke (TERMINAL_READ_CLIPBOARD)
Main->>Clipboard: clipboard.readText()
alt Success
Clipboard-->>Main: text
Main-->>Preload: text
Preload-->>Renderer: Promise resolves
Renderer->>Renderer: Paste text to terminal
else IPC Fails
Main-->>Preload: error
Preload-->>Renderer: Promise rejects
Renderer->>Navigator: fallback navigator.clipboard.readText()
Navigator-->>Renderer: text
Renderer->>Renderer: Paste text to terminal
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Labels
Poem
🚥 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 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 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| navigator.clipboard.readText() | ||
| // Use Electron's main-process clipboard API via IPC, which is reliable on all | ||
| // platforms including Windows (where navigator.clipboard.readText() may silently | ||
| // fail due to renderer permission restrictions, causing paste to appear and vanish). |
There was a problem hiding this comment.
Bug: A synchronous TypeError occurs if window.electronAPI is undefined, preventing the fallback to navigator.clipboard and breaking paste functionality in non-Electron contexts.
Severity: HIGH
Suggested Fix
Use optional chaining (?.) on window.electronAPI when calling readClipboardText(). This will prevent a synchronous error and allow the .catch() block to execute the fallback logic as intended.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/desktop/src/renderer/components/terminal/useXterm.ts#L174
Potential issue: In non-Electron contexts where `window.electronAPI` is `undefined`, the
direct property access in `window.electronAPI.readClipboardText()` will throw a
synchronous `TypeError`. This error is not handled by the `.catch()` block, which only
catches promise rejections. As a result, the fallback logic that uses
`navigator.clipboard.readText()` is unreachable. This breaks the clipboard paste
functionality in any environment where the Electron IPC API is not available, which
contradicts the stated goal of supporting non-Electron web contexts.
Did we get this right? 👍 / 👎 to inform future reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces automatic staging and committing of uncommitted changes during the PR creation process and adds a check to verify that the branch is ahead of the base. It also implements a main-process IPC handler for clipboard access to resolve permission issues on Windows. Feedback was provided to improve the robustness of the commit count check by handling cases where the remote is not named 'origin'.
Comment on lines
+372
to
+388
| try { | ||
| const commitCount = execFileSync( | ||
| gitPath, | ||
| ['rev-list', '--count', `origin/${effectiveBase}..HEAD`], | ||
| { cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' }, | ||
| ).trim(); | ||
| if (commitCount === '0') { | ||
| return { | ||
| success: false, | ||
| error: `No commits found between '${effectiveBase}' and '${branchName}'. ` + | ||
| 'The agent may not have committed any changes. ' + | ||
| 'Please verify the implementation completed successfully before creating a PR.', | ||
| }; | ||
| } | ||
| } catch { | ||
| // Unable to count commits — proceed and let `gh pr create` surface the error | ||
| } |
Contributor
There was a problem hiding this comment.
The rev-list check assumes the remote is always named origin and that the base branch can be reached via origin/${effectiveBase}. This will fail if the remote has a different name (e.g., upstream) or if the base branch already includes a remote prefix.
Consider using a fallback pattern similar to the one used in gatherPRContext to improve robustness.
try {
let commitCount: string;
try {
commitCount = execFileSync(
gitPath,
['rev-list', '--count', `origin/${effectiveBase}..HEAD`],
{ cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
).trim();
} catch {
// Fallback to using the base branch directly if origin/ prefix fails
commitCount = execFileSync(
gitPath,
['rev-list', '--count', `${baseBranch}..HEAD`],
{ cwd: worktreePath, encoding: 'utf-8', stdio: 'pipe' },
).trim();
}
if (commitCount === '0') {
return {
success: false,
error: `No commits found between '${effectiveBase}' and '${branchName}'. ` +
'The agent may not have committed any changes. ' +
'Please verify the implementation completed successfully before creating a PR.',
};
}
} catch {
// Unable to count commits — proceed and let `gh pr create` surface the error
}![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)
apps/desktop/src/main/ai/runners/github/pr-creator.ts (1)
391-398: 🧹 Nitpick | 🔵 TrivialUse
effectiveBaseconsistently for context generation and PR prompt metadata.You normalize the base branch at Line 349-352, but context gathering and AI prompt generation still use
baseBranch. IfbaseBranchisorigin/*, this triggers avoidable fallback exceptions ingatherPRContextand can make PR metadata inconsistent.Proposed refactor
- const { diffSummary, commitLog } = gatherPRContext(worktreePath, gitPath, baseBranch); + const { diffSummary, commitLog } = gatherPRContext(worktreePath, gitPath, effectiveBase); const aiBody = await generatePRBody( specId, title, - baseBranch, + effectiveBase, branchName, diffSummary, commitLog,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ai/runners/github/pr-creator.ts` around lines 391 - 398, The code normalizes the base branch into effectiveBase but continues to pass the raw baseBranch into context and prompt generation; update calls that use baseBranch—specifically gatherPRContext(...) and generatePRBody(... where aiBody is produced)—to use effectiveBase instead (retain worktreePath, gitPath, branchName, specId, title as-is) so context gathering and AI prompt metadata consistently reference the normalized base branch and avoid fallback/exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/ai/runners/github/pr-creator.ts`:
- Around line 354-358: autoCommitWorktreeChanges(...) is being called and its
result ignored, which can let PRs be created without new worktree changes;
change the call to await and handle failures: call await
autoCommitWorktreeChanges(worktreePath, gitPath, specId) and if it returns a
falsy value or throws, log an error including specId and either abort PR
creation (return/throw) or surface the error to the caller so the push/PR step
does not proceed with stale commits; update any surrounding control flow in the
PR creator to propagate the failure instead of silently continuing.
In `@apps/desktop/src/renderer/components/terminal/useXterm.ts`:
- Around line 172-176: The call to window.electronAPI.readClipboardText() can
throw synchronously when the preload API is absent; update the paste logic in
useXterm.ts to first guard that window.electronAPI exists and that
readClipboardText is a function before invoking it, and fall back to
navigator.clipboard.readText() (or other existing fallback path) if the guard
fails; apply the same protective check/try-catch pattern to the other clipboard
invocation in the same module (the later readClipboardText usage) so both places
avoid synchronous exceptions in web/test contexts.
---
Outside diff comments:
In `@apps/desktop/src/main/ai/runners/github/pr-creator.ts`:
- Around line 391-398: The code normalizes the base branch into effectiveBase
but continues to pass the raw baseBranch into context and prompt generation;
update calls that use baseBranch—specifically gatherPRContext(...) and
generatePRBody(... where aiBody is produced)—to use effectiveBase instead
(retain worktreePath, gitPath, branchName, specId, title as-is) so context
gathering and AI prompt metadata consistently reference the normalized base
branch and avoid fallback/exceptions.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f4524bd3-d7c0-4be1-b5c7-23829f997e83
📒 Files selected for processing (5)
apps/desktop/src/main/ai/runners/github/pr-creator.tsapps/desktop/src/main/ipc-handlers/terminal-handlers.tsapps/desktop/src/preload/api/terminal-api.tsapps/desktop/src/renderer/components/terminal/useXterm.tsapps/desktop/src/shared/constants/ipc.ts
Comment on lines
+354
to
+358
| // Step 1a: Auto-commit any uncommitted changes in the worktree before pushing. | ||
| // This handles the case where the agent wrote files but didn't run `git commit`. | ||
| // A failure here is non-fatal — we still attempt the push in case prior commits exist. | ||
| autoCommitWorktreeChanges(worktreePath, gitPath, specId); | ||
|
|
Contributor
There was a problem hiding this comment.
Handle auto-commit errors instead of discarding them.
autoCommitWorktreeChanges(...) can fail, but its return value is ignored. That can silently create a PR from stale commits while newly generated worktree changes remain uncommitted.
Proposed fix
- autoCommitWorktreeChanges(worktreePath, gitPath, specId);
+ const autoCommitError = autoCommitWorktreeChanges(worktreePath, gitPath, specId);
+ if (autoCommitError) {
+ return {
+ success: false,
+ error: `Failed to auto-commit worktree changes: ${autoCommitError}`,
+ };
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/ai/runners/github/pr-creator.ts` around lines 354 -
358, autoCommitWorktreeChanges(...) is being called and its result ignored,
which can let PRs be created without new worktree changes; change the call to
await and handle failures: call await autoCommitWorktreeChanges(worktreePath,
gitPath, specId) and if it returns a falsy value or throws, log an error
including specId and either abort PR creation (return/throw) or surface the
error to the caller so the push/PR step does not proceed with stale commits;
update any surrounding control flow in the PR creator to propagate the failure
instead of silently continuing.
Comment on lines
+172
to
176
| // Use Electron's main-process clipboard API via IPC, which is reliable on all | ||
| // platforms including Windows (where navigator.clipboard.readText() may silently | ||
| // fail due to renderer permission restrictions, causing paste to appear and vanish). | ||
| window.electronAPI.readClipboardText() | ||
| .then((text) => { |
Contributor
There was a problem hiding this comment.
Guard window.electronAPI before invoking clipboard IPC.
This call can throw synchronously when preload API is unavailable, which bypasses the .catch fallback and breaks paste in web/test contexts.
💡 Proposed fix
const MAX_PASTE_BYTES = 1_048_576; // 1 MB
const handlePasteFromClipboard = (): void => {
+ const pasteText = (text: string) => {
+ if (!text) return;
+ if (text.length > MAX_PASTE_BYTES) {
+ console.warn(`[useXterm] Paste truncated from ${text.length} to ${MAX_PASTE_BYTES} bytes`);
+ xterm.paste(text.slice(0, MAX_PASTE_BYTES));
+ } else {
+ xterm.paste(text);
+ }
+ };
+
+ const readFromNavigator = () =>
+ navigator.clipboard.readText()
+ .then(pasteText)
+ .catch((err) => {
+ console.error('[useXterm] Failed to read clipboard:', err);
+ });
+
// Use Electron's main-process clipboard API via IPC, which is reliable on all
// platforms including Windows (where navigator.clipboard.readText() may silently
// fail due to renderer permission restrictions, causing paste to appear and vanish).
- window.electronAPI.readClipboardText()
- .then((text) => {
- if (text) {
- if (text.length > MAX_PASTE_BYTES) {
- console.warn(`[useXterm] Paste truncated from ${text.length} to ${MAX_PASTE_BYTES} bytes`);
- xterm.paste(text.slice(0, MAX_PASTE_BYTES));
- } else {
- xterm.paste(text);
- }
- }
- })
- .catch(() => {
- // Fall back to navigator.clipboard if IPC unavailable
- navigator.clipboard.readText()
- .then((text) => {
- if (text) {
- xterm.paste(text.length > MAX_PASTE_BYTES ? text.slice(0, MAX_PASTE_BYTES) : text);
- }
- })
- .catch((err) => {
- console.error('[useXterm] Failed to read clipboard:', err);
- });
- });
+ if (window.electronAPI?.readClipboardText) {
+ window.electronAPI.readClipboardText()
+ .then(pasteText)
+ .catch(() => {
+ // Fall back to navigator.clipboard if IPC is unavailable or fails
+ readFromNavigator();
+ });
+ } else {
+ readFromNavigator();
+ }
};Also applies to: 186-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/components/terminal/useXterm.ts` around lines 172 -
176, The call to window.electronAPI.readClipboardText() can throw synchronously
when the preload API is absent; update the paste logic in useXterm.ts to first
guard that window.electronAPI exists and that readClipboardText is a function
before invoking it, and fall back to navigator.clipboard.readText() (or other
existing fallback path) if the guard fails; apply the same protective
check/try-catch pattern to the other clipboard invocation in the same module
(the later readClipboardText usage) so both places avoid synchronous exceptions
in web/test contexts.
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
2 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.
Fixes #1911
Problem
On Windows, pressing Ctrl+V in the agent terminal briefly shows the pasted text and then clears it. This happens because
navigator.clipboard.readText()in Electron's renderer process requires theclipboard-readpermission, which can be silently denied on Windows. The text that briefly appears comes from xterm.js's native paste event handler, which fires independently of our keyboard event handler. Our async clipboard read either fails silently or races with xterm's internal handling, causing the paste to vanish.Solution
Add a
TERMINAL_READ_CLIPBOARDIPC channel that reads clipboard text from the Electron main process usingelectron.clipboard.readText(). The main process has unconditional clipboard access on all platforms without requiring browser-level permissions.Changes:
ipc.ts: AddTERMINAL_READ_CLIPBOARDchannel constantterminal-handlers.ts: Register the IPC handler usingelectron.clipboardterminal-api.ts: ExposereadClipboardText()in theTerminalAPIpreload interfaceuseXterm.ts: Usewindow.electronAPI.readClipboardText()with a fallback tonavigator.clipboard.readText()Testing
navigator.clipboardensures the fix doesn't break non-Electron web contextsSummary by CodeRabbit
New Features