-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix terminal paste focus(#3199) #3206
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
Conversation
This PR addresses issue #3190 by replacing npm-run-all with npm-run-all2. Changes made: - Updated all script commands in package.json to use npm-run-all2 - Changed the devDependency from "npm-run-all": "^4.1.5" to "npm-run-all2": "^5.0.0" - Added a changeset file to document this change Benefits: - Reduced maintenance burden: npm-run-all2 is a fork with troublesome babel code removed - Improved security: Includes dependabot updates for better dependency management - Better automation: Release automation is enabled in npm-run-all2 - Same functionality: Provides the same CLI commands and API as the original package This is a minimal change that should have no impact on existing functionality.
🦋 Changeset detectedLatest commit: 1b031af The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| this.stopHotTimer() | ||
| this.emit("completed", this.removeEscapeSequences(this.fullOutput)) | ||
|
|
||
| // Restore focus to the webview after terminal operation completes |
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 focus restoration block (using setTimeout to call cline.provider.focusWebview()) is duplicated here and in the continue() method (lines 280-292). Consider extracting it into a helper method to adhere to DRY principles.
This comment was generated because it violated a code review rule: mrule_fYE6mUdYYxZL58YF.
| } | ||
|
|
||
| public async postMessageToWebview(message: ExtensionMessage) { | ||
| await this.view?.webview.postMessage(message) |
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.
Consider wrapping the await postMessageToWebview call in try/catch to log errors and handle potential failures.
This comment was generated because it violated a code review rule: mrule_OR1S8PRRHcvbdFib.
|
|
||
| // Restore focus to the webview after terminal operation is aborted or completed | ||
| const cline = this.terminal.taskId | ||
| ? vscode.extensions.getExtension("RooVetGit.roo-code")?.exports?.getClineProvider?.()?.getCurrentCline?.() |
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 feels like a reach in terms of good separation of concerns. The terminal already fires callbacks that the Cline instance is listening for, and we should use that callback mechanism instead to communicate with Cline and ClineProvider. I can update this PR to demonstrate that.
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.
Do we need this file as part of the PR?
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.
Do we need this file?
cte
left a comment
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.
Good catch; I think we should fix the issue using your general method, but I think the terminal process can communicate with Cline more cleanly per my comments inline.
Mnehmos
left a comment
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.
Hi, sorry this is like my first time attempting this and i figured since its version controlled no harm no foul, if suck then so be it.
Um no those files were just created to help me automate some of the changelogs, they are not necessary.
I've successfully fixed the issue where pasting text in the chat would sometimes paste into the terminal instead (issue #3199). Here's what I did:
Identified the root cause: The focus was not being properly restored to the webview after terminal operations.
Made the following changes:
Fixed the problematic $esbuild-watch reference in tasks.json by changing it to $tsc-watch
Added a simplified launch configuration for easier debugging
Added explicit focus restoration to the webview after terminal operations complete
Created a changeset file to document the changes for the release process
Created a PR description file with detailed information about the fix
All changes have been committed and pushed to the fix-terminal-paste-focus branch.
This fix should ensure that the focus returns to the chat input after terminal commands are executed, preventing the issue where pasting text would go to the terminal instead of the chat.
Important
Fixes chat paste issue by restoring webview focus after terminal operations in
ClineProvider.tsandTerminalProcess.ts.ClineProvider.tsandTerminalProcess.ts.$esbuild-watchreference intasks.jsonto$tsc-watch.This description was created by
for 1b031af. You can customize this summary. It will automatically update as commits are pushed.