[ZEPPELIN-6429] Focus paragraph editor on clone/insert in New UI

[voidmatcha]

What is this PR for?

After cloning or inserting a paragraph in the New UI, the cursor stays on the wrapper element instead of the editor, so you have to click before typing. This focuses the new paragraph's editor one tick after PARAGRAPH_ADDED, gated to clone/insert initiated by this client so auto-append on run and other clients' inserts don't steal focus. It also skips dirty-marking on programmatic editor setValue (isFlush) so the cloned content isn't discarded. (The clone content loss itself is handled separately in #5254 (review); this covers the cursor part.)

What type of PR is it?

Bug Fix

Todos

What is the Jira issue?

ZEPPELIN-6429

How should this be tested?

Screenshots (if appropriate)

M0_COMPARISON_master_asis-vs-tobe.mp4

Questions:

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

[jongyoul]

@voidmatcha By the way, is the CI failure irrelevant?

[tbonelee]

@jongyoul The job failed is only relevant to the legacy frontend app, so we could ignore it for here.

[tbonelee]

Thanks for working on this. It works as intended, I just have a suggestion about the approach.

localAddFocusPending (message.service.ts:43) assumes the next PARAGRAPH_ADDED is the one caused by this client's own request. That's fine for a single user, but it can break with more than one client on the same note. For example: A clicks clone (flag set to true), and before A's own response returns, a PARAGRAPH_ADDED for a paragraph that user B just added arrives first. A's flag gets consumed by B's paragraph, so A's cursor lands on a paragraph A never created. The root issue is that the client can't tell whether an incoming PARAGRAPH_ADDED came from its own request, so it has to guess.

The msgId we already pass around solves this cleanly. The client sends a msgId on every request (message.ts:165), the server knows it (Message.java:234-239), and several broadcasts already echo it back (e.g. broadcastParagraph, NotebookServer.java:673-674). The only gap is broadcastNewParagraph (NotebookServer.java:695-701), so it's a small change, the same thing broadcastParagraph already does:

// NotebookServer.java:695: take a msgId and pass it through (same as broadcastParagraph)
Message message = new Message(OP.PARAGRAPH_ADDED)
  .withMsgId(msgId)                       // <-- this line
  .put("paragraph", para).put("index", paraIndex);

The server then only reports which request an event came from, and each client decides for itself whether to focus by checking if the msgId is its own:

// focus only if the incoming PARAGRAPH_ADDED's msgId is one this client sent
// (the msgId contains a client id, so match by prefix, or track the msgIds you send)
if (isOwnMessage(msgId)) {
  // focus the new paragraph's editor
}

So before going with the heuristic, what do you think about doing it this way instead? It already works today, so this is really about the approach, and I'm happy to help with it.

[tbonelee]

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):

Suggested change

	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);
	}
	}
	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;
	}
	}

[tbonelee]

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?

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?