mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-14 01:36:14 +00:00
fix(manager): drive failure toast from task history, not promise rejection
Address review feedback: the previous toast logic fired on Promise rejection from usePackInstall, but managerStore.installPack routes through useCachedRequest which catches errors into null, and the real install outcome arrives asynchronously via task-completion events. This meant failures never surfaced a toast in practice. Move the toast to comfyManagerStore and trigger it from a watch on failedTasksIds length increases, with a short debounce to coalesce a burst of failures into a single toast. The watcher ignores resets (oldCount -> 0) so resetTaskState does not trigger a false alarm. Replace the composable-level mock tests with store-level tests that exercise the real contract (task history -> partitionTasks -> failedTasksIds).
This commit is contained in:
@@ -1,146 +0,0 @@
|
||||
import { createTestingPinia } from '@pinia/testing'
|
||||
import { setActivePinia } from 'pinia'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import type { components } from '@/types/comfyRegistryTypes'
|
||||
import { usePackInstall } from '@/workbench/extensions/manager/composables/nodePack/usePackInstall'
|
||||
|
||||
type NodePack = components['schemas']['Node']
|
||||
|
||||
const toastAdd = vi.fn()
|
||||
const installPackCall = vi.fn()
|
||||
const installPackClear = vi.fn()
|
||||
const isPackInstalled = vi.fn(() => false)
|
||||
|
||||
vi.mock('@/platform/updates/common/toastStore', () => ({
|
||||
useToastStore: vi.fn(() => ({
|
||||
add: toastAdd
|
||||
}))
|
||||
}))
|
||||
|
||||
vi.mock('@/workbench/extensions/manager/stores/comfyManagerStore', () => ({
|
||||
useComfyManagerStore: vi.fn(() => ({
|
||||
installPack: {
|
||||
call: installPackCall,
|
||||
clear: installPackClear
|
||||
},
|
||||
isPackInstalled,
|
||||
isPackInstalling: vi.fn(() => false)
|
||||
}))
|
||||
}))
|
||||
|
||||
vi.mock(
|
||||
'@/workbench/extensions/manager/composables/useNodeConflictDialog',
|
||||
() => ({
|
||||
useNodeConflictDialog: vi.fn(() => ({
|
||||
show: vi.fn()
|
||||
}))
|
||||
})
|
||||
)
|
||||
|
||||
vi.mock(
|
||||
'@/workbench/extensions/manager/composables/useConflictDetection',
|
||||
() => ({
|
||||
useConflictDetection: vi.fn(() => ({
|
||||
checkNodeCompatibility: vi.fn(() => ({
|
||||
hasConflict: false,
|
||||
conflicts: []
|
||||
}))
|
||||
}))
|
||||
})
|
||||
)
|
||||
|
||||
vi.mock('vue-i18n', () => ({
|
||||
useI18n: vi.fn(() => ({
|
||||
t: (key: string, ...args: unknown[]) => {
|
||||
const named = args.find(
|
||||
(a): a is Record<string, unknown> =>
|
||||
typeof a === 'object' && a !== null && !Array.isArray(a)
|
||||
)
|
||||
if (named && 'count' in named) {
|
||||
return `${key}:${String(named.count)}`
|
||||
}
|
||||
return key
|
||||
}
|
||||
}))
|
||||
}))
|
||||
|
||||
const createMockPack = (id: string): NodePack =>
|
||||
({
|
||||
id,
|
||||
name: `Pack ${id}`,
|
||||
repository: 'https://github.com/test/pack',
|
||||
publisher: { name: 'TestPublisher' },
|
||||
latest_version: { version: '1.0.0' }
|
||||
}) as unknown as NodePack
|
||||
|
||||
describe('usePackInstall', () => {
|
||||
beforeEach(() => {
|
||||
setActivePinia(createTestingPinia({ stubActions: false }))
|
||||
toastAdd.mockClear()
|
||||
installPackCall.mockReset()
|
||||
installPackClear.mockClear()
|
||||
isPackInstalled.mockReturnValue(false)
|
||||
vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
it('does not show a toast when all installations succeed', async () => {
|
||||
installPackCall.mockResolvedValue(undefined)
|
||||
const packs = [createMockPack('a'), createMockPack('b')]
|
||||
const { performInstallation } = usePackInstall(() => packs)
|
||||
|
||||
await performInstallation(packs)
|
||||
|
||||
expect(toastAdd).not.toHaveBeenCalled()
|
||||
expect(installPackClear).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('shows a single error toast summarising failures when installations fail', async () => {
|
||||
installPackCall
|
||||
.mockRejectedValueOnce(new Error('boom-a'))
|
||||
.mockRejectedValueOnce(new Error('boom-b'))
|
||||
const packs = [createMockPack('a'), createMockPack('b')]
|
||||
const { performInstallation } = usePackInstall(() => packs)
|
||||
|
||||
await performInstallation(packs)
|
||||
|
||||
expect(toastAdd).toHaveBeenCalledTimes(1)
|
||||
const message = toastAdd.mock.calls[0][0]
|
||||
expect(message.severity).toBe('error')
|
||||
expect(message.summary).toBe('manager.installFailureToast.summary')
|
||||
expect(message.detail).toBe('manager.installFailureToast.detail:2')
|
||||
})
|
||||
|
||||
it('shows the toast even when only one of many installs fails', async () => {
|
||||
installPackCall
|
||||
.mockResolvedValueOnce(undefined)
|
||||
.mockRejectedValueOnce(new Error('boom'))
|
||||
.mockResolvedValueOnce(undefined)
|
||||
const packs = [
|
||||
createMockPack('a'),
|
||||
createMockPack('b'),
|
||||
createMockPack('c')
|
||||
]
|
||||
const { performInstallation } = usePackInstall(() => packs)
|
||||
|
||||
await performInstallation(packs)
|
||||
|
||||
expect(toastAdd).toHaveBeenCalledTimes(1)
|
||||
const message = toastAdd.mock.calls[0][0]
|
||||
expect(message.detail).toBe('manager.installFailureToast.detail:1')
|
||||
})
|
||||
|
||||
it('always clears the install cache, even on failure', async () => {
|
||||
installPackCall.mockRejectedValue(new Error('boom'))
|
||||
const packs = [createMockPack('a')]
|
||||
const { performInstallation } = usePackInstall(() => packs)
|
||||
|
||||
await performInstallation(packs)
|
||||
|
||||
expect(installPackClear).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
})
|
||||
@@ -1,7 +1,6 @@
|
||||
import { computed } from 'vue'
|
||||
import { useI18n } from 'vue-i18n'
|
||||
|
||||
import { useToastStore } from '@/platform/updates/common/toastStore'
|
||||
import type { components } from '@/types/comfyRegistryTypes'
|
||||
import type { components as ManagerComponents } from '@/workbench/extensions/manager/types/generatedManagerTypes'
|
||||
import { useConflictDetection } from '@/workbench/extensions/manager/composables/useConflictDetection'
|
||||
@@ -63,16 +62,6 @@ export function usePackInstall(
|
||||
'[usePackInstall] Some installations failed:',
|
||||
failures.map((f) => f.reason)
|
||||
)
|
||||
useToastStore().add({
|
||||
severity: 'error',
|
||||
summary: t('manager.installFailureToast.summary'),
|
||||
detail: t(
|
||||
'manager.installFailureToast.detail',
|
||||
{ count: failures.length },
|
||||
failures.length
|
||||
),
|
||||
life: 8000
|
||||
})
|
||||
}
|
||||
} finally {
|
||||
managerStore.installPack.clear()
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { createTestingPinia } from '@pinia/testing'
|
||||
import { setActivePinia } from 'pinia'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { nextTick, ref } from 'vue'
|
||||
|
||||
import { useComfyManagerService } from '@/workbench/extensions/manager/services/comfyManagerService'
|
||||
@@ -55,6 +55,21 @@ vi.mock('vue-i18n', () => ({
|
||||
}))
|
||||
}))
|
||||
|
||||
const toastAddMock = vi.fn()
|
||||
|
||||
vi.mock('@/platform/updates/common/toastStore', () => ({
|
||||
useToastStore: vi.fn(() => ({
|
||||
add: toastAddMock
|
||||
}))
|
||||
}))
|
||||
|
||||
vi.mock('@/i18n', () => ({
|
||||
t: (key: string, params?: Record<string, unknown>) => {
|
||||
if (params && 'count' in params) return `${key}:${String(params.count)}`
|
||||
return key
|
||||
}
|
||||
}))
|
||||
|
||||
interface EnabledDisabledTestCase {
|
||||
desc: string
|
||||
installed: Record<string, ManagerPackInstalled>
|
||||
@@ -79,6 +94,8 @@ describe('useComfyManagerStore', () => {
|
||||
beforeEach(() => {
|
||||
setActivePinia(createTestingPinia({ stubActions: false }))
|
||||
vi.clearAllMocks()
|
||||
toastAddMock.mockClear()
|
||||
vi.useFakeTimers()
|
||||
mockManagerService = {
|
||||
isLoading: ref(false),
|
||||
error: ref(null),
|
||||
@@ -102,6 +119,10 @@ describe('useComfyManagerStore', () => {
|
||||
vi.mocked(useComfyManagerService).mockReturnValue(mockManagerService)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers()
|
||||
})
|
||||
|
||||
const testCases: EnabledDisabledTestCase[] = [
|
||||
{
|
||||
desc: 'Two enabled versions',
|
||||
@@ -501,4 +522,85 @@ describe('useComfyManagerStore', () => {
|
||||
expect(store.isPackInstalled('disabled-pack')).toBe(true)
|
||||
})
|
||||
})
|
||||
|
||||
describe('installation failure toast', () => {
|
||||
const setTaskHistory = (
|
||||
store: ReturnType<typeof useComfyManagerStore>,
|
||||
history: Record<string, ManagerComponents['schemas']['TaskHistoryItem']>
|
||||
) => {
|
||||
store.taskHistory = history
|
||||
}
|
||||
|
||||
const errorTask = (
|
||||
id: string
|
||||
): ManagerComponents['schemas']['TaskHistoryItem'] => ({
|
||||
ui_id: id,
|
||||
client_id: 'test',
|
||||
kind: 'install',
|
||||
result: 'failed',
|
||||
status: { status_str: 'error', completed: false, messages: ['boom'] },
|
||||
timestamp: new Date().toISOString()
|
||||
})
|
||||
|
||||
const successTask = (
|
||||
id: string
|
||||
): ManagerComponents['schemas']['TaskHistoryItem'] => ({
|
||||
ui_id: id,
|
||||
client_id: 'test',
|
||||
kind: 'install',
|
||||
result: 'success',
|
||||
status: { status_str: 'success', completed: true, messages: [] },
|
||||
timestamp: new Date().toISOString()
|
||||
})
|
||||
|
||||
it('shows an error toast when a task fails', async () => {
|
||||
const store = useComfyManagerStore()
|
||||
setTaskHistory(store, { a: errorTask('a') })
|
||||
await nextTick()
|
||||
await vi.runAllTimersAsync()
|
||||
|
||||
expect(toastAddMock).toHaveBeenCalledTimes(1)
|
||||
const message = toastAddMock.mock.calls[0][0]
|
||||
expect(message.severity).toBe('error')
|
||||
expect(message.summary).toBe('manager.installFailureToast.summary')
|
||||
expect(message.detail).toBe('manager.installFailureToast.detail:1')
|
||||
})
|
||||
|
||||
it('does not show a toast when all tasks succeed', async () => {
|
||||
const store = useComfyManagerStore()
|
||||
setTaskHistory(store, { a: successTask('a'), b: successTask('b') })
|
||||
await nextTick()
|
||||
await vi.runAllTimersAsync()
|
||||
|
||||
expect(toastAddMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('coalesces multiple failures that land in quick succession', async () => {
|
||||
const store = useComfyManagerStore()
|
||||
setTaskHistory(store, { a: errorTask('a') })
|
||||
await nextTick()
|
||||
setTaskHistory(store, { a: errorTask('a'), b: errorTask('b') })
|
||||
await nextTick()
|
||||
await vi.runAllTimersAsync()
|
||||
|
||||
expect(toastAddMock).toHaveBeenCalledTimes(1)
|
||||
expect(toastAddMock.mock.calls[0][0].detail).toBe(
|
||||
'manager.installFailureToast.detail:2'
|
||||
)
|
||||
})
|
||||
|
||||
it('does not show a toast when failures are cleared via resetTaskState', async () => {
|
||||
const store = useComfyManagerStore()
|
||||
setTaskHistory(store, { a: errorTask('a') })
|
||||
await nextTick()
|
||||
await vi.runAllTimersAsync()
|
||||
toastAddMock.mockClear()
|
||||
|
||||
store.resetTaskState()
|
||||
await nextTick()
|
||||
await vi.runAllTimersAsync()
|
||||
|
||||
expect(toastAddMock).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useEventListener, whenever } from '@vueuse/core'
|
||||
import { useDebounceFn, useEventListener, whenever } from '@vueuse/core'
|
||||
import { defineStore } from 'pinia'
|
||||
import { v4 as uuidv4 } from 'uuid'
|
||||
import { ref, watch } from 'vue'
|
||||
@@ -6,6 +6,7 @@ import { ref, watch } from 'vue'
|
||||
import { t } from '@/i18n'
|
||||
import { useCachedRequest } from '@/composables/useCachedRequest'
|
||||
import { useServerLogs } from '@/composables/useServerLogs'
|
||||
import { useToastStore } from '@/platform/updates/common/toastStore'
|
||||
import { api } from '@/scripts/api'
|
||||
import { app } from '@/scripts/app'
|
||||
|
||||
@@ -112,6 +113,30 @@ export const useComfyManagerStore = defineStore('comfyManager', () => {
|
||||
{ deep: true }
|
||||
)
|
||||
|
||||
const pendingFailureCount = ref(0)
|
||||
|
||||
const flushFailureToast = useDebounceFn(() => {
|
||||
if (pendingFailureCount.value === 0) return
|
||||
const count = pendingFailureCount.value
|
||||
pendingFailureCount.value = 0
|
||||
useToastStore().add({
|
||||
severity: 'error',
|
||||
summary: t('manager.installFailureToast.summary'),
|
||||
detail: t('manager.installFailureToast.detail', { count }, count),
|
||||
life: 8000
|
||||
})
|
||||
}, 300)
|
||||
|
||||
watch(
|
||||
() => failedTasksIds.value.length,
|
||||
(newCount, oldCount) => {
|
||||
const delta = newCount - (oldCount ?? 0)
|
||||
if (delta <= 0) return
|
||||
pendingFailureCount.value += delta
|
||||
void flushFailureToast()
|
||||
}
|
||||
)
|
||||
|
||||
const getPackId = (pack: ManagerPackInstalled) => pack.cnr_id || pack.aux_id
|
||||
|
||||
const isInstalledPackId = (packName: string | undefined): boolean =>
|
||||
|
||||
Reference in New Issue
Block a user