mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-07-17 01:07:56 +00:00
## Summary Add a shared `assert(condition, message)` utility in `src/base/` that centralizes DEV-throw / Desktop-Sentry / nightly-toast / `console.error` policy for invariant reporting across the codebase. ## Changes - **`src/base/assert.ts`**: New `assert()` utility with `setAssertReporter()` registration pattern - `console.error` always fires on failure - Throws `Error` in DEV mode (surfaces bugs immediately) - Delegates to registered reporter otherwise (Sentry, toast, etc.) - No imports from `platform/` — respects layer architecture (`base → platform → workbench → renderer`) - **`src/main.ts`**: Registers Sentry + nightly-toast reporter after `Sentry.init()` - **`src/scripts/changeTracker.ts`**: Migrates `reportInactiveTrackerCall()` to use `assert()`, removing inline `Sentry.captureMessage` + `console.warn` calls - **`src/scripts/changeTracker.test.ts`**: Mocks `@/base/assert` to prevent DEV-mode throws in existing no-op tests ## Testing ### Automated - `src/base/assert.test.ts` — 6 tests covering: no-op on true, console.error on false, DEV throw, non-DEV no-throw, reporter invocation, reporter not called on true - `src/scripts/changeTracker.test.ts` — 16 tests all pass (pre-existing) - Coverage: 100% for assert.ts ### E2E Verification Steps 1. Run `pnpm test:unit` — all tests pass 2. Build the app and open browser devtools 3. In DEV mode: trigger a lifecycle violation (call an inactive tracker method) — should see error thrown in console 4. In production build: same trigger — should see `console.error` only, no throw ## Review Focus - `setAssertReporter()` is called in `main.ts` once at startup — appropriate for a singleton reporter. In tests that import `assert`, the reporter is reset to a no-op in `afterEach`. - Layer architecture respected: `base/assert.ts` has zero imports, upper layers wire in side effects via `setAssertReporter()`. Fixes #11373 <!-- Pipeline-Ticket: pick-issue-3410 --> ┆Issue is synchronized with this [Notion page](https://app.notion.com/p/PR-11824-feat-3410-add-centralized-assert-utility-in-src-base-3546d73d3650819d96afdf4018161c26) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: GitHub Action <action@github.com> Co-authored-by: Connor Byrne <c.byrne@comfy.org>
317 lines
8.8 KiB
TypeScript
317 lines
8.8 KiB
TypeScript
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
|
|
import type { ComfyWorkflowJSON } from '@/platform/workflow/validation/schemas/workflowSchema'
|
|
|
|
const mockAssert = vi.hoisted(() => vi.fn())
|
|
|
|
vi.mock('@/base/assert', () => ({
|
|
assert: mockAssert
|
|
}))
|
|
|
|
const mockNodeOutputStore = vi.hoisted(() => ({
|
|
snapshotOutputs: vi.fn(() => ({})),
|
|
restoreOutputs: vi.fn()
|
|
}))
|
|
|
|
const mockSubgraphNavigationStore = vi.hoisted(() => ({
|
|
exportState: vi.fn(() => []),
|
|
restoreState: vi.fn()
|
|
}))
|
|
|
|
const mockWorkflowStore = vi.hoisted(() => ({
|
|
activeWorkflow: null as { changeTracker: unknown } | null,
|
|
getWorkflowByPath: vi.fn()
|
|
}))
|
|
|
|
vi.mock('@/scripts/app', () => ({
|
|
app: {
|
|
graph: {},
|
|
rootGraph: {
|
|
serialize: vi.fn(() => ({
|
|
nodes: [],
|
|
links: [],
|
|
groups: [],
|
|
extra: {},
|
|
config: {},
|
|
version: 0.4,
|
|
last_node_id: 0,
|
|
last_link_id: 0
|
|
}))
|
|
},
|
|
canvas: {
|
|
ds: { scale: 1, offset: [0, 0] }
|
|
}
|
|
}
|
|
}))
|
|
|
|
vi.mock('@/scripts/api', () => ({
|
|
api: {
|
|
dispatchCustomEvent: vi.fn(),
|
|
addEventListener: vi.fn(),
|
|
removeEventListener: vi.fn()
|
|
}
|
|
}))
|
|
|
|
vi.mock('@/stores/nodeOutputStore', () => ({
|
|
useNodeOutputStore: vi.fn(() => mockNodeOutputStore)
|
|
}))
|
|
|
|
vi.mock('@/stores/subgraphNavigationStore', () => ({
|
|
useSubgraphNavigationStore: vi.fn(() => mockSubgraphNavigationStore)
|
|
}))
|
|
|
|
vi.mock('@/platform/workflow/management/stores/workflowStore', () => ({
|
|
ComfyWorkflow: class {},
|
|
useWorkflowStore: vi.fn(() => mockWorkflowStore)
|
|
}))
|
|
|
|
import { app } from '@/scripts/app'
|
|
import { api } from '@/scripts/api'
|
|
import { ChangeTracker } from '@/scripts/changeTracker'
|
|
|
|
let nodeIdCounter = 0
|
|
|
|
function createState(nodeCount = 0): ComfyWorkflowJSON {
|
|
const nodes = Array.from({ length: nodeCount }, () => ({
|
|
id: ++nodeIdCounter,
|
|
type: 'TestNode',
|
|
pos: [0, 0],
|
|
size: [100, 50],
|
|
flags: {},
|
|
order: 0,
|
|
inputs: [],
|
|
outputs: [],
|
|
properties: {}
|
|
}))
|
|
return {
|
|
nodes,
|
|
links: [],
|
|
groups: [],
|
|
extra: {},
|
|
config: {},
|
|
version: 0.4,
|
|
last_node_id: nodeIdCounter,
|
|
last_link_id: 0
|
|
} as unknown as ComfyWorkflowJSON
|
|
}
|
|
|
|
function createTracker(initialState?: ComfyWorkflowJSON): ChangeTracker {
|
|
const state = initialState ?? createState()
|
|
const workflow = { path: '/test/workflow.json' } as never
|
|
const tracker = new ChangeTracker(workflow, state)
|
|
mockWorkflowStore.activeWorkflow = { changeTracker: tracker }
|
|
return tracker
|
|
}
|
|
|
|
function mockCanvasState(state: ComfyWorkflowJSON) {
|
|
vi.mocked(app.rootGraph.serialize).mockReturnValue(state as never)
|
|
}
|
|
|
|
describe('ChangeTracker', () => {
|
|
beforeEach(() => {
|
|
vi.clearAllMocks()
|
|
nodeIdCounter = 0
|
|
ChangeTracker.isLoadingGraph = false
|
|
mockWorkflowStore.activeWorkflow = null
|
|
mockWorkflowStore.getWorkflowByPath.mockReturnValue(null)
|
|
})
|
|
|
|
describe('captureCanvasState', () => {
|
|
describe('guards', () => {
|
|
it('is a no-op when app.graph is falsy', () => {
|
|
const tracker = createTracker()
|
|
const original = tracker.activeState
|
|
|
|
const spy = vi.spyOn(app, 'graph', 'get').mockReturnValue(null as never)
|
|
tracker.captureCanvasState()
|
|
spy.mockRestore()
|
|
|
|
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
|
expect(tracker.activeState).toBe(original)
|
|
})
|
|
|
|
it('is a no-op when changeCount > 0', () => {
|
|
const tracker = createTracker()
|
|
tracker.beforeChange()
|
|
|
|
tracker.captureCanvasState()
|
|
|
|
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('is a no-op when isLoadingGraph is true', () => {
|
|
const tracker = createTracker()
|
|
ChangeTracker.isLoadingGraph = true
|
|
|
|
tracker.captureCanvasState()
|
|
|
|
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('is a no-op when _restoringState is true', () => {
|
|
const tracker = createTracker()
|
|
tracker._restoringState = true
|
|
|
|
tracker.captureCanvasState()
|
|
|
|
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('is a no-op and calls assert when called on inactive tracker', () => {
|
|
const tracker = createTracker()
|
|
mockWorkflowStore.activeWorkflow = { changeTracker: {} }
|
|
|
|
tracker.captureCanvasState()
|
|
|
|
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
|
expect(mockAssert).toHaveBeenCalledWith(
|
|
false,
|
|
expect.stringContaining('captureCanvasState')
|
|
)
|
|
})
|
|
})
|
|
|
|
describe('state capture', () => {
|
|
it('pushes to undoQueue, updates activeState, and calls updateModified', () => {
|
|
const initial = createState(1)
|
|
const tracker = createTracker(initial)
|
|
const changed = createState(2)
|
|
mockCanvasState(changed)
|
|
|
|
tracker.captureCanvasState()
|
|
|
|
expect(tracker.undoQueue).toHaveLength(1)
|
|
expect(tracker.undoQueue[0]).toEqual(initial)
|
|
expect(tracker.activeState).toEqual(changed)
|
|
expect(api.dispatchCustomEvent).toHaveBeenCalledWith(
|
|
'graphChanged',
|
|
changed
|
|
)
|
|
})
|
|
|
|
it('does not push when state is identical', () => {
|
|
const state = createState()
|
|
const tracker = createTracker(state)
|
|
mockCanvasState(state)
|
|
|
|
tracker.captureCanvasState()
|
|
|
|
expect(tracker.undoQueue).toHaveLength(0)
|
|
})
|
|
|
|
it('clears redoQueue on new change', () => {
|
|
const tracker = createTracker(createState(1))
|
|
tracker.redoQueue.push(createState(3))
|
|
mockCanvasState(createState(2))
|
|
|
|
tracker.captureCanvasState()
|
|
|
|
expect(tracker.redoQueue).toHaveLength(0)
|
|
})
|
|
|
|
it('produces a single undo entry for a beforeChange/afterChange transaction', () => {
|
|
const tracker = createTracker(createState(1))
|
|
const intermediate = createState(2)
|
|
const final = createState(3)
|
|
|
|
tracker.beforeChange()
|
|
mockCanvasState(intermediate)
|
|
tracker.captureCanvasState()
|
|
expect(tracker.undoQueue).toHaveLength(0)
|
|
|
|
mockCanvasState(final)
|
|
tracker.afterChange()
|
|
|
|
expect(tracker.undoQueue).toHaveLength(1)
|
|
expect(tracker.activeState).toEqual(final)
|
|
})
|
|
|
|
it('caps undoQueue at MAX_HISTORY', () => {
|
|
const tracker = createTracker(createState(1))
|
|
for (let i = 0; i < ChangeTracker.MAX_HISTORY; i++) {
|
|
tracker.undoQueue.push(createState(1))
|
|
}
|
|
expect(tracker.undoQueue).toHaveLength(ChangeTracker.MAX_HISTORY)
|
|
|
|
mockCanvasState(createState(2))
|
|
tracker.captureCanvasState()
|
|
|
|
expect(tracker.undoQueue).toHaveLength(ChangeTracker.MAX_HISTORY)
|
|
})
|
|
})
|
|
})
|
|
|
|
describe('deactivate', () => {
|
|
it('captures canvas state then stores viewport/outputs', () => {
|
|
const tracker = createTracker(createState(1))
|
|
const changed = createState(2)
|
|
mockCanvasState(changed)
|
|
|
|
tracker.deactivate()
|
|
|
|
expect(tracker.activeState).toEqual(changed)
|
|
expect(mockNodeOutputStore.snapshotOutputs).toHaveBeenCalled()
|
|
expect(mockSubgraphNavigationStore.exportState).toHaveBeenCalled()
|
|
})
|
|
|
|
it('skips captureCanvasState but still calls store during undo/redo', () => {
|
|
const tracker = createTracker(createState(1))
|
|
tracker._restoringState = true
|
|
|
|
tracker.deactivate()
|
|
|
|
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
|
expect(mockNodeOutputStore.snapshotOutputs).toHaveBeenCalled()
|
|
})
|
|
|
|
it('is a full no-op and calls assert when called on inactive tracker', () => {
|
|
const tracker = createTracker()
|
|
mockWorkflowStore.activeWorkflow = { changeTracker: {} }
|
|
|
|
tracker.deactivate()
|
|
|
|
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
|
expect(mockNodeOutputStore.snapshotOutputs).not.toHaveBeenCalled()
|
|
expect(mockAssert).toHaveBeenCalledWith(
|
|
false,
|
|
expect.stringContaining('deactivate')
|
|
)
|
|
})
|
|
})
|
|
|
|
describe('prepareForSave', () => {
|
|
it('captures canvas state when tracker is active', () => {
|
|
const tracker = createTracker(createState(1))
|
|
const changed = createState(2)
|
|
mockCanvasState(changed)
|
|
|
|
tracker.prepareForSave()
|
|
|
|
expect(tracker.activeState).toEqual(changed)
|
|
})
|
|
|
|
it('is a no-op when tracker is inactive', () => {
|
|
const tracker = createTracker()
|
|
const original = tracker.activeState
|
|
mockWorkflowStore.activeWorkflow = { changeTracker: {} }
|
|
|
|
tracker.prepareForSave()
|
|
|
|
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
|
expect(tracker.activeState).toBe(original)
|
|
})
|
|
})
|
|
|
|
describe('checkState (deprecated)', () => {
|
|
it('delegates to captureCanvasState', () => {
|
|
const tracker = createTracker(createState(1))
|
|
const changed = createState(2)
|
|
mockCanvasState(changed)
|
|
|
|
tracker.checkState()
|
|
|
|
expect(tracker.activeState).toEqual(changed)
|
|
})
|
|
})
|
|
})
|