From eea24166e08da260652b55d9cb2df1409972ba3c Mon Sep 17 00:00:00 2001
From: Jin Yi <jin12cc@gmail.com>
Date: Fri, 9 Jan 2026 07:52:49 +0900
Subject: [PATCH] Refactor/code-reivew (#7893)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## 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           |  4 +-
 .../dialog/content/MissingNodesHeader.vue     |  2 +-
 src/components/topbar/TopbarBadge.test.ts     |  6 ++-
 src/components/topbar/TopbarBadge.vue         |  4 +-
 .../manager/ImportFailedNodeHeader.vue        |  2 +-
 .../manager/ManagerDialogContent.vue          |  4 +-
 .../components/manager/NodeConflictHeader.vue |  2 +-
 .../PackVersionSelectorPopover.test.ts        |  8 ++--
 .../manager/PackVersionSelectorPopover.vue    |  2 +-
 .../manager/button/PackEnableToggle.test.ts   |  6 +--
 .../manager/button/PackEnableToggle.vue       |  7 +--
 .../manager/button/PackInstallButton.vue      |  2 +-
 .../composables/useConflictDetection.ts       | 45 ++++++++-----------
 13 files changed, 46 insertions(+), 48 deletions(-)

diff --git a/src/components/button/IconGroup.vue b/src/components/button/IconGroup.vue
index c533125d6..81a23c5ce 100644
--- a/src/components/button/IconGroup.vue
+++ b/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
       )
     "
   >
diff --git a/src/components/dialog/content/MissingNodesHeader.vue b/src/components/dialog/content/MissingNodesHeader.vue
index 4dd483fd0..1e150c3d3 100644
--- a/src/components/dialog/content/MissingNodesHeader.vue
+++ b/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
diff --git a/src/components/topbar/TopbarBadge.test.ts b/src/components/topbar/TopbarBadge.test.ts
index ffb8abdcd..1383c002f 100644
--- a/src/components/topbar/TopbarBadge.test.ts
+++ b/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
+      )
     })
   })
 
diff --git a/src/components/topbar/TopbarBadge.vue b/src/components/topbar/TopbarBadge.vue
index 552daead0..34fd6454d 100644
--- a/src/components/topbar/TopbarBadge.vue
+++ b/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
diff --git a/src/workbench/extensions/manager/components/manager/ImportFailedNodeHeader.vue b/src/workbench/extensions/manager/components/manager/ImportFailedNodeHeader.vue
index 0588e8c5f..b21b315a3 100644
--- a/src/workbench/extensions/manager/components/manager/ImportFailedNodeHeader.vue
+++ b/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>
diff --git a/src/workbench/extensions/manager/components/manager/ManagerDialogContent.vue b/src/workbench/extensions/manager/components/manager/ManagerDialogContent.vue
index d961c026a..c05f375e2 100644
--- a/src/workbench/extensions/manager/components/manager/ManagerDialogContent.vue
+++ b/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') }}
diff --git a/src/workbench/extensions/manager/components/manager/NodeConflictHeader.vue b/src/workbench/extensions/manager/components/manager/NodeConflictHeader.vue
index 24a1820d3..979b9830a 100644
--- a/src/workbench/extensions/manager/components/manager/NodeConflictHeader.vue
+++ b/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') }}
diff --git a/src/workbench/extensions/manager/components/manager/PackVersionSelectorPopover.test.ts b/src/workbench/extensions/manager/components/manager/PackVersionSelectorPopover.test.ts
index 6c4976f91..792983c22 100644
--- a/src/workbench/extensions/manager/components/manager/PackVersionSelectorPopover.test.ts
+++ b/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)
     })
   })
diff --git a/src/workbench/extensions/manager/components/manager/PackVersionSelectorPopover.vue b/src/workbench/extensions/manager/components/manager/PackVersionSelectorPopover.vue
index 46a45abc5..f4a9896c3 100644
--- a/src/workbench/extensions/manager/components/manager/PackVersionSelectorPopover.vue
+++ b/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>
diff --git a/src/workbench/extensions/manager/components/manager/button/PackEnableToggle.test.ts b/src/workbench/extensions/manager/components/manager/button/PackEnableToggle.test.ts
index 80e8417f2..54bbe5bb9 100644
--- a/src/workbench/extensions/manager/components/manager/button/PackEnableToggle.test.ts
+++ b/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)
     })
   })
diff --git a/src/workbench/extensions/manager/components/manager/button/PackEnableToggle.vue b/src/workbench/extensions/manager/components/manager/button/PackEnableToggle.vue
index 2317810cf..a43ec76fc 100644
--- a/src/workbench/extensions/manager/components/manager/button/PackEnableToggle.vue
+++ b/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<{
diff --git a/src/workbench/extensions/manager/components/manager/button/PackInstallButton.vue b/src/workbench/extensions/manager/components/manager/button/PackInstallButton.vue
index de49fd6e4..3000a4a33 100644
--- a/src/workbench/extensions/manager/components/manager/button/PackInstallButton.vue
+++ b/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"
diff --git a/src/workbench/extensions/manager/composables/useConflictDetection.ts b/src/workbench/extensions/manager/composables/useConflictDetection.ts
index 4483e25d9..aad300abe 100644
--- a/src/workbench/extensions/manager/composables/useConflictDetection.ts
+++ b/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