This is an automated email from the ASF dual-hosted git repository.
xiaozhenliu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/texera.git
The following commit(s) were added to refs/heads/main by this push:
new 379ac816a5 fix(gui): enable frontend undo-redo with bug-fix for shared
editing (#3836)
379ac816a5 is described below
commit 379ac816a5c00341fd4f1df9cbde6d1c86638ff1
Author: Xiaozhen Liu <[email protected]>
AuthorDate: Thu Oct 9 21:24:28 2025 -0700
fix(gui): enable frontend undo-redo with bug-fix for shared editing (#3836)
## Purpose
#3571 disabled frontend undo/redo due to an existing bug with the
undo/redo manager during shared editing. This PR fixes that bug and
re-enables undo/redo.
## Bug with shared editing
The bug can be minimally reproduced as follows with two users editing
the same workflow (or two tabs opened by the same user):
1. User A deletes a link E from operator X to Y on the canvas,
2. User B deletes operator Y.
3. User A clicks "undo", and the workflow reaches an erroneous state,
where there is a link E that connects to an operator Y that no longer
exists. Note E exists in the frontend data but is not visible on the UI.
The following gif shows this process.
## Shared-editing Architecture
Shared editing (#1674) is achieved by letting the frontend rely on data
structures from yjs (a CRDT library) as its data model, as any
manipulation to these data structures can be propagated to other users
with automatic conflict-resolution.
There are two layers of data on each user's Texera frontend, one being
the UI data (jointjs), and the other being this shared "Y data". The two
layers in each user's UI are synched by our application code, and the Y
data between users of a shared-editing sessions are kept in sync with
automatic conflict resolution by relying on yjs. The following diagram
shows what happens when a user adds a link and how the other user sees
this change in real-time.
Yjs's CRDT guarantees the eventual **consistency** of this underlying
data model among concurrent editors, i.e., it makes sure this data model
is correctly synced in each editor's frontend.
## The core problem
Yjs does not offer a "graph" data structure, and currently in Texera,
the shared data structures for operators and links are two separate
`Map`s:
- `operatorIDMap`: `operatorID`->`Operator`
- `operatorLinkMap`: `linkID`-> `OperatorLink`
There is an application-specific "referential constraint" in Texera's
frontend that "a link must connect to an operator that exists", and this
kind of sanity checking on the data is not the concern of CRDT. It can
only be enforced by the application (i.e., ourselves). Ideally, before
making any changes to the shared data model, we should do sanity
checking and reject changes that violate our application-specific
constraints.
As shown below, in each user's frontend, there are 3 paths where the
shared data model can be modified.
**Path 1**: The first is path includes those changes initiated by a
user's UI actions (e.g., add a link on the UI). For this path, we do
have existing sanity checking logic:
```
public addLink(link: OperatorLink): void {
this.assertLinkNotExists(link);
this.assertLinkIsValid(link);
this.sharedModel.operatorLinkMap.set(link.linkID, link);
}
```
**Path 2**: Another path is undo/redo, which is purely managed by an
`UndoManager`, also offered by Yjs. This module is local to each user's
frontend, and it automatically tracks local changes to the shared data
model. When a user clicks "undo", `UndoManager` directly applies changes
to the shared data model. **The core of the problem is there is no
sanity checking on this path.**
**Path 3**: The third path is remote changes from another collaborator.
There is also no sanity checking on this path, but the correctness of
such changes depends on whether the change was sanity-checked on the
collaborator's side (i.e., if it is a UI change from User A, the
propagated change to User B's frontend would be sanity-checked; if it is
a undo change, however, the propagated changed to User B would not be
sanity-checked and could cause issues.)
## Cause of the bug
The following diagram shows how the bug happens from the perspective of
the shared model.
When user A clicks "Undo" after 2), the `UndoManager` simply applies the
reverse-operation of "Delete E", and add the link `E` to
`operatorLinkMap `. As there is no sanity checking during this process,
this operation succeeds, and the shared model reaches a state that
violates the constraint.
## Solution
Unfortunately, due to the limitations of Yjs's APIs, it is not possible
to add sanity checking to Path 2 or 3 **before** a change is applied, as
an undo/redo operation on the `UndoManager`'s stack is not exposed as a
meaningful action (i.e., there is no way to tell that an action to be
applied to the shared model is an `addLink` if it is an undo operation).
Nevertheless, we can react to a change to the shared model that is
initiated from Path 2 or Path 3 after the change has been applied, and
add sanity checking logic there to "repair" unsanitary changes.
This places (`SharedModelChangeHandler`) is exactly where we sync the
changes from the shared model to the UI: any changes to the shared model
not initiated by the UI (i.e., changes from the `UndoManager` or remote
changes by other users) go through this place, and such changes are
parsed as meaningful changes such as "add a link", "delete an operator",
etc.
Currently, the only sanity checking needed is to check if a newly added
link connects to operator / ports that exist and that it is not a
duplicate link. We add such checking logic in
`SharedModelChangeHandler`, and revert unsanitary operations before it
is reflected on the UI.
## Demo
The following gif shows the experience after the fix. When unsanitary
actions caused by undo happens, it would fail and we log it in the
console. The workflow JSON no longer gets damaged.
---------
Co-authored-by: Yicong Huang
<[email protected]>
---
.../workspace/component/menu/menu.component.html | 32 +++----
.../workflow-editor.component.spec.ts | 100 ++++++++++-----------
.../workflow-editor/workflow-editor.component.ts | 32 ++++---
.../model/shared-model-change-handler.ts | 36 +++++++-
.../service/workflow-graph/model/workflow-graph.ts | 13 +++
5 files changed, 126 insertions(+), 87 deletions(-)
diff --git a/core/gui/src/app/workspace/component/menu/menu.component.html
b/core/gui/src/app/workspace/component/menu/menu.component.html
index d75d918d43..fc691f18b7 100644
--- a/core/gui/src/app/workspace/component/menu/menu.component.html
+++ b/core/gui/src/app/workspace/component/menu/menu.component.html
@@ -276,22 +276,22 @@
nzType="database"
nzTheme="twotone"></i>
</button>
- <!-- <button-->
- <!-- (click)="undoRedoService.undoAction()"-->
- <!-- [disabled]="displayParticularWorkflowVersion ||
!undoRedoService.canUndo()"-->
- <!-- nz-button>-->
- <!-- <i-->
- <!-- nz-icon-->
- <!-- nzType="undo"></i>-->
- <!-- </button>-->
- <!-- <button-->
- <!-- (click)="undoRedoService.redoAction()"-->
- <!-- [disabled]="displayParticularWorkflowVersion ||
!undoRedoService.canRedo()"-->
- <!-- nz-button>-->
- <!-- <i-->
- <!-- nz-icon-->
- <!-- nzType="redo"></i>-->
- <!-- </button>-->
+ <button
+ (click)="undoRedoService.undoAction()"
+ [disabled]="displayParticularWorkflowVersion ||
!undoRedoService.canUndo()"
+ nz-button>
+ <i
+ nz-icon
+ nzType="undo"></i>
+ </button>
+ <button
+ (click)="undoRedoService.redoAction()"
+ [disabled]="displayParticularWorkflowVersion ||
!undoRedoService.canRedo()"
+ nz-button>
+ <i
+ nz-icon
+ nzType="redo"></i>
+ </button>
</nz-button-group>
</ng-template>
<nz-dropdown-menu #menu="nzDropdownMenu">
diff --git
a/core/gui/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts
b/core/gui/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts
index 40574b0f7e..161d217fd8 100644
---
a/core/gui/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts
+++
b/core/gui/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts
@@ -897,56 +897,54 @@ describe("WorkflowEditorComponent", () => {
expect(jointGraphWrapper.getCurrentHighlightedOperatorIDs()).toContain(mockResultPredicate.operatorID);
});
- // Temporarily disabling undo-redo because of a bug that can cause invalid
workflow structures.
- // TODO: enable after fixing the bug.
- // //undo
- // it("should undo action when user presses command + Z or control + Z",
() => {
- // spyOn(workflowVersionService,
"getDisplayParticularVersionStream").and.returnValue(of(false));
- // spyOn(undoRedoService, "canUndo").and.returnValue(true);
- // let undoSpy = spyOn(undoRedoService, "undoAction");
- // fixture.detectChanges();
- // const commandZEvent = new KeyboardEvent("keydown", { key: "Z",
metaKey: true, shiftKey: false });
- // (document.activeElement as HTMLElement)?.blur();
- // document.dispatchEvent(commandZEvent);
- // fixture.detectChanges();
- // expect(undoSpy).toHaveBeenCalledTimes(1);
- //
- // const controlZEvent = new KeyboardEvent("keydown", { key: "Z",
ctrlKey: true, shiftKey: false });
- // (document.activeElement as HTMLElement)?.blur();
- // document.dispatchEvent(controlZEvent);
- // fixture.detectChanges();
- // expect(undoSpy).toHaveBeenCalledTimes(2);
- // });
- //
- // //redo
- // it("should redo action when user presses command/control + Y or
command/control + shift + Z", () => {
- // spyOn(workflowVersionService,
"getDisplayParticularVersionStream").and.returnValue(of(false));
- // spyOn(undoRedoService, "canRedo").and.returnValue(true);
- // let redoSpy = spyOn(undoRedoService, "redoAction");
- // fixture.detectChanges();
- // const commandYEvent = new KeyboardEvent("keydown", { key: "y",
metaKey: true, shiftKey: false });
- // (document.activeElement as HTMLElement)?.blur();
- // document.dispatchEvent(commandYEvent);
- // fixture.detectChanges();
- // expect(redoSpy).toHaveBeenCalledTimes(1);
- //
- // const controlYEvent = new KeyboardEvent("keydown", { key: "y",
ctrlKey: true, shiftKey: false });
- // (document.activeElement as HTMLElement)?.blur();
- // document.dispatchEvent(controlYEvent);
- // fixture.detectChanges();
- // expect(redoSpy).toHaveBeenCalledTimes(2);
- //
- // const commandShitZEvent = new KeyboardEvent("keydown", { key: "z",
metaKey: true, shiftKey: true });
- // (document.activeElement as HTMLElement)?.blur();
- // document.dispatchEvent(commandShitZEvent);
- // fixture.detectChanges();
- // expect(redoSpy).toHaveBeenCalledTimes(3);
- //
- // const controlShitZEvent = new KeyboardEvent("keydown", { key: "z",
ctrlKey: true, shiftKey: true });
- // (document.activeElement as HTMLElement)?.blur();
- // document.dispatchEvent(controlShitZEvent);
- // fixture.detectChanges();
- // expect(redoSpy).toHaveBeenCalledTimes(4);
- // });
+ //undo
+ it("should undo action when user presses command + Z or control + Z", ()
=> {
+ spyOn(workflowVersionService,
"getDisplayParticularVersionStream").and.returnValue(of(false));
+ spyOn(undoRedoService, "canUndo").and.returnValue(true);
+ let undoSpy = spyOn(undoRedoService, "undoAction");
+ fixture.detectChanges();
+ const commandZEvent = new KeyboardEvent("keydown", { key: "Z", metaKey:
true, shiftKey: false });
+ (document.activeElement as HTMLElement)?.blur();
+ document.dispatchEvent(commandZEvent);
+ fixture.detectChanges();
+ expect(undoSpy).toHaveBeenCalledTimes(1);
+
+ const controlZEvent = new KeyboardEvent("keydown", { key: "Z", ctrlKey:
true, shiftKey: false });
+ (document.activeElement as HTMLElement)?.blur();
+ document.dispatchEvent(controlZEvent);
+ fixture.detectChanges();
+ expect(undoSpy).toHaveBeenCalledTimes(2);
+ });
+
+ //redo
+ it("should redo action when user presses command/control + Y or
command/control + shift + Z", () => {
+ spyOn(workflowVersionService,
"getDisplayParticularVersionStream").and.returnValue(of(false));
+ spyOn(undoRedoService, "canRedo").and.returnValue(true);
+ let redoSpy = spyOn(undoRedoService, "redoAction");
+ fixture.detectChanges();
+ const commandYEvent = new KeyboardEvent("keydown", { key: "y", metaKey:
true, shiftKey: false });
+ (document.activeElement as HTMLElement)?.blur();
+ document.dispatchEvent(commandYEvent);
+ fixture.detectChanges();
+ expect(redoSpy).toHaveBeenCalledTimes(1);
+
+ const controlYEvent = new KeyboardEvent("keydown", { key: "y", ctrlKey:
true, shiftKey: false });
+ (document.activeElement as HTMLElement)?.blur();
+ document.dispatchEvent(controlYEvent);
+ fixture.detectChanges();
+ expect(redoSpy).toHaveBeenCalledTimes(2);
+
+ const commandShitZEvent = new KeyboardEvent("keydown", { key: "z",
metaKey: true, shiftKey: true });
+ (document.activeElement as HTMLElement)?.blur();
+ document.dispatchEvent(commandShitZEvent);
+ fixture.detectChanges();
+ expect(redoSpy).toHaveBeenCalledTimes(3);
+
+ const controlShitZEvent = new KeyboardEvent("keydown", { key: "z",
ctrlKey: true, shiftKey: true });
+ (document.activeElement as HTMLElement)?.blur();
+ document.dispatchEvent(controlShitZEvent);
+ fixture.detectChanges();
+ expect(redoSpy).toHaveBeenCalledTimes(4);
+ });
});
});
diff --git
a/core/gui/src/app/workspace/component/workflow-editor/workflow-editor.component.ts
b/core/gui/src/app/workspace/component/workflow-editor/workflow-editor.component.ts
index 3f419d0561..6c2cd8c8d7 100644
---
a/core/gui/src/app/workspace/component/workflow-editor/workflow-editor.component.ts
+++
b/core/gui/src/app/workspace/component/workflow-editor/workflow-editor.component.ts
@@ -190,23 +190,21 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
.pipe(takeUntil(this._onProcessKeyboardActionObservable))
.subscribe(displayParticularWorkflowVersion => {
if (!displayParticularWorkflowVersion) {
- // Temporarily disabling undo-redo because of a bug that can cause
invalid workflow structures.
- // TODO: enable after fixing the bug.
- // // cmd/ctrl+z undo ; ctrl+y or cmd/ctrl + shift+z for redo
- // if ((event.metaKey || event.ctrlKey) && !event.shiftKey &&
event.key.toLowerCase() === "z") {
- // // UNDO
- // if (this.undoRedoService.canUndo()) {
- // this.undoRedoService.undoAction();
- // }
- // } else if (
- // ((event.metaKey || event.ctrlKey) && !event.shiftKey &&
event.key.toLowerCase() === "y") ||
- // ((event.metaKey || event.ctrlKey) && event.shiftKey &&
event.key.toLowerCase() === "z")
- // ) {
- // // redo
- // if (this.undoRedoService.canRedo()) {
- // this.undoRedoService.redoAction();
- // }
- // }
+ // cmd/ctrl+z undo ; ctrl+y or cmd/ctrl + shift+z for redo
+ if ((event.metaKey || event.ctrlKey) && !event.shiftKey &&
event.key.toLowerCase() === "z") {
+ // UNDO
+ if (this.undoRedoService.canUndo()) {
+ this.undoRedoService.undoAction();
+ }
+ } else if (
+ ((event.metaKey || event.ctrlKey) && !event.shiftKey &&
event.key.toLowerCase() === "y") ||
+ ((event.metaKey || event.ctrlKey) && event.shiftKey &&
event.key.toLowerCase() === "z")
+ ) {
+ // redo
+ if (this.undoRedoService.canRedo()) {
+ this.undoRedoService.redoAction();
+ }
+ }
// below for future hotkeys
}
this._onProcessKeyboardActionObservable.complete();
diff --git
a/core/gui/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.ts
b/core/gui/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.ts
index 4ad5bda5c8..e5eab7a812 100644
---
a/core/gui/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.ts
+++
b/core/gui/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.ts
@@ -153,9 +153,12 @@ export class SharedModelChangeHandler {
event.changes.keys.forEach((change, key) => {
if (change.action === "add") {
const newLink =
this.texeraGraph.sharedModel.operatorLinkMap.get(key) as OperatorLink;
- const jointLinkCell = JointUIService.getJointLinkCell(newLink);
- jointElementsToAdd.push(jointLinkCell);
- linksToAdd.push(newLink);
+ // Validate the link first
+ if (this.validateAndRepairNewLink(newLink)) {
+ const jointLinkCell = JointUIService.getJointLinkCell(newLink);
+ jointElementsToAdd.push(jointLinkCell);
+ linksToAdd.push(newLink);
+ }
}
if (change.action === "delete") {
keysToDelete.push(key);
@@ -200,6 +203,33 @@ export class SharedModelChangeHandler {
});
}
+ /**
+ * Check the sanity of a newly added link. We have constraints on a new link
(it should connect to operators and
+ * ports that exist, and it should not be duplicated with another link
connecting to the same operator ports.) Such
+ * constraints are enforced if the change to the shared model comes from
local UI (`WorkflowGraph.addLink()`). If
+ * the change is initiated by the `UndoManager` or from remote
collaborators, however, due to the limitations of Yjs,
+ * it is not possible to check the sanity of this operation before it is
applied to the shared model. To ensure the
+ * integrity of the shared model, we validate the link add operation here
instead, and repair the shared model if it
+ * violates the constraints.
+ * @param newLink A new link that has already been added to the shared model
+ * @returns Whether this new link passes the sanity check. If it does, this
change can be applied to the UI. Otherwise
+ * this link is already deleted from the shared model.
+ */
+ private validateAndRepairNewLink(newLink: OperatorLink): boolean {
+ try {
+ this.texeraGraph.assertLinkNotDuplicated(newLink);
+ // Verify the link connects to operators and ports that exist.
+ this.texeraGraph.assertLinkIsValid(newLink);
+ return true;
+ } catch (error) {
+ // Invalid link, repair the shared model
+ this.texeraGraph.sharedModel.operatorLinkMap.delete(newLink.linkID);
+ // This is treated as a normal repair step and not an error.
+ console.log("failed to add link. cause: ", (error as Error).message);
+ return false;
+ }
+ }
+
/**
* Syncs element positions. Will temporarily block local updates.
* @private
diff --git
a/core/gui/src/app/workspace/service/workflow-graph/model/workflow-graph.ts
b/core/gui/src/app/workspace/service/workflow-graph/model/workflow-graph.ts
index 8a3ff7a51c..0a8f85ae6c 100644
--- a/core/gui/src/app/workspace/service/workflow-graph/model/workflow-graph.ts
+++ b/core/gui/src/app/workspace/service/workflow-graph/model/workflow-graph.ts
@@ -1111,6 +1111,19 @@ export class WorkflowGraph {
}
}
+ /**
+ * Checks if a link is unique in the graph. Throws an error if more than one
link with the same source and target
+ * as the given link exists.
+ */
+ public assertLinkNotDuplicated(link: OperatorLink): void {
+ const links = this.getAllLinks().filter(
+ value => isEqual(value.source, link.source) && isEqual(value.target,
link.target)
+ );
+ if (links.length > 1) {
+ throw new Error(`duplicate link found with same source and target as
link ${link}`);
+ }
+ }
+
/**
* Retrieves a subgraph (subDAG) from the workflow graph. This method
excludes disabled operators and links.
*
