mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-19 22:09:37 +00:00
fix: address remaining coderabbit review findings
- Add aria-live="polite" to runtime error panel (TabErrors.vue) - Return commandStore.execute instead of void (useErrorActions.ts) - Replace onMounted with watch(immediate:true) for cardSource reactivity (useErrorReport.ts) - Clear interaction state (expandState, uploadState, pendingSelection) when removing candidates by name or widget (missingMediaStore.ts) - Replace waitForTimeout with expect.poll in E2E locate test - Add 3 regression tests for interaction state cleanup
This commit is contained in:
@@ -205,18 +205,16 @@ test.describe('Missing media inputs in Error Tab', () => {
|
||||
await expect(locateButton).toBeVisible()
|
||||
await locateButton.click()
|
||||
|
||||
await comfyPage.page.waitForTimeout(500)
|
||||
|
||||
const offsetAfter = await comfyPage.page.evaluate(() => {
|
||||
const canvas = window['app']?.canvas
|
||||
return canvas?.ds?.offset
|
||||
? [canvas.ds.offset[0], canvas.ds.offset[1]]
|
||||
: null
|
||||
})
|
||||
|
||||
expect(offsetBefore).not.toBeNull()
|
||||
expect(offsetAfter).not.toBeNull()
|
||||
expect(offsetAfter).not.toEqual(offsetBefore)
|
||||
await expect
|
||||
.poll(async () => {
|
||||
return await comfyPage.page.evaluate(() => {
|
||||
const canvas = window['app']?.canvas
|
||||
return canvas?.ds?.offset
|
||||
? [canvas.ds.offset[0], canvas.ds.offset[1]]
|
||||
: null
|
||||
})
|
||||
})
|
||||
.not.toEqual(offsetBefore)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -15,6 +15,7 @@
|
||||
<div
|
||||
v-if="singleRuntimeErrorCard"
|
||||
data-testid="runtime-error-panel"
|
||||
aria-live="polite"
|
||||
class="flex min-h-0 flex-1 flex-col overflow-hidden px-4 py-3"
|
||||
>
|
||||
<div
|
||||
|
||||
@@ -20,7 +20,7 @@ export function useErrorActions() {
|
||||
is_external: true,
|
||||
source: 'error_dialog'
|
||||
})
|
||||
void commandStore.execute('Comfy.ContactSupport')
|
||||
return commandStore.execute('Comfy.ContactSupport')
|
||||
}
|
||||
|
||||
function findOnGitHub(errorMessage: string) {
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { computed, onMounted, onUnmounted, reactive, toValue } from 'vue'
|
||||
import { computed, reactive, toValue, watch } from 'vue'
|
||||
|
||||
import type { MaybeRefOrGetter } from 'vue'
|
||||
|
||||
@@ -28,66 +28,73 @@ export function useErrorReport(cardSource: MaybeRefOrGetter<ErrorCardData>) {
|
||||
)
|
||||
})
|
||||
|
||||
let cancelled = false
|
||||
onUnmounted(() => {
|
||||
cancelled = true
|
||||
})
|
||||
watch(
|
||||
() => toValue(cardSource),
|
||||
async (card, _, onCleanup) => {
|
||||
let cancelled = false
|
||||
onCleanup(() => {
|
||||
cancelled = true
|
||||
})
|
||||
|
||||
onMounted(async () => {
|
||||
const card = toValue(cardSource)
|
||||
const runtimeErrors = card.errors
|
||||
.map((error, idx) => ({ error, idx }))
|
||||
.filter(({ error }) => error.isRuntimeError)
|
||||
for (const key of Object.keys(enrichedDetails)) {
|
||||
delete enrichedDetails[Number(key)]
|
||||
}
|
||||
|
||||
if (runtimeErrors.length === 0) return
|
||||
const runtimeErrors = card.errors
|
||||
.map((error, idx) => ({ error, idx }))
|
||||
.filter(({ error }) => error.isRuntimeError)
|
||||
|
||||
if (!systemStatsStore.systemStats) {
|
||||
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 (runtimeErrors.length === 0) return
|
||||
|
||||
if (!systemStatsStore.systemStats) {
|
||||
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
|
||||
if (!systemStatsStore.systemStats || cancelled) return
|
||||
|
||||
const logs = await api
|
||||
.getLogs()
|
||||
.catch(() => 'Failed to retrieve server logs')
|
||||
if (cancelled) return
|
||||
const logs = await api
|
||||
.getLogs()
|
||||
.catch(() => 'Failed to retrieve server logs')
|
||||
if (cancelled) return
|
||||
|
||||
const workflow = (() => {
|
||||
try {
|
||||
return app.rootGraph.serialize()
|
||||
} catch (e) {
|
||||
console.warn('Failed to serialize workflow for error report:', e)
|
||||
return null
|
||||
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 {
|
||||
const report = generateErrorReport({
|
||||
exceptionType: error.exceptionType ?? FALLBACK_EXCEPTION_TYPE,
|
||||
exceptionMessage: error.message,
|
||||
traceback: error.details,
|
||||
nodeId: card.nodeId,
|
||||
nodeType: card.title,
|
||||
systemStats: systemStatsStore.systemStats,
|
||||
serverLogs: logs,
|
||||
workflow
|
||||
})
|
||||
enrichedDetails[idx] = report
|
||||
} catch (e) {
|
||||
console.warn('Failed to generate error report:', e)
|
||||
}
|
||||
}
|
||||
})()
|
||||
if (!workflow) return
|
||||
|
||||
for (const { error, idx } of runtimeErrors) {
|
||||
try {
|
||||
const report = generateErrorReport({
|
||||
exceptionType: error.exceptionType ?? FALLBACK_EXCEPTION_TYPE,
|
||||
exceptionMessage: error.message,
|
||||
traceback: error.details,
|
||||
nodeId: card.nodeId,
|
||||
nodeType: card.title,
|
||||
systemStats: systemStatsStore.systemStats,
|
||||
serverLogs: logs,
|
||||
workflow
|
||||
})
|
||||
enrichedDetails[idx] = report
|
||||
} catch (e) {
|
||||
console.warn('Failed to generate error report:', e)
|
||||
}
|
||||
}
|
||||
})
|
||||
},
|
||||
{ immediate: true }
|
||||
)
|
||||
|
||||
return { displayedDetailsMap }
|
||||
}
|
||||
|
||||
@@ -145,6 +145,47 @@ describe('useMissingMediaStore', () => {
|
||||
expect(store.missingMediaCandidates).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('removeMissingMediaByName clears interaction state for removed name', () => {
|
||||
const store = useMissingMediaStore()
|
||||
store.setMissingMedia([makeCandidate('1', 'photo.png')])
|
||||
store.expandState['photo.png'] = true
|
||||
store.uploadState['photo.png'] = {
|
||||
fileName: 'photo.png',
|
||||
status: 'uploaded'
|
||||
}
|
||||
store.pendingSelection['photo.png'] = 'uploaded/photo.png'
|
||||
|
||||
store.removeMissingMediaByName('photo.png')
|
||||
|
||||
expect(store.expandState['photo.png']).toBeUndefined()
|
||||
expect(store.uploadState['photo.png']).toBeUndefined()
|
||||
expect(store.pendingSelection['photo.png']).toBeUndefined()
|
||||
})
|
||||
|
||||
it('removeMissingMediaByWidget clears interaction state for removed name', () => {
|
||||
const store = useMissingMediaStore()
|
||||
store.setMissingMedia([makeCandidate('1', 'photo.png')])
|
||||
store.pendingSelection['photo.png'] = 'library/photo.png'
|
||||
|
||||
store.removeMissingMediaByWidget('1', 'image')
|
||||
|
||||
expect(store.pendingSelection['photo.png']).toBeUndefined()
|
||||
})
|
||||
|
||||
it('removeMissingMediaByWidget preserves interaction state when other candidates share the name', () => {
|
||||
const store = useMissingMediaStore()
|
||||
store.setMissingMedia([
|
||||
makeCandidate('1', 'photo.png'),
|
||||
makeCandidate('2', 'photo.png')
|
||||
])
|
||||
store.pendingSelection['photo.png'] = 'library/photo.png'
|
||||
|
||||
store.removeMissingMediaByWidget('1', 'image')
|
||||
|
||||
expect(store.missingMediaCandidates).toHaveLength(1)
|
||||
expect(store.pendingSelection['photo.png']).toBe('library/photo.png')
|
||||
})
|
||||
|
||||
it('createVerificationAbortController aborts previous controller', () => {
|
||||
const store = useMissingMediaStore()
|
||||
const first = store.createVerificationAbortController()
|
||||
|
||||
@@ -88,20 +88,39 @@ export const useMissingMediaStore = defineStore('missingMedia', () => {
|
||||
return activeMissingMediaGraphIds.value.has(String(node.id))
|
||||
}
|
||||
|
||||
function clearInteractionStateForName(name: string) {
|
||||
delete expandState.value[name]
|
||||
delete uploadState.value[name]
|
||||
delete pendingSelection.value[name]
|
||||
}
|
||||
|
||||
function removeMissingMediaByName(name: string) {
|
||||
if (!missingMediaCandidates.value) return
|
||||
missingMediaCandidates.value = missingMediaCandidates.value.filter(
|
||||
(m) => m.name !== name
|
||||
)
|
||||
clearInteractionStateForName(name)
|
||||
if (!missingMediaCandidates.value.length)
|
||||
missingMediaCandidates.value = null
|
||||
}
|
||||
|
||||
function removeMissingMediaByWidget(nodeId: string, widgetName: string) {
|
||||
if (!missingMediaCandidates.value) return
|
||||
const removedNames = new Set(
|
||||
missingMediaCandidates.value
|
||||
.filter(
|
||||
(m) => String(m.nodeId) === nodeId && m.widgetName === widgetName
|
||||
)
|
||||
.map((m) => m.name)
|
||||
)
|
||||
missingMediaCandidates.value = missingMediaCandidates.value.filter(
|
||||
(m) => !(String(m.nodeId) === nodeId && m.widgetName === widgetName)
|
||||
)
|
||||
for (const name of removedNames) {
|
||||
if (!missingMediaCandidates.value.some((m) => m.name === name)) {
|
||||
clearInteractionStateForName(name)
|
||||
}
|
||||
}
|
||||
if (!missingMediaCandidates.value.length)
|
||||
missingMediaCandidates.value = null
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user