Skip to content

fix(errors): keep missing node packs across prompt submissions #14900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
christian-byrne merged 3 commits into from
Aug 16, 2026
Merged

fix(errors): keep missing node packs across prompt submissions #14900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3f5dca
fix(errors): keep missing node packs across prompt submissions
jaeone94 Aug 7, 2026
42d51fc
fix(errors): address review on the A1111 clear and its tests
jaeone94 Aug 9, 2026
1d878c2
docs(errors): state that graph discard also invalidates missing-resou…
jaeone94 Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions browser_tests/tests/propertiesPanel/errorsTabMissingNodes.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,32 @@ test.describe('Errors tab - Missing nodes', { tag: ['@ui', '@canvas'] }, () => {
).toHaveText(/\S/)
})

test('Should keep the missing node pack card after submitting a prompt', async ({
comfyPage
}) => {
test.info().annotations.push({
type: 'regression',
description:
'Submitting a prompt cleared missing-node state, emptying the Errors tab'
})

await loadWorkflowAndOpenErrorsTab(comfyPage, 'missing/missing_nodes')

const missingNodeCard = comfyPage.page.getByTestId(
TestIds.dialogs.missingNodeCard
)
await expect(missingNodeCard).toBeVisible()

const prompted = comfyPage.page.waitForResponse((response) =>
response.url().includes('/api/prompt')
)
await comfyPage.runButton.click()
await prompted

await expect(missingNodeCard).toBeVisible()
await expect(missingNodeCard.getByText('Unknown pack')).toBeVisible()
})

test('Should show unknown pack node rows by default', async ({
comfyPage
}) => {
Expand Down
95 changes: 95 additions & 0 deletions src/scripts/app.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,34 @@ describe('ComfyApp', () => {
vi.spyOn(api, 'dispatchCustomEvent').mockImplementation(() => true)
}

it('preserves missing node packs when submitting a prompt', async () => {
prepareEmptyPromptQueue()
const missingNodesStore = useMissingNodesErrorStore()
const executionErrorStore = useExecutionErrorStore()
missingNodesStore.setMissingNodeTypes(['test/UninstalledLiveNode'])
executionErrorStore.recordExecutionError({
prompt_id: 'previous-run',
timestamp: 0,
node_id: '1',
node_type: 'Test',
executed: [],
exception_message: 'fail',
exception_type: 'RuntimeError',
traceback: []
})
vi.spyOn(api, 'queuePrompt').mockResolvedValue({
prompt_id: 'job-1',
error: ''
})

await app.queuePrompt(0)

expect(missingNodesStore.missingNodesError?.nodeTypes).toEqual([
'test/UninstalledLiveNode'
])
expect(executionErrorStore.lastExecutionError).toBeNull()
})

it('shows the error overlay for successful prompt responses with node errors', async () => {
const graph = new LGraph()
const workflow = new ComfyWorkflow({
Expand Down Expand Up @@ -1032,6 +1060,73 @@ describe('ComfyApp', () => {
])
})
})
describe('A1111 import', () => {

@christian-byrne christian-byrne Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: (non-blocking) The A1111 tests cover the success path (callback fires, setMissingNodeTypes([]) runs) and several failure modes. One gap: no test asserts that missing-node state is NOT cleared when importA1111 returns 'not-a1111' or 'core-nodes-unavailable' (without calling beforeGraphClear). The PR description says "a failed import keeps the current state" -- that is correct by inspection, but consider adding a test case:

@jaeone94 jaeone94 Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, for both outcomes:

it.for(['not-a1111', 'core-nodes-unavailable'] as const)(
  'keeps missing node packs when the import fails with %s',
  async (outcome) => {
    missingNodesStore.setMissingNodeTypes(['OutgoingMissingNode'])
    mockImportA1111.mockResolvedValue(outcome)

    await app.handleFile(createTestFile('a1111.png', 'image/png'))

    expect(missingNodesStore.missingNodesError?.nodeTypes).toEqual([
      'OutgoingMissingNode'
    ])
  }
)

Worth doing because the PR description asserted it and nothing pinned it. Mutation check: hoisting the clear out of the callback so it runs unconditionally fails both cases.

it('clears missing node packs, which its graph swap skips clean() for', async () => {
const graph = new LGraph()
Reflect.set(app, 'rootGraphInternal', graph)
const missingNodesStore = useMissingNodesErrorStore()
missingNodesStore.setMissingNodeTypes(['OutgoingMissingNode'])
vi.mocked(getWorkflowDataFromFile).mockResolvedValue({
parameters: 'positive\nNegative prompt: negative\nSteps: 20'
})
mockImportA1111.mockImplementation(
async (_graph, _parameters, beforeGraphClear) => {
beforeGraphClear?.()
return 'imported'
}
)

await app.handleFile(createTestFile('a1111.png', 'image/png'))

expect(missingNodesStore.missingNodesError).toBeNull()
})

it.for(['not-a1111', 'core-nodes-unavailable'] as const)(
'keeps missing node packs when the import fails with %s',
async (outcome) => {
const graph = new LGraph()
Reflect.set(app, 'rootGraphInternal', graph)
const missingNodesStore = useMissingNodesErrorStore()
missingNodesStore.setMissingNodeTypes(['OutgoingMissingNode'])
vi.mocked(getWorkflowDataFromFile).mockResolvedValue({
parameters: 'positive\nNegative prompt: negative\nSteps: 20'
})
mockImportA1111.mockResolvedValue(outcome)

await app.handleFile(createTestFile('a1111.png', 'image/png'))

expect(missingNodesStore.missingNodesError?.nodeTypes).toEqual([
'OutgoingMissingNode'
])
}
)
})

describe('clean', () => {
it('clears missing node packs when the graph is discarded', () => {
const graph = new LGraph()
Reflect.set(app, 'rootGraphInternal', graph)
const missingNodesStore = useMissingNodesErrorStore()
const executionErrorStore = useExecutionErrorStore()
missingNodesStore.setMissingNodeTypes(['MissingGroupNode'])
executionErrorStore.recordExecutionError({
prompt_id: 'previous-run',
timestamp: 0,
node_id: '1',
node_type: 'Test',
executed: [],
exception_message: 'fail',
exception_type: 'RuntimeError',
traceback: []
})

app.clean()

expect(missingNodesStore.missingNodesError).toBeNull()
expect(executionErrorStore.lastExecutionError).toBeNull()
})
})

describe('refreshComboInNodes', () => {
it('shows success toast and removes the pending toast after node defs reload', async () => {
app.vueAppReady = true
Expand Down
12 changes: 9 additions & 3 deletions src/scripts/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ import { normalizePromptError } from '@/utils/executionErrorUtil'
import { graphToPrompt } from '@/utils/executionUtil'
import { parseJsonWithNonFinite } from '@/utils/jsonUtil'
import { getCnrIdFromProperties } from '@/platform/nodeReplacement/cnrIdUtil'
import { useMissingNodesErrorStore } from '@/platform/nodeReplacement/missingNodesErrorStore'
import { rescanAndSurfaceMissingNodes } from '@/platform/nodeReplacement/missingNodeScan'
import {
refreshMissingModelPipeline,
Expand Down Expand Up @@ -1645,7 +1646,7 @@ export class ComfyApp {
const executionStore = useExecutionStore()
const executionErrorStore = useExecutionErrorStore()
const telemetry = useTelemetry()
executionErrorStore.clearAllErrors()
executionErrorStore.clearRunErrors()
let queueResultOverride: boolean | null = null

// Get auth token for backend nodes - uses workspace token if enabled, otherwise Firebase token
Expand Down Expand Up @@ -2019,7 +2020,11 @@ export class ComfyApp {
// Use parameters strictly as the final fallback
if (parameters && typeof parameters === 'string') {
const outcome = await importA1111(this.rootGraph, parameters, () => {
useWorkflowService().beforeLoadNewGraph()
try {
useWorkflowService().beforeLoadNewGraph()
} finally {
useMissingNodesErrorStore().setMissingNodeTypes([])
}
this.canvas.setGraph(this.rootGraph)
})
if (outcome === 'core-nodes-unavailable') {
Expand Down Expand Up @@ -2430,7 +2435,8 @@ export class ComfyApp {
const nodeOutputStore = useNodeOutputStore()
nodeOutputStore.resetAllOutputsAndPreviews()
const executionErrorStore = useExecutionErrorStore()
executionErrorStore.clearAllErrors()
executionErrorStore.clearRunErrors()
useMissingNodesErrorStore().setMissingNodeTypes([])

@christian-byrne christian-byrne Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: (non-blocking) clean() now calls useMissingNodesErrorStore().setMissingNodeTypes([]) directly, as does the A1111 callback at line 2025. executionErrorStore already holds the missingNodesStore reference internally -- a thin clearGraphErrors() method there (combining clearRunErrors() + missingNodesStore.setMissingNodeTypes([])) would give both sites a single call and remove the new direct missingNodesErrorStore import from app.ts. If a fourth missing-resource type is added later, one place to update instead of two.

@jaeone94 jaeone94 Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the direction, deferring it — and I want to be explicit about why rather than just declining.

An earlier version of this PR did exactly this: a shared reset covering all three stores, called from clean() and the other graph-discard sites. It was abandoned. Three independent unbriefed reviews found five defects in it, all on the model/media side: clean() empties the stores but never syncs pendingWarnings, so a later showPendingWarnings() resurrects them; per-node removal has the same gap from the other direction; clean() does not abort in-flight verification, so a late pipeline result repopulates an emptied store; sharing the abort controller passed a signal that was then never re-checked after the await; and the A1111 path bypassed the whole thing for models and media.

The root of it is that missing nodes are a synchronous derived fact — one ref, no async, no controller — while models and media are the result of network verification that outlives the graph that started it, with a per-workflow cache and a two-tier repair state. A clearGraphErrors() that unifies the three reads as tidier and quietly puts models and media on a path they have never been on.

So the same seam you are pointing at is the right one, but it wants a graph generation token and cache ownership to go with it, not just a combined call. That is written up and will be its own change. This PR stays at two production lines plus the A1111 compensation.


useDomWidgetStore().clear()

Expand Down
23 changes: 12 additions & 11 deletions src/stores/executionErrorStore.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { createPinia, setActivePinia } from 'pinia'
import { beforeEach, describe, expect, it, vi } from 'vitest'

import { nodeError, validationError } from '@/utils/__tests__/nodeErrorHelpers'
import type { MissingNodeType } from '@/types/comfy'
import {
createBoundaryLinkedSubgraph,
createTestRootGraph,
Expand Down Expand Up @@ -779,7 +778,9 @@ describe('hasMissingError', () => {
{
type: 'nodes',
seedMissingError: () => {
useMissingNodesErrorStore().missingNodesError = fromAny({})
useMissingNodesErrorStore().setMissingNodeTypes([
{ type: 'TestNode', hint: '' }
])
}
},
{
Expand All @@ -804,7 +805,7 @@ describe('hasMissingError', () => {
expect(executionErrorStore.hasMissingError).toBe(true)
})

it('excludes node validation errors', () => {
it('returns false when only node validation errors exist', () => {
const executionErrorStore = useExecutionErrorStore()
executionErrorStore.recordNodeErrors({
'1': nodeError([validationError('required_input_missing', 'input')])
Expand All @@ -814,7 +815,7 @@ describe('hasMissingError', () => {
})
})

describe('clearAllErrors', () => {
describe('clearRunErrors', () => {
let executionErrorStore: ReturnType<typeof useExecutionErrorStore>
let missingNodesStore: ReturnType<typeof useMissingNodesErrorStore>

Expand All @@ -825,7 +826,7 @@ describe('clearAllErrors', () => {
missingNodesStore = useMissingNodesErrorStore()
})

it('resets all error categories and closes error overlay', () => {
it('resets run errors and closes the overlay, leaving missing resources', () => {
executionErrorStore.recordExecutionError({
prompt_id: 'test',
timestamp: 0,
Expand Down Expand Up @@ -855,19 +856,19 @@ describe('clearAllErrors', () => {
class_type: 'Test'
}
})
missingNodesStore.setMissingNodeTypes(
fromAny<MissingNodeType[], unknown>([{ type: 'MissingNode', hint: '' }])
)
missingNodesStore.setMissingNodeTypes([{ type: 'MissingNode', hint: '' }])
executionErrorStore.showErrorOverlay()

executionErrorStore.clearAllErrors()
executionErrorStore.clearRunErrors()

expect(executionErrorStore.lastExecutionError).toBeNull()
expect(executionErrorStore.lastPromptError).toBeNull()
expect(executionErrorStore.lastNodeErrors).toBeNull()
expect(missingNodesStore.missingNodesError).toBeNull()
expect(executionErrorStore.isErrorOverlayOpen).toBe(false)
expect(executionErrorStore.hasAnyError).toBe(false)
expect(missingNodesStore.missingNodesError?.nodeTypes).toEqual([
{ type: 'MissingNode', hint: '' }
])
expect(executionErrorStore.hasAnyError).toBe(true)
})
})

Expand Down
12 changes: 5 additions & 7 deletions src/stores/executionErrorStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,13 @@ export const useExecutionErrorStore = defineStore('executionError', () => {
isErrorOverlayOpen.value = false
}

/** Clear all error state.
* Missing model state is intentionally preserved here to avoid wiping
* in-progress model repairs (importTaskIds, URL inputs, etc.).
* Missing models are cleared separately during workflow load/clean paths. */
function clearAllErrors() {
/** Clear error state produced by a run. Missing-resource state describes the
* loaded graph rather than the run, so only replacing or discarding the
* graph invalidates it. */
function clearRunErrors() {

@christian-byrne christian-byrne Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: (non-blocking) hasMissingError was already present and exported before this commit (HEAD~1 confirms it at line 397). The change here is that hasAnyError now delegates to it rather than inlining the three sub-flags -- which is an improvement. Worth correcting the PR description to avoid confusion for bisect/blame readers: the computed is not new.

@jaeone94 jaeone94 Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the description and there is nothing to correct — it does not mention hasMissingError or hasAnyError at all.

The delegation you are describing is also not in this diff. hasAnyError already delegated to hasMissingError at the base commit (7de29c4cf3); this branch was cut narrow and takes executionErrorStore.ts from there, so the only change to that file here is clearAllErrors → clearRunErrors losing its setMissingNodeTypes([]) line. git show 7de29c4cf3:src/stores/executionErrorStore.ts confirms it.

Flagging it in case it came from an earlier revision of this branch — there was one, and it did touch more of this file.

lastExecutionError.value = null
lastPromptError.value = null
lastNodeErrors.value = null
missingNodesStore.setMissingNodeTypes([])
isErrorOverlayOpen.value = false
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Expand Down Expand Up @@ -585,7 +583,7 @@ export const useExecutionErrorStore = defineStore('executionError', () => {
recordPromptError,

// Clearing
clearAllErrors,
clearRunErrors,
clearExecutionStartErrors,
clearPromptError,

Expand Down
Loading