From 55b4779beb0d9784d20a529255e1c019e2b6ba30 Mon Sep 17 00:00:00 2001 From: Xiao-zhen-Liu Date: Mon, 6 Oct 2025 14:46:30 -0700 Subject: [PATCH 1/4] Revert "fix(ui): temporarily disable frontend undo and redo (#3571)" This reverts commit ece56ec9a4645bd6c4223a9ecd27a3ff099ceeb0. --- .../component/menu/menu.component.html | 32 +++--- .../workflow-editor.component.spec.ts | 100 +++++++++--------- .../workflow-editor.component.ts | 32 +++--- 3 files changed, 80 insertions(+), 84 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 e212169321f..c7c486ae284 100644 --- a/core/gui/src/app/workspace/component/menu/menu.component.html +++ b/core/gui/src/app/workspace/component/menu/menu.component.html @@ -291,22 +291,22 @@ nzType="database" nzTheme="twotone"> - - - - - - - - - - - - - - - - + + 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 40574b0f7ef..161d217fd86 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 3f419d05611..6c2cd8c8d76 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(); From 2fb1a9e7d61f39701c4c7ad4118dfbd8dd786a08 Mon Sep 17 00:00:00 2001 From: Xiao-zhen-Liu Date: Mon, 6 Oct 2025 18:29:58 -0700 Subject: [PATCH 2/4] fix(gui): enable frontend undo-redo with bugfix for shared editing --- .../model/shared-model-change-handler.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) 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 4ad5bda5c8d..00e5343412c 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,17 @@ 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 + try { + this.texeraGraph.assertLinkIsValid(newLink); + const jointLinkCell = JointUIService.getJointLinkCell(newLink); + jointElementsToAdd.push(jointLinkCell); + linksToAdd.push(newLink); + } catch (error) { + // Invalid link + console.log("cannot add invalid link. cause: ", error); + this.texeraGraph.sharedModel.operatorLinkMap.delete(key); + } } if (change.action === "delete") { keysToDelete.push(key); From 5d343ace3e59ccf47a50b1d193a074201171a288 Mon Sep 17 00:00:00 2001 From: Xiao-zhen-Liu Date: Tue, 7 Oct 2025 11:34:35 -0700 Subject: [PATCH 3/4] fix(gui): enable frontend undo-redo with bugfix for shared editing --- .../model/shared-model-change-handler.ts | 1 + .../service/workflow-graph/model/workflow-graph.ts | 13 +++++++++++++ 2 files changed, 14 insertions(+) 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 00e5343412c..a4017316818 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 @@ -155,6 +155,7 @@ export class SharedModelChangeHandler { const newLink = this.texeraGraph.sharedModel.operatorLinkMap.get(key) as OperatorLink; // Validate the link first try { + this.texeraGraph.assertLinkNotDuplicated(newLink); this.texeraGraph.assertLinkIsValid(newLink); const jointLinkCell = JointUIService.getJointLinkCell(newLink); jointElementsToAdd.push(jointLinkCell); 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 8a3ff7a51c5..0a8f85ae6c6 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. * From ed3b1c53f275c418b4fdb462842ae36e7380b8e8 Mon Sep 17 00:00:00 2001 From: Xiao-zhen-Liu Date: Thu, 9 Oct 2025 10:56:56 -0700 Subject: [PATCH 4/4] refactoring. --- .../model/shared-model-change-handler.ts | 35 +++++++++++++++---- 1 file changed, 28 insertions(+), 7 deletions(-) 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 a4017316818..e5eab7a812e 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 @@ -154,16 +154,10 @@ export class SharedModelChangeHandler { if (change.action === "add") { const newLink = this.texeraGraph.sharedModel.operatorLinkMap.get(key) as OperatorLink; // Validate the link first - try { - this.texeraGraph.assertLinkNotDuplicated(newLink); - this.texeraGraph.assertLinkIsValid(newLink); + if (this.validateAndRepairNewLink(newLink)) { const jointLinkCell = JointUIService.getJointLinkCell(newLink); jointElementsToAdd.push(jointLinkCell); linksToAdd.push(newLink); - } catch (error) { - // Invalid link - console.log("cannot add invalid link. cause: ", error); - this.texeraGraph.sharedModel.operatorLinkMap.delete(key); } } if (change.action === "delete") { @@ -209,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