mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-22 15:54:09 +00:00
refactor: encapsulate error extraction in TaskItemImpl getters (#7650)
## Summary - Add `errorMessage` and `executionError` getters to `TaskItemImpl` that extract error info from status messages - Update `useJobErrorReporting` composable to use these getters instead of standalone function - Remove the standalone `extractExecutionError` function This encapsulates error extraction within `TaskItemImpl`, preparing for the Jobs API migration where the underlying data format will change but the getter interface will remain stable. ## Test plan - [x] All existing tests pass - [x] New tests added for `TaskItemImpl.errorMessage` and `TaskItemImpl.executionError` getters - [x] TypeScript, lint, and knip checks pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7650-refactor-encapsulate-error-extraction-in-TaskItemImpl-getters-2ce6d73d365081caae33dcc7e1e07720) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Christian Byrne <cbyrne@comfy.org>
This commit is contained in:
@@ -262,7 +262,7 @@ const focusAssetInSidebar = async (item: JobListItem) => {
|
||||
|
||||
const inspectJobAsset = wrapWithErrorHandlingAsync(
|
||||
async (item: JobListItem) => {
|
||||
openResultGallery(item)
|
||||
await openResultGallery(item)
|
||||
await focusAssetInSidebar(item)
|
||||
}
|
||||
)
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
import type { Meta, StoryObj } from '@storybook/vue3-vite'
|
||||
|
||||
import type { TaskStatus } from '@/schemas/apiSchema'
|
||||
import type {
|
||||
JobListItem,
|
||||
JobStatus
|
||||
} from '@/platform/remote/comfyui/jobs/jobTypes'
|
||||
import { useExecutionStore } from '@/stores/executionStore'
|
||||
import { TaskItemImpl, useQueueStore } from '@/stores/queueStore'
|
||||
|
||||
@@ -37,91 +40,86 @@ function resetStores() {
|
||||
exec.nodeProgressStatesByPrompt = {}
|
||||
}
|
||||
|
||||
function makeTask(
|
||||
id: string,
|
||||
priority: number,
|
||||
fields: Partial<JobListItem> & { status: JobStatus; create_time: number }
|
||||
): TaskItemImpl {
|
||||
const job: JobListItem = {
|
||||
id,
|
||||
priority,
|
||||
last_state_update: null,
|
||||
update_time: fields.create_time,
|
||||
...fields
|
||||
}
|
||||
return new TaskItemImpl(job)
|
||||
}
|
||||
|
||||
function makePendingTask(
|
||||
id: string,
|
||||
index: number,
|
||||
createTimeMs?: number
|
||||
priority: number,
|
||||
createTimeMs: number
|
||||
): TaskItemImpl {
|
||||
const extraData = {
|
||||
client_id: 'c1',
|
||||
...(typeof createTimeMs === 'number' ? { create_time: createTimeMs } : {})
|
||||
}
|
||||
return new TaskItemImpl('Pending', [index, id, {}, extraData, []])
|
||||
return makeTask(id, priority, {
|
||||
status: 'pending',
|
||||
create_time: createTimeMs
|
||||
})
|
||||
}
|
||||
|
||||
function makeRunningTask(
|
||||
id: string,
|
||||
index: number,
|
||||
createTimeMs?: number
|
||||
priority: number,
|
||||
createTimeMs: number
|
||||
): TaskItemImpl {
|
||||
const extraData = {
|
||||
client_id: 'c1',
|
||||
...(typeof createTimeMs === 'number' ? { create_time: createTimeMs } : {})
|
||||
}
|
||||
return new TaskItemImpl('Running', [index, id, {}, extraData, []])
|
||||
return makeTask(id, priority, {
|
||||
status: 'in_progress',
|
||||
create_time: createTimeMs
|
||||
})
|
||||
}
|
||||
|
||||
function makeRunningTaskWithStart(
|
||||
id: string,
|
||||
index: number,
|
||||
priority: number,
|
||||
startedSecondsAgo: number
|
||||
): TaskItemImpl {
|
||||
const start = Date.now() - startedSecondsAgo * 1000
|
||||
const status: TaskStatus = {
|
||||
status_str: 'success',
|
||||
completed: false,
|
||||
messages: [['execution_start', { prompt_id: id, timestamp: start } as any]]
|
||||
}
|
||||
return new TaskItemImpl(
|
||||
'Running',
|
||||
[index, id, {}, { client_id: 'c1', create_time: start - 5000 }, []],
|
||||
status
|
||||
)
|
||||
return makeTask(id, priority, {
|
||||
status: 'in_progress',
|
||||
create_time: start - 5000,
|
||||
update_time: start
|
||||
})
|
||||
}
|
||||
|
||||
function makeHistoryTask(
|
||||
id: string,
|
||||
index: number,
|
||||
priority: number,
|
||||
durationSec: number,
|
||||
ok: boolean,
|
||||
errorMessage?: string
|
||||
): TaskItemImpl {
|
||||
const start = Date.now() - durationSec * 1000 - 1000
|
||||
const end = start + durationSec * 1000
|
||||
const messages: TaskStatus['messages'] = ok
|
||||
? [
|
||||
['execution_start', { prompt_id: id, timestamp: start } as any],
|
||||
['execution_success', { prompt_id: id, timestamp: end } as any]
|
||||
]
|
||||
: [
|
||||
['execution_start', { prompt_id: id, timestamp: start } as any],
|
||||
[
|
||||
'execution_error',
|
||||
{
|
||||
prompt_id: id,
|
||||
timestamp: end,
|
||||
node_id: '1',
|
||||
node_type: 'Node',
|
||||
executed: [],
|
||||
exception_message:
|
||||
errorMessage || 'Demo error: Node failed during execution',
|
||||
exception_type: 'RuntimeError',
|
||||
traceback: [],
|
||||
current_inputs: {},
|
||||
current_outputs: {}
|
||||
} as any
|
||||
]
|
||||
]
|
||||
const status: TaskStatus = {
|
||||
status_str: ok ? 'success' : 'error',
|
||||
completed: true,
|
||||
messages
|
||||
}
|
||||
return new TaskItemImpl(
|
||||
'History',
|
||||
[index, id, {}, { client_id: 'c1', create_time: start }, []],
|
||||
status
|
||||
)
|
||||
const now = Date.now()
|
||||
const executionEndTime = now
|
||||
const executionStartTime = now - durationSec * 1000
|
||||
return makeTask(id, priority, {
|
||||
status: ok ? 'completed' : 'failed',
|
||||
create_time: executionStartTime - 5000,
|
||||
update_time: now,
|
||||
execution_start_time: executionStartTime,
|
||||
execution_end_time: executionEndTime,
|
||||
execution_error: errorMessage
|
||||
? {
|
||||
prompt_id: id,
|
||||
timestamp: now,
|
||||
node_id: '1',
|
||||
node_type: 'ExampleNode',
|
||||
exception_message: errorMessage,
|
||||
exception_type: 'RuntimeError',
|
||||
traceback: [],
|
||||
current_inputs: {},
|
||||
current_outputs: {}
|
||||
}
|
||||
: undefined
|
||||
})
|
||||
}
|
||||
|
||||
export const Queued: Story = {
|
||||
@@ -140,8 +138,12 @@ export const Queued: Story = {
|
||||
makePendingTask(jobId, queueIndex, Date.now() - 90_000)
|
||||
]
|
||||
// Add some other pending jobs to give context
|
||||
queue.pendingTasks.push(makePendingTask('job-older-1', 100))
|
||||
queue.pendingTasks.push(makePendingTask('job-older-2', 101))
|
||||
queue.pendingTasks.push(
|
||||
makePendingTask('job-older-1', 100, Date.now() - 60_000)
|
||||
)
|
||||
queue.pendingTasks.push(
|
||||
makePendingTask('job-older-2', 101, Date.now() - 30_000)
|
||||
)
|
||||
|
||||
// Queued at (in metadata on prompt[4])
|
||||
|
||||
|
||||
@@ -12,7 +12,7 @@
|
||||
v-for="ji in group.items"
|
||||
:key="ji.id"
|
||||
:job-id="ji.id"
|
||||
:workflow-id="ji.taskRef?.workflow?.id"
|
||||
:workflow-id="ji.taskRef?.workflowId"
|
||||
:state="ji.state"
|
||||
:title="ji.title"
|
||||
:right-text="ji.meta"
|
||||
|
||||
@@ -2,116 +2,49 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { computed, ref } from 'vue'
|
||||
import type { ComputedRef } from 'vue'
|
||||
|
||||
import type { ExecutionErrorWsMessage } from '@/schemas/apiSchema'
|
||||
import type { TaskItemImpl } from '@/stores/queueStore'
|
||||
import type {
|
||||
JobErrorDialogService,
|
||||
UseJobErrorReportingOptions
|
||||
} from '@/components/queue/job/useJobErrorReporting'
|
||||
import * as jobErrorReporting from '@/components/queue/job/useJobErrorReporting'
|
||||
import type { JobErrorDialogService } from '@/components/queue/job/useJobErrorReporting'
|
||||
import { useJobErrorReporting } from '@/components/queue/job/useJobErrorReporting'
|
||||
import type { ExecutionError } from '@/platform/remote/comfyui/jobs/jobTypes'
|
||||
|
||||
const createExecutionErrorMessage = (
|
||||
overrides: Partial<ExecutionErrorWsMessage> = {}
|
||||
): ExecutionErrorWsMessage => ({
|
||||
prompt_id: 'prompt',
|
||||
timestamp: 100,
|
||||
node_id: 'node-1',
|
||||
node_type: 'KSampler',
|
||||
executed: [],
|
||||
exception_message: 'default failure',
|
||||
exception_type: 'RuntimeError',
|
||||
traceback: ['Trace line'],
|
||||
current_inputs: {},
|
||||
current_outputs: {},
|
||||
...overrides
|
||||
})
|
||||
|
||||
const createTaskWithMessages = (
|
||||
messages: Array<[string, unknown]> | undefined = []
|
||||
const createTaskWithError = (
|
||||
promptId: string,
|
||||
errorMessage?: string,
|
||||
executionError?: ExecutionError,
|
||||
createTime?: number
|
||||
): TaskItemImpl =>
|
||||
({
|
||||
status: {
|
||||
status_str: 'error',
|
||||
completed: false,
|
||||
messages
|
||||
}
|
||||
}) as TaskItemImpl
|
||||
|
||||
describe('extractExecutionError', () => {
|
||||
it('returns null when task has no execution error messages', () => {
|
||||
expect(jobErrorReporting.extractExecutionError(null)).toBeNull()
|
||||
expect(
|
||||
jobErrorReporting.extractExecutionError({
|
||||
status: undefined
|
||||
} as TaskItemImpl)
|
||||
).toBeNull()
|
||||
expect(
|
||||
jobErrorReporting.extractExecutionError({
|
||||
status: {
|
||||
status_str: 'error',
|
||||
completed: false,
|
||||
messages: {} as unknown as Array<[string, unknown]>
|
||||
}
|
||||
} as TaskItemImpl)
|
||||
).toBeNull()
|
||||
expect(
|
||||
jobErrorReporting.extractExecutionError(createTaskWithMessages([]))
|
||||
).toBeNull()
|
||||
expect(
|
||||
jobErrorReporting.extractExecutionError(
|
||||
createTaskWithMessages([
|
||||
['execution_start', { prompt_id: 'prompt', timestamp: 1 }]
|
||||
] as Array<[string, unknown]>)
|
||||
)
|
||||
).toBeNull()
|
||||
})
|
||||
|
||||
it('returns detail and message for execution_error entries', () => {
|
||||
const detail = createExecutionErrorMessage({ exception_message: 'Kaboom' })
|
||||
const result = jobErrorReporting.extractExecutionError(
|
||||
createTaskWithMessages([
|
||||
['execution_success', { prompt_id: 'prompt', timestamp: 2 }],
|
||||
['execution_error', detail]
|
||||
] as Array<[string, unknown]>)
|
||||
)
|
||||
expect(result).toEqual({
|
||||
detail,
|
||||
message: 'Kaboom'
|
||||
})
|
||||
})
|
||||
|
||||
it('falls back to an empty message when the tuple lacks detail', () => {
|
||||
const result = jobErrorReporting.extractExecutionError(
|
||||
createTaskWithMessages([
|
||||
['execution_error'] as unknown as [string, ExecutionErrorWsMessage]
|
||||
])
|
||||
)
|
||||
expect(result).toEqual({ detail: undefined, message: '' })
|
||||
})
|
||||
})
|
||||
promptId,
|
||||
errorMessage,
|
||||
executionError,
|
||||
createTime: createTime ?? Date.now()
|
||||
}) as Partial<TaskItemImpl> as TaskItemImpl
|
||||
|
||||
describe('useJobErrorReporting', () => {
|
||||
let taskState = ref<TaskItemImpl | null>(null)
|
||||
let taskForJob: ComputedRef<TaskItemImpl | null>
|
||||
let copyToClipboard: UseJobErrorReportingOptions['copyToClipboard']
|
||||
let showExecutionErrorDialog: JobErrorDialogService['showExecutionErrorDialog']
|
||||
let showErrorDialog: JobErrorDialogService['showErrorDialog']
|
||||
let copyToClipboard: ReturnType<typeof vi.fn>
|
||||
let showErrorDialog: ReturnType<typeof vi.fn>
|
||||
let showExecutionErrorDialog: ReturnType<typeof vi.fn>
|
||||
let dialog: JobErrorDialogService
|
||||
let composable: ReturnType<typeof jobErrorReporting.useJobErrorReporting>
|
||||
let composable: ReturnType<typeof useJobErrorReporting>
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
taskState = ref<TaskItemImpl | null>(null)
|
||||
taskForJob = computed(() => taskState.value)
|
||||
copyToClipboard = vi.fn()
|
||||
showExecutionErrorDialog = vi.fn()
|
||||
showErrorDialog = vi.fn()
|
||||
showExecutionErrorDialog = vi.fn()
|
||||
dialog = {
|
||||
showExecutionErrorDialog,
|
||||
showErrorDialog
|
||||
}
|
||||
composable = jobErrorReporting.useJobErrorReporting({
|
||||
showErrorDialog,
|
||||
showExecutionErrorDialog
|
||||
} as unknown as JobErrorDialogService
|
||||
composable = useJobErrorReporting({
|
||||
taskForJob,
|
||||
copyToClipboard,
|
||||
copyToClipboard: copyToClipboard as (
|
||||
value: string
|
||||
) => void | Promise<void>,
|
||||
dialog
|
||||
})
|
||||
})
|
||||
@@ -121,73 +54,87 @@ describe('useJobErrorReporting', () => {
|
||||
})
|
||||
|
||||
it('exposes a computed message that reflects the current task error', () => {
|
||||
taskState.value = createTaskWithMessages([
|
||||
[
|
||||
'execution_error',
|
||||
createExecutionErrorMessage({ exception_message: 'First failure' })
|
||||
]
|
||||
])
|
||||
taskState.value = createTaskWithError('job-1', 'First failure')
|
||||
expect(composable.errorMessageValue.value).toBe('First failure')
|
||||
|
||||
taskState.value = createTaskWithMessages([
|
||||
[
|
||||
'execution_error',
|
||||
createExecutionErrorMessage({ exception_message: 'Second failure' })
|
||||
]
|
||||
])
|
||||
taskState.value = createTaskWithError('job-2', 'Second failure')
|
||||
expect(composable.errorMessageValue.value).toBe('Second failure')
|
||||
})
|
||||
|
||||
it('returns empty string when no error message', () => {
|
||||
taskState.value = createTaskWithError('job-1')
|
||||
expect(composable.errorMessageValue.value).toBe('')
|
||||
})
|
||||
|
||||
it('returns empty string when task is null', () => {
|
||||
taskState.value = null
|
||||
expect(composable.errorMessageValue.value).toBe('')
|
||||
})
|
||||
|
||||
it('only calls the copy handler when a message exists', () => {
|
||||
taskState.value = createTaskWithMessages([
|
||||
[
|
||||
'execution_error',
|
||||
createExecutionErrorMessage({ exception_message: 'Clipboard failure' })
|
||||
]
|
||||
])
|
||||
taskState.value = createTaskWithError('job-1', 'Clipboard failure')
|
||||
composable.copyErrorMessage()
|
||||
expect(copyToClipboard).toHaveBeenCalledTimes(1)
|
||||
expect(copyToClipboard).toHaveBeenCalledWith('Clipboard failure')
|
||||
|
||||
vi.mocked(copyToClipboard).mockClear()
|
||||
taskState.value = createTaskWithMessages([])
|
||||
copyToClipboard.mockClear()
|
||||
taskState.value = createTaskWithError('job-2')
|
||||
composable.copyErrorMessage()
|
||||
expect(copyToClipboard).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('prefers the detailed execution dialog when detail is available', () => {
|
||||
const detail = createExecutionErrorMessage({
|
||||
exception_message: 'Detailed failure'
|
||||
})
|
||||
taskState.value = createTaskWithMessages([['execution_error', detail]])
|
||||
it('shows simple error dialog when only errorMessage present', () => {
|
||||
taskState.value = createTaskWithError('job-1', 'Queue job error')
|
||||
composable.reportJobError()
|
||||
expect(showExecutionErrorDialog).toHaveBeenCalledTimes(1)
|
||||
expect(showExecutionErrorDialog).toHaveBeenCalledWith(detail)
|
||||
expect(showErrorDialog).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('shows a fallback dialog when only a message is available', () => {
|
||||
const message = 'Queue job error'
|
||||
taskState.value = createTaskWithMessages([])
|
||||
const valueSpy = vi
|
||||
.spyOn(composable.errorMessageValue, 'value', 'get')
|
||||
.mockReturnValue(message)
|
||||
|
||||
expect(composable.errorMessageValue.value).toBe(message)
|
||||
composable.reportJobError()
|
||||
expect(showExecutionErrorDialog).not.toHaveBeenCalled()
|
||||
expect(showErrorDialog).toHaveBeenCalledTimes(1)
|
||||
const [errorArg, optionsArg] = vi.mocked(showErrorDialog).mock.calls[0]
|
||||
const [errorArg, optionsArg] = showErrorDialog.mock.calls[0]
|
||||
expect(errorArg).toBeInstanceOf(Error)
|
||||
expect(errorArg.message).toBe(message)
|
||||
expect(errorArg.message).toBe('Queue job error')
|
||||
expect(optionsArg).toEqual({ reportType: 'queueJobError' })
|
||||
valueSpy.mockRestore()
|
||||
expect(showExecutionErrorDialog).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does nothing when no error could be extracted', () => {
|
||||
taskState.value = createTaskWithMessages([])
|
||||
it('does nothing when no task exists', () => {
|
||||
taskState.value = null
|
||||
composable.reportJobError()
|
||||
expect(showExecutionErrorDialog).not.toHaveBeenCalled()
|
||||
expect(showErrorDialog).not.toHaveBeenCalled()
|
||||
expect(showExecutionErrorDialog).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('shows rich error dialog when execution_error available on task', () => {
|
||||
const executionError: ExecutionError = {
|
||||
prompt_id: 'job-1',
|
||||
timestamp: 12345,
|
||||
node_id: '5',
|
||||
node_type: 'KSampler',
|
||||
executed: ['1', '2'],
|
||||
exception_message: 'CUDA out of memory',
|
||||
exception_type: 'RuntimeError',
|
||||
traceback: ['line 1', 'line 2'],
|
||||
current_inputs: {},
|
||||
current_outputs: {}
|
||||
}
|
||||
taskState.value = createTaskWithError(
|
||||
'job-1',
|
||||
'CUDA out of memory',
|
||||
executionError,
|
||||
12345
|
||||
)
|
||||
|
||||
composable.reportJobError()
|
||||
|
||||
expect(showExecutionErrorDialog).toHaveBeenCalledTimes(1)
|
||||
expect(showExecutionErrorDialog).toHaveBeenCalledWith(executionError)
|
||||
expect(showErrorDialog).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does nothing when no error message and no execution_error', () => {
|
||||
taskState.value = createTaskWithError('job-1')
|
||||
|
||||
composable.reportJobError()
|
||||
|
||||
expect(showErrorDialog).not.toHaveBeenCalled()
|
||||
expect(showExecutionErrorDialog).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,13 +1,13 @@
|
||||
import { computed } from 'vue'
|
||||
import type { ComputedRef } from 'vue'
|
||||
|
||||
import type { ExecutionErrorWsMessage } from '@/schemas/apiSchema'
|
||||
import type { ExecutionErrorDialogInput } from '@/services/dialogService'
|
||||
import type { TaskItemImpl } from '@/stores/queueStore'
|
||||
|
||||
type CopyHandler = (value: string) => void | Promise<void>
|
||||
|
||||
export type JobErrorDialogService = {
|
||||
showExecutionErrorDialog: (error: ExecutionErrorWsMessage) => void
|
||||
showExecutionErrorDialog: (executionError: ExecutionErrorDialogInput) => void
|
||||
showErrorDialog: (
|
||||
error: Error,
|
||||
options?: {
|
||||
@@ -17,30 +17,7 @@ export type JobErrorDialogService = {
|
||||
) => void
|
||||
}
|
||||
|
||||
type JobExecutionError = {
|
||||
detail?: ExecutionErrorWsMessage
|
||||
message: string
|
||||
}
|
||||
|
||||
export const extractExecutionError = (
|
||||
task: TaskItemImpl | null
|
||||
): JobExecutionError | null => {
|
||||
const status = (task as TaskItemImpl | null)?.status
|
||||
const messages = (status as { messages?: unknown[] } | undefined)?.messages
|
||||
if (!Array.isArray(messages) || !messages.length) return null
|
||||
const record = messages.find((entry: unknown) => {
|
||||
return Array.isArray(entry) && entry[0] === 'execution_error'
|
||||
}) as [string, ExecutionErrorWsMessage?] | undefined
|
||||
if (!record) return null
|
||||
const detail = record[1]
|
||||
const message = String(detail?.exception_message ?? '')
|
||||
return {
|
||||
detail,
|
||||
message
|
||||
}
|
||||
}
|
||||
|
||||
export type UseJobErrorReportingOptions = {
|
||||
type UseJobErrorReportingOptions = {
|
||||
taskForJob: ComputedRef<TaskItemImpl | null>
|
||||
copyToClipboard: CopyHandler
|
||||
dialog: JobErrorDialogService
|
||||
@@ -51,10 +28,7 @@ export const useJobErrorReporting = ({
|
||||
copyToClipboard,
|
||||
dialog
|
||||
}: UseJobErrorReportingOptions) => {
|
||||
const errorMessageValue = computed(() => {
|
||||
const error = extractExecutionError(taskForJob.value)
|
||||
return error?.message ?? ''
|
||||
})
|
||||
const errorMessageValue = computed(() => taskForJob.value?.errorMessage ?? '')
|
||||
|
||||
const copyErrorMessage = () => {
|
||||
if (errorMessageValue.value) {
|
||||
@@ -63,11 +37,12 @@ export const useJobErrorReporting = ({
|
||||
}
|
||||
|
||||
const reportJobError = () => {
|
||||
const error = extractExecutionError(taskForJob.value)
|
||||
if (error?.detail) {
|
||||
dialog.showExecutionErrorDialog(error.detail)
|
||||
const executionError = taskForJob.value?.executionError
|
||||
if (executionError) {
|
||||
dialog.showExecutionErrorDialog(executionError)
|
||||
return
|
||||
}
|
||||
|
||||
if (errorMessageValue.value) {
|
||||
dialog.showErrorDialog(new Error(errorMessageValue.value), {
|
||||
reportType: 'queueJobError'
|
||||
|
||||
Reference in New Issue
Block a user