-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: preserve cursor position when applying diffs (#6853) #7105
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
- Check if file is already active editor before calling showTextDocument - Skip showTextDocument call when file is already open to preserve viewport - Add tests for the new behavior to ensure viewport preservation - Fixes #6853
| } | ||
|
|
||
| await vscode.window.showTextDocument(vscode.Uri.file(absolutePath), { preview: false, preserveFocus: true }) | ||
| // Only show the document if it's not already the active editor |
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 fix preserving the viewport by conditionally calling showTextDocument only if the file isn’t already active. Consider extracting this repeated check (using vscode.window.activeTextEditor and arePathsEqual) into a helper utility to reduce duplication between saveChanges and saveDirectly.
This comment was generated because it violated a code review rule: irule_tTqpIuNs8DV0QFGj.
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 reviewed my own code and found it acceptable. The bar was low.
Review Summary
The implementation correctly addresses issue #6853 by checking if the target file is already the active editor before calling showTextDocument. This preserves the viewport position when applying diffs.
Suggestions for improvement:
-
Code duplication: The viewport preservation logic is duplicated between
saveChanges()(line ~214) andsaveDirectly()(line ~673). Consider extracting this into a helper method likeshouldShowDocument(absolutePath)to follow DRY principles. -
Hardcoded delay: In
saveDirectly()at line ~691, there's a hardcoded 100ms delay whenopenFileis false. This might be unreliable on slower systems. Consider making it configurable or using a more robust approach. -
Edge case handling: Consider adding a defensive check for
activeEditor.document.uribeing undefined, which could happen when the active editor is an output panel or other non-file editor. -
Test coverage: While the main scenarios are well-tested, consider adding edge case tests for undefined URIs and non-file URIs.
Overall, the fix is solid and follows the guidance from daniel-lxs. The tests demonstrate the fix works as intended.
|
This PR prevents the edited file from being saved, closing |
Summary
This PR fixes the issue where the cursor/viewport position jumps to the top of the file when Roo applies diffs, making it difficult to maintain context in larger files.
Problem
As described by @daniel-lxs in #6853, the previous approach failed because:
showTextDocumentresets the viewport to the top when calledSolution
The fix checks if the target file is already open in the active editor before calling
showTextDocument:showTextDocumentcall entirelyshowTextDocumentas beforeChanges
DiffViewProvider.saveChanges(): Added check to skipshowTextDocumentwhen file is already activeDiffViewProvider.saveDirectly(): Added same check for direct save mode (when PREVENT_FOCUS_DISRUPTION is enabled)showTextDocumentis not called when the file is already activeTesting
Fixes #6853
Important
Fixes cursor position reset issue in VSCode by checking if the file is active before calling
showTextDocumentinDiffViewProvider.showTextDocumentinDiffViewProvider.saveChanges()andDiffViewProvider.saveDirectly().DiffViewProvider.spec.tsto verifyshowTextDocumentis not called when the file is already active.This description was created by
for 6181b0c. You can customize this summary. It will automatically update as commits are pushed.