fix(gui): enable frontend undo-redo with bug-fix for shared editing

[Xiao-zhen-Liu]

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

• 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.

[chenlica]

It will be useful to add some diagrams to illustrate the problem and solution per our discussion.

[Xiao-zhen-Liu]

It will be useful to add some diagrams to illustrate the problem and solution per our discussion.

Added.

[aglinxinyuan]

LGTM!

Although there are many code changes, only a few new lines were actually added for link validation. The remaining changes simply involve uncommenting the existing undo/redo code that was already part of the codebase.

[chenlica]

@Xiao-zhen-Liu Thanks a lot for the great summary and fix. Just want to confirm: let's treat the "repair" action as a normal step, without calling the resulting data state as an error. The message in the console should not show the state as an "error."

[chenlica]

Please check the comment.

[chenlica]

@seongjinyoon Since we found and investigated this bug in the summer, you may want to read this PR and the issue for your curiosity. No need to review it.

[seongjinyoon]

@chenlica I have read it and looked at the code as well. Thank you.

[chenlica]

LGTM now.

[chenlica]

@seongjinyoon As this PR is merged, you can try to reproduce the problem we found in the summer and confirm it's indeed solved.