Con 991: Diff Manager Improvements #1137
Conversation
| const diffHandler = this.createVerticalPerLineDiffHandler( | ||
| filepath, | ||
| existingHandler?.range.start.line ?? startLine, |
There was a problem hiding this comment.
I wasn't sure why flexibility to reuse the existing handler was enabled - was the intention at some point to support multiple diffs per file?
There was a problem hiding this comment.
I believe this is used for cmd+I to "add further instructions". https://github.com/continuedev/continue/blob/preview/extensions/vscode/src/commands.ts#L238
This might actually complicate this PR a bit. If there is a currently opened diff, then using cmd+I again should prompt the user to add further instructions, which looks like ", ".
One way of moving forward is to remove the previous diff (as you've implemented here) if there is any text selected, or "re-prompt" if the cursor selection is empty
There was a problem hiding this comment.
I passed the 'previousInput' variable from commands into streamEdit as an optional parameter and used it as a flag.
This variable will have a value if it comes from cmd+I, or will be undefined otherwise.
| await new Promise((resolve) => { | ||
| setTimeout(resolve, 200); | ||
| }); | ||
|
|
||
| const startLine = editor.selection.start.line; |
There was a problem hiding this comment.
determine start/end after the old block has been deleted
|
Last commits makes these changes compatible with cmd+I as well as actually reject/accept all blocks in the given file when the user clicks the 'reject/accept all' code lens. |
| @@ -4,6 +4,7 @@ import { getMarkdownLanguageTagForFile } from "core/util"; | |||
| import { streamDiffLines } from "core/util/verticalEdit"; | |||
| import * as vscode from "vscode"; | |||
| import { VerticalPerLineDiffHandler } from "./handler"; | |||
| import { start } from "repl"; | |||
There was a problem hiding this comment.
@justinmilner1 this is last thing, assuming it's safe to delete I can go ahead and do that while I merge
There was a problem hiding this comment.
@sestinj whoops yeah that's not supposed to be in there
| toHandle = blocks | ||
| } | ||
|
|
||
| for (const block of toHandle) { |
There was a problem hiding this comment.
@justinmilner1 cd extensions/vscode && npx tsc raises an error here, and it looks like toHandle could be either a list or single value from blocks. I assume the intended logic is to loop through the list if it's a list, otherwise handler.acceptRejectBlock for just blocks[index]?
There was a problem hiding this comment.
@sestinj Sorry that was sloppy.
Pushed up a fix. Tested more thoroughly.
|
@justinmilner1 I'm doing some tests now locally. First thing that comes up is accept/reject (not all, but the buttons to do it for each block individually) seems like it's regressed a bit. The first thing I saw is that if I were to "Accept" the top block, then the full "Accept All / Reject All / Accept / Reject" codelens would remain in the same position. Whereas in current versions of the extension, you can see it move down to the new top block. I'm not certain which part of the PR this is coming from, but my guess is that whatever attempts to solve the new problem you found are the cause? |
This work has been continued in #1198
Description
This MR fixes two main issues:
Also, a bug was discovered: Reject/Accept all does not work when there are multiple blocks - instead only the first block receives the apply (I plan on fixing this soon (this weekend))
Update: 4/18 - added accept/reject all blocks on 'accept/reject all'
Resolves:
#991