mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 06:20:11 +00:00
fix: address CodeRabbit review — bug fixes, test hardening, code quality
- Fix focusedErrorNodeId watch collapsing non-execution groups (#10085) - Fix missingNodeCount always returning 0/1 instead of actual count - Add ShowErrorsTab setting to useNodeErrorFlagSync watch sources - Guard rootGraph.serialize() with try/catch, use const declarations - Deduplicate cancelled guard in useErrorReport - Await initial systemStats loading before refetching - Fix mock return type null → undefined in missingNodeScan test - Update stale doc comments referencing executionErrorStore - Strengthen test assertions: onWidgetChanged restoration, runtime error uniqueness, missing-node title verification, fallback logs
This commit is contained in:
@@ -237,6 +237,11 @@ describe('ErrorNodeCard.vue', () => {
|
||||
|
||||
// Report is still generated with fallback log message
|
||||
expect(mockGenerateErrorReport).toHaveBeenCalledOnce()
|
||||
expect(mockGenerateErrorReport).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
serverLogs: 'Failed to retrieve server logs'
|
||||
})
|
||||
)
|
||||
expect(wrapper.text()).toContain('ComfyUI Error Report')
|
||||
})
|
||||
|
||||
|
||||
@@ -242,5 +242,9 @@ describe('TabErrors.vue', () => {
|
||||
// Should render in the dedicated runtime error panel, not inside accordion
|
||||
const runtimePanel = wrapper.find('[data-testid="runtime-error-panel"]')
|
||||
expect(runtimePanel.exists()).toBe(true)
|
||||
// Verify the error message appears exactly once (not duplicated in accordion)
|
||||
expect(
|
||||
wrapper.text().match(/RuntimeError: Out of memory/g) ?? []
|
||||
).toHaveLength(1)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -371,13 +371,13 @@ watch(
|
||||
if (!graphNodeId) return
|
||||
const prefix = `${graphNodeId}:`
|
||||
for (const group of allErrorGroups.value) {
|
||||
const hasMatch =
|
||||
group.type === 'execution' &&
|
||||
group.cards.some(
|
||||
(card) =>
|
||||
card.graphNodeId === graphNodeId ||
|
||||
(card.nodeId?.startsWith(prefix) ?? false)
|
||||
)
|
||||
if (group.type !== 'execution') continue
|
||||
|
||||
const hasMatch = group.cards.some(
|
||||
(card) =>
|
||||
card.graphNodeId === graphNodeId ||
|
||||
(card.nodeId?.startsWith(prefix) ?? false)
|
||||
)
|
||||
setSectionCollapsed(group.title, !hasMatch)
|
||||
}
|
||||
rightSidePanelStore.focusedErrorNodeId = null
|
||||
|
||||
@@ -551,7 +551,11 @@ describe('useErrorGroups', () => {
|
||||
])
|
||||
await nextTick()
|
||||
|
||||
expect(groups.groupedErrorMessages.value.length).toBeGreaterThan(0)
|
||||
const missingGroup = groups.allErrorGroups.value.find(
|
||||
(g) => g.type === 'missing_node'
|
||||
)
|
||||
expect(missingGroup).toBeDefined()
|
||||
expect(groups.groupedErrorMessages.value).toContain(missingGroup!.title)
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -2,6 +2,8 @@ import { computed, onMounted, onUnmounted, reactive, toValue } from 'vue'
|
||||
|
||||
import type { MaybeRefOrGetter } from 'vue'
|
||||
|
||||
import { until } from '@vueuse/core'
|
||||
|
||||
import { api } from '@/scripts/api'
|
||||
import { app } from '@/scripts/app'
|
||||
import { useSystemStatsStore } from '@/stores/systemStatsStore'
|
||||
@@ -40,26 +42,33 @@ export function useErrorReport(cardSource: MaybeRefOrGetter<ErrorCardData>) {
|
||||
if (runtimeErrors.length === 0) return
|
||||
|
||||
if (!systemStatsStore.systemStats) {
|
||||
try {
|
||||
await systemStatsStore.refetchSystemStats()
|
||||
} catch (e) {
|
||||
console.warn('Failed to fetch system stats for error report:', e)
|
||||
return
|
||||
if (systemStatsStore.isLoading) {
|
||||
await until(systemStatsStore.isLoading).toBe(false)
|
||||
} else {
|
||||
try {
|
||||
await systemStatsStore.refetchSystemStats()
|
||||
} catch (e) {
|
||||
console.warn('Failed to fetch system stats for error report:', e)
|
||||
return
|
||||
}
|
||||
}
|
||||
}
|
||||
if (!systemStatsStore.systemStats || cancelled) return
|
||||
|
||||
let logs: string
|
||||
try {
|
||||
logs = await api.getLogs()
|
||||
} catch {
|
||||
logs = 'Failed to retrieve server logs'
|
||||
}
|
||||
const logs = await api
|
||||
.getLogs()
|
||||
.catch(() => 'Failed to retrieve server logs')
|
||||
if (cancelled) return
|
||||
|
||||
if (cancelled) return
|
||||
|
||||
const workflow = app.rootGraph.serialize()
|
||||
const workflow = (() => {
|
||||
try {
|
||||
return app.rootGraph.serialize()
|
||||
} catch (e) {
|
||||
console.warn('Failed to serialize workflow for error report:', e)
|
||||
return null
|
||||
}
|
||||
})()
|
||||
if (!workflow) return
|
||||
|
||||
for (const { error, idx } of runtimeErrors) {
|
||||
try {
|
||||
|
||||
@@ -320,20 +320,25 @@ describe('installErrorClearingHooks lifecycle', () => {
|
||||
const graph = new LGraph()
|
||||
const node = new LGraphNode('test')
|
||||
node.addInput('clip', 'CLIP')
|
||||
node.addWidget('number', 'steps', 20, () => undefined, {})
|
||||
const originalOnConnectionsChange = vi.fn()
|
||||
const originalOnWidgetChanged = vi.fn()
|
||||
node.onConnectionsChange = originalOnConnectionsChange
|
||||
node.onWidgetChanged = originalOnWidgetChanged
|
||||
graph.add(node)
|
||||
|
||||
installErrorClearingHooks(graph)
|
||||
|
||||
// Callbacks should be chained (not the originals)
|
||||
expect(node.onConnectionsChange).not.toBe(originalOnConnectionsChange)
|
||||
expect(node.onWidgetChanged).not.toBe(originalOnWidgetChanged)
|
||||
|
||||
// Simulate node removal via the graph hook
|
||||
graph.onNodeRemoved!(node)
|
||||
|
||||
// Original callbacks should be restored
|
||||
expect(node.onConnectionsChange).toBe(originalOnConnectionsChange)
|
||||
expect(node.onWidgetChanged).toBe(originalOnWidgetChanged)
|
||||
})
|
||||
|
||||
it('does not double-wrap callbacks when installErrorClearingHooks is called twice', () => {
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import type { Ref } from 'vue'
|
||||
import { watch } from 'vue'
|
||||
import { computed, watch } from 'vue'
|
||||
|
||||
import type { LGraph, LGraphNode } from '@/lib/litegraph/src/litegraph'
|
||||
import type { useMissingModelStore } from '@/platform/missingModel/missingModelStore'
|
||||
@@ -80,21 +80,27 @@ export function useNodeErrorFlagSync(
|
||||
lastNodeErrors: Ref<Record<string, NodeError> | null>,
|
||||
missingModelStore: ReturnType<typeof useMissingModelStore>
|
||||
): () => void {
|
||||
const settingStore = useSettingStore()
|
||||
const showErrorsTab = computed(() =>
|
||||
settingStore.get('Comfy.RightSidePanel.ShowErrorsTab')
|
||||
)
|
||||
|
||||
const stop = watch(
|
||||
[lastNodeErrors, () => missingModelStore.missingModelNodeIds],
|
||||
[
|
||||
lastNodeErrors,
|
||||
() => missingModelStore.missingModelNodeIds,
|
||||
showErrorsTab
|
||||
],
|
||||
() => {
|
||||
if (!app.isGraphReady) return
|
||||
// Legacy (LGraphNode) only: suppress missing-model error flags when
|
||||
// the Errors tab is hidden, since legacy nodes lack the per-widget
|
||||
// red highlight that Vue nodes use to indicate *why* a node has errors.
|
||||
// Vue nodes compute hasAnyError independently and are unaffected.
|
||||
const showErrorsTab = useSettingStore().get(
|
||||
'Comfy.RightSidePanel.ShowErrorsTab'
|
||||
)
|
||||
reconcileNodeErrorFlags(
|
||||
app.rootGraph,
|
||||
lastNodeErrors.value,
|
||||
showErrorsTab
|
||||
showErrorsTab.value
|
||||
? missingModelStore.missingModelAncestorExecutionIds
|
||||
: new Set()
|
||||
)
|
||||
|
||||
@@ -21,7 +21,7 @@ vi.mock('@/utils/graphTraversalUtil', () => ({
|
||||
}))
|
||||
|
||||
vi.mock('@/platform/nodeReplacement/cnrIdUtil', () => ({
|
||||
getCnrIdFromNode: vi.fn(() => null)
|
||||
getCnrIdFromNode: vi.fn(() => undefined)
|
||||
}))
|
||||
|
||||
vi.mock('@/i18n', () => ({
|
||||
|
||||
@@ -82,7 +82,9 @@ export const useMissingNodesErrorStore = defineStore(
|
||||
|
||||
const hasMissingNodes = computed(() => !!missingNodesError.value)
|
||||
|
||||
const missingNodeCount = computed(() => (missingNodesError.value ? 1 : 0))
|
||||
const missingNodeCount = computed(
|
||||
() => missingNodesError.value?.nodeTypes.length ?? 0
|
||||
)
|
||||
|
||||
/**
|
||||
* Set of all execution ID prefixes derived from missing node execution IDs,
|
||||
|
||||
@@ -329,7 +329,7 @@ export function useNodeReplacement() {
|
||||
|
||||
/**
|
||||
* Replaces all nodes in a single swap group and removes successfully
|
||||
* replaced types from the execution error store.
|
||||
* replaced types from the missing nodes error store.
|
||||
*/
|
||||
function replaceGroup(group: ReplacementGroup): void {
|
||||
const replaced = replaceNodesInPlace(group.nodeTypes)
|
||||
@@ -340,7 +340,7 @@ export function useNodeReplacement() {
|
||||
|
||||
/**
|
||||
* Replaces every available node across all swap groups and removes
|
||||
* the succeeded types from the execution error store.
|
||||
* the succeeded types from the missing nodes error store.
|
||||
*/
|
||||
function replaceAllGroups(groups: ReplacementGroup[]): void {
|
||||
const allNodeTypes = groups.flatMap((g) => g.nodeTypes)
|
||||
|
||||
Reference in New Issue
Block a user