Refactor/code-reivew (#7893)

## Summary

<!-- One sentence describing what changed and why. -->
https://github.com/Comfy-Org/ComfyUI_frontend/pull/7871
https://github.com/Comfy-Org/ComfyUI_frontend/pull/7858
I refactored the code based on the reviews I received on those two PRs.

## Changes

- **What**: 

1. Updated IconGroup to address the backgroundClass handling.
2. Replaced text-gold-600 with a semantic color token.
3. Replaced PrimeVue Icon with a lucide icon.

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-7893-Refactor-code-reivew-2e26d73d365081e68a44e89ed1163062)
by [Unito](https://www.unito.io)

---------

Co-authored-by: Alexander Brown <drjkl@comfy.org>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: GitHub Action <action@github.com>

src/components/button/IconGroup.vue

@@ -2,8 +2,8 @@
   <div
     :class="
       cn(
-        'flex justify-center items-center shrink-0 outline-hidden border-none p-0 rounded-lg shadow-sm transition-all duration-200 cursor-pointer',
-        backgroundClass || 'bg-secondary-background'
+        'flex justify-center items-center shrink-0 outline-hidden border-none p-0 rounded-lg shadow-sm transition-all duration-200 cursor-pointer bg-secondary-background',
+        backgroundClass
       )
     "
   >

src/components/dialog/content/MissingNodesHeader.vue

@@ -1,7 +1,7 @@
 <template>
   <div class="flex w-full items-center justify-between p-4">
     <div class="flex items-center gap-2">
-      <i class="icon-[lucide--triangle-alert] text-gold-600"></i>
+      <i class="icon-[lucide--triangle-alert] text-warning-background"></i>
       <p class="m-0 text-sm">
         {{
           isCloud

src/components/topbar/TopbarBadge.test.ts

@@ -161,7 +161,7 @@ describe('TopbarBadge', () => {
       )
 
       expect(wrapper.find('.bg-gold-600').exists()).toBe(true)
-      expect(wrapper.find('.text-gold-600').exists()).toBe(true)
+      expect(wrapper.find('.text-warning-background').exists()).toBe(true)
     })
 
     it('uses default error icon for error variant', () => {
@@ -185,7 +185,9 @@ describe('TopbarBadge', () => {
         'full'
       )
 
-      expect(wrapper.find('.pi-exclamation-triangle').exists()).toBe(true)
+      expect(wrapper.find('.icon-\\[lucide--triangle-alert\\]').exists()).toBe(
+        true
+      )
     })
   })
 

src/components/topbar/TopbarBadge.vue

@@ -174,7 +174,7 @@ const textClasses = computed(() => {
     case 'error':
       return 'text-danger-100'
     case 'warning':
-      return 'text-gold-600'
+      return 'text-warning-background'
     case 'info':
     default:
       return 'text-text-primary'
@@ -191,7 +191,7 @@ const iconClass = computed(() => {
     case 'error':
       return 'pi pi-exclamation-circle'
     case 'warning':
-      return 'pi pi-exclamation-triangle'
+      return 'icon-[lucide--triangle-alert]'
     case 'info':
     default:
       return undefined

src/workbench/extensions/manager/components/manager/ImportFailedNodeHeader.vue

@@ -1,7 +1,7 @@
 <template>
   <div class="flex w-full items-center justify-between p-4">
     <div class="flex items-center gap-2">
-      <i class="icon-[lucide--triangle-alert] text-gold-600"></i>
+      <i class="icon-[lucide--triangle-alert] text-warning-background" />
       <p class="m-0 text-sm">
         {{ $t('importFailed.title') }}
       </p>

src/workbench/extensions/manager/components/manager/ManagerDialogContent.vue

@@ -38,7 +38,9 @@
             v-if="shouldShowManagerBanner"
             class="relative mt-3 mb-4 flex items-center gap-6 rounded-lg bg-yellow-500/20 p-4"
           >
-            <i class="pi pi-exclamation-triangle text-lg text-yellow-600"></i>
+            <i
+              class="icon-[lucide--triangle-alert] text-lg text-warning-background"
+            />
             <div class="flex flex-1 flex-col gap-2">
               <p class="m-0 text-sm font-bold">
                 {{ $t('manager.conflicts.warningBanner.title') }}

src/workbench/extensions/manager/components/manager/NodeConflictHeader.vue

@@ -2,7 +2,7 @@
   <div class="flex h-12 w-full items-center justify-between pl-6">
     <div class="flex items-center gap-2">
       <!-- Warning Icon -->
-      <i class="icon-[lucide--triangle-alert] text-gold-600"></i>
+      <i class="icon-[lucide--triangle-alert] text-warning-background" />
       <!-- Title -->
       <p class="text-base font-bold">
         {{ $t('manager.conflicts.title') }}

src/workbench/extensions/manager/components/manager/PackVersionSelectorPopover.test.ts

@@ -411,7 +411,7 @@ describe('PackVersionSelectorPopover', () => {
       expect(mockCheckNodeCompatibility).toHaveBeenCalled()
 
       // The warning icon should be shown for incompatible versions
-      const warningIcons = wrapper.findAll('.pi-exclamation-triangle')
+      const warningIcons = wrapper.findAll('.icon-\\[lucide--triangle-alert\\]')
       expect(warningIcons.length).toBeGreaterThan(0)
     })
 
@@ -536,7 +536,7 @@ describe('PackVersionSelectorPopover', () => {
       expect(mockCheckNodeCompatibility).toHaveBeenCalled()
 
       // The warning icon should be shown for version incompatible packages
-      const warningIcons = wrapper.findAll('.pi-exclamation-triangle')
+      const warningIcons = wrapper.findAll('.icon-\\[lucide--triangle-alert\\]')
       expect(warningIcons.length).toBeGreaterThan(0)
     })
 
@@ -662,7 +662,7 @@ describe('PackVersionSelectorPopover', () => {
       await wrapper.vm.$nextTick()
 
       // The warning icon should be shown for banned packages in the dropdown options
-      const warningIcons = wrapper.findAll('.pi-exclamation-triangle')
+      const warningIcons = wrapper.findAll('.icon-\\[lucide--triangle-alert\\]')
       expect(warningIcons.length).toBeGreaterThan(0)
     })
 
@@ -705,7 +705,7 @@ describe('PackVersionSelectorPopover', () => {
       expect(mockCheckNodeCompatibility).toHaveBeenCalled()
 
       // The warning icon should be shown for security pending packages
-      const warningIcons = wrapper.findAll('.pi-exclamation-triangle')
+      const warningIcons = wrapper.findAll('.icon-\\[lucide--triangle-alert\\]')
       expect(warningIcons.length).toBeGreaterThan(0)
     })
   })

src/workbench/extensions/manager/components/manager/PackVersionSelectorPopover.vue

@@ -45,7 +45,7 @@
                   value: slotProps.option.conflictMessage,
                   showDelay: 300
                 }"
-                class="pi pi-exclamation-triangle text-yellow-500"
+                class="icon-[lucide--triangle-alert] text-warning-background"
               />
               <VerifiedIcon v-else :size="20" class="relative right-0.5" />
             </template>

src/workbench/extensions/manager/components/manager/button/PackEnableToggle.test.ts

@@ -192,9 +192,9 @@ describe('PackEnableToggle', () => {
       const wrapper = mountComponent()
 
       // Check if warning icon exists
-      const warningIcon = wrapper.find('.pi-exclamation-triangle')
+      const warningIcon = wrapper.find('.icon-\\[lucide--triangle-alert\\]')
       expect(warningIcon.exists()).toBe(true)
-      expect(warningIcon.classes()).toContain('text-yellow-500')
+      expect(warningIcon.classes()).toContain('text-warning-background')
     })
 
     it('should not show warning icon when package has no conflicts', () => {
@@ -204,7 +204,7 @@ describe('PackEnableToggle', () => {
       const wrapper = mountComponent()
 
       // Check if warning icon does not exist
-      const warningIcon = wrapper.find('.pi-exclamation-triangle')
+      const warningIcon = wrapper.find('.icon-\\[lucide--triangle-alert\\]')
       expect(warningIcon.exists()).toBe(false)
     })
   })

src/workbench/extensions/manager/components/manager/button/PackEnableToggle.vue

@@ -9,7 +9,9 @@
       class="flex h-6 w-6 cursor-pointer items-center justify-center"
       @click="showConflictModal(true)"
     >
-      <i class="pi pi-exclamation-triangle text-xl text-yellow-500"></i>
+      <i
+        class="icon-[lucide--triangle-alert] text-xl text-warning-background"
+      />
     </div>
     <ToggleSwitch
       v-if="!canToggleDirectly"
@@ -37,12 +39,11 @@ import { useI18n } from 'vue-i18n'
 import { useDialogService } from '@/services/dialogService'
 import type { components } from '@/types/comfyRegistryTypes'
 import { useConflictAcknowledgment } from '@/workbench/extensions/manager/composables/useConflictAcknowledgment'
+import { useImportFailedDetection } from '@/workbench/extensions/manager/composables/useImportFailedDetection'
 import { useComfyManagerStore } from '@/workbench/extensions/manager/stores/comfyManagerStore'
 import { useConflictDetectionStore } from '@/workbench/extensions/manager/stores/conflictDetectionStore'
 import type { components as ManagerComponents } from '@/workbench/extensions/manager/types/generatedManagerTypes'
 
-import { useImportFailedDetection } from '../../../composables/useImportFailedDetection'
-
 const TOGGLE_DEBOUNCE_MS = 256
 
 const { nodePack } = defineProps<{

src/workbench/extensions/manager/components/manager/button/PackInstallButton.vue

@@ -7,7 +7,7 @@
   >
     <i
       v-if="hasConflict && !isInstalling && !isLoading"
-      class="pi pi-exclamation-triangle text-yellow-500"
+      class="icon-[lucide--triangle-alert] text-warning-background"
     />
     <DotSpinner
       v-else-if="isLoading || isInstalling"

src/workbench/extensions/manager/composables/useConflictDetection.ts

@@ -401,36 +401,29 @@ export function useConflictDetection() {
 
     // Process import failures
     for (const [packageId, failureInfo] of Object.entries(importFailInfo)) {
-      if (failureInfo && typeof failureInfo === 'object') {
-        // Extract error information from Manager API response
-        const errorMsg = failureInfo.error || 'Unknown import error'
-        const traceback = failureInfo.traceback || ''
+      if (!failureInfo || typeof failureInfo !== 'object') continue
 
-        // Combine error and traceback for display
-        const fullErrorInfo = traceback || errorMsg
+      const errorMsg = failureInfo.error || 'Unknown import error'
+      const fullErrorInfo = failureInfo.traceback || errorMsg
 
-        results.push({
-          package_id: packageId,
-          package_name: packageId,
-          has_conflict: true,
-          conflicts: [
-            {
-              type: 'import_failed',
-              current_value: errorMsg,
-              required_value: fullErrorInfo
-            }
-          ],
-          is_compatible: false
-        })
-
-        console.warn(
-          `[ConflictDetection] Python import failure detected for ${packageId}:`,
+      results.push({
+        package_id: packageId,
+        package_name: packageId,
+        has_conflict: true,
+        conflicts: [
           {
-            error: errorMsg,
-            hasTraceback: !!traceback
+            type: 'import_failed',
+            current_value: errorMsg,
+            required_value: fullErrorInfo
           }
-        )
-      }
+        ],
+        is_compatible: false
+      })
+
+      console.warn(
+        `[ConflictDetection] Python import failure detected for ${packageId}:`,
+        errorMsg
+      )
     }
 
     return results