This is an automated email from the ASF dual-hosted git repository.
voidmatcha pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new a041e9422c [ZEPPELIN-6735] Fix code editor model leaks and diff updates
a041e9422c is described below
commit a041e9422cf623a423dde0fd00cd013438f494a2
Author: Minho Jang <[email protected]>
AuthorDate: Fri Oct 16 22:24:30 2026 +0900
[ZEPPELIN-6735] Fix code editor model leaks and diff updates
### What is this PR for?
The shared code editor leaves its Monaco text models behind when destroyed.
This affects the regular paragraph editor.
In diff mode, the initial original and modified text are reversed. Value
changes create new models, and changing only `nzOriginalText` does not update
the original text.
This PR disposes of component-owned models, fixes the diff orientation, and
updates the existing models when either input changes.
### What type of PR is it?
Bug Fix
### Todos
* [x] Fix model disposal and diff initialization and updates.
* [x] Add regression tests.
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6735
### How should this be tested?
Run the regression tests from `zeppelin-web-angular`:
```sh
npm run test:shell --
src/app/share/code-editor/code-editor.component.spec.ts
```
Make sure that the tests pass for these behaviors:
* Diff mode displays the original text on the left and the modified text on
the right.
* Value changes update the existing models rather than create new ones.
* Changing only `nzOriginalText` leaves the modified text unchanged.
* Destroying the component disposes of its models but leaves caller-owned
models intact.
* Normal-mode writes update the attached model after a caller replaces it.
### Questions:
* Does the license files need to update? No.
* Is there breaking changes for older versions? No.
* Does this needs documentation? No.
Closes #5574 from miinhho/ZEPPELIN-6735-code-editor-model-lifecycle.
Signed-off-by: YONGJAE LEE <[email protected]>
---
.../code-editor/code-editor.component.spec.ts | 212 +++++++++++++++++++++
.../app/share/code-editor/code-editor.component.ts | 27 ++-
2 files changed, 231 insertions(+), 8 deletions(-)
diff --git
a/zeppelin-web-angular/src/app/share/code-editor/code-editor.component.spec.ts
b/zeppelin-web-angular/src/app/share/code-editor/code-editor.component.spec.ts
new file mode 100644
index 0000000000..9e47a3cdb2
--- /dev/null
+++
b/zeppelin-web-angular/src/app/share/code-editor/code-editor.component.spec.ts
@@ -0,0 +1,212 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { ElementRef, NgZone, SimpleChange } from '@angular/core';
+import { editor } from 'monaco-editor';
+import { BehaviorSubject, of } from 'rxjs';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+import { CodeEditorComponent } from './code-editor.component';
+import { CodeEditorService } from './code-editor.service';
+import { JoinedEditorOptions, NzEditorMode } from
'./nz-code-editor.definitions';
+
+describe('CodeEditorComponent models', () => {
+ const components: CodeEditorComponent[] = [];
+ let initialModels: Set<editor.ITextModel>;
+
+ beforeEach(() => {
+ initialModels = new Set(editor.getModels());
+ // Replace only the DOM views; model contents, events and disposal use
Monaco.
+ vi.spyOn(editor, 'create').mockImplementation(() => {
+ let model: editor.ITextModel | null = null;
+ return {
+ getModel: () => model,
+ setModel: (next: editor.ITextModel | null) => {
+ model = next;
+ },
+ dispose: () => {},
+ updateOptions: () => {}
+ } as unknown as editor.IStandaloneCodeEditor;
+ });
+ vi.spyOn(editor, 'createDiffEditor').mockImplementation(() => {
+ let model: editor.IDiffEditorModel | null = null;
+ return {
+ getModel: () => model,
+ setModel: (next: editor.IDiffEditorModel | null) => {
+ model = next;
+ },
+ dispose: () => {},
+ updateOptions: () => {}
+ } as unknown as editor.IStandaloneDiffEditor;
+ });
+ });
+
+ afterEach(() => {
+ for (const component of components.splice(0)) {
+ component.ngOnDestroy();
+ }
+ // Clean up external models and any leaks left by a failing regression.
+ for (const model of editor.getModels()) {
+ if (!initialModels.has(model)) {
+ model.dispose();
+ }
+ }
+ vi.restoreAllMocks();
+ });
+
+ const createComponent = (mode: NzEditorMode = 'normal') => {
+ const service = {
+ requestToInit: () => of({}),
+ option$: new BehaviorSubject<JoinedEditorOptions>({})
+ } as Pick<CodeEditorService, 'requestToInit' | 'option$'>;
+ const component = new CodeEditorComponent(
+ service as CodeEditorService,
+ new NgZone({}),
+ new ElementRef(document.createElement('div'))
+ );
+ component.nzEditorMode = mode;
+ components.push(component);
+ return component;
+ };
+
+ const initialize = (component: CodeEditorComponent) => {
+ let instance: editor.IEditor | editor.IDiffEditor | undefined;
+ component.nzEditorInitialized.subscribe(value => {
+ instance = value;
+ });
+ component.ngAfterViewInit();
+ return instance!;
+ };
+
+ const destroy = (component: CodeEditorComponent) => {
+ components.splice(components.indexOf(component), 1);
+ component.ngOnDestroy();
+ };
+
+ it('initializes the original on the left and the bound value on the right',
() => {
+ const component = createComponent('diff');
+ component.nzOriginalText = 'before';
+ component.writeValue('after');
+ const instance = initialize(component) as editor.IDiffEditor;
+
+ expect(instance.getModel()!.original.getValue()).toBe('before');
+ expect(instance.getModel()!.modified.getValue()).toBe('after');
+ });
+
+ it('reuses both diff models across bound updates and continues reporting
modified edits', () => {
+ const component = createComponent('diff');
+ const onChange = vi.fn();
+ component.registerOnChange(onChange);
+ const instance = initialize(component) as editor.IDiffEditor;
+ const { original, modified } = instance.getModel()!;
+ component.writeValue('second');
+ component.writeValue('third');
+
+ expect(instance.getModel()!.original).toBe(original);
+ expect(instance.getModel()!.modified).toBe(modified);
+ expect(modified.getValue()).toBe('third');
+ modified.applyEdits([{ range: modified.getFullModelRange(), text: 'user
edit' }]);
+ expect(onChange).toHaveBeenLastCalledWith('user edit');
+ });
+
+ it('updates the original input without changing the modified text or model
identities', () => {
+ const component = createComponent('diff');
+ component.nzOriginalText = 'before';
+ component.writeValue('after');
+ const instance = initialize(component) as editor.IDiffEditor;
+ const { original, modified } = instance.getModel()!;
+ modified.applyEdits([{ range: modified.getFullModelRange(), text: 'user
edit' }]);
+ const onChange = vi.fn();
+ component.registerOnChange(onChange);
+
+ component.nzOriginalText = 'new baseline';
+ component.ngOnChanges({ nzOriginalText: new SimpleChange('before', 'new
baseline', false) });
+
+ expect(original.getValue()).toBe('new baseline');
+ expect(modified.getValue()).toBe('user edit');
+ expect(instance.getModel()!.original).toBe(original);
+ expect(instance.getModel()!.modified).toBe(modified);
+ expect(onChange).not.toHaveBeenCalled();
+ });
+
+ it('clears either diff input without replacing its models', () => {
+ const component = createComponent('diff');
+ component.nzOriginalText = 'before';
+ component.writeValue('after');
+ const instance = initialize(component) as editor.IDiffEditor;
+ const { original, modified } = instance.getModel()!;
+ component.writeValue('');
+ expect(modified.getValue()).toBe('');
+ expect(original.getValue()).toBe('before');
+
+ component.nzOriginalText = '';
+ component.ngOnChanges({ nzOriginalText: new SimpleChange('before', '',
false) });
+ expect(original.getValue()).toBe('');
+ expect(instance.getModel()!.original).toBe(original);
+ expect(instance.getModel()!.modified).toBe(modified);
+ });
+
+ it('preserves normal model reuse on bound updates', () => {
+ const component = createComponent();
+ component.writeValue('before');
+ const instance = initialize(component) as editor.IEditor;
+ const model = instance.getModel() as editor.ITextModel;
+ component.writeValue('after');
+
+ expect(instance.getModel()).toBe(model);
+ expect(model.getValue()).toBe('after');
+ });
+
+ it.each(['normal', 'diff'] as const)('leaves no owned models after repeated
%s lifecycles', mode => {
+ const baseline = editor.getModels().map(model => model.uri.toString());
+ for (let iteration = 0; iteration < 3; iteration++) {
+ const component = createComponent(mode);
+ initialize(component);
+ component.writeValue('updated');
+ destroy(component);
+ }
+
+ expect(editor.getModels().map(model =>
model.uri.toString())).toEqual(baseline);
+ });
+
+ it('updates a normal replacement model while disposing only its detached
owned model', () => {
+ const component = createComponent();
+ const instance = initialize(component) as editor.IEditor;
+ const owned = instance.getModel() as editor.ITextModel;
+ const external = editor.createModel('caller owned');
+ instance.setModel(external);
+ component.writeValue('bound update');
+
+ expect((instance.getModel() as editor.ITextModel).getValue()).toBe('bound
update');
+ expect(owned.getValue()).toBe('');
+ destroy(component);
+ expect(owned.isDisposed()).toBe(true);
+ expect(external.isDisposed()).toBe(false);
+ });
+
+ it('disposes both detached diff models without disposing a caller-owned
replacement pair', () => {
+ const component = createComponent('diff');
+ const instance = initialize(component) as editor.IDiffEditor;
+ const owned = instance.getModel()!;
+ const external = {
+ original: editor.createModel('external original'),
+ modified: editor.createModel('external modified')
+ };
+ instance.setModel(external);
+ destroy(component);
+
+ expect(owned.original.isDisposed()).toBe(true);
+ expect(owned.modified.isDisposed()).toBe(true);
+ expect(external.original.isDisposed()).toBe(false);
+ expect(external.modified.isDisposed()).toBe(false);
+ });
+});
diff --git
a/zeppelin-web-angular/src/app/share/code-editor/code-editor.component.ts
b/zeppelin-web-angular/src/app/share/code-editor/code-editor.component.ts
index 5ee71131ff..df1553befe 100644
--- a/zeppelin-web-angular/src/app/share/code-editor/code-editor.component.ts
+++ b/zeppelin-web-angular/src/app/share/code-editor/code-editor.component.ts
@@ -20,8 +20,10 @@ import {
HostBinding,
Input,
NgZone,
+ OnChanges,
OnDestroy,
Output,
+ SimpleChanges,
TemplateRef,
ViewEncapsulation
} from '@angular/core';
@@ -56,7 +58,7 @@ import ITextModel = editor.ITextModel;
],
standalone: false
})
-export class CodeEditorComponent implements OnDestroy, AfterViewInit {
+export class CodeEditorComponent implements OnChanges, OnDestroy,
AfterViewInit {
@HostBinding('class.ant-code-editor') antCodeEditor = true;
@Input() nzEditorMode: NzEditorMode = 'normal';
@Input() nzOriginalText = '';
@@ -79,6 +81,7 @@ export class CodeEditorComponent implements OnDestroy,
AfterViewInit {
private editorInstance?: IEditor | IDiffEditor;
private value = '';
private modelSet = false;
+ private readonly ownedModels: ITextModel[] = [];
constructor(
private nzCodeEditorService: CodeEditorService,
@@ -88,6 +91,12 @@ export class CodeEditorComponent implements OnDestroy,
AfterViewInit {
this.el = elementRef.nativeElement;
}
+ ngOnChanges(changes: SimpleChanges): void {
+ if (changes.nzOriginalText && this.nzEditorMode === 'diff' &&
this.modelSet) {
+ (this.editorInstance as
IDiffEditor).getModel()?.original.setValue(this.nzOriginalText);
+ }
+ }
+
/**
* Initialize a monaco editor instance.
*/
@@ -99,6 +108,7 @@ export class CodeEditorComponent implements OnDestroy,
AfterViewInit {
if (this.editorInstance) {
this.editorInstance.dispose();
}
+ this.ownedModels.forEach(model => model.dispose());
this.destroy$.next();
this.destroy$.complete();
@@ -203,9 +213,9 @@ export class CodeEditorComponent implements OnDestroy,
AfterViewInit {
if (this.modelSet) {
(this.editorInstance.getModel() as ITextModel).setValue(this.value);
} else {
- (this.editorInstance as IEditor).setModel(
- editor.createModel(this.value, (this.editorOptionCached as
EditorOptions).language)
- );
+ const model = editor.createModel(this.value, (this.editorOptionCached
as EditorOptions).language);
+ this.ownedModels.push(model);
+ (this.editorInstance as IEditor).setModel(model);
this.modelSet = true;
}
} else {
@@ -215,10 +225,11 @@ export class CodeEditorComponent implements OnDestroy,
AfterViewInit {
model.original.setValue(this.nzOriginalText);
} else {
const language = (this.editorOptionCached as EditorOptions).language;
- (this.editorInstance as IDiffEditor).setModel({
- original: editor.createModel(this.value, language),
- modified: editor.createModel(this.nzOriginalText, language)
- });
+ const original = editor.createModel(this.nzOriginalText, language);
+ const modified = editor.createModel(this.value, language);
+ this.ownedModels.push(original, modified);
+ (this.editorInstance as IDiffEditor).setModel({ original, modified });
+ this.modelSet = true;
}
}
}
