Skip to content
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

Adding Smart Selection to FixupRecipe #1317

Merged
merged 19 commits into from
Oct 16, 2023

Conversation

arafatkatze
Copy link
Contributor

@arafatkatze arafatkatze commented Oct 4, 2023

Original PR -> #1083 (Closed because @abeatrix added some really amazing folding range functions to get the resize the selection range)

Related Issues -> #585 and #223

Problem Summary

Currently the selection made by users is not always fully inclusive of the entire range where changes might need to be made by Cody to satisfy the request of fixup by the user. Because we're limiting the changes to only the region that was selected by the user, that creates quite a few problems:

  1. Empty Selections don't work( shown by @sqs )
image
  1. Variable renaming within a function would miss some parts of the function with the original variable name that wasn't selected
  2. Bad selection boundaries cause issues with indentation with the changes are applied coz the diffs function doesn't work perfectly in some edge cases
  3. Refactors -> If you only selected the return value then any refactor would miss other parts of the function where changes need to be applied
  4. Imperfect selection boundaries sometimes lead to repetition of code outside the selection range and sometimes it doesn't generate outputs
  5. Hard mode Problem: Its hard to do a mega refactor where change request is on a small selection but that edits multiple files and other places in the codebase to make it all work together in synergy.

Original solution

Originally i leveraged an LLM call to decide the folding range and that was a cool algorithm but it had the latency of an extra LLM call. Now that @abeatrix added some really cool folding range functions I can just leverage them to get a better range for the selection.

Video

Before

Original.Jaccard.mp4
original.partial.selection.mp4

After

https://drive.google.com/file/d/1RVShpNGiWK4wHJW6N_tHEGOdkzSwxM6d/view?usp=sharing

Test plan

Tested in my local machine a few times. Works perfectly on edge cases too.

Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the direction this is going in but have some suggestions inline.

lib/shared/src/chat/recipes/fixup.ts Outdated Show resolved Hide resolved
vscode/src/editor/vscode-editor.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
@arafatkatze
Copy link
Contributor Author

@dominiccooney Thanks a lot for the helpful comments. I fixed all the suggestions.

Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs more work. Sorry for the slow reply.

lib/shared/src/chat/recipes/fixup.ts Outdated Show resolved Hide resolved
const intent = await this.getIntent(originalFixupTask, context)

// Default to the initial task. It will be overwritten if the intent requires modification.
let finalFixupTask = originalFixupTask
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be one variable, fixupTask. It's the same task, you're just changing the selection range and text associated with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So some thoughts for this comment + this comment

I had a few different considerations here but most of the absurdity came from const and let keyword.

If I were to use const then the subsequent calls to await fixupController.getTaskRecipeData(taskId) would NOT change the value of originalFixupTask.selectionRange verified this in console so I decided to use the original and the final fixup task but you bought up a very important point that it's essentially the same fixup which is true.

So now I have to use the let key word but the keyword but the keyword can only be used if I do a reset of the fixupTask variable which is what I did in the. current solution has to do this
https://github.com/sourcegraph/cody/blob/fc363aeb1fb7006f37875eb73ce41873c9e9b81e/lib/shared/src/chat/recipes/fixup.ts#L78-L82

But that's to pass the nullity check of the original function, It's absurd but I couldn't think of a cleaner way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I understand what you're grappling with here!

I missed that getTaskRecipeData isn't returning the FixupTask, but a bag of properties based on it. You need to get the updated bag of properties after you modify the selection of the underlying FixupTask and the types including undefined is a pain.

The design of FixupController/FixupTask is weak at this point. I think we are missing a third kind of object, which is that snapshot of the state when you make a submission to the LLM. Original text should definitely live on that object, for example. But let's NOT try to clean that up now.

We are also missing an opportunity to start intent detection earlier and be more consistent. It's not 100% clear to me if every fixup task routes through this recipe, but maybe intent detection and task modification could/should be part of the fixup controller and the task's life cycle. (Today we have idle, working, ready, applying, fixed, error in CodyTaskState; maybe we can have a setup step where they get intent detection and adjustment.)

It's also clear that some paths want to skip intent detection, and the way that happens is a bit clumsy: IF the secret word /edit appears then skip intent detection. Probably because intent detection will be so slow, and it only detects "edit" or "document", which does not sound super high value (but could be if we generalize fixups so they can put their output in other places.) Instead createTask could have a hint about what intent detection the caller wants.

This are big clean-ups and I would like @abeatrix 's input/buy in on them too.

Here's what I suggest you do, short term:

  • Make getActiveFixupTextEditorSmartSelection not use the active editor. Using the active editor couples creating the task, and doing this manipulation, in time. That coupling will be fragile (if some intervening Promise, like the intent detector, slows down now it is more likely that the active editor has changed and you're reading the wrong document.) Instead, use the FixupTask and its document to get the updated range and text. Be bold digging into the FixupController and friends and making an API that's nice for this.
  • Consider making the intent detector work with the FixupTask and not the bag of data, although if it needs to retrieve the bag of data within the detector that's fine.
  • In the code you're modifying here, retrieve the fixup task from the controller.
  • If the intent is to edit, update the fixup's task range. That should happen through the controller and not by modifying the task directly because later if we had fixup requests in flight at this point, we might want to stop them, and that would involve the controller. Also we might need to coordinate with the document edit watcher.
  • Then, finally, get the bag of task data.

How does that sound?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make getActiveFixupTextEditorSmartSelection not use the active editor

That sounds good to me. @arafatkatze created getActiveFixupTextEditorSmartSelection for Fixup, so I don't think it would break other things that are using the old getActiveTextEditorSmartSelection and what you suggested makes sense.

@umpox is the one who added intent detector for Fixup, so he might have more insights to share.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domiccooney sounds great!! Lemme do this tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dominiccooney Here's the response to your comments.

Make getActiveFixupTextEditorSmartSelection not use the active editor. Using the active editor couples creating the task, and doing this manipulation, in time. That coupling will be fragile (if some intervening Promise, like the intent detector, slows down now it is more likely that the active editor has changed and you're reading the wrong document.) Instead, use the FixupTask and its document to get the updated range and text. Be bold digging into the FixupController and friends and making an API that's nice for this.

I've eliminated the editor's role entirely and transitioned the entire logic to a private function within fixupController. It's now designed to execute only when enableSmartSelection is truthy, which primarily occurs when the intent is edit.

Consider making the intent detector work with the FixupTask and not the bag of data, although if it needs to retrieve the bag of data within the detector that's fine.

I've integrated it into fixupController to leverage FixupTask directly, bypassing the initial reliance on the bag of data. This seemed more fitting, given the data bag's downstream utility and its limited relevance for intent detection. I trust this aligns with your vision.

In the code you're modifying here, retrieve the fixup task from the controller.
If the intent is to edit, update the fixup's task range. That should happen through the controller and not by modifying the task directly because later if we had fixup requests in flight at this point, we might want to stop them, and that would involve the controller. Also we might need to coordinate with the document edit watcher.

I've centralized the task range modification within fixupController, ensuring better coordination, especially in potential scenarios where we might need synchronization with the document edit watcher.

Then, finally, get the bag of task data.

This has been streamlined to ensure we fetch the data bag only once.

I hope these modifications resonate with your initial suggestions. While I acknowledge further refactoring potential, I'm hoping these changes fit the current PR's scope. If you see areas for further refinement, please guide me accordingly. I'm more than happy to iterate further.

@@ -158,6 +160,10 @@ export class NoopEditor implements Editor {
return Promise.resolve(null)
}

public getActiveFixupTextEditorSmartSelection(): Promise<vscode.Range | null> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not JavaScripty to use null as much as this interface does. just make it vscode.Range | undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree(given the rest of the codebase) but I wanted to maintain consensus with the current file itself so for now i would prefer to keep this and will change it in a separate PR AFTER this is merged

vscode/src/editor/vscode-editor.ts Outdated Show resolved Hide resolved
Comment on lines 167 to 172
* 2. Determines the current selection's start and end line numbers.
* 3. For both the start and end line numbers, it tries to find a more encompassing folding range.
* - If a folding range is found, it uses that. Otherwise, it defaults to the original selection.
* 4. The function then creates a new selection that starts at the beginning of the expanded start range
* and ends at the end of the expanded end range.
* 5. Returns this combined range as the final "smart" selection.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments make it sound more complicated than it is.

Suggested change
* 2. Determines the current selection's start and end line numbers.
* 3. For both the start and end line numbers, it tries to find a more encompassing folding range.
* - If a folding range is found, it uses that. Otherwise, it defaults to the original selection.
* 4. The function then creates a new selection that starts at the beginning of the expanded start range
* and ends at the end of the expanded end range.
* 5. Returns this combined range as the final "smart" selection.
* 2. If the selection starts in a folding range, moves the selection start back to the start of that folding range.
* 3. If the selection ends in a folding range, moves the selection end forward to the end of that folding range.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I changed this.

@@ -255,6 +255,18 @@ export class FixupController
return editOk
}

/**
* OverWrites the selectionRange of the FixupTask and selects new text corresponding to that range
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OverWrites → Overwrites

In what sense does it "select" new text? Not really. You want to leave the selected text as-is anyway so if there's a task in flight it can detect that the selected text changed so it should respin and not try to merge the result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point. I made the change.

@@ -47,4 +47,9 @@ export class FixupTask {
public get state(): CodyTaskState {
return this.state_
}

// Overwrites a selection range with a newly computed range
public overwriteSelectionRange(newRange: vscode.Range): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gut feel, do this through the fixup controller so if we need to update requests in flight the controller can do it. Who uses this anyway? It looks duplicative of resetSelectionRange on the fixup controller. And it has a name which varies from the name on the controller, seems like it is making a distinction, but is it? It seems to be doing exactly the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant so i removed it.

* and ends at the end of the expanded end range.
* 5. Returns this combined range as the final "smart" selection.
*
* @returns A Promise that resolves to an `ActiveTextEditorSelection` which represents the combined "smart" selection.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is declared to return vscode.Range, not ActiveTextEditorSelection.

@@ -99,6 +99,10 @@ export class AgentEditor implements Editor {
throw new Error('Method not implemented.')
}

public getActiveFixupTextEditorSmartSelection(): Promise<ActiveTextEditorSelection | null> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This design seems weak. What's the difference between this and getActiveTextEditorSmartSelection? If it is for a fixup, why does it operate on the text editor's active selection and not a fixup task's selection?

return null
}

const selection = activeEditor.selection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem right. Fixups have selections. If you're doing something to fixups' selections, you should be doing it to the fixup task, not based on the active text editor.

We want to make a system for doing edits to documents. It's OK to look at the active editor and selection when creating the fixup task, but after that point it is an independent object that is updated to track changes in the editor it was based off of. Dipping into the active editor this way goes against that.

So I would either expand the selection when you're creating the fixup task in the first place, and to be so focused on editing the fixup. I think this is best.

Or if you really want to edit fixups, then edit a fixup... don't dip back into the active editor but use the editor associated with the fixup and ignore that editor's active selection.

Copy link
Contributor Author

@arafatkatze arafatkatze Oct 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem right. Fixups have selections. If you're doing something to fixups' selections, you should be doing it to the fixup task, not based on the active text editor. We want to make a system for doing edits to documents. It's OK to look at the active editor and selection when creating the fixup task, but after that point it is an independent object that is updated to track changes in the editor it was based off of. Dipping into the active editor this way goes against that. So I would either expand the selection when you're creating the fixup task in the first place, and to be so focused on editing the fixup. I think this is best.

I totally agree and I contemplated this idea and wanted to do expansion right at the start of the fixup recipe. Here

cody/vscode/src/main.ts

Lines 284 to 295 in aecdd88

vscode.commands.registerCommand(
'cody.command.edit-code',
(
args: {
range?: vscode.Range
instruction?: string
document?: vscode.TextDocument
auto?: boolean
insertMode?: boolean
},
source?: string
) => executeFixup(args, source)

The only problem was that doing the expansion here would render the add intent basically untouchable coz we can't have the "no selection" case.

if (task.selectedText.trim().length === 0) {
// Nothing selected, assume this is always 'add'.
return 'add'
}

That's why I had do it at the place that I did.

Or if you really want to edit fixups, then edit a fixup... don't dip back into the active editor but use the editor associated with the fixup and ignore that editor's active selection.

I also totally understand why you are hesitant about using active editor so

        selectionRange: ActiveTextEditorSelectionRange,
        fileName: string
    ): Promise<vscode.Range | null> {
        if (!selectionRange) {
            return null
        }
        const documentUri = vscode.Uri.file(fileName)

        // Retreive the start position of the current selection
        const activeCursorStartPosition = selectionRange.start
        // If we find a new expanded selection positon then we set it as the new start position
        // and if we don't then we fallback to the original selection made by the user
        const newSelectionStartingPosition =
            (await getSmartSelection(documentUri, activeCursorStartPosition.line))?.start || selectionRange.start

        // Retreive the ending line of the current selection
        const activeCursorEndPosition = selectionRange.end
        // If we find a new expanded selection positon then we set it as the new ending position
        // and if we don't then we fallback to the original selection made by the user
        const newSelectionEndingPosition =
            (await getSmartSelection(documentUri, activeCursorEndPosition.line))?.end || selectionRange.end

        // Create a new range that starts from the beginning of the folding range at the start position
        // and ends at the end of the folding range at the end position.
        return new vscode.Range(
            newSelectionStartingPosition.line,
            newSelectionStartingPosition.character,
            newSelectionEndingPosition.line,
            newSelectionEndingPosition.character
        )
    }

SO now it doesn't use the active editor at all. I only put this function here coz I just wanted to use getSmartSelection and I couldn't think of a better way than this but it doesn't even tough any objects of the activeEditor. I think this is better than before but I am open to new ideas to improve things.

@arafatkatze
Copy link
Contributor Author

@dominiccooney Big thanks for the thorough review! Took your feedback on board and made the necessary tweaks. I realize the initial version wasn’t up to par—sorry about that. After another look and your pointers, I think it's in better shape now. Let me know if anything else jumps out.

Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for clarifying! I have some suggestions inline.

lib/shared/src/chat/recipes/fixup.ts Outdated Show resolved Hide resolved
await context.editor.showWarningMessage('Select some code to fixup.')
return null
}

const quarterFileContext = Math.floor(MAX_CURRENT_FILE_TOKENS / 4)
if (truncateText(fixupTask.selectedText, MAX_CURRENT_FILE_TOKENS) !== fixupTask.selectedText) {
if (truncateText(originalFixupTask.selectedText, quarterFileContext * 2) !== originalFixupTask.selectedText) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious but why did we change this? Is this a fudge factor so that when it broadens, it probably fits the budget?

const intent = await this.getIntent(originalFixupTask, context)

// Default to the initial task. It will be overwritten if the intent requires modification.
let finalFixupTask = originalFixupTask
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I understand what you're grappling with here!

I missed that getTaskRecipeData isn't returning the FixupTask, but a bag of properties based on it. You need to get the updated bag of properties after you modify the selection of the underlying FixupTask and the types including undefined is a pain.

The design of FixupController/FixupTask is weak at this point. I think we are missing a third kind of object, which is that snapshot of the state when you make a submission to the LLM. Original text should definitely live on that object, for example. But let's NOT try to clean that up now.

We are also missing an opportunity to start intent detection earlier and be more consistent. It's not 100% clear to me if every fixup task routes through this recipe, but maybe intent detection and task modification could/should be part of the fixup controller and the task's life cycle. (Today we have idle, working, ready, applying, fixed, error in CodyTaskState; maybe we can have a setup step where they get intent detection and adjustment.)

It's also clear that some paths want to skip intent detection, and the way that happens is a bit clumsy: IF the secret word /edit appears then skip intent detection. Probably because intent detection will be so slow, and it only detects "edit" or "document", which does not sound super high value (but could be if we generalize fixups so they can put their output in other places.) Instead createTask could have a hint about what intent detection the caller wants.

This are big clean-ups and I would like @abeatrix 's input/buy in on them too.

Here's what I suggest you do, short term:

  • Make getActiveFixupTextEditorSmartSelection not use the active editor. Using the active editor couples creating the task, and doing this manipulation, in time. That coupling will be fragile (if some intervening Promise, like the intent detector, slows down now it is more likely that the active editor has changed and you're reading the wrong document.) Instead, use the FixupTask and its document to get the updated range and text. Be bold digging into the FixupController and friends and making an API that's nice for this.
  • Consider making the intent detector work with the FixupTask and not the bag of data, although if it needs to retrieve the bag of data within the detector that's fine.
  • In the code you're modifying here, retrieve the fixup task from the controller.
  • If the intent is to edit, update the fixup's task range. That should happen through the controller and not by modifying the task directly because later if we had fixup requests in flight at this point, we might want to stop them, and that would involve the controller. Also we might need to coordinate with the document edit watcher.
  • Then, finally, get the bag of task data.

How does that sound?

@dominiccooney
Copy link
Contributor

I realize the initial version wasn’t up to par—sorry about that.

Don't be! I like the direction of this change and I'm always happy to discuss. I'm just trying to process how we can use this to make the fixups system easier to use. Sorry for all the loaded footguns we left lying around.

Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Some feedback inline. Definitely need && and not || in the condition noted below.

vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
vscode/src/non-stop/FixupController.ts Outdated Show resolved Hide resolved
arafatkatze and others added 6 commits October 13, 2023 09:05
Co-authored-by: Dominic Cooney <dominic.cooney@gmail.com>
Co-authored-by: Dominic Cooney <dominic.cooney@gmail.com>
Co-authored-by: Dominic Cooney <dominic.cooney@gmail.com>
Co-authored-by: Dominic Cooney <dominic.cooney@gmail.com>
Co-authored-by: Dominic Cooney <dominic.cooney@gmail.com>
@arafatkatze
Copy link
Contributor Author

@dominiccooney Thanks a lot for the feedback. I made all the changes and tested things one last time. You can merge this after you take a look.

@dominiccooney
Copy link
Contributor

test:e2e passed locally for me on macOS. Will follow up with dev experience about why those integration tests are failing because secrets.

@dominiccooney dominiccooney enabled auto-merge (squash) October 16, 2023 02:39
@arafatkatze
Copy link
Contributor Author

arafatkatze commented Oct 16, 2023

Will follow up with dev experience about why those integration tests are failing because secrets.

@dominiccooney That's expected behaviour: By default, GitHub does not pass secrets to workflows that are triggered from forked repositories. This is a measure to prevent malicious users from creating PRs in open-source projects and using workflows to extract secret values.
My e2e tests have always failed coz of secrets.

1 week ago
3 weeks ago

If you would like I can make extra workflows with conditional checks for forked repos and skipping steps in those cases.

@dominiccooney
Copy link
Contributor

@arafatkatze Yep, I guessed as much, but I didn't know the workflow for actually merging this because branch protection prevents me from doing that. Seems I create a copy of your PR #1398 to run the tests and then this one picks up the results.

@arafatkatze
Copy link
Contributor Author

@dominiccooney that works too thank you!!

@dominiccooney dominiccooney merged commit 4477ca2 into sourcegraph:main Oct 16, 2023
25 of 27 checks passed
abeatrix added a commit that referenced this pull request Oct 19, 2023
Fix regression caused by changes in
#1317

The commit fixes an issue where getSmartSelection was always
recalculating a selection even when a valid selectionRange was already
provided.

Currently, the /edit command does not respect user selection when it's
available:


https://github.com/sourcegraph/cody/assets/68532117/1b07e72f-3802-4f2a-8f14-f2abbb33ae84

This becomes an issue as folding range is still in the experimental
stage. It also cause the code lens to jump around, unless it's intended
as I haven't read through the PR 😅

This PR adds a check to use selectionRange when it is available and not
an empty range.

This provides a fallback in case where "smart" selection is not working
for users.

## Test plan

<!-- Required. See
https://docs.sourcegraph.com/dev/background-information/testing_principles.
-->

Select code in your editor and run the /edit command, the range should
not be expanded for you.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants