-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: restore cursor position after applying diffs #6855
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
Fixes #6853 - When Roo applies diffs to a file, the cursor/scroll position is now preserved instead of jumping to the top of the file. Changes: - Added savedCursorPosition and savedVisibleRanges properties to DiffViewProvider - Capture cursor position and visible ranges before opening diff view - Restore cursor position and visible ranges after saving changes - Also restore position when reverting changes or using saveDirectly method
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but somehow still broken.
| await fs.writeFile(absolutePath, content, "utf-8") | ||
|
|
||
| // Capture the current cursor position and visible ranges before any file operations | ||
| const activeEditor = vscode.window.activeTextEditor |
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.
Is this intentional? The cursor position is captured AFTER writing the file (line 705). If the file write triggers an automatic reload in VSCode, we might miss the original cursor position. Consider moving this capture block before the fs.writeFile call to ensure we always get the correct position.
|
|
||
| // Restore the visible range if it was saved | ||
| if (this.savedVisibleRanges && this.savedVisibleRanges.length > 0) { | ||
| editor.revealRange(this.savedVisibleRanges[0], vscode.TextEditorRevealType.Default) |
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 notice we're using TextEditorRevealType.Default here for saved ranges, but InCenterIfOutsideViewport when no saved range exists (line 237). The same pattern appears in revertChanges() and saveDirectly(). Would it be better to use InCenterIfOutsideViewport consistently to ensure the cursor is always visible to the user?
| }) | ||
|
|
||
| // Restore the saved cursor position and visible range | ||
| if (this.savedCursorPosition) { |
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 cursor restoration logic is duplicated across saveChanges(), revertChanges(), and saveDirectly(). Could we extract this into a private helper method like:
private restoreCursorPosition(editor: vscode.TextEditor): void {
if (this.savedCursorPosition) {
editor.selection = new vscode.Selection(this.savedCursorPosition, this.savedCursorPosition);
if (this.savedVisibleRanges && this.savedVisibleRanges.length > 0) {
editor.revealRange(this.savedVisibleRanges[0], vscode.TextEditorRevealType.Default);
} else {
editor.revealRange(
new vscode.Range(this.savedCursorPosition, this.savedCursorPosition),
vscode.TextEditorRevealType.InCenterIfOutsideViewport
);
}
}
}This would reduce duplication and make future maintenance easier.
|
Closing this PR as the approach has a fundamental issue: VSCode's API doesn't provide a way to get the actual cursor position (caret position) - only the selection. The property returns the position where the selection ends, not necessarily where the cursor is visible to the user. The real issue is that when we apply diffs, we're showing the document which causes VSCode to reset the viewport. A better approach might be to:
Going back to the issue to properly scope this solution. |
Fixes #6853
Problem
When Roo applies diffs to a file that the user is currently viewing, the scroll position jumps to the top of the file instead of staying where the user was. This makes it difficult to maintain context in larger files, requiring users to constantly scroll back to where they were working.
Solution
This PR preserves the cursor position and visible range when applying diffs:
Capture cursor state: Before opening the diff view or applying changes, we now capture the current cursor position and visible ranges if the file is open in the active editor.
Restore after save: After saving changes through the diff view, we restore the saved cursor position and visible range, keeping the user at the same location they were before the diff was applied.
Handle all scenarios: The fix works for:
saveChanges()saveDirectly()revertChanges()Changes
savedCursorPositionandsavedVisibleRangesproperties toDiffViewProviderclassopen()method to capture cursor state before showing diffsaveChanges()to restore cursor position after savingrevertChanges()to restore cursor position when file was already opensaveDirectly()to preserve cursor position when PREVENT_FOCUS_DISRUPTION is enabledreset()methodTesting
This significantly improves the user experience when working with Roo on larger files, as users no longer lose their place after each diff application.
Important
Preserves cursor position and visible range in
DiffViewProviderwhen applying diffs, enhancing user experience.DiffViewProvider.saveChanges(),saveDirectly(), andrevertChanges().savedCursorPositionandsavedVisibleRangesproperties toDiffViewProvider.open()to capture cursor state before showing diff.saveChanges(),revertChanges(), andsaveDirectly()to restore cursor position after operations.reset()method.This description was created by
for 990ba4a. You can customize this summary. It will automatically update as commits are pushed.