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

Try better fix for syncing code blocks once complete #233455

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mjbvz
Copy link
Collaborator

@mjbvz mjbvz commented Nov 8, 2024

Follow up on #232536
Also related #232538

This code is already complicated and swapping models broke some cases. Let's try a more proper fix by letting us control when shouldSynchronizeModel returns true

Follow up on microsoft#232536
Also related microsoft#232538

This code is already complicated and swapping models broke some cases. Let's try a more proper fix by letting us control when `shouldSynchronizeModel` returns true
@mjbvz mjbvz added this to the November 2024 milestone Nov 8, 2024
@mjbvz mjbvz requested a review from alexdima November 8, 2024 23:43
@mjbvz mjbvz self-assigned this Nov 8, 2024
@alexdima
Copy link
Member

Making text buffers that are not synced become synced and vice-versa is a quite tricky change and might introduce subtle bugs.

I don’t quite think this is the right approach because it ignores the same problem occurring in Inline Chat and for Copilot Edits. Here in my previous comment I was trying to express that we need a solution that covers all our use-cases of rapidly updating a text buffer:

The same problem exists with cross file edits. We stream edits to the text buffer directly. The text buffer is even usually shown in an editor. I would prefer to address the problematic language support or add API to indicate that more changes are expected to avoid triggering expensive computations

So in other words, while this suggested approach solves the problem for code blocks streaming in to the panel, it doesn't solve the problem for regular files being rapidly edited by Copilot. Would it be possible to find a holistic approach to this problem, I'm thinking onWillStartRapidEdits / onDidEndRapidEdits if language servers need this.

One final qq, why is editing a file an expensive operation for languages? Is it our /contrib/ code that triggers various things like semantic editing, folding, etc. Can we make our /contrib/ code aware of the onWillStartRapidEdits/onDidEndRapidEdits or make a new model.isUndergoingRapidEdits which could prevent some UI /contrib/s from making language requests?

@mjbvz
Copy link
Collaborator Author

mjbvz commented Nov 14, 2024

Thanks @alexdima. I like the idea of putting the model into a temporary state for rapid updates. I will take a look into doing this. Since I think that may time some time though, for now I will submit a separate PR that reverts the original dual model approach as this is causing UI issues in chat

One final qq, why is editing a file an expensive operation for languages

For TypeScript specifically, the issues was actually triggered just by synchronizing the document. Sending many rapid updates often meant that TypeScript saw the code in an incomplete state that you rarely end up in during normal coding. This revealed some crashing bugs in the TypeScript server. We've already fixed the main crash but these rapid updates still cause TS to do a lot of processing of the file changes, even when no language feature requests are being made

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.

2 participants