mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-24 22:58:08 +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>
This commit is contained in:
91
src/base/assert.test.ts
Normal file
91
src/base/assert.test.ts
Normal file
@@ -0,0 +1,91 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { assert, setAssertReporter } from '@/base/assert'
|
||||
|
||||
describe('assert', () => {
|
||||
let consoleErrorSpy: ReturnType<typeof vi.spyOn>
|
||||
|
||||
beforeEach(() => {
|
||||
consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
vi.unstubAllEnvs()
|
||||
setAssertReporter(null)
|
||||
})
|
||||
|
||||
it('does nothing when condition is true', () => {
|
||||
expect(() => assert(true, 'should not throw')).not.toThrow()
|
||||
expect(consoleErrorSpy).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('logs console.error when condition is false', () => {
|
||||
vi.stubEnv('DEV', false)
|
||||
assert(false, 'test message')
|
||||
expect(consoleErrorSpy).toHaveBeenCalledWith(
|
||||
'[Assertion failed]: test message'
|
||||
)
|
||||
})
|
||||
|
||||
it('throws in DEV mode when condition is false', () => {
|
||||
vi.stubEnv('DEV', true)
|
||||
const reporter = vi.fn()
|
||||
setAssertReporter(reporter)
|
||||
expect(() => assert(false, 'dev error')).toThrow(
|
||||
'[Assertion failed]: dev error'
|
||||
)
|
||||
expect(consoleErrorSpy).toHaveBeenCalledWith(
|
||||
'[Assertion failed]: dev error'
|
||||
)
|
||||
expect(reporter).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not throw in non-DEV mode when condition is false', () => {
|
||||
vi.stubEnv('DEV', false)
|
||||
expect(() => assert(false, 'non-dev error')).not.toThrow()
|
||||
})
|
||||
|
||||
it('calls registered reporter in non-DEV mode with formatted message', () => {
|
||||
vi.stubEnv('DEV', false)
|
||||
const reporter = vi.fn()
|
||||
setAssertReporter(reporter)
|
||||
assert(false, 'reporter message')
|
||||
expect(reporter).toHaveBeenCalledWith(
|
||||
'[Assertion failed]: reporter message'
|
||||
)
|
||||
})
|
||||
|
||||
it('does not call reporter when condition is true', () => {
|
||||
vi.stubEnv('DEV', false)
|
||||
const reporter = vi.fn()
|
||||
setAssertReporter(reporter)
|
||||
assert(true, 'no call')
|
||||
expect(reporter).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('handles null reporter gracefully in non-DEV mode', () => {
|
||||
vi.stubEnv('DEV', false)
|
||||
setAssertReporter(null)
|
||||
expect(() => assert(false, 'null reporter')).not.toThrow()
|
||||
expect(consoleErrorSpy).toHaveBeenCalledWith(
|
||||
'[Assertion failed]: null reporter'
|
||||
)
|
||||
})
|
||||
|
||||
it('swallows reporter exceptions in non-DEV mode', () => {
|
||||
vi.stubEnv('DEV', false)
|
||||
const reporter = vi.fn(() => {
|
||||
throw new Error('reporter blew up')
|
||||
})
|
||||
setAssertReporter(reporter)
|
||||
expect(() => assert(false, 'safe under reporter failure')).not.toThrow()
|
||||
expect(consoleErrorSpy).toHaveBeenCalledWith(
|
||||
'[Assertion failed]: safe under reporter failure'
|
||||
)
|
||||
expect(consoleErrorSpy).toHaveBeenCalledWith(
|
||||
'[Assertion reporter failed]',
|
||||
expect.any(Error)
|
||||
)
|
||||
})
|
||||
})
|
||||
36
src/base/assert.ts
Normal file
36
src/base/assert.ts
Normal file
@@ -0,0 +1,36 @@
|
||||
type AssertReporter = (message: string) => void
|
||||
|
||||
let reporter: AssertReporter | null = null
|
||||
|
||||
/**
|
||||
* Register a reporter for assertion failures in non-DEV environments.
|
||||
* Called once at app startup by platform/ or higher layers to wire in
|
||||
* Sentry, toast notifications, etc.
|
||||
*/
|
||||
export function setAssertReporter(fn: AssertReporter | null): void {
|
||||
reporter = fn
|
||||
}
|
||||
|
||||
/**
|
||||
* Centralized invariant assertion.
|
||||
*
|
||||
* - Always: console.error
|
||||
* - DEV: throws (surfaces bugs immediately)
|
||||
* - Otherwise: delegates to registered reporter (Sentry, toast, etc.)
|
||||
*/
|
||||
export function assert(condition: unknown, message: string): asserts condition {
|
||||
if (condition) return
|
||||
|
||||
const formatted = `[Assertion failed]: ${message}`
|
||||
console.error(formatted)
|
||||
|
||||
if (import.meta.env.DEV) {
|
||||
throw new Error(formatted)
|
||||
}
|
||||
|
||||
try {
|
||||
reporter?.(formatted)
|
||||
} catch (error) {
|
||||
console.error('[Assertion reporter failed]', error)
|
||||
}
|
||||
}
|
||||
19
src/main.ts
19
src/main.ts
@@ -11,6 +11,7 @@ import Tooltip from 'primevue/tooltip'
|
||||
import { createApp } from 'vue'
|
||||
import { VueFire, VueFireAuth } from 'vuefire'
|
||||
|
||||
import { setAssertReporter } from '@/base/assert'
|
||||
import { getFirebaseConfig } from '@/config/firebase'
|
||||
import {
|
||||
configValueOrDefault,
|
||||
@@ -18,6 +19,8 @@ import {
|
||||
} from '@/platform/remoteConfig/remoteConfig'
|
||||
import '@/lib/litegraph/public/css/litegraph.css'
|
||||
import router from '@/router'
|
||||
import { isDesktop, isNightly } from '@/platform/distribution/types'
|
||||
import { useToastStore } from '@/platform/updates/common/toastStore'
|
||||
import { useBootstrapStore } from '@/stores/bootstrapStore'
|
||||
|
||||
import App from './App.vue'
|
||||
@@ -81,6 +84,22 @@ Sentry.init({
|
||||
defaultIntegrations: false
|
||||
})
|
||||
})
|
||||
// Assertion reporter receives pre-formatted messages (with "[Assertion failed]: " prefix).
|
||||
// Strings here are intentionally not i18n'd: they're developer/nightly diagnostics,
|
||||
// not user-facing in stable releases.
|
||||
setAssertReporter((message) => {
|
||||
if (isDesktop) {
|
||||
Sentry.captureMessage(message, { level: 'warning' })
|
||||
}
|
||||
if (isNightly) {
|
||||
useToastStore(pinia).add({
|
||||
severity: 'warn',
|
||||
summary: 'Assertion failed',
|
||||
detail: message
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
app.directive('tooltip', Tooltip)
|
||||
app
|
||||
.use(router)
|
||||
|
||||
@@ -2,6 +2,12 @@ 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()
|
||||
@@ -151,13 +157,17 @@ describe('ChangeTracker', () => {
|
||||
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('is a no-op and logs error when called on inactive tracker', () => {
|
||||
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')
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -254,7 +264,7 @@ describe('ChangeTracker', () => {
|
||||
expect(mockNodeOutputStore.snapshotOutputs).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('is a full no-op when called on inactive tracker', () => {
|
||||
it('is a full no-op and calls assert when called on inactive tracker', () => {
|
||||
const tracker = createTracker()
|
||||
mockWorkflowStore.activeWorkflow = { changeTracker: {} }
|
||||
|
||||
@@ -262,6 +272,10 @@ describe('ChangeTracker', () => {
|
||||
|
||||
expect(app.rootGraph.serialize).not.toHaveBeenCalled()
|
||||
expect(mockNodeOutputStore.snapshotOutputs).not.toHaveBeenCalled()
|
||||
expect(mockAssert).toHaveBeenCalledWith(
|
||||
false,
|
||||
expect.stringContaining('deactivate')
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -1,9 +1,8 @@
|
||||
import * as Sentry from '@sentry/vue'
|
||||
import _ from 'es-toolkit/compat'
|
||||
|
||||
import { assert } from '@/base/assert'
|
||||
import type { CanvasPointerEvent } from '@/lib/litegraph/src/litegraph'
|
||||
import { LGraphCanvas, LiteGraph } from '@/lib/litegraph/src/litegraph'
|
||||
import { isDesktop } from '@/platform/distribution/types'
|
||||
import type { ComfyWorkflow } from '@/platform/workflow/management/stores/workflowStore'
|
||||
import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore'
|
||||
import type { ComfyWorkflowJSON } from '@/platform/workflow/validation/schemas/workflowSchema'
|
||||
@@ -24,23 +23,20 @@ function isActiveTracker(tracker: ChangeTracker): boolean {
|
||||
return useWorkflowStore().activeWorkflow?.changeTracker === tracker
|
||||
}
|
||||
|
||||
const reportedInactiveCalls = new Set<string>()
|
||||
|
||||
/**
|
||||
* Report a ChangeTracker method being called on an inactive tracker —
|
||||
* a lifecycle violation that usually indicates stale extension state or
|
||||
* an incorrect call ordering.
|
||||
* Report a ChangeTracker method being called on an inactive tracker.
|
||||
* Deduplicates per method+workflow per session to avoid signal noise on hot paths.
|
||||
*/
|
||||
function reportInactiveTrackerCall(method: string, workflowPath: string) {
|
||||
console.warn(`${method}() called on inactive tracker for: ${workflowPath}`)
|
||||
|
||||
if (isDesktop) {
|
||||
Sentry.captureMessage(
|
||||
`ChangeTracker.${method}() called on inactive tracker`,
|
||||
{
|
||||
level: 'warning',
|
||||
tags: { workflow: workflowPath }
|
||||
}
|
||||
)
|
||||
}
|
||||
const key = `${method}:${workflowPath}`
|
||||
if (reportedInactiveCalls.has(key)) return
|
||||
reportedInactiveCalls.add(key)
|
||||
assert(
|
||||
false,
|
||||
`ChangeTracker.${method}() called on inactive tracker for: ${workflowPath}`
|
||||
)
|
||||
}
|
||||
|
||||
export class ChangeTracker {
|
||||
|
||||
Reference in New Issue
Block a user