From d96be3d6685cb0c3789ffdfc7542b0ac7d4fd9bc Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Wed, 13 May 2026 19:42:16 -0700 Subject: [PATCH] feat(#3410): add centralized assert() utility in src/base/ (#11824) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 ┆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 Co-authored-by: GitHub Action Co-authored-by: Connor Byrne --- src/base/assert.test.ts | 91 +++++++++++++++++++++++++++++++ src/base/assert.ts | 36 ++++++++++++ src/main.ts | 19 +++++++ src/scripts/changeTracker.test.ts | 18 +++++- src/scripts/changeTracker.ts | 28 ++++------ 5 files changed, 174 insertions(+), 18 deletions(-) create mode 100644 src/base/assert.test.ts create mode 100644 src/base/assert.ts diff --git a/src/base/assert.test.ts b/src/base/assert.test.ts new file mode 100644 index 0000000000..1448198385 --- /dev/null +++ b/src/base/assert.test.ts @@ -0,0 +1,91 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { assert, setAssertReporter } from '@/base/assert' + +describe('assert', () => { + let consoleErrorSpy: ReturnType + + 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) + ) + }) +}) diff --git a/src/base/assert.ts b/src/base/assert.ts new file mode 100644 index 0000000000..829285ffde --- /dev/null +++ b/src/base/assert.ts @@ -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) + } +} diff --git a/src/main.ts b/src/main.ts index a32242468c..7898779cf5 100644 --- a/src/main.ts +++ b/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) diff --git a/src/scripts/changeTracker.test.ts b/src/scripts/changeTracker.test.ts index 5513d83915..53724fb52d 100644 --- a/src/scripts/changeTracker.test.ts +++ b/src/scripts/changeTracker.test.ts @@ -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') + ) }) }) diff --git a/src/scripts/changeTracker.ts b/src/scripts/changeTracker.ts index da7d861405..54312ea239 100644 --- a/src/scripts/changeTracker.ts +++ b/src/scripts/changeTracker.ts @@ -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() + /** - * 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 {