-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: preserve chat input focus during automated file editing (#4574) #5282
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
- Add preserveFocus: true to showTextDocument call in DiffViewProvider - Prevents focus from being stolen from chat input during auto-mode file edits - Update tests to reflect new expected behavior Fixes #4574
|
✅ No security or compliance issues detected. Reviewed everything up to 18dcd17. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. Reply to this PR with |
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.
Pull Request Overview
This PR ensures the chat input keeps focus during automated file edits by adding preserveFocus: true to showTextDocument calls and updating related tests.
- Added
preserveFocus: truetovscode.window.showTextDocumentinDiffViewProvider.ts. - Updated unit tests in
DiffViewProvider.spec.tsto expect the newpreserveFocusoption.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/integrations/editor/DiffViewProvider.ts | Added preserveFocus: true to showTextDocument options. |
| src/integrations/editor/tests/DiffViewProvider.spec.ts | Updated tests to assert preserveFocus: true in options. |
Comments suppressed due to low confidence (1)
src/integrations/editor/DiffViewProvider.ts:504
- [nitpick] Update this comment to mention that
preserveFocus: trueis now used to keep the chat input focused during automated edits, so the comment stays aligned with the code change.
// Pre-open the file as a text document to ensure it doesn't open in preview mode
| vi.mocked(vscode.window.showTextDocument).mockImplementation(async (uri, options) => { | ||
| callOrder.push("showTextDocument") | ||
| expect(options).toEqual({ preview: false, viewColumn: vscode.ViewColumn.Active }) | ||
| expect(options).toEqual({ preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true }) |
Copilot
AI
Jun 30, 2025
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.
[nitpick] This strict equality assertion can be brittle if more options are added in the future. Consider using expect.objectContaining or toMatchObject to only verify the relevant keys.
| expect(options).toEqual({ preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true }) | |
| expect(options).toEqual(expect.objectContaining({ preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true })) |
| expect(vscode.window.showTextDocument).toHaveBeenCalledWith( | ||
| expect.objectContaining({ fsPath: `${mockCwd}/test.md` }), | ||
| { preview: false, viewColumn: vscode.ViewColumn.Active }, | ||
| { preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true }, |
Copilot
AI
Jun 30, 2025
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.
[nitpick] Similarly, this exact match could break if additional options are introduced. Using expect.objectContaining for the options object will make the test more resilient.
| { preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true }, | |
| expect.objectContaining({ preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true }), |
| // This fixes issues with files that have custom editor associations (like markdown preview) | ||
| vscode.window | ||
| .showTextDocument(uri, { preview: false, viewColumn: vscode.ViewColumn.Active }) | ||
| .showTextDocument(uri, { preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true }) |
Copilot
AI
Jun 30, 2025
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.
[nitpick] Extract the options object into a named constant (e.g., const showOptions = { preview: false, viewColumn: ..., preserveFocus: true };) to reduce duplication and improve readability.
| .showTextDocument(uri, { preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true }) | |
| .showTextDocument(uri, showOptions) |
- Use expect.objectContaining in tests for better resilience to future option additions - Extract showTextDocument options to named constant for improved readability - Addresses Copilot review feedback on PR #5282
hannesrudolph
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.
This PR successfully addresses issue #4574 by adding preserveFocus: true to the showTextDocument call in DiffViewProvider. The implementation prevents the chat input from losing focus during automated file editing workflows.
The Copilot review suggestions have been addressed in the latest commit:
- Options extracted to a named constant
- Tests updated to use
expect.objectContainingfor better resilience
I've identified a few areas for consideration:
- Other
showTextDocumentcalls in the codebase might benefit from the same pattern - The comment could be updated to reflect the focus preservation purpose
- Integration tests for actual focus behavior could help prevent regressions
The core implementation effectively solves the reported issue where user input could be accidentally typed into files during automated workflows.
|
|
||
| // Pre-open the file as a text document to ensure it doesn't open in preview mode | ||
| // This fixes issues with files that have custom editor associations (like markdown preview) | ||
| const showOptions = { preview: false, viewColumn: vscode.ViewColumn.Active, preserveFocus: true } |
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.
I noticed that preserveFocus: true is consistently used in other showTextDocument calls within this file (lines 199, 374, 441), which is great. However, there are other places in the codebase that might benefit from this pattern:
src/services/marketplace/MarketplaceManager.ts:129src/integrations/misc/export-markdown.ts:40
Is it intentional that these other locations don't preserve focus? If they could also cause focus issues during automated workflows, would it make sense to apply the same pattern there for consistency?
| }), | ||
| ) | ||
|
|
||
| // Pre-open the file as a text document to ensure it doesn't open in preview mode |
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.
Since we're now also preserving focus to fix the chat input issue, would it be helpful to update this comment to reflect both purposes? Something like:
// Pre-open the file as a text document to ensure it doesn't open in preview mode
// This fixes issues with files that have custom editor associations (like markdown preview)
// and preserves focus on the chat input during automated editing workflows| expect.objectContaining({ | ||
| preview: false, | ||
| viewColumn: vscode.ViewColumn.Active, | ||
| preserveFocus: true, |
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.
While the tests correctly verify that preserveFocus: true is passed to the API, they don't test the actual focus behavior. Have you considered adding an integration test that verifies the chat input maintains focus during file operations?
I understand this might be challenging in a unit test environment, but it could be valuable for preventing regressions of this specific bug. Perhaps this could be added to the E2E test suite if it's not feasible here?
|
This doesn't seem to work, it seems like |
|
It seems that the focus is lost sometimes but overall it seems to work most of the times, maybe we can make some more tweaks to make sure it keeps the focus all the time. |
|
Closing since @hannesrudolph will open a new one |
Description
Fixes #4574
This PR fixes the issue where the chat input box loses focus during automated file editing workflows, causing user input to be typed into files instead of the chat.
Changes Made
preserveFocus: trueoption to theshowTextDocumentcall inDiffViewProvider.ts(line 507)DiffViewProvider.spec.tsto reflect the new expected behaviorTesting
preserveFocus: trueVerification of Acceptance Criteria
Checklist
Important
Adds
preserveFocus: trueto maintain chat input focus during automated file editing inDiffViewProvider.ts.preserveFocus: truetoshowTextDocumentinDiffViewProvider.tsto maintain chat input focus during automated file editing.DiffViewProvider.spec.tsto verifypreserveFocus: truebehavior.This description was created by
for 18dcd17. You can customize this summary. It will automatically update as commits are pushed.