fix(smart-apply): error when applying code to an empty file #7439
+53
−19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Adds a test case to handle insertion into an empty document. - Fixes a bug where due to `getDefaultSelectionRange` bump of end range document.atLine(endLineIndex) triggered an error since that line number was higher than the available lines of an empty file.
…de-3-5-sonnet-latest to get full input and output of given model Updates the default model used for smart apply to `anthropic::2024-10-22::claude-3-5-sonnet-latest`. This change allows the full 15k input for smart apply operations rather than the default 7k.
This commit updates the `getModelByID` and `getModelInfo` functions to correctly parse model IDs that use either `/` or `::` as separators. This allows for more flexible model ID formats, such as "openai::v1::gpt-4o-mini" or "openai/gpt-4o-mini". The `getModelByID` function now splits the model ID on both `/` and `::` to extract the model identifier. The `getModelInfo` function has also been updated to handle both separators. This fixes two cases: 1. You want to choose the context window for a model 2. You want to select a provider when we have a task intent In both cases you want for the system to choose the right model regardless if the dev choose to use the new ModelWithRefID style or the legacy model name (which is still used by the BYOK models)
![sourcegraph-review-agent-eap[bot]](https://avatars.githubusercontent.com/in/1118858?s=60&v=4){kind=small}
|
|
||
| const endLine = document.lineAt(startLineIndex) | ||
| if (range.end.character === 0 || endLine.text.length === 0) { | ||
| const endLine = document.lineAt(endLineIndex) |
There was a problem hiding this comment.
array-index-bug Ensure endLineIndex >= 0 before accessing document.lineAt(). Current check 'endLineIndex > 0' could still allow -1 values.
This commit updates the `getContextWindowByID` function to pass the `models` array to the `getModelByID` function. This change allows for the use of a custom model list when retrieving context window information. This is useful in scenarios where a different set of models needs to be considered than the default set.
…tion This commit adds error handling to the execution of edit tasks. Specifically, it wraps the `startStreamingEdit` call in a try-catch block. If an error occurs during the edit task, the fixup controller cancels the task, and an error message is displayed to the user. This prevents the user from being left in an inconsistent state if the edit fails, where the user sees only applying without any error. The error message prompts the user to check the logs for more details.
umpox
reviewed
Mar 19, 2025
Comment on lines
+594
to
602
| public getModelByID(modelID: string, models = this.models): Model | undefined { | ||
| // TODO(sqs)#observe: remove synchronous access here, return an Observable<Model|undefined> instead | ||
| return this.models.find(m => m.id === modelID) | ||
| // split on :: or / to get the model id | ||
| // e.g. "openai::v1::gpt-4o-mini" or "openai/gpt-4o-mini" | ||
| const modelParameters = modelID.split(/[:\/]{2}|\//) | ||
| const modelId = modelParameters.at(-1) || '' | ||
|
|
||
| return models.find(m => m.id.endsWith(modelId)) | ||
| } |
There was a problem hiding this comment.
Can we consider breaking this logic out into a different PR? It's worrying to me that it seems this logic was broken before, do we have the incorrect context window for other models too?
It'd also be useful to get some tests as splitting via a regex is going to be harder to maintain for future models than just matching an id
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refs CODY-5396
Fixed bug where inserting any code within an empty file triggered an error
which doesn't abort the smart apply action, leaving the user to think it's super slow.
This started after the work here #6768
Because
startStreamingEditTaskcalls fixupstartDecoratorhttps://github.com/sourcegraph/cody/pull/6768/files#diff-aab0c624f666dd0c22f4415b5d9cf6e2b1a9d3ceb14526e2714896beade885dfR291
And we bump the end line by one even when the file is empty
https://github.com/sourcegraph/cody/blob/main/vscode/src/non-stop/FixupTask.ts#L155
The
trimEmptyLinesFromRangeended up with an index higher than available whichcrashed https://github.com/sourcegraph/cody/pull/7439/files#diff-e0078048d45c5a1b5f5eea5af56c3919ecf75b0fe610dfee87578e87d0944ae0R87
Also used the correct model name to ensure we get 15k input instead of the default
fallback of 7k in case we have a large file that requires edits.
Test plan
Apply code in an empty file and observe it working