[ZEPPELIN-6661] Cover notebook editor save timing by voidmatcha · Pull Request #5483 · apache/zeppelin

voidmatcha · 2026-09-14T14:51:56Z

What is this PR for?

When a paragraph save response arrives late, ParagraphBase treats it as a remote update and replaces the editor text, losing edits typed in the meantime.

This PR tracks pending saves by msgId. A response to one of the user's own saves no longer replaces the editor text when a newer edit or save followed it; otherwise the response is the server copy and is applied as before. Other paragraph fields in that response always apply.

Related behavior changes in ParagraphBase:

A remote edit that differs from the local unsaved text now replaces it and resets the saved baseline, so the discarded local text is not re-committed later.
An empty local edit (all text deleted) is now treated as a local edit and protected like any other.
A broadcast that does not match a pending local save and repeats the saved text no longer rewinds the editor while the user is typing.

This PR adds the E2E scenarios for ZEPPELIN-6661 (idle autosave, edit during an in-flight save). The in-flight scenario fails without this fix, so the fix is included here.

ParagraphBase reads the save acknowledgement msgId from the message envelope (@MessageEnvelopeListener(OP.PARAGRAPH)). The SDK commitParagraph(), insertParagraph() and copyParagraph() now return the sent msgId, and MessageService records the insert/copy msgId used for editor focus from that return value instead of subscribing to sent().

What type of PR is it?

Bug Fix

Todos

Match save responses to pending saves by msgId
Add unit tests and browser scenarios for save timing
Register the scenarios in the notebook parity inventory

What is the Jira issue?

ZEPPELIN-6661

How should this be tested?

cd zeppelin-web-angular
npx vitest run --config vitest.shell.config.mts src/app/core/paragraph-base/paragraph-base.spec.ts projects/zeppelin-sdk/src/message.spec.ts src/app/services/message.service.spec.ts
npm run check:notebook-parity-scenarios
CI=true PLAYWRIGHT_HTML_OPEN=never npm run e2e -- --reporter=list tests/notebook/persistence/notebook-save-timing.spec.ts

Screenshots (if appropriate)

N/A

Questions:

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

voidmatcha · 2026-09-19T03:45:11Z

This is why the PR changes Angular code. On master, an edit typed while a save is in flight is overwritten when the delayed save response arrives. This branch puts only this PR's E2E files on master, without the fix: https://github.com/voidmatcha/zeppelin/tree/ZEPPELIN-6661-repro-on-master

CI results (Playwright, anonymous mode):

App code	NB-PARITY-050	NB-PARITY-051
master (run)	pass	fails on Chromium, Firefox and WebKit: the editor rewinds to `%md First pending save` (expected `%md First pending save; latest edit wins`)
this PR	pass	pass

Angular changes:

ParagraphBase: tracks sent saves by msgId; a response to the user's own save no longer changes the editor text.
NotebookParagraphComponent: records the msgId when it commits a paragraph.
MessageService: passes the msgId of PARAGRAPH responses to the handler.
SDK Message.send() and commitParagraph(): return the sent msgId.

miinhho · 2026-09-22T07:51:33Z

The reproduction on master makes the Angular fix clear 😄

The msgId handling addresses the reproduced race: the earlier save response no longer replaces text typed afterward.

The Playwright tests cover both timing cases, and the unit tests cover the relevant edge cases. No blockers from me 👍

tbonelee

This PR branched before ZEPPELIN-6683 (#5487) landed, and it conflicts with master in message.service.ts.

#5487 removed the interceptReceived block that copied the envelope msgId into data, along with the ParagraphAdded.msgId field, and replaced them with Message.receiveEnvelope() and the @MessageEnvelopeListener decorator. notebook.component.ts is the example. Since this PR extends that same copying to OP.PARAGRAPH and adds UpdateParagraph.msgId, it would be good to rebase onto master and move to the envelope path. ParagraphBase.paragraphData would then take a @MessageEnvelopeListener(OP.PARAGRAPH).

One thing to watch out for: only message.service.ts conflicts. UpdateParagraph.msgId merges cleanly and survives, so it would be worth removing it along the way since resolving the conflict alone will not surface it.

On a separate note, master recovers the sent msgId in MessageService.captureLocalAddFocusMsgId() by subscribing to sent() with take(1). That would coexist with this PR's send(): string. Which way would you like to converge? A follow-up issue is fine if it is out of scope here.

The rest is inline.

tbonelee · 2026-09-25T13:28:53Z

+      // Keep an edit typed or saved after this save; otherwise the response is the server copy.
+      // Saves still pending after consumeParagraphSave are newer than the acknowledged one.
+      const hasLocalEdit = this.dirtyText !== undefined && this.dirtyText !== newPara.text;
+      if (acknowledged && (hasLocalEdit || this.pendingParagraphSaves.size > 0)) {
+        this.cdr.markForCheck();
+        return;
+      }
+      if (this.dirtyText !== undefined) {
        // check if editor has local update
        if (this.dirtyText === newPara.text) {
          // when local update is the same from remote, clear local update
          this.paragraph.text = newPara.text;
          this.dirtyText = undefined;
          this.originalText = newPara.text;
+        } else if (this.originalText === newPara.text) {
+          // An earlier save response must not replace an edit made while it was in flight.
        } else {
-          // if there're local update, keep it.
+          // A different server value is a remote edit, not an acknowledgement of the
+          // last local save. Accept it and discard the superseded local edit.
          this.paragraph.text = newPara.text;
+          this.dirtyText = undefined;
+          this.originalText = newPara.text;
        }
      } else {
        this.paragraph.text = newPara.text;


This PR added dirtyText = undefined and originalText = newPara.text to the last branch, which makes its body identical to the first one, and left an empty branch carrying only a comment in between. Three of the four branches now do the same thing.

There are really only two conditions for not taking the server text: a stale save acknowledgement identified by msgId, and, when no msgId is available, the same situation inferred from the text. Lifting both into named predicates flattens the control flow and removes four comments.

One side effect is worth noting. dirtyText !== undefined is currently spelled twice, in hasLocalEdit and in the branch below, so one can be wrong while the other covers for it. Changing hasLocalEdit to a truthy check breaks no test today. After merging there is a single spelling, and the "an empty local edit is treated as a local edit" behaviour from the PR description becomes pinned by the existing it.each(['latest edit', '']) test.

I ran the unit suite and lint against the result of the suggestion below: 31 passed, 0 lint errors.

Suggested change

// Keep an edit typed or saved after this save; otherwise the response is the server copy.

// Saves still pending after consumeParagraphSave are newer than the acknowledged one.

const hasLocalEdit = this.dirtyText !== undefined && this.dirtyText !== newPara.text;

if (acknowledged && (hasLocalEdit || this.pendingParagraphSaves.size > 0)) {

this.cdr.markForCheck();

return;

}

if (this.dirtyText !== undefined) {

// check if editor has local update

if (this.dirtyText === newPara.text) {

// when local update is the same from remote, clear local update

this.paragraph.text = newPara.text;

this.dirtyText = undefined;

this.originalText = newPara.text;

} else if (this.originalText === newPara.text) {

// An earlier save response must not replace an edit made while it was in flight.

} else {

// if there're local update, keep it.

// A different server value is a remote edit, not an acknowledgement of the

// last local save. Accept it and discard the superseded local edit.

this.paragraph.text = newPara.text;

this.dirtyText = undefined;

this.originalText = newPara.text;

}

} else {

this.paragraph.text = newPara.text;

const hasLocalEdit = this.dirtyText !== undefined && this.dirtyText !== newPara.text;

// Saves still pending after consumeParagraphSave are newer than the acknowledged one.

const staleSaveAcknowledgement = acknowledged && (hasLocalEdit || this.pendingParagraphSaves.size > 0);

const staleBroadcastOfSavedText = hasLocalEdit && this.originalText === newPara.text;

if (!staleSaveAcknowledgement && !staleBroadcastOfSavedText) {

this.paragraph.text = newPara.text;

this.dirtyText = undefined;

One step further would remove the remaining comment too. pendingParagraphSaves.size > 0 is the same value consumeParagraphSave already computes as hasNewerPendingSave, because that method sweeps every entry up to the acknowledged sequence. That is why the comment is needed here, and why updateAllScopeTexts ends up depending on another method's cleanup rule. Returning { acknowledged, hasNewerSave } from consumeParagraphSave drops both the comment and that coupling. It cannot go in the suggestion since it also changes the other method's signature. I checked that shape passes the 31 tests as well.

tbonelee · 2026-09-25T13:29:01Z

          "symbol": "NotebookComponent"
        }
      ],


The behaviour NB-PARITY-051 describes is implemented by ParagraphBase (pendingParagraphSaves / consumeParagraphSave / updateAllScopeTexts), which this PR adds, but implementationEvidence is still what it was while the scenario was a gap. The check script only verifies that the paths exist, so it passes, but someone porting this to React from the registry would miss the mechanism.

Suggested change

"symbol": "NotebookComponent"

}

],

"symbol": "NotebookComponent"

},

{

"path": "zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.ts",

"symbol": "ParagraphBase"

}

],

The .md is generated, so running npm run generate:notebook-parity-scenarios alongside will keep it in sync. I confirmed check:notebook-parity-scenarios passes after applying this.

voidmatcha · 2026-09-25T14:44:15Z

@tbonelee Thanks for the review. Rebased onto master and addressed each point:

Moved paragraphData to @MessageEnvelopeListener(OP.PARAGRAPH) and removed UpdateParagraph.msgId, with a unit test for the envelope path (79020d9).
Applied the updateAllScopeTexts suggestion, with consumeParagraphSave returning { acknowledged, hasNewerSave } (79020d9).
Added ParagraphBase to the NB-PARITY-051 evidence and regenerated the .md (79020d9).
Converged on send(): string: insertParagraph() / copyParagraph() now return the sent msgId like commitParagraph(), and captureLocalAddFocusMsgId() is removed (16c388d). This avoids assuming the next sent() emission is our own message.

tbonelee · 2026-09-26T05:56:15Z

Merged into master

voidmatcha force-pushed the ZEPPELIN-6661-editor-save-timing branch 4 times, most recently from 532b535 to 68184b0 Compare September 15, 2026 16:36

voidmatcha marked this pull request as ready for review September 16, 2026 13:17

voidmatcha force-pushed the ZEPPELIN-6661-editor-save-timing branch 7 times, most recently from ee99efb to 6d323f2 Compare September 17, 2026 18:17

voidmatcha force-pushed the ZEPPELIN-6661-editor-save-timing branch from 5697900 to f350173 Compare September 20, 2026 02:12

tbonelee requested changes Sep 25, 2026

View reviewed changes

voidmatcha added 2 commits September 25, 2026 22:35

[ZEPPELIN-6661] Cover notebook editor save timing

7491262

[ZEPPELIN-6661] Apply an acknowledged save when no newer edit follows it

785c709

voidmatcha force-pushed the ZEPPELIN-6661-editor-save-timing branch 2 times, most recently from 18514b2 to bb7a1f8 Compare September 25, 2026 14:39

voidmatcha added 2 commits September 25, 2026 23:42

Read the save acknowledgement msgId from the message envelope

79020d9

Track local paragraph add msgIds from the send return value

16c388d

voidmatcha force-pushed the ZEPPELIN-6661-editor-save-timing branch from bb7a1f8 to 16c388d Compare September 25, 2026 14:42

tbonelee approved these changes Sep 25, 2026

View reviewed changes

tbonelee merged commit a19d488 into apache:master Sep 26, 2026
43 of 46 checks passed

Navigation Menu

[ZEPPELIN-6661] Cover notebook editor save timing - #5483

[ZEPPELIN-6661] Cover notebook editor save timing#5483
tbonelee merged 4 commits into
apache:masterfrom
voidmatcha:ZEPPELIN-6661-editor-save-timing

voidmatcha commented Sep 14, 2026 •

edited

Loading

Uh oh!

voidmatcha commented Sep 19, 2026

Uh oh!

miinhho commented Sep 22, 2026

Uh oh!

tbonelee left a comment

Uh oh!

tbonelee Sep 25, 2026

Uh oh!

tbonelee Sep 25, 2026

Uh oh!

voidmatcha commented Sep 25, 2026

Uh oh!

Uh oh!

tbonelee commented Sep 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

voidmatcha commented Sep 14, 2026 • 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!

voidmatcha commented Sep 19, 2026

Uh oh!

miinhho commented Sep 22, 2026

Uh oh!

tbonelee left a comment

Choose a reason for hiding this comment

Uh oh!

tbonelee Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

tbonelee Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

voidmatcha commented Sep 25, 2026

Uh oh!

Uh oh!

tbonelee commented Sep 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

voidmatcha commented Sep 14, 2026 •

edited

Loading