mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-21 07:14:11 +00:00
fix: import fail info warning icon (#7753)
# Fix Import Failed Warning Icon ## Problem Description Warning icons were not displayed when import failed errors occurred in installed packages. ## Root Cause Conflict detection logic mismatch between `PackCardFooter` and `PackEnableToggle`: - **PackCardFooter**: Uses `checkNodeCompatibility()` - System compatibility check **before** installation (OS, accelerator, version, etc.) - Does not include import failed information - **PackEnableToggle**: Uses `getConflictsForPackageByID()` - Actual conflict data **after** installation (including import failed) - But was dependent on parent component's `hasConflict` prop ## Changes Made ### 1. PackEnableToggle.vue ```diff - <div v-if="hasConflict"> + <div v-if="packageConflict?.has_conflict"> ``` - Removed `hasConflict` prop dependency - Changed to use only internal store data (`packageConflict`) ### 2. PackCardFooter.vue ```diff - <PackEnableToggle :has-conflict="hasConflicts" :node-pack="nodePack" /> + <PackEnableToggle :node-pack="nodePack" /> ``` - Removed unnecessary `has-conflict` prop passing ## Result - ✅ Warning icon properly displays for installed packages with import failed errors - ✅ Conflict modal works correctly when clicked - ✅ Each component uses appropriate conflict detection logic [after.webm](https://github.com/user-attachments/assets/80576018-0a5b-4e32-9df6-686be3774313) ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7753-fix-import-fail-info-warning-icon-2d36d73d365081518fbeedf539a19040) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: github-actions <github-actions@github.com>
This commit is contained in:
Binary file not shown.
|
Before Width: | Height: | Size: 62 KiB After Width: | Height: | Size: 62 KiB |
@@ -1,5 +1,5 @@
|
||||
<template>
|
||||
<div class="flex w-[552px] flex-col">
|
||||
<div class="flex w-138 flex-col">
|
||||
<ContentDivider :width="1" />
|
||||
<div class="flex h-full w-full flex-col gap-2 px-4 py-6">
|
||||
<!-- Description -->
|
||||
@@ -37,7 +37,7 @@
|
||||
: 'pi pi-chevron-right text-xs'
|
||||
"
|
||||
text
|
||||
class="!bg-transparent text-muted"
|
||||
class="bg-transparent text-muted"
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
@@ -45,12 +45,12 @@
|
||||
<div
|
||||
v-if="importFailedExpanded"
|
||||
data-testid="conflict-dialog-panel-expanded"
|
||||
class="flex max-h-[142px] scrollbar-hide flex-col gap-2.5 overflow-y-auto px-4 py-2"
|
||||
class="flex max-h-35.5 scrollbar-hide flex-col gap-2.5 overflow-y-auto px-4 py-2"
|
||||
>
|
||||
<div
|
||||
v-for="(packageName, i) in importFailedConflicts"
|
||||
:key="i"
|
||||
class="conflict-list-item flex h-6 flex-shrink-0 items-center justify-between px-4"
|
||||
class="conflict-list-item flex h-6 shrink-0 items-center justify-between px-4"
|
||||
>
|
||||
<span class="text-xs text-muted">
|
||||
{{ packageName }}
|
||||
@@ -60,7 +60,10 @@
|
||||
</div>
|
||||
</div>
|
||||
<!-- Conflict List Wrapper -->
|
||||
<div class="flex min-h-8 w-full flex-col rounded-lg bg-base-background">
|
||||
<div
|
||||
v-if="allConflictDetails.length > 0"
|
||||
class="flex min-h-8 w-full flex-col rounded-lg bg-base-background"
|
||||
>
|
||||
<div
|
||||
data-testid="conflict-dialog-panel-toggle"
|
||||
class="flex h-8 w-full items-center justify-between gap-2 pl-4"
|
||||
@@ -82,7 +85,7 @@
|
||||
: 'pi pi-chevron-right text-xs'
|
||||
"
|
||||
text
|
||||
class="!bg-transparent text-muted"
|
||||
class="bg-transparent text-muted"
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
@@ -95,7 +98,7 @@
|
||||
<div
|
||||
v-for="(conflict, i) in allConflictDetails"
|
||||
:key="i"
|
||||
class="conflict-list-item flex h-6 flex-shrink-0 items-center justify-between px-4"
|
||||
class="conflict-list-item flex h-6 shrink-0 items-center justify-between px-4"
|
||||
>
|
||||
<span class="text-xs text-muted">{{
|
||||
getConflictMessage(conflict, t)
|
||||
@@ -105,7 +108,10 @@
|
||||
</div>
|
||||
</div>
|
||||
<!-- Extension List Wrapper -->
|
||||
<div class="flex min-h-8 w-full flex-col rounded-lg bg-base-background">
|
||||
<div
|
||||
v-if="conflictData.length > 0"
|
||||
class="flex min-h-8 w-full flex-col rounded-lg bg-base-background"
|
||||
>
|
||||
<div
|
||||
data-testid="conflict-dialog-panel-toggle"
|
||||
class="flex h-8 w-full items-center justify-between gap-2 pl-4"
|
||||
@@ -127,7 +133,7 @@
|
||||
: 'pi pi-chevron-right text-xs'
|
||||
"
|
||||
text
|
||||
class="!bg-transparent text-muted"
|
||||
class="bg-transparent text-muted"
|
||||
/>
|
||||
</div>
|
||||
</div>
|
||||
@@ -135,12 +141,12 @@
|
||||
<div
|
||||
v-if="extensionsExpanded"
|
||||
data-testid="conflict-dialog-panel-expanded"
|
||||
class="flex max-h-[142px] scrollbar-hide flex-col gap-2.5 overflow-y-auto px-4 py-2"
|
||||
class="flex max-h-35.5 scrollbar-hide flex-col gap-2.5 overflow-y-auto px-4 py-2"
|
||||
>
|
||||
<div
|
||||
v-for="conflictResult in conflictData"
|
||||
:key="conflictResult.package_id"
|
||||
class="conflict-list-item flex h-6 flex-shrink-0 items-center justify-between px-4"
|
||||
class="conflict-list-item flex h-6 shrink-0 items-center justify-between px-4"
|
||||
>
|
||||
<span class="text-xs text-muted">
|
||||
{{ conflictResult.package_name }}
|
||||
|
||||
@@ -33,6 +33,8 @@ const mockNodePack = {
|
||||
const mockIsPackEnabled = vi.fn()
|
||||
const mockEnablePack = vi.fn().mockResolvedValue(undefined)
|
||||
const mockDisablePack = vi.fn().mockResolvedValue(undefined)
|
||||
const mockGetConflictsForPackageByID = vi.fn()
|
||||
|
||||
vi.mock('@/workbench/extensions/manager/stores/comfyManagerStore', () => ({
|
||||
useComfyManagerStore: vi.fn(() => ({
|
||||
isPackEnabled: mockIsPackEnabled,
|
||||
@@ -42,6 +44,12 @@ vi.mock('@/workbench/extensions/manager/stores/comfyManagerStore', () => ({
|
||||
}))
|
||||
}))
|
||||
|
||||
vi.mock('@/workbench/extensions/manager/stores/conflictDetectionStore', () => ({
|
||||
useConflictDetectionStore: vi.fn(() => ({
|
||||
getConflictsForPackageByID: mockGetConflictsForPackageByID
|
||||
}))
|
||||
}))
|
||||
|
||||
describe('PackEnableToggle', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
@@ -163,4 +171,41 @@ describe('PackEnableToggle', () => {
|
||||
await nextTick()
|
||||
expect(wrapper.findComponent(ToggleSwitch).props('disabled')).toBe(false)
|
||||
})
|
||||
|
||||
describe('conflict warning icon', () => {
|
||||
it('should show warning icon when package has conflicts', () => {
|
||||
mockGetConflictsForPackageByID.mockReturnValue({
|
||||
package_id: 'test-pack',
|
||||
package_name: 'Test Pack',
|
||||
has_conflict: true,
|
||||
conflicts: [
|
||||
{
|
||||
type: 'import_failed',
|
||||
current_value: 'installed',
|
||||
required_value: 'error message'
|
||||
}
|
||||
],
|
||||
is_compatible: false
|
||||
})
|
||||
|
||||
mockIsPackEnabled.mockReturnValue(true)
|
||||
const wrapper = mountComponent()
|
||||
|
||||
// Check if warning icon exists
|
||||
const warningIcon = wrapper.find('.pi-exclamation-triangle')
|
||||
expect(warningIcon.exists()).toBe(true)
|
||||
expect(warningIcon.classes()).toContain('text-yellow-500')
|
||||
})
|
||||
|
||||
it('should not show warning icon when package has no conflicts', () => {
|
||||
mockGetConflictsForPackageByID.mockReturnValue(null)
|
||||
|
||||
mockIsPackEnabled.mockReturnValue(true)
|
||||
const wrapper = mountComponent()
|
||||
|
||||
// Check if warning icon does not exist
|
||||
const warningIcon = wrapper.find('.pi-exclamation-triangle')
|
||||
expect(warningIcon.exists()).toBe(false)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
<template>
|
||||
<div class="flex items-center gap-2">
|
||||
<div
|
||||
v-if="hasConflict"
|
||||
v-if="packageConflict?.has_conflict"
|
||||
v-tooltip="{
|
||||
value: $t('manager.conflicts.warningTooltip'),
|
||||
showDelay: 300
|
||||
@@ -43,9 +43,8 @@ import type { components as ManagerComponents } from '@/workbench/extensions/man
|
||||
|
||||
const TOGGLE_DEBOUNCE_MS = 256
|
||||
|
||||
const { nodePack, hasConflict } = defineProps<{
|
||||
const { nodePack } = defineProps<{
|
||||
nodePack: components['schemas']['Node']
|
||||
hasConflict?: boolean
|
||||
}>()
|
||||
|
||||
const { t } = useI18n()
|
||||
@@ -68,14 +67,14 @@ const version = computed(() => {
|
||||
)
|
||||
})
|
||||
|
||||
const packageConflict = computed(() =>
|
||||
getConflictsForPackageByID(nodePack.id || '')
|
||||
)
|
||||
const packageConflict = computed(() => {
|
||||
if (!nodePack.id) return undefined
|
||||
return getConflictsForPackageByID(nodePack.id)
|
||||
})
|
||||
const canToggleDirectly = computed(() => {
|
||||
return !(
|
||||
hasConflict &&
|
||||
!acknowledgmentState.value.modal_dismissed &&
|
||||
packageConflict.value
|
||||
packageConflict.value?.has_conflict &&
|
||||
!acknowledgmentState.value.modal_dismissed
|
||||
)
|
||||
})
|
||||
|
||||
|
||||
@@ -0,0 +1,177 @@
|
||||
import type { VueWrapper } from '@vue/test-utils'
|
||||
import { mount } from '@vue/test-utils'
|
||||
import { createPinia } from 'pinia'
|
||||
import PrimeVue from 'primevue/config'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { ref } from 'vue'
|
||||
import { createI18n } from 'vue-i18n'
|
||||
|
||||
import enMessages from '@/locales/en/main.json' with { type: 'json' }
|
||||
import { IsInstallingKey } from '@/workbench/extensions/manager/types/comfyManagerTypes'
|
||||
|
||||
import PackCardFooter from './PackCardFooter.vue'
|
||||
|
||||
// Mock the child components
|
||||
vi.mock(
|
||||
'@/workbench/extensions/manager/components/manager/button/PackInstallButton.vue',
|
||||
() => ({
|
||||
default: { template: '<div data-testid="pack-install-button"></div>' }
|
||||
})
|
||||
)
|
||||
|
||||
vi.mock(
|
||||
'@/workbench/extensions/manager/components/manager/button/PackEnableToggle.vue',
|
||||
() => ({
|
||||
default: { template: '<div data-testid="pack-enable-toggle"></div>' }
|
||||
})
|
||||
)
|
||||
|
||||
// Mock composables
|
||||
const mockIsPackInstalled = vi.fn()
|
||||
const mockCheckNodeCompatibility = vi.fn()
|
||||
|
||||
vi.mock('@/workbench/extensions/manager/stores/comfyManagerStore', () => ({
|
||||
useComfyManagerStore: vi.fn(() => ({
|
||||
isPackInstalled: mockIsPackInstalled
|
||||
}))
|
||||
}))
|
||||
|
||||
vi.mock(
|
||||
'@/workbench/extensions/manager/composables/useConflictDetection',
|
||||
() => ({
|
||||
useConflictDetection: vi.fn(() => ({
|
||||
checkNodeCompatibility: mockCheckNodeCompatibility
|
||||
}))
|
||||
})
|
||||
)
|
||||
|
||||
// Remove the mock for injection key since we're importing it directly
|
||||
|
||||
const mockNodePack = {
|
||||
id: 'test-pack',
|
||||
name: 'Test Pack',
|
||||
downloads: 1000
|
||||
}
|
||||
|
||||
describe('PackCardFooter', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
mockIsPackInstalled.mockReset()
|
||||
mockCheckNodeCompatibility.mockReset()
|
||||
})
|
||||
|
||||
const mountComponent = (props = {}): VueWrapper => {
|
||||
const i18n = createI18n({
|
||||
legacy: false,
|
||||
locale: 'en',
|
||||
messages: { en: enMessages }
|
||||
})
|
||||
|
||||
return mount(PackCardFooter, {
|
||||
props: {
|
||||
nodePack: mockNodePack,
|
||||
...props
|
||||
},
|
||||
global: {
|
||||
plugins: [PrimeVue, createPinia(), i18n],
|
||||
provide: {
|
||||
[IsInstallingKey]: ref(false)
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
describe('component rendering', () => {
|
||||
it('shows download count when available', () => {
|
||||
mockIsPackInstalled.mockReturnValue(false)
|
||||
mockCheckNodeCompatibility.mockReturnValue({
|
||||
hasConflict: false,
|
||||
conflicts: []
|
||||
})
|
||||
const wrapper = mountComponent()
|
||||
|
||||
expect(wrapper.text()).toContain('1,000')
|
||||
})
|
||||
|
||||
it('shows install button for uninstalled packages', () => {
|
||||
mockIsPackInstalled.mockReturnValue(false)
|
||||
mockCheckNodeCompatibility.mockReturnValue({
|
||||
hasConflict: false,
|
||||
conflicts: []
|
||||
})
|
||||
|
||||
const wrapper = mountComponent()
|
||||
|
||||
expect(wrapper.find('[data-testid="pack-install-button"]').exists()).toBe(
|
||||
true
|
||||
)
|
||||
expect(wrapper.find('[data-testid="pack-enable-toggle"]').exists()).toBe(
|
||||
false
|
||||
)
|
||||
})
|
||||
|
||||
it('shows enable toggle for installed packages', () => {
|
||||
mockIsPackInstalled.mockReturnValue(true)
|
||||
|
||||
const wrapper = mountComponent()
|
||||
|
||||
expect(wrapper.find('[data-testid="pack-enable-toggle"]').exists()).toBe(
|
||||
true
|
||||
)
|
||||
expect(wrapper.find('[data-testid="pack-install-button"]').exists()).toBe(
|
||||
false
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
describe('conflict detection for uninstalled packages', () => {
|
||||
it('passes conflict info to install button when conflicts exist', () => {
|
||||
mockIsPackInstalled.mockReturnValue(false)
|
||||
mockCheckNodeCompatibility.mockReturnValue({
|
||||
hasConflict: true,
|
||||
conflicts: [
|
||||
{
|
||||
type: 'os_conflict',
|
||||
current_value: 'windows',
|
||||
required_value: 'linux'
|
||||
}
|
||||
]
|
||||
})
|
||||
|
||||
const wrapper = mountComponent()
|
||||
|
||||
const installButton = wrapper.find('[data-testid="pack-install-button"]')
|
||||
expect(installButton.exists()).toBe(true)
|
||||
// The install button should receive has-conflict prop as true
|
||||
expect(installButton.attributes()).toHaveProperty('has-conflict')
|
||||
})
|
||||
|
||||
it('does not pass conflict info when no conflicts exist', () => {
|
||||
mockIsPackInstalled.mockReturnValue(false)
|
||||
mockCheckNodeCompatibility.mockReturnValue({
|
||||
hasConflict: false,
|
||||
conflicts: []
|
||||
})
|
||||
|
||||
const wrapper = mountComponent()
|
||||
|
||||
const installButton = wrapper.find('[data-testid="pack-install-button"]')
|
||||
expect(installButton.exists()).toBe(true)
|
||||
// The install button should receive has-conflict prop as false
|
||||
expect(installButton.attributes()['has-conflict']).toBe('false')
|
||||
})
|
||||
})
|
||||
|
||||
describe('installed packages', () => {
|
||||
it('does not pass has-conflict prop to enable toggle', () => {
|
||||
mockIsPackInstalled.mockReturnValue(true)
|
||||
|
||||
const wrapper = mountComponent()
|
||||
|
||||
const enableToggle = wrapper.find('[data-testid="pack-enable-toggle"]')
|
||||
expect(enableToggle.exists()).toBe(true)
|
||||
// The enable toggle should not receive has-conflict prop (removed in our fix)
|
||||
expect(enableToggle.attributes()).not.toHaveProperty('has-conflict')
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -13,11 +13,7 @@
|
||||
:has-conflict="hasConflicts"
|
||||
:conflict-info="conflictInfo"
|
||||
/>
|
||||
<PackEnableToggle
|
||||
v-else
|
||||
:has-conflict="hasConflicts"
|
||||
:node-pack="nodePack"
|
||||
/>
|
||||
<PackEnableToggle v-else :node-pack="nodePack" />
|
||||
</div>
|
||||
</template>
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@ import { useSystemStatsStore } from '@/stores/systemStatsStore'
|
||||
import type { components } from '@/types/comfyRegistryTypes'
|
||||
import { useInstalledPacks } from '@/workbench/extensions/manager/composables/nodePack/useInstalledPacks'
|
||||
import { useConflictAcknowledgment } from '@/workbench/extensions/manager/composables/useConflictAcknowledgment'
|
||||
import { useManagerState } from '@/workbench/extensions/manager/composables/useManagerState'
|
||||
import { useComfyManagerService } from '@/workbench/extensions/manager/services/comfyManagerService'
|
||||
import { useComfyManagerStore } from '@/workbench/extensions/manager/stores/comfyManagerStore'
|
||||
import { useConflictDetectionStore } from '@/workbench/extensions/manager/stores/conflictDetectionStore'
|
||||
@@ -87,9 +88,11 @@ export function useConflictDetection() {
|
||||
try {
|
||||
// Get system stats from store (primary source of system information)
|
||||
// Wait for systemStats to be initialized if not already
|
||||
const { systemStats, isInitialized: systemStatsInitialized } =
|
||||
useSystemStatsStore()
|
||||
await until(systemStatsInitialized)
|
||||
const systemStatsStore = useSystemStatsStore()
|
||||
const { systemStats } = systemStatsStore
|
||||
|
||||
// Wait for initialization using the store's isInitialized property (correct reactive way)
|
||||
await until(() => systemStatsStore.isInitialized).toBe(true)
|
||||
|
||||
const frontendVersion = getFrontendVersion()
|
||||
|
||||
@@ -548,9 +551,11 @@ export function useConflictDetection() {
|
||||
*/
|
||||
async function initializeConflictDetection(): Promise<void> {
|
||||
try {
|
||||
// Check if manager is new Manager before proceeding
|
||||
const { useManagerState } =
|
||||
await import('@/workbench/extensions/manager/composables/useManagerState')
|
||||
// First, wait for systemStats to be initialized
|
||||
const systemStatsStore = useSystemStatsStore()
|
||||
await until(() => systemStatsStore.isInitialized).toBe(true)
|
||||
|
||||
// Now check if manager is new Manager
|
||||
const managerState = useManagerState()
|
||||
|
||||
if (!managerState.isNewManagerUI.value) {
|
||||
|
||||
@@ -137,9 +137,9 @@ describe('NodeConflictDialogContent', () => {
|
||||
|
||||
const wrapper = createWrapper()
|
||||
|
||||
expect(wrapper.text()).toContain('0')
|
||||
expect(wrapper.text()).toContain('Conflicts')
|
||||
expect(wrapper.text()).toContain('Extensions at Risk')
|
||||
// When there are no conflicts, the conflict sections should not be rendered
|
||||
expect(wrapper.text()).not.toContain('Conflicts')
|
||||
expect(wrapper.text()).not.toContain('Extensions at Risk')
|
||||
expect(wrapper.find('[class*="Import Failed Extensions"]').exists()).toBe(
|
||||
false
|
||||
)
|
||||
@@ -364,9 +364,9 @@ describe('NodeConflictDialogContent', () => {
|
||||
mockConflictData.value = []
|
||||
const wrapper = createWrapper()
|
||||
|
||||
expect(wrapper.text()).toContain('0')
|
||||
expect(wrapper.text()).toContain('Conflicts')
|
||||
expect(wrapper.text()).toContain('Extensions at Risk')
|
||||
// When there are no conflicts, none of the sections should be visible
|
||||
expect(wrapper.text()).not.toContain('Conflicts')
|
||||
expect(wrapper.text()).not.toContain('Extensions at Risk')
|
||||
// Import failed section should not be visible when there are no import failures
|
||||
expect(wrapper.text()).not.toContain('Import Failed Extensions')
|
||||
})
|
||||
|
||||
@@ -14,6 +14,17 @@ import { useConflictDetectionStore } from '@/workbench/extensions/manager/stores
|
||||
import type { ConflictDetectionResult } from '@/workbench/extensions/manager/types/conflictDetectionTypes'
|
||||
import { checkVersionCompatibility } from '@/workbench/extensions/manager/utils/versionUtil'
|
||||
|
||||
// Mock @vueuse/core until function
|
||||
vi.mock('@vueuse/core', async () => {
|
||||
const actual = await vi.importActual('@vueuse/core')
|
||||
return {
|
||||
...actual,
|
||||
until: vi.fn(() => ({
|
||||
toBe: vi.fn(() => Promise.resolve())
|
||||
}))
|
||||
}
|
||||
})
|
||||
|
||||
// Mock dependencies
|
||||
vi.mock('@/workbench/extensions/manager/services/comfyManagerService', () => ({
|
||||
useComfyManagerService: vi.fn()
|
||||
@@ -159,6 +170,7 @@ describe('useConflictDetection', () => {
|
||||
clearConflicts: vi.fn()
|
||||
} as unknown as ReturnType<typeof useConflictDetectionStore>
|
||||
|
||||
const mockIsInitialized = ref(true)
|
||||
const mockSystemStatsStore = {
|
||||
systemStats: {
|
||||
system: {
|
||||
@@ -171,7 +183,7 @@ describe('useConflictDetection', () => {
|
||||
'3.11.0 (main, Oct 13 2023, 09:34:16) [Clang 15.0.0 (clang-1500.0.40.1)]',
|
||||
pytorch_version: '2.1.0',
|
||||
embedded_python: false,
|
||||
argv: []
|
||||
argv: ['--enable-manager']
|
||||
},
|
||||
devices: [
|
||||
{
|
||||
@@ -185,7 +197,7 @@ describe('useConflictDetection', () => {
|
||||
}
|
||||
]
|
||||
},
|
||||
isInitialized: ref(true),
|
||||
isInitialized: mockIsInitialized,
|
||||
$state: {} as never,
|
||||
$patch: vi.fn(),
|
||||
$reset: vi.fn(),
|
||||
|
||||
Reference in New Issue
Block a user