Skip to content

feat: improve multi-package selection handling #5116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 5 commits into
base: manager/conflict-notification/restore
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@
</template>

<script setup lang="ts">
import { filter, flatMap, map, some } from 'lodash'
import { filter, flatMap, map, some } from 'es-toolkit/compat'
import Button from 'primevue/button'
import { computed, ref } from 'vue'
import { useI18n } from 'vue-i18n'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,13 @@ import { useComfyManagerStore } from '@/stores/comfyManagerStore'
import PackEnableToggle from './PackEnableToggle.vue'

// Mock debounce to execute immediately
vi.mock('es-toolkit/compat', () => ({
debounce: <T extends (...args: any[]) => any>(fn: T) => fn
}))
vi.mock('es-toolkit/compat', async () => {
const actual = await vi.importActual('es-toolkit/compat')
return {
...actual,
debounce: <T extends (...args: any[]) => any>(fn: T) => fn
}
})

const mockNodePack = {
id: 'test-pack',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import { inject, ref } from 'vue'

import PackActionButton from '@/components/dialog/content/manager/button/PackActionButton.vue'
import { useConflictDetection } from '@/composables/useConflictDetection'
import { t } from '@/i18n'
import { useDialogService } from '@/services/dialogService'
import { useComfyManagerStore } from '@/stores/comfyManagerStore'
Expand Down Expand Up @@ -75,15 +76,20 @@ const installAllPacks = async () => {
if (!nodePacks?.length) return

if (hasConflict && conflictInfo) {
const conflictedPackages: ConflictDetectionResult[] = nodePacks.map(
(pack) => ({
package_id: pack.id || '',
package_name: pack.name || '',
has_conflict: true,
conflicts: conflictInfo || [],
is_compatible: false
// Check each package individually for conflicts
const { checkNodeCompatibility } = useConflictDetection()
const conflictedPackages: ConflictDetectionResult[] = nodePacks
.map((pack) => {
const compatibilityCheck = checkNodeCompatibility(pack)
return {
package_id: pack.id || '',
package_name: pack.name || '',
has_conflict: compatibilityCheck.hasConflict,
conflicts: compatibilityCheck.conflicts,
is_compatible: !compatibilityCheck.hasConflict
}
})
)
.filter((result) => result.has_conflict) // Only show packages with conflicts

showNodeConflictDialog({
conflictedPackages,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,32 @@
</div>
</template>
<template #install-button>
<PackInstallButton :full-width="true" :node-packs="nodePacks" />
<!-- Mixed: Don't show any button -->
<div v-if="isMixed" class="text-sm text-neutral-500">
{{ $t('manager.mixedSelectionMessage') }}
</div>
<!-- All installed: Show uninstall button -->
<PackUninstallButton
v-else-if="isAllInstalled"
:full-width="true"
:node-packs="installedPacks"
/>
<!-- None installed: Show install button -->
<PackInstallButton
v-else-if="isNoneInstalled"
:full-width="true"
:node-packs="notInstalledPacks"
:has-conflict="hasConflicts"
:conflict-info="conflictInfo"
/>
</template>
</InfoPanelHeader>
<div class="mb-6">
<MetadataRow :label="$t('g.status')">
<PackStatusMessage status-type="NodeVersionStatusActive" />
<PackStatusMessage
:status-type="overallStatus"
:has-compatibility-issues="hasConflicts"
/>
</MetadataRow>
<MetadataRow
:label="$t('manager.totalNodes')"
Expand All @@ -35,22 +55,80 @@

<script setup lang="ts">
import { useAsyncState } from '@vueuse/core'
import { computed, onUnmounted } from 'vue'
import { computed, onUnmounted, provide, toRef } from 'vue'

import PackStatusMessage from '@/components/dialog/content/manager/PackStatusMessage.vue'
import PackInstallButton from '@/components/dialog/content/manager/button/PackInstallButton.vue'
import PackUninstallButton from '@/components/dialog/content/manager/button/PackUninstallButton.vue'
import InfoPanelHeader from '@/components/dialog/content/manager/infoPanel/InfoPanelHeader.vue'
import MetadataRow from '@/components/dialog/content/manager/infoPanel/MetadataRow.vue'
import PackIconStacked from '@/components/dialog/content/manager/packIcon/PackIconStacked.vue'
import { usePacksSelection } from '@/composables/nodePack/usePacksSelection'
import { usePacksStatus } from '@/composables/nodePack/usePacksStatus'
import { useConflictDetection } from '@/composables/useConflictDetection'
import { useComfyRegistryStore } from '@/stores/comfyRegistryStore'
import { components } from '@/types/comfyRegistryTypes'
import type { ConflictDetail } from '@/types/conflictDetectionTypes'
import { ImportFailedKey } from '@/types/importFailedTypes'

const { nodePacks } = defineProps<{
nodePacks: components['schemas']['Node'][]
}>()

const nodePacksRef = toRef(() => nodePacks)

// Use new composables for cleaner code
const {
installedPacks,
notInstalledPacks,
isAllInstalled,
isNoneInstalled,
isMixed
} = usePacksSelection(nodePacksRef)

const { hasImportFailed, overallStatus } = usePacksStatus(nodePacksRef)

const { checkNodeCompatibility } = useConflictDetection()
const { getNodeDefs } = useComfyRegistryStore()

// Provide import failed context for PackStatusMessage
provide(ImportFailedKey, {
importFailed: hasImportFailed,
showImportFailedDialog: () => {} // No-op for multi-selection
})

// Check for conflicts in not-installed packages - keep original logic but simplified
const packageConflicts = computed(() => {
const conflictsByPackage = new Map<string, ConflictDetail[]>()

for (const pack of notInstalledPacks.value) {
const compatibilityCheck = checkNodeCompatibility(pack)
if (compatibilityCheck.hasConflict && pack.id) {
conflictsByPackage.set(pack.id, compatibilityCheck.conflicts)
}
}

return conflictsByPackage
})

// Aggregate all unique conflicts for display
const conflictInfo = computed<ConflictDetail[]>(() => {
const conflictMap = new Map<string, ConflictDetail>()

packageConflicts.value.forEach((conflicts) => {
conflicts.forEach((conflict) => {
const key = `${conflict.type}-${conflict.current_value}-${conflict.required_value}`
if (!conflictMap.has(key)) {
conflictMap.set(key, conflict)
}
})
})

return Array.from(conflictMap.values())
})

const hasConflicts = computed(() => conflictInfo.value.length > 0)

const getPackNodes = async (pack: components['schemas']['Node']) => {
if (!pack.latest_version?.version) return []
const nodeDefs = await getNodeDefs.call({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<!-- blur background -->
<div
v-if="imgSrc"
class="absolute inset-0 bg-cover bg-center bg-no-repeat opacity-30"
class="absolute inset-0 bg-cover bg-center bg-no-repeat"
:style="{
backgroundImage: `url(${imgSrc})`,
filter: 'blur(10px)'
Expand Down
51 changes: 51 additions & 0 deletions src/composables/nodePack/usePacksSelection.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { type Ref, computed } from 'vue'

import { useComfyManagerStore } from '@/stores/comfyManagerStore'
import type { components } from '@/types/comfyRegistryTypes'

type NodePack = components['schemas']['Node']

export type SelectionState = 'all-installed' | 'none-installed' | 'mixed'

/**
* Composable for managing multi-package selection states
* Handles installation status tracking and selection state determination
*/
export function usePacksSelection(nodePacks: Ref<NodePack[]>) {
const managerStore = useComfyManagerStore()

const installedPacks = computed(() =>
nodePacks.value.filter((pack) => managerStore.isPackInstalled(pack.id))
)

const notInstalledPacks = computed(() =>
nodePacks.value.filter((pack) => !managerStore.isPackInstalled(pack.id))
)

const isAllInstalled = computed(
() => installedPacks.value.length === nodePacks.value.length
)

const isNoneInstalled = computed(
() => notInstalledPacks.value.length === nodePacks.value.length
)

const isMixed = computed(
() => installedPacks.value.length > 0 && notInstalledPacks.value.length > 0
)

const selectionState = computed<SelectionState>(() => {
if (isAllInstalled.value) return 'all-installed'
if (isNoneInstalled.value) return 'none-installed'
return 'mixed'
})

return {
installedPacks,
notInstalledPacks,
isAllInstalled,
isNoneInstalled,
isMixed,
selectionState
}
}
63 changes: 63 additions & 0 deletions src/composables/nodePack/usePacksStatus.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
import { type Ref, computed } from 'vue'

import { useConflictDetectionStore } from '@/stores/conflictDetectionStore'
import type { components } from '@/types/comfyRegistryTypes'

type NodePack = components['schemas']['Node']
type NodeStatus = components['schemas']['NodeStatus']
type NodeVersionStatus = components['schemas']['NodeVersionStatus']

const STATUS_PRIORITY = [
'NodeStatusBanned',
'NodeVersionStatusBanned',
'NodeStatusDeleted',
'NodeVersionStatusDeleted',
'NodeVersionStatusFlagged',
'NodeVersionStatusPending',
'NodeStatusActive',
'NodeVersionStatusActive'
] as const

/**
* Composable for managing package status with priority
* Handles import failures and determines the most important status
*/
export function usePacksStatus(nodePacks: Ref<NodePack[]>) {
const conflictDetectionStore = useConflictDetectionStore()

const hasImportFailed = computed(() => {
return nodePacks.value.some((pack) => {
if (!pack.id) return false
const conflicts = conflictDetectionStore.getConflictsForPackageByID(
pack.id
)
return (
conflicts?.conflicts?.some((c) => c.type === 'import_failed') || false
)
})
})

const overallStatus = computed<NodeStatus | NodeVersionStatus>(() => {
// Check for import failed first (highest priority for installed packages)
if (hasImportFailed.value) {
// Import failed doesn't have a specific status enum, so we return active
// but the PackStatusMessage will handle it via hasImportFailed prop
return 'NodeVersionStatusActive' as NodeVersionStatus
}

// Find the highest priority status from all packages
for (const priorityStatus of STATUS_PRIORITY) {
if (nodePacks.value.some((pack) => pack.status === priorityStatus)) {
return priorityStatus as NodeStatus | NodeVersionStatus
}
}

// Default to active if no specific status found
return 'NodeVersionStatusActive' as NodeVersionStatus
})

return {
hasImportFailed,
overallStatus
}
}
2 changes: 1 addition & 1 deletion src/composables/useConflictDetection.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { uniqBy } from 'lodash'
import { uniqBy } from 'es-toolkit/compat'
import { computed, getCurrentInstance, onUnmounted, readonly, ref } from 'vue'

import { useInstalledPacks } from '@/composables/nodePack/useInstalledPacks'
Expand Down
1 change: 1 addition & 0 deletions src/locales/en/main.json
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@
"installSelected": "Install Selected",
"installAllMissingNodes": "Install All Missing Nodes",
"packsSelected": "packs selected",
"mixedSelectionMessage": "Cannot perform bulk action on mixed selection",
"status": {
"active": "Active",
"pending": "Pending",
Expand Down
2 changes: 1 addition & 1 deletion src/stores/comfyManagerStore.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { whenever } from '@vueuse/core'
import { mapKeys } from 'lodash'
import { mapKeys } from 'es-toolkit/compat'
import { defineStore } from 'pinia'
import { ref, watch } from 'vue'
import { useI18n } from 'vue-i18n'
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { mount } from '@vue/test-utils'
import { createPinia, setActivePinia } from 'pinia'
import PrimeVue from 'primevue/config'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { nextTick } from 'vue'
Expand Down Expand Up @@ -52,6 +53,9 @@ vi.mock('@/stores/workspace/colorPaletteStore', () => ({

// Helper function to mount component with required setup
const mountComponent = (options: { captureError?: boolean } = {}) => {
const pinia = createPinia()
setActivePinia(pinia)

const i18n = createI18n({
legacy: false,
locale: 'en',
Expand All @@ -62,7 +66,7 @@ const mountComponent = (options: { captureError?: boolean } = {}) => {

const config: any = {
global: {
plugins: [PrimeVue, i18n],
plugins: [pinia, PrimeVue, i18n],
mocks: {
$t: (key: string) => key // Mock i18n translation
}
Expand Down Expand Up @@ -158,6 +162,10 @@ describe('ManagerProgressFooter', () => {

beforeEach(() => {
vi.clearAllMocks()
// Create new pinia instance for each test
const pinia = createPinia()
setActivePinia(pinia)

// Reset task logs
mockTaskLogs.length = 0
mockComfyManagerStore.taskLogs = mockTaskLogs
Expand Down
Loading
Loading