fix: detach diff models before disposal#4
Conversation
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
Co-authored-by: WillBooster (Codex CLI) <agent@willbooster.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the disposeEditor function in DiffEditor.tsx to ensure more reliable cleanup of Monaco editor instances and models. Key changes include explicitly setting the model to null, disposing of the editor, and resetting internal state flags. Feedback was provided to address potential TypeError exceptions when checking the disposal status of models, as the current implementation lacks necessary safety checks for null or undefined model references.
| if (!keepCurrentOriginalModel && !models?.original.isDisposed()) { | ||
| models?.original?.dispose(); | ||
| } |
There was a problem hiding this comment.
The expression models?.original.isDisposed() will throw a TypeError if models is null or undefined, because it attempts to access the .isDisposed() method on an undefined value. Since editor?.getModel() can return null if no model is set, you should verify that models.original exists before checking its disposal state.
| if (!keepCurrentOriginalModel && !models?.original.isDisposed()) { | |
| models?.original?.dispose(); | |
| } | |
| if (!keepCurrentOriginalModel && models?.original && !models.original.isDisposed()) { | |
| models.original.dispose(); | |
| } |
| if (!keepCurrentModifiedModel && !models?.modified.isDisposed()) { | ||
| models?.modified?.dispose(); | ||
| } |
There was a problem hiding this comment.
Similar to the original model check, models?.modified.isDisposed() will crash if models is null. Ensure safe access to the modified model before calling isDisposed().
| if (!keepCurrentModifiedModel && !models?.modified.isDisposed()) { | |
| models?.modified?.dispose(); | |
| } | |
| if (!keepCurrentModifiedModel && models?.modified && !models.modified.isDisposed()) { | |
| models.modified.dispose(); | |
| } |
1 participant
Summary
Upstream issue triage
TextModel got disposed before DiffEditorWidget model got reset. Fixed here.editor.api.js.monaco-editor@0.55.1,@willbooster/monaco-loader@1.0.1, and peer range^0.55.1.0.55.1and declaresdompurify@3.2.7. Since Monaco is a peer dependency here, this package cannot force consumers' nested DOMPurify version until Monaco publishes a fixed release.onValidatethrough marker changes.Verification
yarn check-for-aiyarn typecheckyarn build