From f8ec87ddea1ffdfe15038283e74ad155870e8900 Mon Sep 17 00:00:00 2001 From: Chenlei Hu Date: Fri, 8 Nov 2024 22:24:35 -0500 Subject: [PATCH] Fix changeTracker modified state (#1481) * Add jsondiffpatch * Add logs * Add graphDiff helper * Fix changeTracker * Add loglevel * Add playwright test * Fix jest test * nit * nit * Fix test url * nit --- browser_tests/changeTracker.spec.ts | 37 +++++++++++++ browser_tests/fixtures/ComfyPage.ts | 4 +- package-lock.json | 45 +++++++++++++++- package.json | 2 + src/scripts/app.ts | 2 +- src/scripts/changeTracker.ts | 80 ++++++++++++++++++++++++++--- tests-ui/tests/fast/globalSetup.ts | 6 +++ tests-ui/tests/globalSetup.ts | 6 +++ 8 files changed, 171 insertions(+), 11 deletions(-) diff --git a/browser_tests/changeTracker.spec.ts b/browser_tests/changeTracker.spec.ts index 655f07f05..c2bf290bb 100644 --- a/browser_tests/changeTracker.spec.ts +++ b/browser_tests/changeTracker.spec.ts @@ -16,6 +16,43 @@ async function afterChange(comfyPage: ComfyPage) { } test.describe('Change Tracker', () => { + test.describe('Undo/Redo', () => { + test.beforeEach(async ({ comfyPage }) => { + await comfyPage.setSetting('Comfy.UseNewMenu', 'Top') + }) + + // Flaky https://github.com/Comfy-Org/ComfyUI_frontend/pull/1481 + // The collapse can be recognized as several changes. + test.skip('Can undo multiple operations', async ({ comfyPage }) => { + function isModified() { + return comfyPage.page.evaluate(async () => { + return window['app'].extensionManager.workflow.activeWorkflow + .isModified + }) + } + + await comfyPage.menu.topbar.saveWorkflow('undo-redo-test') + expect(await isModified()).toBe(false) + + const node = (await comfyPage.getFirstNodeRef())! + await node.click('collapse') + await expect(node).toBeCollapsed() + expect(await isModified()).toBe(true) + + await comfyPage.ctrlB() + await expect(node).toBeBypassed() + expect(await isModified()).toBe(true) + + await comfyPage.ctrlZ() + await expect(node).not.toBeBypassed() + expect(await isModified()).toBe(true) + + await comfyPage.ctrlZ() + await expect(node).not.toBeCollapsed() + expect(await isModified()).toBe(false) + }) + }) + test('Can group multiple change actions into a single transaction', async ({ comfyPage }) => { diff --git a/browser_tests/fixtures/ComfyPage.ts b/browser_tests/fixtures/ComfyPage.ts index 222865f92..65950f4dc 100644 --- a/browser_tests/fixtures/ComfyPage.ts +++ b/browser_tests/fixtures/ComfyPage.ts @@ -167,7 +167,7 @@ export class ComfyPage { } async setupUser(username: string) { - const res = await this.request.get(`${this.url}/users`) + const res = await this.request.get(`${this.url}/api/users`) if (res.status() !== 200) throw new Error(`Failed to retrieve users: ${await res.text()}`) @@ -181,7 +181,7 @@ export class ComfyPage { } async createUser(username: string) { - const resp = await this.request.post(`${this.url}/users`, { + const resp = await this.request.post(`${this.url}/api/users`, { data: { username } }) diff --git a/package-lock.json b/package-lock.json index fe1c422be..86c27f9a0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -17,7 +17,9 @@ "axios": "^1.7.4", "dotenv": "^16.4.5", "fuse.js": "^7.0.0", + "jsondiffpatch": "^0.6.0", "lodash": "^4.17.21", + "loglevel": "^1.9.2", "pinia": "^2.1.7", "primeicons": "^7.0.0", "primevue": "^4.0.5", @@ -3890,6 +3892,12 @@ "@babel/types": "^7.20.7" } }, + "node_modules/@types/diff-match-patch": { + "version": "1.0.36", + "resolved": "https://registry.npmjs.org/@types/diff-match-patch/-/diff-match-patch-1.0.36.tgz", + "integrity": "sha512-xFdR6tkm0MWvBfO8xXCSsinYxHcqkQUlcHeSpMC2ukzOb6lwQAfDmW+Qt0AvlGd8HpsS28qKsB+oPeJn9I39jg==", + "license": "MIT" + }, "node_modules/@types/estree": { "version": "1.0.5", "resolved": "https://registry.npmjs.org/@types/estree/-/estree-1.0.5.tgz", @@ -5379,7 +5387,6 @@ "version": "5.3.0", "resolved": "https://registry.npmjs.org/chalk/-/chalk-5.3.0.tgz", "integrity": "sha512-dLitG79d+GV1Nb/VYcCDFivJeK1hiukt9QjRNVOsUtTy1rR1YJsmpGGTZ3qJos+uw7WmWF4wUwBd9jxjocFC2w==", - "dev": true, "engines": { "node": "^12.17.0 || ^14.13 || >=16.0.0" }, @@ -5991,6 +5998,12 @@ "node": ">=0.3.1" } }, + "node_modules/diff-match-patch": { + "version": "1.0.5", + "resolved": "https://registry.npmjs.org/diff-match-patch/-/diff-match-patch-1.0.5.tgz", + "integrity": "sha512-IayShXAgj/QMXgB0IWmKx+rOPuGMhqm5w6jvFxmVenXKIzRqTAAsbBPT3kWQeGANj3jGgvcvv4yK6SxqYmikgw==", + "license": "Apache-2.0" + }, "node_modules/diff-sequences": { "version": "29.6.3", "resolved": "https://registry.npmjs.org/diff-sequences/-/diff-sequences-29.6.3.tgz", @@ -9402,6 +9415,23 @@ "node": ">=6" } }, + "node_modules/jsondiffpatch": { + "version": "0.6.0", + "resolved": "https://registry.npmjs.org/jsondiffpatch/-/jsondiffpatch-0.6.0.tgz", + "integrity": "sha512-3QItJOXp2AP1uv7waBkao5nCvhEv+QmJAd38Ybq7wNI74Q+BBmnLn4EDKz6yI9xGAIQoUF87qHt+kc1IVxB4zQ==", + "license": "MIT", + "dependencies": { + "@types/diff-match-patch": "^1.0.36", + "chalk": "^5.3.0", + "diff-match-patch": "^1.0.5" + }, + "bin": { + "jsondiffpatch": "bin/jsondiffpatch.js" + }, + "engines": { + "node": "^18.0.0 || >=20.0.0" + } + }, "node_modules/jsonfile": { "version": "6.1.0", "resolved": "https://registry.npmjs.org/jsonfile/-/jsonfile-6.1.0.tgz", @@ -9934,6 +9964,19 @@ "url": "https://github.com/chalk/strip-ansi?sponsor=1" } }, + "node_modules/loglevel": { + "version": "1.9.2", + "resolved": "https://registry.npmjs.org/loglevel/-/loglevel-1.9.2.tgz", + "integrity": "sha512-HgMmCqIJSAKqo68l0rS2AanEWfkxaZ5wNiEFb5ggm08lDs9Xl2KxBlX3PTcaD2chBM1gXAYf491/M2Rv8Jwayg==", + "license": "MIT", + "engines": { + "node": ">= 0.6.0" + }, + "funding": { + "type": "tidelift", + "url": "https://tidelift.com/funding/github/npm/loglevel" + } + }, "node_modules/loupe": { "version": "3.1.1", "resolved": "https://registry.npmjs.org/loupe/-/loupe-3.1.1.tgz", diff --git a/package.json b/package.json index d0a8e78ea..2d47e58d7 100644 --- a/package.json +++ b/package.json @@ -79,7 +79,9 @@ "axios": "^1.7.4", "dotenv": "^16.4.5", "fuse.js": "^7.0.0", + "jsondiffpatch": "^0.6.0", "lodash": "^4.17.21", + "loglevel": "^1.9.2", "pinia": "^2.1.7", "primeicons": "^7.0.0", "primevue": "^4.0.5", diff --git a/src/scripts/app.ts b/src/scripts/app.ts index fac5a0ac8..5a6a2d8b4 100644 --- a/src/scripts/app.ts +++ b/src/scripts/app.ts @@ -2363,7 +2363,7 @@ export class ComfyApp { } await this.#invokeExtensionsAsync('afterConfigureGraph', missingNodeTypes) // @ts-expect-error zod types issue. Will be fixed after we enable ts-strict - workflowService.afterLoadNewGraph(workflow, this.graph.serialize()) + await workflowService.afterLoadNewGraph(workflow, this.graph.serialize()) requestAnimationFrame(() => { this.graph.setDirtyCanvas(true, true) }) diff --git a/src/scripts/changeTracker.ts b/src/scripts/changeTracker.ts index 84acf5c87..b9972224a 100644 --- a/src/scripts/changeTracker.ts +++ b/src/scripts/changeTracker.ts @@ -7,6 +7,8 @@ import type { ComfyWorkflowJSON } from '@/types/comfyWorkflow' import type { ExecutedWsMessage } from '@/types/apiTypes' import { useExecutionStore } from '@/stores/executionStore' import _ from 'lodash' +import * as jsondiffpatch from 'jsondiffpatch' +import log from 'loglevel' function clone(obj: any) { try { @@ -20,6 +22,10 @@ function clone(obj: any) { return JSON.parse(JSON.stringify(obj)) } +const logger = log.getLogger('ChangeTracker') +// Change to debug for more verbose logging +logger.setLevel('info') + export class ChangeTracker { static MAX_HISTORY = 50 /** @@ -29,6 +35,10 @@ export class ChangeTracker { undoQueue: ComfyWorkflowJSON[] = [] redoQueue: ComfyWorkflowJSON[] = [] changeCount: number = 0 + /** + * Whether the redo/undo restoring is in progress. + */ + private restoringState: boolean = false ds?: { scale: number; offset: [number, number] } nodeOutputs?: Record @@ -55,8 +65,12 @@ export class ChangeTracker { * Save the current state as the initial state. */ reset(state?: ComfyWorkflowJSON) { + // Do not reset the state if we are restoring. + if (this.restoringState) return + + logger.debug('Reset State') this.activeState = state ?? this.activeState - this.initialState = this.activeState + this.initialState = clone(this.activeState) } store() { @@ -85,6 +99,13 @@ export class ChangeTracker { this.initialState, this.activeState ) + if (workflow.isModified) { + const diff = ChangeTracker.graphDiff( + this.initialState, + this.activeState + ) + logger.debug('Graph diff:', diff) + } } } @@ -101,6 +122,8 @@ export class ChangeTracker { if (this.undoQueue.length > ChangeTracker.MAX_HISTORY) { this.undoQueue.shift() } + logger.debug('Diff detected. Undo queue length:', this.undoQueue.length) + this.activeState = clone(currentState) this.redoQueue.length = 0 api.dispatchEvent( @@ -114,21 +137,38 @@ export class ChangeTracker { const prevState = source.pop() if (prevState) { target.push(this.activeState!) - await this.app.loadGraphData(prevState, false, false, this.workflow, { - showMissingModelsDialog: false, - showMissingNodesDialog: false - }) - this.activeState = prevState - this.updateModified() + this.restoringState = true + try { + await this.app.loadGraphData(prevState, false, false, this.workflow, { + showMissingModelsDialog: false, + showMissingNodesDialog: false + }) + this.activeState = prevState + this.updateModified() + } finally { + this.restoringState = false + } } } async undo() { await this.updateState(this.undoQueue, this.redoQueue) + logger.debug( + 'Undo. Undo queue length:', + this.undoQueue.length, + 'Redo queue length:', + this.redoQueue.length + ) } async redo() { await this.updateState(this.redoQueue, this.undoQueue) + logger.debug( + 'Redo. Undo queue length:', + this.undoQueue.length, + 'Redo queue length:', + this.redoQueue.length + ) } async undoRedo(e: KeyboardEvent) { @@ -198,6 +238,7 @@ export class ChangeTracker { // If our active element is some type of input then handle changes after they're done if (ChangeTracker.bindInput(app, bindInputEl)) return + logger.debug('checkState on keydown') changeTracker.checkState() }) }, @@ -207,21 +248,25 @@ export class ChangeTracker { window.addEventListener('keyup', (e) => { if (keyIgnored) { keyIgnored = false + logger.debug('checkState on keyup') checkState() } }) // Handle clicking DOM elements (e.g. widgets) window.addEventListener('mouseup', () => { + logger.debug('checkState on mouseup') checkState() }) // Handle prompt queue event for dynamic widget changes api.addEventListener('promptQueued', () => { + logger.debug('checkState on promptQueued') checkState() }) api.addEventListener('graphCleared', () => { + logger.debug('checkState on graphCleared') checkState() }) @@ -229,12 +274,14 @@ export class ChangeTracker { const processMouseUp = LGraphCanvas.prototype.processMouseUp LGraphCanvas.prototype.processMouseUp = function (e) { const v = processMouseUp.apply(this, [e]) + logger.debug('checkState on processMouseUp') checkState() return v } const processMouseDown = LGraphCanvas.prototype.processMouseDown LGraphCanvas.prototype.processMouseDown = function (e) { const v = processMouseDown.apply(this, [e]) + logger.debug('checkState on processMouseDown') checkState() return v } @@ -251,6 +298,7 @@ export class ChangeTracker { callback(v) checkState() } + logger.debug('checkState on prompt') return prompt.apply(this, [title, value, extendedCallback, event]) } @@ -258,6 +306,7 @@ export class ChangeTracker { const close = LiteGraph.ContextMenu.prototype.close LiteGraph.ContextMenu.prototype.close = function (e: MouseEvent) { const v = close.apply(this, [e]) + logger.debug('checkState on contextMenuClose') checkState() return v } @@ -267,6 +316,7 @@ export class ChangeTracker { LiteGraph.LGraph.prototype.onNodeAdded = function (node: LGraphNode) { const v = onNodeAdded?.apply(this, [node]) if (!app?.configuringGraph) { + logger.debug('checkState on onNodeAdded') checkState() } return v @@ -357,4 +407,20 @@ export class ChangeTracker { return false } + + static graphDiff(a: ComfyWorkflowJSON, b: ComfyWorkflowJSON) { + function sortGraphNodes(graph: ComfyWorkflowJSON) { + return { + links: graph.links, + groups: graph.groups, + nodes: graph.nodes.sort((a, b) => { + if (typeof a.id === 'number' && typeof b.id === 'number') { + return a.id - b.id + } + return 0 + }) + } + } + return jsondiffpatch.diff(sortGraphNodes(a), sortGraphNodes(b)) + } } diff --git a/tests-ui/tests/fast/globalSetup.ts b/tests-ui/tests/fast/globalSetup.ts index c5d89ee96..82a8d4f69 100644 --- a/tests-ui/tests/fast/globalSetup.ts +++ b/tests-ui/tests/fast/globalSetup.ts @@ -19,4 +19,10 @@ module.exports = async function () { useI18n: jest.fn() } }) + + jest.mock('jsondiffpatch', () => { + return { + diff: jest.fn() + } + }) } diff --git a/tests-ui/tests/globalSetup.ts b/tests-ui/tests/globalSetup.ts index 512bc907d..995dbd422 100644 --- a/tests-ui/tests/globalSetup.ts +++ b/tests-ui/tests/globalSetup.ts @@ -74,4 +74,10 @@ module.exports = async function () { useI18n: jest.fn() } }) + + jest.mock('jsondiffpatch', () => { + return { + diff: jest.fn() + } + }) }