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

Fix monaco model reference creation #14957

Merged
merged 1 commit into from
Feb 19, 2025
Merged

Conversation

msujew
Copy link
Member

@msujew msujew commented Feb 18, 2025

What it does

Closes #7671
Closes #14842

As outlined in #14842, the culprit for a lot of these issues seem to be that we don't actually create real MonacoEditorModel references for our inline editors. These models weren't synced with the plugin host and so we encountered a ton of errors within the plugin host which were then simply propagated and logged in the frontend.

This fixes the issue by ensuring that we actually create a Resource for each MonacoEditorModel that we attempt to create. This ensures that the models are correctly send to the plugin host.

Additionally adjusts how we create editor models for the notebook to ensure that they are all placed in one single reference list. Previously we had 2 lists, which then led to clashes inside of monaco.

Also fixes a very minor issue related to completion and language support in notebook editors.

How to test

  1. Open a JS file and add a conditional breakpoint.
  2. Open the AI chat widget and type something.
  3. Ensure that no errors about unknown models/files is being logged
  4. Open a notebook (.ipynb) file and scroll through it.
  5. Ensure that no errors about duplicate monaco models are being logged.

Please also ensure that #14924 remains fixed. I couldn't reproduce it (but I also only tested on Windows)

Review checklist

Reminder for reviewers

@msujew msujew added monaco issues related to monaco notebook issues related to notebooks labels Feb 18, 2025
@msujew msujew requested a review from jonah-iden February 18, 2025 11:26
@msujew msujew force-pushed the msujew/fix-monaco-model-references branch from 28499a7 to 4db1f75 Compare February 18, 2025 11:27
@msujew msujew force-pushed the msujew/fix-monaco-model-references branch from 4db1f75 to e66f900 Compare February 18, 2025 12:37
@msujew
Copy link
Member Author

msujew commented Feb 18, 2025

cc @sdirix Can you check whether this does not reintroduce #14924?

@msujew msujew requested a review from sdirix February 18, 2025 12:39
@sdirix
Copy link
Member

sdirix commented Feb 18, 2025

@msujew Thanks for the work!

Question: Can we somehow disconnect a Monaco Editor from the PluginHost instead of fully integrating them? For example we don't want the gitLense extension to be active within the inline editors like the Chat view, or the vim extension interfering with an inline Monaco.

If this is off-topic we can also deal with this in a follow up of course

Edit: Looking at the code, it seems you did exactly that with the MonacoEditorModelFilter, right?

@msujew
Copy link
Member Author

msujew commented Feb 18, 2025

Edit: Looking at the code, it seems you did exactly that with the MonacoEditorModelFilter, right?

Yes, the filter will prevent the model from being propagated to to plugin host. However, in order to prevent the errors from being logged, we currently want to send the models to the plugin host. A different fix for this would include 2 parts:

  1. Create filters for those specific models
  2. Check whether a model reference exists for a URI before calling the plugin host (i.e. when calling provideCodeActions).

@tsmaeder
Copy link
Contributor

@sdirix the monaco StandaloneCodeEditor can be largely configured. See IEditorOptions in the VS Code codebase. In particular code lens can be turned off. Not sure we're surfacing this configuration when creating inline editors, but it feels like a cleaner way to turn off editor features.

Copy link
Contributor

@jonah-iden jonah-iden left a comment

Choose a reason for hiding this comment

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

Looks like a nice solution to me.

Im just getting a few different errors sometimes when opening notebooks like:

Assertion failed (document 'vscode-notebook-cell:/c%3A/Typefox/Open_Source/theia_test_workspace/notebooks/1-cell-test.ipynb#W3sZmlsZQ%3D%3D' doesn't exist or
A resource provider for 'vscode-notebook-cell:/c%3A/Typefox/Open_Source/theia_test_workspace/notebooks/1-cell-test.ipynb#W0sZmlsZQ%3D%3D

But thats probably out of scope for this PR?

@sdirix
Copy link
Member

sdirix commented Feb 18, 2025

I can confirm that #14924 is still fixed!

@msujew
Copy link
Member Author

msujew commented Feb 19, 2025

But thats probably out of scope for this PR?

@jonah-iden I can't really reproduce those error logs. I would assume they're either unrelated or a consequence of existing errors that are only uncovered through this change. I would indeed try to fix them in a separate PR.

@msujew msujew merged commit 9e43c93 into master Feb 19, 2025
10 of 11 checks passed
@msujew msujew deleted the msujew/fix-monaco-model-references branch February 19, 2025 16:30
@github-actions github-actions bot added this to the 1.59.0 milestone Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco notebook issues related to notebooks
Projects
Archived in project
4 participants