mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-20 20:39:30 +00:00
## Summary Harden the `ChangeTracker` lifecycle to eliminate the class of bugs where an inactive workflow's tracker silently captures the wrong graph state. Renames `checkState()` to `captureCanvasState()` with a self-defending assertion, introduces `deactivate()` and `prepareForSave()` lifecycle methods, and closes a latent undo-history corruption bug discovered during code review. ## Background ComfyUI supports multiple workflows open as tabs, but only one canvas (`app.rootGraph`) exists at a time. When the user switches tabs, the old workflow's graph is unloaded and the new one is loaded into this shared canvas. The old `checkState()` method serialized `app.rootGraph` into `activeState` to track changes for undo/redo. It had no awareness of *which* workflow it belonged to -- if called on an inactive tab's tracker, it would capture the active tab's graph data and silently overwrite the inactive workflow's state. This caused permanent data loss (fixed in PR #10745 with caller-side `isActive` guards). The caller-side guards were fragile: every new call site had to remember to add the guard, and forgetting would reintroduce the same silent data corruption. Additionally, `beforeLoadNewGraph` only called `store()` (viewport/outputs) without `checkState()`, meaning canvas state could be stale if a tab switch happened without a preceding mouseup event. ### Before (fragile) ``` saveWorkflow(workflow): if (isActive(workflow)) <-- caller must remember this guard workflow.changeTracker.checkState() <-- name implies "read", actually writes ... beforeLoadNewGraph(): activeWorkflow.changeTracker.store() <-- only saves viewport, NOT graph state ``` ### After (self-defending) ``` saveWorkflow(workflow): workflow.changeTracker.prepareForSave() <-- handles active/inactive internally ... beforeLoadNewGraph(): activeWorkflow.changeTracker.deactivate() <-- captures graph + viewport together ``` ## Changes - Rename `checkState` to `captureCanvasState` with active-tracker assertion - Add `deactivate()` and `prepareForSave()` lifecycle methods - Fix undo-history corruption: `captureCanvasState()` guarded by `_restoringState` - Fix viewport regression during undo: `deactivate()` skips `captureCanvasState()` during undo/redo but always calls `store()` to preserve viewport (regression from PR #10247) - Log inactive tracker warnings unconditionally at warn level (not DEV-only) - Deprecated `checkState()` wrapper for extension compatibility - Rename `checkState` to `captureCanvasState` in `useWidgetSelectActions` composable - Add `appModeStore.ts` to manual call sites documentation - Add `checkState()` deprecation note to architecture docs - Add 16 unit tests covering all guard conditions, lifecycle methods, and undo behavior - Add E2E test: "Undo preserves viewport offset" ## New ChangeTracker Public API | Method | Caller | Purpose | |--------|--------|---------| | `captureCanvasState()` | Event handlers, UI interactions | Snapshots canvas into activeState, pushes undo. Asserts active tracker. | | `deactivate()` | `beforeLoadNewGraph` only | `captureCanvasState()` (skipped during undo/redo) + `store()`. Freezes state for tab switch. | | `prepareForSave()` | Save paths only | Active: `captureCanvasState()`. Inactive: no-op. | | `checkState()` | **Deprecated** -- extensions only | Wrapper that delegates to `captureCanvasState()` with deprecation warning. | | `store()` | Internal to `deactivate()` | Saves viewport, outputs, subgraph navigation. | | `restore()` | `afterLoadNewGraph` | Restores viewport, outputs, subgraph navigation. | | `reset()` | `afterLoadNewGraph`, save | Resets initial state (marks as "clean"). | ## Test plan - [x] Unit tests: 16 tests covering all guard conditions, state capture, undo queue behavior - [x] E2E test: "Undo preserves viewport offset" verifies no viewport drift on undo - [x] E2E test: "Prevents captureCanvasState from corrupting workflow state during tab switch" - [x] Existing E2E: "Closing an inactive tab with save preserves its own content" - [ ] Manual: rapidly switch tabs during undo/redo, verify no viewport drift - [ ] Manual: verify extensions calling `checkState()` see deprecation warning in console
486 lines
14 KiB
TypeScript
486 lines
14 KiB
TypeScript
import { createPinia, setActivePinia } from 'pinia'
|
|
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
import { ref } from 'vue'
|
|
|
|
import { useCoreCommands } from '@/composables/useCoreCommands'
|
|
import type { LGraphNode } from '@/lib/litegraph/src/litegraph'
|
|
import { useSettingStore } from '@/platform/settings/settingStore'
|
|
import { api } from '@/scripts/api'
|
|
import { app } from '@/scripts/app'
|
|
import { createMockLGraphNode } from '@/utils/__tests__/litegraphTestUtils'
|
|
|
|
// Mock vue-i18n for useExternalLink
|
|
const mockLocale = ref('en')
|
|
vi.mock('vue-i18n', async () => {
|
|
const actual = await vi.importActual('vue-i18n')
|
|
return {
|
|
...actual,
|
|
useI18n: vi.fn(() => ({
|
|
locale: mockLocale
|
|
}))
|
|
}
|
|
})
|
|
|
|
vi.mock('@/scripts/app', () => {
|
|
const mockGraphClear = vi.fn()
|
|
const mockCanvas = {
|
|
subgraph: undefined,
|
|
selectedItems: new Set(),
|
|
copyToClipboard: vi.fn(),
|
|
pasteFromClipboard: vi.fn(),
|
|
selectItems: vi.fn()
|
|
}
|
|
|
|
return {
|
|
app: {
|
|
clean: vi.fn(() => {
|
|
// Simulate app.clean() calling graph.clear() only when not in subgraph
|
|
if (!mockCanvas.subgraph) {
|
|
mockGraphClear()
|
|
}
|
|
}),
|
|
canvas: mockCanvas,
|
|
rootGraph: {
|
|
clear: mockGraphClear
|
|
}
|
|
}
|
|
}
|
|
})
|
|
|
|
vi.mock('@/scripts/api', () => ({
|
|
api: {
|
|
dispatchCustomEvent: vi.fn(),
|
|
apiURL: vi.fn(() => 'http://localhost:8188')
|
|
}
|
|
}))
|
|
|
|
vi.mock('@/platform/settings/settingStore')
|
|
|
|
vi.mock('@/stores/authStore', () => ({
|
|
useAuthStore: vi.fn(() => ({}))
|
|
}))
|
|
|
|
vi.mock('@/composables/auth/useFirebaseAuth', () => ({
|
|
useFirebaseAuth: vi.fn(() => null)
|
|
}))
|
|
|
|
vi.mock('firebase/auth', () => ({
|
|
setPersistence: vi.fn(),
|
|
browserLocalPersistence: {},
|
|
onAuthStateChanged: vi.fn()
|
|
}))
|
|
|
|
vi.mock('@/platform/workflow/core/services/workflowService', () => ({
|
|
useWorkflowService: vi.fn(() => ({}))
|
|
}))
|
|
|
|
const mockDialogService = vi.hoisted(() => ({
|
|
prompt: vi.fn()
|
|
}))
|
|
vi.mock('@/services/dialogService', () => ({
|
|
useDialogService: vi.fn(() => mockDialogService)
|
|
}))
|
|
|
|
vi.mock('@/services/litegraphService', () => ({
|
|
useLitegraphService: vi.fn(() => ({}))
|
|
}))
|
|
|
|
vi.mock('@/stores/executionStore', () => ({
|
|
useExecutionStore: vi.fn(() => ({}))
|
|
}))
|
|
|
|
vi.mock('@/stores/toastStore', () => ({
|
|
useToastStore: vi.fn(() => ({}))
|
|
}))
|
|
|
|
const mockChangeTracker = vi.hoisted(() => ({
|
|
captureCanvasState: vi.fn()
|
|
}))
|
|
const mockWorkflowStore = vi.hoisted(() => ({
|
|
activeWorkflow: {
|
|
changeTracker: mockChangeTracker
|
|
}
|
|
}))
|
|
vi.mock('@/platform/workflow/management/stores/workflowStore', () => ({
|
|
useWorkflowStore: vi.fn(() => mockWorkflowStore)
|
|
}))
|
|
|
|
vi.mock('@/stores/subgraphStore', () => ({
|
|
useSubgraphStore: vi.fn(() => ({}))
|
|
}))
|
|
|
|
vi.mock('@/renderer/core/canvas/canvasStore', () => ({
|
|
useCanvasStore: vi.fn(() => ({
|
|
getCanvas: () => app.canvas,
|
|
canvas: app.canvas
|
|
})),
|
|
useTitleEditorStore: vi.fn(() => ({
|
|
titleEditorTarget: null
|
|
}))
|
|
}))
|
|
|
|
vi.mock('@/stores/workspace/colorPaletteStore', () => ({
|
|
useColorPaletteStore: vi.fn(() => ({}))
|
|
}))
|
|
|
|
vi.mock('@/composables/auth/useAuthActions', () => ({
|
|
useAuthActions: vi.fn(() => ({}))
|
|
}))
|
|
|
|
vi.mock('@/platform/cloud/subscription/composables/useSubscription', () => ({
|
|
useSubscription: vi.fn(() => ({
|
|
isActiveSubscription: vi.fn().mockReturnValue(true),
|
|
showSubscriptionDialog: vi.fn()
|
|
}))
|
|
}))
|
|
|
|
vi.mock('@/composables/billing/useBillingContext', () => ({
|
|
useBillingContext: vi.fn(() => ({
|
|
isActiveSubscription: { value: true },
|
|
showSubscriptionDialog: vi.fn()
|
|
}))
|
|
}))
|
|
|
|
describe('useCoreCommands', () => {
|
|
const createMockNode = (id: number, comfyClass: string): LGraphNode => {
|
|
const baseNode = createMockLGraphNode({ id })
|
|
return Object.assign(baseNode, {
|
|
constructor: {
|
|
...baseNode.constructor,
|
|
comfyClass
|
|
}
|
|
})
|
|
}
|
|
|
|
const createMockSubgraph = () => {
|
|
const mockNodes = [
|
|
// Mock input node
|
|
createMockNode(1, 'SubgraphInputNode'),
|
|
// Mock output node
|
|
createMockNode(2, 'SubgraphOutputNode'),
|
|
// Mock user node
|
|
createMockNode(3, 'SomeUserNode'),
|
|
// Another mock user node
|
|
createMockNode(4, 'AnotherUserNode')
|
|
]
|
|
|
|
return {
|
|
nodes: mockNodes,
|
|
remove: vi.fn(),
|
|
events: {
|
|
dispatch: vi.fn(),
|
|
addEventListener: vi.fn(),
|
|
removeEventListener: vi.fn(),
|
|
dispatchEvent: vi.fn()
|
|
},
|
|
name: 'test-subgraph',
|
|
inputNode: undefined,
|
|
outputNode: undefined,
|
|
add: vi.fn(),
|
|
clear: vi.fn(),
|
|
serialize: vi.fn(),
|
|
configure: vi.fn(),
|
|
start: vi.fn(),
|
|
stop: vi.fn(),
|
|
runStep: vi.fn(),
|
|
findNodeByTitle: vi.fn(),
|
|
findNodesByTitle: vi.fn(),
|
|
findNodesByType: vi.fn(),
|
|
findNodeById: vi.fn(),
|
|
getNodeById: vi.fn(),
|
|
setDirtyCanvas: vi.fn(),
|
|
sendActionToCanvas: vi.fn(),
|
|
extra: {} as Record<string, unknown>
|
|
} as Partial<typeof app.canvas.subgraph> as typeof app.canvas.subgraph
|
|
}
|
|
|
|
const mockSubgraph = createMockSubgraph()!
|
|
|
|
function createMockSettingStore(
|
|
getReturnValue: boolean
|
|
): ReturnType<typeof useSettingStore> {
|
|
return {
|
|
get: vi.fn().mockReturnValue(getReturnValue),
|
|
addSetting: vi.fn(),
|
|
load: vi.fn(),
|
|
set: vi.fn(),
|
|
setMany: vi.fn(),
|
|
exists: vi.fn(),
|
|
getDefaultValue: vi.fn(),
|
|
isReady: true,
|
|
isLoading: false,
|
|
error: undefined,
|
|
settingValues: {},
|
|
settingsById: {},
|
|
$id: 'setting',
|
|
$state: {
|
|
settingValues: {},
|
|
settingsById: {},
|
|
isReady: true,
|
|
isLoading: false,
|
|
error: undefined
|
|
},
|
|
$patch: vi.fn(),
|
|
$reset: vi.fn(),
|
|
$subscribe: vi.fn(),
|
|
$onAction: vi.fn(),
|
|
$dispose: vi.fn(),
|
|
_customProperties: new Set()
|
|
} satisfies ReturnType<typeof useSettingStore>
|
|
}
|
|
|
|
beforeEach(() => {
|
|
vi.clearAllMocks()
|
|
|
|
// Set up Pinia
|
|
setActivePinia(createPinia())
|
|
|
|
// Reset app state
|
|
app.canvas.subgraph = undefined
|
|
|
|
// Mock settings store
|
|
vi.mocked(useSettingStore).mockReturnValue(createMockSettingStore(false))
|
|
|
|
// Mock global confirm
|
|
global.confirm = vi.fn().mockReturnValue(true)
|
|
})
|
|
|
|
describe('ClearWorkflow command', () => {
|
|
it('should clear main graph when not in subgraph', async () => {
|
|
const commands = useCoreCommands()
|
|
const clearCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.ClearWorkflow'
|
|
)!
|
|
|
|
// Execute the command
|
|
await clearCommand.function()
|
|
|
|
expect(app.clean).toHaveBeenCalled()
|
|
expect(app.rootGraph.clear).toHaveBeenCalled()
|
|
expect(api.dispatchCustomEvent).toHaveBeenCalledWith('graphCleared')
|
|
})
|
|
|
|
it('should preserve input/output nodes when clearing subgraph', async () => {
|
|
// Set up subgraph context
|
|
app.canvas.subgraph = mockSubgraph
|
|
|
|
const commands = useCoreCommands()
|
|
const clearCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.ClearWorkflow'
|
|
)!
|
|
|
|
// Execute the command
|
|
await clearCommand.function()
|
|
|
|
expect(app.clean).toHaveBeenCalled()
|
|
expect(app.rootGraph.clear).not.toHaveBeenCalled()
|
|
|
|
// Should only remove user nodes, not input/output nodes
|
|
const subgraph = app.canvas.subgraph!
|
|
expect(subgraph.remove).toHaveBeenCalledTimes(2)
|
|
expect(subgraph.remove).toHaveBeenCalledWith(subgraph.nodes[2]) // user1
|
|
expect(subgraph.remove).toHaveBeenCalledWith(subgraph.nodes[3]) // user2
|
|
expect(subgraph.remove).not.toHaveBeenCalledWith(subgraph.nodes[0]) // input1
|
|
expect(subgraph.remove).not.toHaveBeenCalledWith(subgraph.nodes[1]) // output1
|
|
|
|
expect(api.dispatchCustomEvent).toHaveBeenCalledWith('graphCleared')
|
|
})
|
|
|
|
it('should respect confirmation setting', async () => {
|
|
// Mock confirmation required
|
|
vi.mocked(useSettingStore).mockReturnValue(createMockSettingStore(true))
|
|
|
|
global.confirm = vi.fn().mockReturnValue(false) // User cancels
|
|
|
|
const commands = useCoreCommands()
|
|
const clearCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.ClearWorkflow'
|
|
)!
|
|
|
|
// Execute the command
|
|
await clearCommand.function()
|
|
|
|
// Should not clear anything when user cancels
|
|
expect(app.clean).not.toHaveBeenCalled()
|
|
expect(app.rootGraph.clear).not.toHaveBeenCalled()
|
|
expect(api.dispatchCustomEvent).not.toHaveBeenCalled()
|
|
})
|
|
})
|
|
|
|
describe('Canvas clipboard commands', () => {
|
|
function findCommand(id: string) {
|
|
return useCoreCommands().find((cmd) => cmd.id === id)!
|
|
}
|
|
|
|
beforeEach(() => {
|
|
app.canvas.selectedItems = new Set()
|
|
vi.mocked(app.canvas.copyToClipboard).mockClear()
|
|
vi.mocked(app.canvas.pasteFromClipboard).mockClear()
|
|
vi.mocked(app.canvas.selectItems).mockClear()
|
|
})
|
|
|
|
it('should copy selected items when selection exists', async () => {
|
|
app.canvas.selectedItems = new Set([
|
|
{}
|
|
]) as typeof app.canvas.selectedItems
|
|
|
|
await findCommand('Comfy.Canvas.CopySelected').function()
|
|
|
|
expect(app.canvas.copyToClipboard).toHaveBeenCalledWith()
|
|
})
|
|
|
|
it('should not copy when no items are selected', async () => {
|
|
await findCommand('Comfy.Canvas.CopySelected').function()
|
|
|
|
expect(app.canvas.copyToClipboard).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('should paste from clipboard', async () => {
|
|
await findCommand('Comfy.Canvas.PasteFromClipboard').function()
|
|
|
|
expect(app.canvas.pasteFromClipboard).toHaveBeenCalledWith()
|
|
})
|
|
|
|
it('should select all items', async () => {
|
|
await findCommand('Comfy.Canvas.SelectAll').function()
|
|
|
|
// No arguments means "select all items on canvas"
|
|
expect(app.canvas.selectItems).toHaveBeenCalledWith()
|
|
})
|
|
})
|
|
|
|
describe('Subgraph metadata commands', () => {
|
|
beforeEach(() => {
|
|
mockSubgraph.extra = {}
|
|
vi.clearAllMocks()
|
|
})
|
|
|
|
describe('SetDescription command', () => {
|
|
it('should do nothing when not in subgraph', async () => {
|
|
app.canvas.subgraph = undefined
|
|
|
|
const commands = useCoreCommands()
|
|
const setDescCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.Subgraph.SetDescription'
|
|
)!
|
|
|
|
await setDescCommand.function()
|
|
|
|
expect(mockDialogService.prompt).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('should set description on subgraph.extra', async () => {
|
|
app.canvas.subgraph = mockSubgraph
|
|
mockDialogService.prompt.mockResolvedValue('Test description')
|
|
|
|
const commands = useCoreCommands()
|
|
const setDescCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.Subgraph.SetDescription'
|
|
)!
|
|
|
|
await setDescCommand.function()
|
|
|
|
expect(mockDialogService.prompt).toHaveBeenCalled()
|
|
expect(mockSubgraph.extra.BlueprintDescription).toBe('Test description')
|
|
expect(mockChangeTracker.captureCanvasState).toHaveBeenCalled()
|
|
})
|
|
|
|
it('should not set description when user cancels', async () => {
|
|
app.canvas.subgraph = mockSubgraph
|
|
mockDialogService.prompt.mockResolvedValue(null)
|
|
|
|
const commands = useCoreCommands()
|
|
const setDescCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.Subgraph.SetDescription'
|
|
)!
|
|
|
|
await setDescCommand.function()
|
|
|
|
expect(mockSubgraph.extra.BlueprintDescription).toBeUndefined()
|
|
expect(mockChangeTracker.captureCanvasState).not.toHaveBeenCalled()
|
|
})
|
|
})
|
|
|
|
describe('SetSearchAliases command', () => {
|
|
it('should do nothing when not in subgraph', async () => {
|
|
app.canvas.subgraph = undefined
|
|
|
|
const commands = useCoreCommands()
|
|
const setAliasesCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.Subgraph.SetSearchAliases'
|
|
)!
|
|
|
|
await setAliasesCommand.function()
|
|
|
|
expect(mockDialogService.prompt).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('should set search aliases on subgraph.extra', async () => {
|
|
app.canvas.subgraph = mockSubgraph
|
|
mockDialogService.prompt.mockResolvedValue('alias1, alias2, alias3')
|
|
|
|
const commands = useCoreCommands()
|
|
const setAliasesCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.Subgraph.SetSearchAliases'
|
|
)!
|
|
|
|
await setAliasesCommand.function()
|
|
|
|
expect(mockDialogService.prompt).toHaveBeenCalled()
|
|
expect(mockSubgraph.extra.BlueprintSearchAliases).toEqual([
|
|
'alias1',
|
|
'alias2',
|
|
'alias3'
|
|
])
|
|
expect(mockChangeTracker.captureCanvasState).toHaveBeenCalled()
|
|
})
|
|
|
|
it('should trim whitespace and filter empty strings', async () => {
|
|
app.canvas.subgraph = mockSubgraph
|
|
mockDialogService.prompt.mockResolvedValue(' alias1 , , alias2 , ')
|
|
|
|
const commands = useCoreCommands()
|
|
const setAliasesCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.Subgraph.SetSearchAliases'
|
|
)!
|
|
|
|
await setAliasesCommand.function()
|
|
|
|
expect(mockSubgraph.extra.BlueprintSearchAliases).toEqual([
|
|
'alias1',
|
|
'alias2'
|
|
])
|
|
})
|
|
|
|
it('should set undefined when empty input', async () => {
|
|
app.canvas.subgraph = mockSubgraph
|
|
mockDialogService.prompt.mockResolvedValue('')
|
|
|
|
const commands = useCoreCommands()
|
|
const setAliasesCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.Subgraph.SetSearchAliases'
|
|
)!
|
|
|
|
await setAliasesCommand.function()
|
|
|
|
expect(mockSubgraph.extra.BlueprintSearchAliases).toBeUndefined()
|
|
})
|
|
|
|
it('should not set aliases when user cancels', async () => {
|
|
app.canvas.subgraph = mockSubgraph
|
|
mockDialogService.prompt.mockResolvedValue(null)
|
|
|
|
const commands = useCoreCommands()
|
|
const setAliasesCommand = commands.find(
|
|
(cmd) => cmd.id === 'Comfy.Subgraph.SetSearchAliases'
|
|
)!
|
|
|
|
await setAliasesCommand.function()
|
|
|
|
expect(mockSubgraph.extra.BlueprintSearchAliases).toBeUndefined()
|
|
expect(mockChangeTracker.captureCanvasState).not.toHaveBeenCalled()
|
|
})
|
|
})
|
|
})
|
|
})
|