Reland "Fix undo / redo filling with empty steps" (#1653)

* Revert "Revert "Fix undo / redo filling with empty steps (#1649)" (#1652)"

This reverts commit 7623810166.

* Update test expectations

* Add dirty flag if workflow is not persisted

* Add dirty flag to other UI areas for new workflows

* Remove redundant code

* Fix regression: undo / redo steps lost on refresh

The history is still be cleared, but any changes made by issuing undo / redo comands prior to refresh are not lost.

* Update test expectations

Partially reverts f8cc2c0d67 - adds dirty flags back to unsaved workflows.

---------

Co-authored-by: filtered <176114999+webfiltered@users.noreply.github.com>

browser_tests/browserTabTitle.spec.ts

@@ -11,7 +11,7 @@ test.describe('Browser tab title', () => {
       const workflowName = await comfyPage.page.evaluate(async () => {
         return window['app'].extensionManager.workflow.activeWorkflow.filename
       })
-      expect(await comfyPage.page.title()).toBe(`${workflowName} - ComfyUI`)
+      expect(await comfyPage.page.title()).toBe(`*${workflowName} - ComfyUI`)
     })
 
     // Failing on CI

browser_tests/changeTracker.spec.ts

@@ -56,9 +56,7 @@ test.describe('Change Tracker', () => {
       expect(await comfyPage.getToastErrorCount()).toBe(0)
       expect(await isModified()).toBe(false)
 
-      // TODO(huchenlei): Investigate why saving the workflow is causing the
-      // undo queue to be triggered.
-      expect(await getUndoQueueSize()).toBe(1)
+      expect(await getUndoQueueSize()).toBe(0)
       expect(await getRedoQueueSize()).toBe(0)
 
       const node = (await comfyPage.getFirstNodeRef())!
@@ -66,25 +64,25 @@ test.describe('Change Tracker', () => {
       await node.click('collapse')
       await expect(node).toBeCollapsed()
       expect(await isModified()).toBe(true)
-      expect(await getUndoQueueSize()).toBe(2)
+      expect(await getUndoQueueSize()).toBe(1)
       expect(await getRedoQueueSize()).toBe(0)
 
       await comfyPage.ctrlB()
       await expect(node).toBeBypassed()
       expect(await isModified()).toBe(true)
-      expect(await getUndoQueueSize()).toBe(3)
+      expect(await getUndoQueueSize()).toBe(2)
       expect(await getRedoQueueSize()).toBe(0)
 
       await comfyPage.ctrlZ()
       await expect(node).not.toBeBypassed()
       expect(await isModified()).toBe(true)
-      expect(await getUndoQueueSize()).toBe(2)
+      expect(await getUndoQueueSize()).toBe(1)
       expect(await getRedoQueueSize()).toBe(1)
 
       await comfyPage.ctrlZ()
       await expect(node).not.toBeCollapsed()
       expect(await isModified()).toBe(false)
-      expect(await getUndoQueueSize()).toBe(1)
+      expect(await getUndoQueueSize()).toBe(0)
       expect(await getRedoQueueSize()).toBe(2)
     })
   })

browser_tests/menu.spec.ts

@@ -394,7 +394,7 @@ test.describe('Menu', () => {
       await tab.newBlankWorkflowButton.click()
       expect(await tab.getOpenedWorkflowNames()).toEqual([
         '*Unsaved Workflow.json',
-        'Unsaved Workflow (2).json'
+        '*Unsaved Workflow (2).json'
       ])
     })
 
@@ -471,6 +471,7 @@ test.describe('Menu', () => {
       const topbar = comfyPage.menu.topbar
       await topbar.saveWorkflow('workflow1.json')
       await topbar.saveWorkflowAs('workflow2.json')
+      await comfyPage.nextFrame()
       expect(
         await comfyPage.menu.workflowsTab.getOpenedWorkflowNames()
       ).toEqual(['workflow1.json', 'workflow2.json'])
@@ -519,7 +520,7 @@ test.describe('Menu', () => {
       await closeButton.click()
       expect(
         await comfyPage.menu.workflowsTab.getOpenedWorkflowNames()
-      ).toEqual(['Unsaved Workflow.json'])
+      ).toEqual(['*Unsaved Workflow.json'])
     })
   })
 

src/components/BrowserTabTitle.vue

@@ -26,7 +26,10 @@ const betaMenuEnabled = computed(
 
 const workflowStore = useWorkflowStore()
 const isUnsavedText = computed(() =>
-  workflowStore.activeWorkflow?.isModified ? ' *' : ''
+  workflowStore.activeWorkflow?.isModified ||
+  !workflowStore.activeWorkflow?.isPersisted
+    ? ' *'
+    : ''
 )
 const workflowNameText = computed(() => {
   const workflowName = workflowStore.activeWorkflow?.filename

src/components/sidebar/tabs/WorkflowsSidebarTab.vue

@@ -50,7 +50,9 @@
             <template #node="{ node }">
               <TreeExplorerTreeNode :node="node">
                 <template #before-label="{ node }">
-                  <span v-if="node.data.isModified">*</span>
+                  <span v-if="node.data.isModified || !node.data.isPersisted"
+                    >*</span
+                  >
                 </template>
                 <template #actions="{ node }">
                   <Button

src/components/topbar/WorkflowTabs.vue

@@ -18,7 +18,10 @@
       <div class="relative">
         <span
           class="status-indicator"
-          v-if="!workspaceStore.shiftDown && option.workflow.isModified"
+          v-if="
+            !workspaceStore.shiftDown &&
+            (option.workflow.isModified || !option.workflow.isPersisted)
+          "
           >•</span
         >
         <Button

src/scripts/changeTracker.ts

@@ -10,15 +10,7 @@ import _ from 'lodash'
 import * as jsondiffpatch from 'jsondiffpatch'
 import log from 'loglevel'
 
-function clone(obj: any) {
-  try {
-    if (typeof structuredClone !== 'undefined') {
-      return structuredClone(obj)
-    }
-  } catch (error) {
-    // structuredClone is stricter than using JSON.parse/stringify so fallback to that
-  }
-
+function clone<T>(obj: T): T {
   return JSON.parse(JSON.stringify(obj))
 }
 
@@ -69,7 +61,7 @@ export class ChangeTracker {
     if (this.restoringState) return
 
     logger.debug('Reset State')
-    this.activeState = state ?? this.activeState
+    if (state) this.activeState = clone(state)
     this.initialState = clone(this.activeState)
   }
 
@@ -91,6 +83,10 @@ export class ChangeTracker {
   }
 
   updateModified() {
+    api.dispatchEvent(
+      new CustomEvent('graphChanged', { detail: this.activeState })
+    )
+
     // Get the workflow from the store as ChangeTracker is raw object, i.e.
     // `this.workflow` is not reactive.
     const workflow = useWorkflowStore().getWorkflowByPath(this.workflow.path)
@@ -112,9 +108,9 @@ export class ChangeTracker {
   checkState() {
     if (!this.app.graph || this.changeCount) return
     // @ts-expect-error zod types issue. Will be fixed after we enable ts-strict
-    const currentState = this.app.graph.serialize() as ComfyWorkflowJSON
+    const currentState = clone(this.app.graph.serialize()) as ComfyWorkflowJSON
     if (!this.activeState) {
-      this.activeState = clone(currentState)
+      this.activeState = currentState
       return
     }
     if (!ChangeTracker.graphEqual(this.activeState, currentState)) {
@@ -124,11 +120,8 @@ export class ChangeTracker {
       }
       logger.debug('Diff detected. Undo queue length:', this.undoQueue.length)
 
-      this.activeState = clone(currentState)
+      this.activeState = currentState
       this.redoQueue.length = 0
-      api.dispatchEvent(
-        new CustomEvent('graphChanged', { detail: this.activeState })
-      )
       this.updateModified()
     }
   }
@@ -136,7 +129,7 @@ export class ChangeTracker {
   async updateState(source: ComfyWorkflowJSON[], target: ComfyWorkflowJSON[]) {
     const prevState = source.pop()
     if (prevState) {
-      target.push(this.activeState!)
+      target.push(this.activeState)
       this.restoringState = true
       try {
         await this.app.loadGraphData(prevState, false, false, this.workflow, {