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

Ensure proper sequencing of notebook cell edits #241350

Merged
merged 4 commits into from
Feb 20, 2025
Merged

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Feb 20, 2025

Problem
Assume we generate a single text edit for a single cell in a notebook
Sequence of messages emitted is as follows

1. Empty NotebookEdit [] (URI = Notebook URI `file://sample.ipynb`)
2. Text Edit for Cell URI  (URI = Cell URI= `vscode-notebook-cell://sample.ipynb?w234`)
3. NotebookEdito isDone=true (URI = Notebook URI `file://sample.ipynb`)
```

What happens with the the messages is as follows:
UpdateContent of `changeModel.ts` stores the messages in the following manner
_responseParts = [
    { 
        kind: 'notebookEditGroup',
        uri: <Notebook URI>,
        edits: [[]],
        done: true
    },
    { 
        kind: 'textEditGroup',
        uri: <Cell URI>,
        edits: [[]],
        done: true
    },
]

And they get process as follows in handleResponseParts of chatEditingServiceImpl:

1. First process the first empty element in `notebookEditGroup`
2. After we're done processing all `edits` in the `notebookEditGroup`, we then move onto `done`.
3. At this point `done = true` for `notebookEditGroup`, hence the ModifiedFileEntry gets the `done` and we complete the stream and resolve chat session, etc.

NOTE: None of the cell edits have been handled.
I.e. the order is now wrong, the text edits were sent before the `notebook.done = true`.

4. Now we process the notebook cell edit operations.
However this is out of sequence.

I think the problem lies with the fact that the textEdits are not treated as part of the notebookEditGroup.
Ideally they should be part of that so that they are processes in order.
Or we bring the knowledge of notebook Cell Edits into this area and look at the Uris and do something else to ensure any text edits for a cell URI that belong to an existing notebookgroup must be processed first and the notebookGroup.done must be processed after all other textEditGroup... feels icky..

@jrieken @connor4312 /cc

@DonJayamanne DonJayamanne self-assigned this Feb 20, 2025
@DonJayamanne DonJayamanne marked this pull request as ready for review February 20, 2025 15:09
export interface IChatNotebookEditGroup {
uri: URI;
edits: ICellEditOperation[][];
edits: (ICellTextEditOperation | ICellEditOperation)[];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Text edits belonging to a notebook must belong to notebookGroup
This way the order is preserved and its clear these text edits belong to notebooks.

@vs-code-engineering vs-code-engineering bot added this to the February 2025 milestone Feb 20, 2025
@connor4312
Copy link
Member

When you call startStreamingEdits, it sequences operations based on the resource URI passed in. Maybe the way you want to do this is to call that for cell URIs individually or something, and then update the ChatEditingService to keep track of them.

@DonJayamanne
Copy link
Contributor Author

DonJayamanne commented Feb 20, 2025

it sequences operations based on the resource URI passed in. Maybe the way you want to do this is to call that for cell URIs individually or something, and then update the ChatEditingService to keep track of them.

Sorry not following you here.
@connor4312 Sounds like you are proposing a different implementation, please could you elaborate, thanks

connor4312
connor4312 previously approved these changes Feb 20, 2025
Copy link
Member

@connor4312 connor4312 left a comment

Choose a reason for hiding this comment

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

Looking more, I think this should work fine :) I might do a pass later and see if I can get rid of the any cast (we had one there before, so not something you need to clean in this PR unless you want to)

@DonJayamanne DonJayamanne enabled auto-merge (squash) February 20, 2025 18:57
@rebornix rebornix closed this Feb 20, 2025
auto-merge was automatically disabled February 20, 2025 23:15

Pull request was closed

@rebornix rebornix reopened this Feb 20, 2025
@rebornix rebornix enabled auto-merge (squash) February 20, 2025 23:17
@rebornix rebornix merged commit 69f9bc0 into main Feb 20, 2025
12 checks passed
@rebornix rebornix deleted the don/wispy-silverfish branch February 20, 2025 23:40
@@ -708,13 +708,20 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio
let didComplete = false;

return {
pushText: edits => {
pushText: (edits) => {
Copy link
Member

Choose a reason for hiding this comment

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

@connor4312 @DonJayamanne Can we align these and just have pushEdits(uri, edits) because all these call _acceptEdits

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.

4 participants