tbonelee commented on code in PR #5267:
URL: https://github.com/apache/zeppelin/pull/5267#discussion_r3369763778
##########
zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts:
##########
@@ -94,19 +94,24 @@ export class NotebookParagraphCodeEditorComponent
this.position = e.position;
});
}),
- editor.onDidChangeModelContent(() => {
+ editor.onDidChangeModelContent(e => {
this.ngZone.run(() => {
const model = editor.getModel();
if (!model) {
throw new Error('Model content changed but model not found.');
}
this.text = model.getValue();
- this.textChanged.emit(this.text);
- this.setParagraphMode(true);
this.autoAdjustEditorHeight();
setTimeout(() => {
this.autoAdjustEditorHeight();
});
+ // A flush is a programmatic setValue (editor init, remote content
update, patch), not a user edit.
+ // Such changes must not mark the paragraph dirty.
+ if (e.isFlush) {
+ return;
+ }
+ this.textChanged.emit(this.text);
+ this.setParagraphMode(true);
Review Comment:
Question (non-blocking): should `setParagraphMode(true)` still run on a
flush?
Skipping `textChanged.emit` on flush makes sense to me, re-emitting
programmatic/remote content would resend it as a local edit (patch in
collaborative mode, autosave otherwise). But `setParagraphMode(true)` looks
different: it only re-detects the interpreter from the `%magic` and refreshes
`config.editorSetting` (editor language), with no echo. Gating it behind
`isFlush` means cloned/remote content stops re-detecting its language until the
next local keystroke.
Would moving just `setParagraphMode(true)` above the guard be safer?
```ts
this.setParagraphMode(true); // re-detect language regardless of source
if (e.isFlush) {
return; // programmatic setValue: do not re-emit (dirty / patch / autosave)
}
this.textChanged.emit(this.text);
```
Or is dropping re-detection on flush intentional? WDYT?
##########
zeppelin-web-angular/src/app/services/message.service.ts:
##########
@@ -78,6 +86,31 @@ export class MessageService extends Message implements
OnDestroy {
return super.receive<K>(op);
}
+ consumeLocalAddFocusMsgId(msgId: string | undefined): boolean {
+ if (!msgId) {
+ return false;
+ }
+ return this.localAddFocusMsgIds.delete(msgId);
+ }
+
+ private captureLocalAddFocusMsgId(sendMessage: () => void): void {
+ let msgId: string | undefined;
+ const subscription = super
+ .sent()
+ .pipe(take(1))
+ .subscribe(message => {
+ msgId = message.msgId;
+ });
+ try {
+ sendMessage();
+ } finally {
+ subscription.unsubscribe();
+ }
+ if (msgId) {
+ this.localAddFocusMsgIds.add(msgId);
+ }
+ }
Review Comment:
nit / non-blocking: this works today, but it quietly relies on
`Message.send()` emitting on `sent$` **synchronously**. The `subscribe`
callback only assigns the outer `msgId` and the real `add()` runs after
`try/finally`, so `finally { unsubscribe() }` is safe only because the emit
already happened by then. If `sent$` ever becomes async, `finally` would
unsubscribe before the callback runs, `msgId` stays `undefined`, and focus
silently stops working with no error.
Registering inside the callback makes it timing-independent (`take(1)`
disposes on emit; the only leak path left is `sendMessage()` throwing before
any emit, which the `catch` covers):
```suggestion
private captureLocalAddFocusMsgId(sendMessage: () => void): void {
const subscription = super
.sent()
.pipe(take(1))
.subscribe(message => {
if (message.msgId) {
this.localAddFocusMsgIds.add(message.msgId);
}
});
try {
sendMessage();
} catch (error) {
// sendMessage() threw before emitting, so take(1) never completed.
Dispose to avoid a leak.
subscription.unsubscribe();
throw error;
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
