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 45f305a765 [ZEPPELIN-6584] Fix reversed line diff direction in New UI
revision comparator
45f305a765 is described below
commit 45f305a765915fe22e616a74c37fd7d6dbdb4d31
Author: JangAyeon <[email protected]>
AuthorDate: Thu Oct 1 00:58:01 2026 +0900
[ZEPPELIN-6584] Fix reversed line diff direction in New UI revision
comparator
### What is this PR for?
The New UI revision comparator labels a comparison as `first --> second`
(e.g. `older --> newer`), but `compareRevisions()` built the line-level diff
from the second revision back to the first. As a result, added lines were
rendered as red deletions and removed lines as green insertions for paragraphs
present in both revisions.
This PR computes the line diff in the same `first --> second` direction
shown in the UI, matching the legacy AngularJS comparator
(`diffLines(firstText, secondText)`). The whole-paragraph `added`/`deleted`
classification, which was already correct, is unchanged.
### What type of PR is it?
Bug Fix
### Todos
* [x] - Fix line diff direction
* [x] - Add focused comparator unit tests
### What is the Jira issue?
[ZEPPELIN-6584](https://issues.apache.org/jira/browse/ZEPPELIN-6584)
### How should this be tested?
* Unit tests:
```
cd zeppelin-web-angular
npm run test:shell -- revisions-comparator.component.spec.ts
npm run lint
```
### Screenshots (if appropriate)
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
Closes #5507 from JangAyeon/ZEPPELIN-6584.
Signed-off-by: YONGJAE LEE <[email protected]>
---
.../revisions-comparator.component.spec.ts | 98 ++++++++++++++++++++++
.../revisions-comparator.component.ts | 39 +++++----
2 files changed, 117 insertions(+), 20 deletions(-)
diff --git
a/zeppelin-web-angular/src/app/pages/workspace/notebook/revisions-comparator/revisions-comparator.component.spec.ts
b/zeppelin-web-angular/src/app/pages/workspace/notebook/revisions-comparator/revisions-comparator.component.spec.ts
new file mode 100644
index 0000000000..7cd14e8233
--- /dev/null
+++
b/zeppelin-web-angular/src/app/pages/workspace/notebook/revisions-comparator/revisions-comparator.component.spec.ts
@@ -0,0 +1,98 @@
+/*
+ * 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 { ChangeDetectorRef } from '@angular/core';
+import { DatePipe } from '@angular/common';
+import { describe, expect, it, vi } from 'vitest';
+
+import { NoteRevisionForCompareReceived } from '@zeppelin/sdk';
+import { MessageService } from '@zeppelin/services';
+
+import { NotebookRevisionsComparatorComponent } from
'./revisions-comparator.component';
+
+// The barrel pulls in monaco-editor; the comparator only needs the
constructor token.
+vi.mock('@zeppelin/services', () => ({ MessageService: class {} }));
+
+interface ParagraphFixture {
+ id: string;
+ text: string;
+}
+
+const revision = (revisionId: string, paragraphs: ParagraphFixture[]):
NoteRevisionForCompareReceived =>
+ ({
+ noteId: 'note',
+ revisionId,
+ position: '',
+ note: { paragraphs }
+ }) as unknown as NoteRevisionForCompareReceived;
+
+const compare = (first: NoteRevisionForCompareReceived, second:
NoteRevisionForCompareReceived) => {
+ const component = new NotebookRevisionsComparatorComponent(
+ {} as MessageService,
+ {} as ChangeDetectorRef,
+ new DatePipe('en-US')
+ );
+ component.firstNoteRevisionForCompare = first;
+ component.secondNoteRevisionForCompare = second;
+ component.compareRevisions();
+ return component.mergeNoteRevisionsDiff;
+};
+
+const segmentTexts = (diff: ReturnType<typeof compare>[number], type: 'insert'
| 'delete') =>
+ (diff.segments || []).filter(s => s.type === type).map(s => s.text);
+
+const older = revision('older', [{ id: 'p', text: 'line common\nold line' }]);
+const newer = revision('newer', [{ id: 'p', text: 'line common\nnew line' }]);
+
+describe('NotebookRevisionsComparatorComponent.compareRevisions', () => {
+ it('marks removed lines as deleted and added lines as inserted for older -->
newer', () => {
+ const [diff] = compare(older, newer);
+
+ expect(diff.type).toBe('compared');
+ expect(diff.identical).toBe(false);
+ expect(segmentTexts(diff, 'delete')).toEqual(['old line']);
+ expect(segmentTexts(diff, 'insert')).toEqual(['new line']);
+ });
+
+ it('reverses the line diff when the revisions are selected in the opposite
order', () => {
+ const [diff] = compare(newer, older);
+
+ expect(segmentTexts(diff, 'delete')).toEqual(['new line']);
+ expect(segmentTexts(diff, 'insert')).toEqual(['old line']);
+ });
+
+ it('marks a paragraph with unchanged text as identical', () => {
+ const [diff] = compare(older, revision('same', [{ id: 'p', text: 'line
common\nold line' }]));
+
+ expect(diff.type).toBe('compared');
+ expect(diff.identical).toBe(true);
+ expect(diff.segments?.every(s => s.type === 'equal')).toBe(true);
+ });
+
+ it('classifies a paragraph only in the second revision as added', () => {
+ const diffs = compare(revision('first', []), revision('second', [{ id:
'new', text: 'added\nbody' }]));
+
+ expect(diffs).toHaveLength(1);
+ expect(diffs[0].type).toBe('added');
+ expect(diffs[0].paragraph.id).toBe('new');
+ expect(diffs[0].firstString).toBe('added');
+ });
+
+ it('classifies a paragraph only in the first revision as deleted', () => {
+ const diffs = compare(revision('first', [{ id: 'gone', text:
'removed\nbody' }]), revision('second', []));
+
+ expect(diffs).toHaveLength(1);
+ expect(diffs[0].type).toBe('deleted');
+ expect(diffs[0].paragraph.id).toBe('gone');
+ expect(diffs[0].firstString).toBe('removed');
+ });
+});
diff --git
a/zeppelin-web-angular/src/app/pages/workspace/notebook/revisions-comparator/revisions-comparator.component.ts
b/zeppelin-web-angular/src/app/pages/workspace/notebook/revisions-comparator/revisions-comparator.component.ts
index b29f42dad5..2bd19ef90f 100644
---
a/zeppelin-web-angular/src/app/pages/workspace/notebook/revisions-comparator/revisions-comparator.component.ts
+++
b/zeppelin-web-angular/src/app/pages/workspace/notebook/revisions-comparator/revisions-comparator.component.ts
@@ -12,7 +12,7 @@
import { DatePipe } from '@angular/common';
import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Input,
OnDestroy, OnInit } from '@angular/core';
-import * as DiffMatchPatch from 'diff-match-patch';
+import { diff_match_patch as DiffMatchPatch } from 'diff-match-patch';
import { Subscription } from 'rxjs';
import { NoteRevisionForCompareReceived, OP, ParagraphItem, RevisionListItem }
from '@zeppelin/sdk';
@@ -127,38 +127,37 @@ export class NotebookRevisionsComparatorComponent
implements OnInit, OnDestroy {
if (!this.firstNoteRevisionForCompare ||
!this.secondNoteRevisionForCompare) {
return;
}
- const baseParagraphs = this.secondNoteRevisionForCompare.note?.paragraphs
|| [];
- const compareParagraphs =
this.firstNoteRevisionForCompare.note?.paragraphs || [];
+ // The UI reads `first --> second`, so every diff runs from first (from)
to second (to).
+ const fromParagraphs = this.firstNoteRevisionForCompare.note?.paragraphs
|| [];
+ const toParagraphs = this.secondNoteRevisionForCompare.note?.paragraphs ||
[];
const paragraphDiffs: MergedParagraphDiff[] = [];
- for (const p1 of baseParagraphs) {
- const p2 = compareParagraphs.find((p: ParagraphItem) => p.id === p1.id)
|| null;
- if (p2 === null) {
+ for (const toParagraph of toParagraphs) {
+ const fromParagraph = fromParagraphs.find((p: ParagraphItem) => p.id ===
toParagraph.id) || null;
+ if (fromParagraph === null) {
paragraphDiffs.push({
- paragraph: p1,
- firstString: (p1.text || '').split('\n')[0],
+ paragraph: toParagraph,
+ firstString: (toParagraph.text || '').split('\n')[0],
type: 'added'
});
} else {
- const text1 = p1.text || '';
- const text2 = p2.text || '';
- const diffResult = this.buildLineDiff(text1, text2);
+ const diffResult = this.buildLineDiff(fromParagraph.text || '',
toParagraph.text || '');
paragraphDiffs.push({
- paragraph: p1,
+ paragraph: toParagraph,
segments: diffResult.segments,
identical: diffResult.identical,
- firstString: (p1.text || '').split('\n')[0],
+ firstString: (toParagraph.text || '').split('\n')[0],
type: 'compared'
});
}
}
- for (const p2 of compareParagraphs) {
- const p1 = baseParagraphs.find((p: ParagraphItem) => p.id === p2.id) ||
null;
- if (p1 === null) {
+ for (const fromParagraph of fromParagraphs) {
+ const toParagraph = toParagraphs.find((p: ParagraphItem) => p.id ===
fromParagraph.id) || null;
+ if (toParagraph === null) {
paragraphDiffs.push({
- paragraph: p2,
- firstString: (p2.text || '').split('\n')[0],
+ paragraph: fromParagraph,
+ firstString: (fromParagraph.text || '').split('\n')[0],
type: 'deleted'
});
}
@@ -183,8 +182,8 @@ export class NotebookRevisionsComparatorComponent
implements OnInit, OnDestroy {
return this.datePipe.transform(time * 1000, 'MMMM d yyyy, h:mm:ss a') ||
'';
}
- private buildLineDiff(text1: string, text2: string): { segments:
DiffSegment[]; identical: boolean } {
- const { chars1, chars2, lineArray } = this.dmp.diff_linesToChars_(text1,
text2);
+ private buildLineDiff(fromText: string, toText: string): { segments:
DiffSegment[]; identical: boolean } {
+ const { chars1, chars2, lineArray } =
this.dmp.diff_linesToChars_(fromText, toText);
const diffs = this.dmp.diff_main(chars1, chars2, false);
this.dmp.diff_charsToLines_(diffs, lineArray);
