[ZEPPELIN-6294] Made "Clone paragraph" button work properly in New UI #5044

dididy · 2025-08-24T15:29:20Z

What is this PR for?

Description:
Clicking the paragraph control’s Clone paragraph button that trigger COPY_PARAGRAPH. Upon receiving PARAGRAPH_ADDED, the Classic UI includes data.paragraph.text as expected, but the New UI receives an empty data.paragraph.text. Additionally, after cloning, the cursor is not positioned in the cloned paragraph in the New UI.

Expected:

Cloned paragraph contains the original text.
Cursor focuses/positions inside the cloned paragraph.

Actual (New UI):

data.paragraph.text is empty on PARAGRAPH_ADDED.
Cursor not focused / not moved to the cloned paragraph.

[Appropriate action - Classic UI]

2025-08-24.11.40.28.mov

[AS-IS]

2025-08-24.11.49.07.mov

[TO-BE]

2025-08-25.12.26.32.mov

What type of PR is it?

Bug Fix

Todos

What is the Jira issue?

[ZEPPELIN-6294]

How should this be tested?

Screenshots (if appropriate)

Questions:

Does the license files need to update? N
Is there breaking changes for older versions? N
Does this needs documentation? N

tbonelee

Thanks for taking a look! From my side, data.paragraph.text appears on PARAGRAPH_ADDED in both UIs, but I might be mistaken. If there’s a case where it’s empty, I’d love the exact repro steps.
One possibility is that the New UI is skipping a valid PARAGRAPH as if it were short-circuited when another request bumps lastMsgIdSeq before the final reply—PARAGRAPH message (see message.ts, L187). I noticed EDITOR_SETTING messages being sent from the client and suspect they might be the trigger. If the only intended short-circuit is RUN_PARAGRAPH → synthetic PARAGRAPH_STATUS, would it make sense to remove the second filter() at L187 rather than adding a special-case branch at the NotebookServer? Happy to adjust based on your feedback.

dididy · 2025-08-26T13:08:12Z

I really appreciate about your review. 👍 👍

As you mentioned, the issue was caused by EDITOR_SETTING being triggered before PARAGRAPH, which led to short-circuit processing. However, modifying the condition in message.ts, L187 didn’t seem optimal from a performance standpoint.(I'm not entirely sure if that's really the case)

So I adjusted the timing in the relevant section of the app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts instead. Because the getEditorSetting method that triggers EDITOR_SETTING is only executed when the parameter of setParagraphMode is true, and the only place it is used in this form is inside onDidChangeModelContent. Therefore, I modified it so that when setParagraphMode(true) is called in that area, the timing is adjusted using setTimeout.

After reverted NotebookServer code, everything worked as intended. Thanks for pointing out a better way to improve it.

tbonelee

Nice fix! Since the setTimeout(..., 200) is a heuristic to dodge a race, could we add a short comment explaining (1) why it's needed and (2) a TODO o how to make this deterministic or order-independent?

dididy · 2025-08-27T14:01:55Z

Nice fix! Since the setTimeout(..., 200) is a heuristic to dodge a race, could we add a short comment explaining (1) why it's needed and (2) a TODO o how to make this deterministic or order-independent?

ab07359

Ensuring the execution order of EDITOR_SETTING seems necessary because it can otherwise cause a bug that passes through the short-circuit condition. Although this issue does not reproduce for everyone, controlling the execution timing appears to be required.

EDITOR_SETTING is executed when the first paragraph component is rendered. From the Classic UI console logs above, we can see that it runs after INTERPRETER_BINDINGS when first entering the note. Therefore, I added a variable editorSettingTriggerAllowed with an initial value of false in /app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts. The getEditorSetting method now only executes if this variable is true.

Then, in /app/pages/workspace/notebook/notebook.component.ts, I added code that sets editorSettingTriggerAllowed to true for all paragraphs and calls getEditorSetting when receiving INTERPRETER_BINDINGS.

<= Classic UI

<= New UI unrevised result

<= New UI revised result

The part where I noticed the issue was at the moment of receiving PARAGRAPH_ADDED op. When a new paragraph component is added by PARAGRAPH_ADDED, getEditorSetting is immediately executed. This triggers EDITOR_SETTING, but since execution is now blocked by editorSettingTriggerAllowed, as I fixed, it does not run at that point. Instead, editorSettingTriggerAllowed is set to true and getEditorSetting is executed when the PARAGRAPH op(/app/core/paragraph-base/paragraph-base.ts) is received.

tbonelee

Thanks for the thorough update — I’ve left some questions and requested changes.

tbonelee · 2025-08-27T17:13:37Z

zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.ts

@@ -54,6 +55,7 @@ export abstract class ParagraphBase extends MessageListenersManager {

  // Initialized by `ViewChildren` in the class which extends ParagraphBase
  notebookParagraphResultComponents!: QueryList<NotebookParagraphResultComponent>;
+  notebookParagraphCodeEditorComponent!: NotebookParagraphCodeEditorComponent;


I think this non-null assertion should be removed and changed to optional. I found it to be undefined when paragraph.config.editorHide === true in the %md editor case. Maybe you could check some errors after running %md interpreter.
(zeppelin-web-angular/src/app/pages/workspace/notebook/paragraph/paragraph.component.html)

<zeppelin-notebook-paragraph-code-editor *ngIf="!paragraph.config.editorHide && !viewOnly" ...

I mistakenly added non-null assertion for notebookParagraphCodeEditorComponent in NotebookParagraphComponent in a previous commit, but that was incorrect. Sorry for the confusion.

tbonelee · 2025-08-27T17:38:35Z

zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.component.ts

@@ -109,6 +109,10 @@ export class NotebookComponent extends MessageListenersManager implements OnInit

  @MessageListener(OP.INTERPRETER_BINDINGS)
  loadInterpreterBindings(data: MessageReceiveDataTypeMap[OP.INTERPRETER_BINDINGS]) {
+    this.listOfNotebookParagraphComponent.forEach(item => {
+      item.notebookParagraphCodeEditorComponent.editorSettingTriggerAllowed = true;


We should handle some case where notebookParagraphCodeEditorComponent is undefined.
Also, should we trigger editor settings for all paragraphs here? Would calling this.setParagraphMode(true) in editor.onDidChangeModelContent() be sufficient to get editor settings only to the paragraph whose interpreter has changed?

In testing, the interpreter setting works correctly when this.setParagraphMode(true) is executed inside editor.onDidChangeModelContent().

Since this runs before the code that executes when receiving INTERPRETER_BINDINGS above (editor.onDidChangeModelContent()), the code that runs after editorSettingTriggerAllowed = true was modified to directly call getEditorSetting() instead of using setParagraphMode(true) when receive PARAGRAPH or INTERPRETER_BINDINGS.

c90cd09

Because if the interpreter is already set inside this.setParagraphMode(true), getEditorSetting() will not be executed due to the condition, and since setParagraphMode(true) has already run, there is no need to execute it again.

tbonelee · 2025-08-27T17:40:01Z

zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.ts

@@ -142,6 +144,8 @@ export abstract class ParagraphBase extends MessageListenersManager {
        }
        this.cdr.markForCheck();
      });
+      this.notebookParagraphCodeEditorComponent.editorSettingTriggerAllowed = true;


We should handle the case where this.notebookParagraphCodeEditorComponent is undefined

...-web-angular/src/app/pages/workspace/notebook/paragraph/code-editor/code-editor.component.ts

dididy · 2025-09-04T14:33:30Z

c90cd09

Made notebookParagraphCodeEditorComponent optional.

5e35dbf

Added a comment for editorSettingTriggerAllowed.

6c993a0

There are two related actions: CLONE_PARAGRAPH and INSERT_PARAGRAPH. Each needs to be handled slightly differently.

Behavior

CLONE_PARAGRAPH
1. send > CLONE_PARAGRAPH
2. receive > PARAGRAPH_ADDED → PARAGRAPH → trigger EDITOR_SETTING
INSERT_PARAGRAPH
1. send > INSERT_PARAGRAPH
2. receive > trigger EDITOR_SETTING after PARAGRAPH_ADDED

Implementation

CLONE_PARAGRAPH was already handled correctly.
For INSERT_PARAGRAPH, we added:
- A variable isTriggeredByInsertParagraph in notebook.component.ts.
- Set it to true when insertParagraph in paragraph.component.ts is executed.
This allows branching based on whether PARAGRAPH_ADDED was triggered by an INSERT_PARAGRAPH or a CLONE_PARAGRAPH.

Reasoning

CLONE_PARAGRAPH: EDITOR_SETTING runs after receiving PARAGRAPH.
INSERT_PARAGRAPH: EDITOR_SETTING runs when PARAGRAPH_ADDED is received.

dididy · 2025-09-07T04:24:07Z

I rebased due to conflicts caused by merged PRs.

tbonelee · 2025-09-07T11:07:13Z

zeppelin-web-angular/src/app/pages/workspace/notebook/notebook.component.ts

+    definedNote.paragraphs[paragraphIndex].focus = true;
+    const addedParagraph = this.listOfNotebookParagraphComponent.find((_, index) => index === paragraphIndex)
+      ?.notebookParagraphCodeEditorComponent;


I think something like this.cdr.detectChanges() should be called, so that the paragraph component is added for the new paragraph in definedNote.paragraph.

5aab81a c496536

Upon review, components using ChangeDetectionStrategy.OnPush—as you mentioned—do not detect changes in the same reference array unless the reference itself is replaced. Since I am currently modifying the array with splice (which does change the reference in place), Angular does not automatically recognize the change. Therefore, calling detectChanges or markForCheck is necessary.

The reason why Clone Paragraph or Add Paragraph (Insert Paragraph) in the current PR appeared to work correctly is due to Angular’s behavior of updating the view during events triggered within the component. Consequently, when executed via a button click, the focus handling works as expected, but when triggered via a shortcut key, the focus does not update properly.

To address this, after receiving PARAGRAPH_ADDED and performing a splice to update the array, we now explicitly call detectChanges to force change detection, which has been confirmed to work correctly.

1cdff50

Even after handling it this way, the focus event did not work correctly when triggered via shortcut keys. To address this, in paragraph.component.ts, I added a blur event on the current paragraph when executing cloneParagraph and insertParagraph.

I rebased again due to conflicts caused by merging the latest PR.

### What is this PR for? **Description:** Cursor behavior in the New UI’s paragraph needs to be fixed for several cursor related actions, including double-clicking, running all above/below, adding(clone), and removing paragraphs. When **cloneParagraph()** is called, it internally calls **addParagraph()**, which has already been tested. The same addParagraph-related code is also applied in #5044. If either PR is merged first, I will rebase accordingly. I have also confirmed that **cloneParagraph()** works correctly through #5044. Due to timing issues, I used `setTimeout` for **removeParagraph()** and **doubleClickParagraph()**. Since this is in the UI area, it likely won’t have major side effects, but I will look into it further. **Expected:** - When **doubleClickParagraph()** is executed, the cursor should move to the end of the paragraph. - When **runAllAbove()** or **runAllBelow()** is executed, the current cursor position should be remembered, and after execution, focus should return to the previous cursor position. - When **addParagraph()** is executed, the newly added paragraph should receive focus. - When **removeParagraph()** is executed, focus should move to the paragraph that takes the deleted paragraph’s place. **Actual (New UI):** - When **doubleClickParagraph()** is executed, the cursor moves to the beginning instead of the end. - After **runAllAbove()** or **runAllBelow()**, focus is lost completely. - When **addParagraph()** is executed, the new paragraph does not automatically receive focus. - After **removeParagraph()**, focus may not move to the correct paragraph. **[Appropriate action - Classic UI]** https://github.com/user-attachments/assets/fc0066f7-4e03-4e3b-9d5b-2f33df415ba7 Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph **[AS-IS]** https://github.com/user-attachments/assets/f699f788-cf29-4c4c-8c47-2ef34d7962f0 Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph **[TO-BE]** https://github.com/user-attachments/assets/1206c524-103f-4328-85ee-04408073b628 Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph ### What type of PR is it? Bug Fix ### Todos ### What is the Jira issue? * [[ZEPPELIN-6298](https://issues.apache.org/jira/browse/ZEPPELIN-6298)] ### How should this be tested? ### Screenshots (if appropriate) ### Questions: * Does the license files need to update? N * Is there breaking changes for older versions? N * Does this needs documentation? N Closes #5057 from dididy/fix/ZEPPELIN-6298. Signed-off-by: ChanHo Lee <[email protected]>

### What is this PR for? **Description:** Cursor behavior in the New UI’s paragraph needs to be fixed for several cursor related actions, including double-clicking, running all above/below, adding(clone), and removing paragraphs. When **cloneParagraph()** is called, it internally calls **addParagraph()**, which has already been tested. The same addParagraph-related code is also applied in #5044. If either PR is merged first, I will rebase accordingly. I have also confirmed that **cloneParagraph()** works correctly through #5044. Due to timing issues, I used `setTimeout` for **removeParagraph()** and **doubleClickParagraph()**. Since this is in the UI area, it likely won’t have major side effects, but I will look into it further. **Expected:** - When **doubleClickParagraph()** is executed, the cursor should move to the end of the paragraph. - When **runAllAbove()** or **runAllBelow()** is executed, the current cursor position should be remembered, and after execution, focus should return to the previous cursor position. - When **addParagraph()** is executed, the newly added paragraph should receive focus. - When **removeParagraph()** is executed, focus should move to the paragraph that takes the deleted paragraph’s place. **Actual (New UI):** - When **doubleClickParagraph()** is executed, the cursor moves to the beginning instead of the end. - After **runAllAbove()** or **runAllBelow()**, focus is lost completely. - When **addParagraph()** is executed, the new paragraph does not automatically receive focus. - After **removeParagraph()**, focus may not move to the correct paragraph. **[Appropriate action - Classic UI]** https://github.com/user-attachments/assets/fc0066f7-4e03-4e3b-9d5b-2f33df415ba7 Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph **[AS-IS]** https://github.com/user-attachments/assets/f699f788-cf29-4c4c-8c47-2ef34d7962f0 Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph **[TO-BE]** https://github.com/user-attachments/assets/1206c524-103f-4328-85ee-04408073b628 Run all above -> Run all below -> Double click .md paragraph -> Add paragraph -> Delete paragraph ### What type of PR is it? Bug Fix ### Todos ### What is the Jira issue? * [[ZEPPELIN-6298](https://issues.apache.org/jira/browse/ZEPPELIN-6298)] ### How should this be tested? ### Screenshots (if appropriate) ### Questions: * Does the license files need to update? N * Is there breaking changes for older versions? N * Does this needs documentation? N Closes #5057 from dididy/fix/ZEPPELIN-6298. Signed-off-by: ChanHo Lee <[email protected]> (cherry picked from commit 4fbfaec) Signed-off-by: ChanHo Lee <[email protected]>

…onal

…RAPH case

…shortcut

… functions cloneParagraph, insterParagraph

…raph is not work properly The clone paragraph functionality wasn’t working correctly at the current position, so I moved it.

tbonelee reviewed Aug 25, 2025

View reviewed changes

tbonelee requested changes Aug 27, 2025

View reviewed changes

dididy mentioned this pull request Aug 30, 2025

[ZEPPELIN-6298] Fix cursor-related issues in New UI's Paragraph #5057

Merged

This was referenced Sep 5, 2025

[ZEPPELIN-6197/6229] Fixed the shortcuts in the New UI so that they match the Classic UI and work correctly #5064

Merged

[ZEPPELIN-6203] Connect shortcut modal for info button #4963

Merged

dididy force-pushed the fix/ZEPPELIN-6294 branch from 5e35dbf to 648c6ef Compare September 7, 2025 04:20

tbonelee reviewed Sep 7, 2025

View reviewed changes

dididy force-pushed the fix/ZEPPELIN-6294 branch from cecec31 to 5aab81a Compare September 7, 2025 13:09

dididy added 12 commits September 7, 2025 22:59

[ZEPPELIN-6294] fix: "Clone paragraph" button work properly in New UI

1129d76

[ZEPPELIN-6294] fix: revert server code, adjust timing of EDITOR_SETTING

dda4ff8

[fix/ZEPPELIN-6294] chore: changed setTimeout position

a012faf

[ZEPPELIN-6294] chore: revert function position

e6e256b

[ZEPPELIN-6294] chore: revert setTimeout delay time

239aaba

[ZEPPELIN-6294] fix: Ensure the execution order of EDITOR_SETTING

31e29cc

[ZEPPELIN-6294] fix: set notebookParagraphCodeEditorComponent as opti…

db93a2c

…onal

[ZEPPELIN-6294] fix: handle so that it also works in the INSERT_PARAG…

6f8901c

…RAPH case

[ZEPPELIN-6294] chore: add a comment for editorSettingTriggerAllowed

4eeb39c

[ZEPPELIN-6294] fix: add detectChanges for addParagraph triggered by …

627a9a5

…shortcut

[ZEPPELIN-6294] fix: execute blur event when call addParagraph relate…

032f535

… functions cloneParagraph, insterParagraph

[ZEPPELIN-6294] fix: change order of detectChanges because cloneParag…

dcd95c3

…raph is not work properly The clone paragraph functionality wasn’t working correctly at the current position, so I moved it.

dididy force-pushed the fix/ZEPPELIN-6294 branch from c496536 to dcd95c3 Compare September 7, 2025 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ZEPPELIN-6294] Made "Clone paragraph" button work properly in New UI #5044

[ZEPPELIN-6294] Made "Clone paragraph" button work properly in New UI #5044

Uh oh!

dididy commented Aug 24, 2025 •

edited

Loading

Uh oh!

tbonelee left a comment

Uh oh!

dididy commented Aug 26, 2025 •

edited

Loading

Uh oh!

tbonelee left a comment

Uh oh!

dididy commented Aug 27, 2025 •

edited

Loading

Uh oh!

tbonelee left a comment

Uh oh!

tbonelee Aug 27, 2025 •

edited

Loading

Uh oh!

tbonelee Aug 27, 2025

Uh oh!

dididy Sep 4, 2025 •

edited

Loading

Uh oh!

tbonelee Aug 27, 2025

Uh oh!

Uh oh!

dididy commented Sep 4, 2025

Uh oh!

dididy commented Sep 7, 2025

Uh oh!

tbonelee Sep 7, 2025

Uh oh!

dididy Sep 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

[ZEPPELIN-6294] Made "Clone paragraph" button work properly in New UI #5044

Are you sure you want to change the base?

[ZEPPELIN-6294] Made "Clone paragraph" button work properly in New UI #5044

Uh oh!

Conversation

dididy commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

tbonelee left a comment

Choose a reason for hiding this comment

Uh oh!

dididy commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbonelee left a comment

Choose a reason for hiding this comment

Uh oh!

dididy commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbonelee left a comment

Choose a reason for hiding this comment

Uh oh!

tbonelee Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbonelee Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

dididy Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbonelee Aug 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dididy commented Sep 4, 2025

Behavior

Implementation

Reasoning

Uh oh!

dididy commented Sep 7, 2025

Uh oh!

tbonelee Sep 7, 2025

Choose a reason for hiding this comment

Uh oh!

dididy Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dididy commented Aug 24, 2025 •

edited

Loading

dididy commented Aug 26, 2025 •

edited

Loading

dididy commented Aug 27, 2025 •

edited

Loading

tbonelee Aug 27, 2025 •

edited

Loading

dididy Sep 4, 2025 •

edited

Loading

dididy Sep 7, 2025 •

edited

Loading