feat(editors): indicate when editor contains error or warning #1816
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| } | ||
| }, | ||
| ); | ||
| window.monaco.editor.onDidChangeMarkers(this.setSeverityLevels.bind(this)); |
There was a problem hiding this comment.
question: does this binding need to be cleaned up on destroy/ in the constructor?
There was a problem hiding this comment.
I think that's a good callout. After looking a bit more, I don't think there's an easy way of disposing of this callback since JavaScript classes don't provide a lifecycle hook for when the class gets destroyed (in the same way that React does when a component gets unmounted).
Since EditorMosaic never gets destroyed in a Fiddle instance (we keep the same instance and recycle editors whenever possible), I think the potential for a large memory leak is reduced (but I could be wrong).
If this proves to be a problem, we might want to look into the FinalizationRegistry API to manually call the dispose() function when the EditorMosaic object gets GCed.
The whole "Avoid where possible" section in that API makes me just a bit wary for now.
Happy for pushback on this assessment, though.
| <RemoveButton id={id} appState={appState} /> | ||
| public render() { | ||
| const { editorMosaic } = this.props.appState; | ||
| const severityLevel = toJS(editorMosaic.editorSeverityMap); |
There was a problem hiding this comment.
question: can we just have const severityLevel = editorMosaic.editorSeverityMap here?
There was a problem hiding this comment.
The toJS call here is to force a re-render when the values change.
Due to the way renderToolbar() is only called deep in the third-party Mosaic component, I don't think React + MobX is able to pick up the changes and re-render without this hack.
There was a problem hiding this comment.
Added a comment indicating this
src/renderer/editor-mosaic.ts
Outdated
| // set a URI for each editor for stable identification for monaco features | ||
| const uri = monaco.Uri.parse(`inmemory://fiddle/${id}`); | ||
| const maybeModel = monaco.editor.getModel(uri); | ||
| const model = maybeModel ?? monaco.editor.createModel(value, language, uri); |
There was a problem hiding this comment.
blocking(?): I believe we have to update the content in maybeModel.
example:
- editorMosaic.set('main.js', 'foo');
- user edits main.js, 'bar'
- if user loads another fiddle, where
main.js: 'foobar'. This is may be a bug, where we usemaybeModeland thus the new fiddle opens with'bar'rather than the expected'foobar'
There was a problem hiding this comment.
let me know if this is unclear, I could be wrong as well
There was a problem hiding this comment.
Yeah pretty sure you're right. That repros in testing. Will fix soon!
There was a problem hiding this comment.
Fixed ba71f34
The unit tests were falsely passing before because I didn't mock the getModel() call properly beforehand and maybeModel was always undefined. I verified that the new test fails without the refactor. 👍
A little quality of life feature. Does a small Monaco minor version bump such that we get access to the
editor.onDidChangeMarkersAPI.I'd recommend reviewing with Hide Whitespace on btw. This will also have some annoying merge conflicts with #1811.
Screen.Recording.2025-11-18.at.20.38.14.mov