Skip to content
Merged
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
5 changes: 4 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ The project uses **Nx** for build orchestration and task management
- Imports:
- sorted/grouped by plugin
- run `pnpm format` before committing
- use separate `import type` statements, not inline `type` in mixed imports
- ✅ `import type { Foo } from './foo'` + `import { bar } from './foo'`
- ❌ `import { bar, type Foo } from './foo'`
Comment on lines +66 to +68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Potentially this is something that can be enforced via linter rather than taking up valuable agent context window.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in the linter, but for some reason the generations tend to favor the inline style which then triggers a lint error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want me to remove the example lines?

- ESLint:
- Vue + TS rules
- no floating promises
Expand Down Expand Up @@ -137,7 +140,7 @@ The project uses **Nx** for build orchestration and task management
8. Implement proper error handling
9. Follow Vue 3 style guide and naming conventions
10. Use Vite for fast development and building
11. Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
11. Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json. Use the plurals system in i18n instead of hardcoding pluralization in templates.
12. Avoid new usage of PrimeVue components
13. Write tests for all changes, especially bug fixes to catch future regressions
14. Write code that is expressive and self-documenting to the furthest degree possible. This reduces the need for code comments which can get out of sync with the code itself. Try to avoid comments unless absolutely necessary
Expand Down
30 changes: 30 additions & 0 deletions src/components/common/StatusBadge.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<script setup lang="ts">
type Severity = 'default' | 'secondary' | 'warn' | 'danger' | 'contrast'

const { label, severity = 'default' } = defineProps<{
label: string
severity?: Severity
}>()

function badgeClasses(sev: Severity): string {
const baseClasses =
'inline-flex h-3.5 items-center justify-center rounded-full px-1 text-xxxs font-semibold uppercase'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xxxs is maybe an indication we should scale out text sizes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're all rem relative: https://tailwindcss.com/docs/font-size


switch (sev) {
case 'danger':
return `${baseClasses} bg-destructive-background text-white`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider using a semantic token instead of hardcoded text-white.

The danger variant uses text-white while all other variants use semantic theme tokens like text-base-foreground or text-base-background. This inconsistency may cause theming issues. Verify whether there's a semantic token (e.g., text-destructive-foreground) that should be used here for consistency with the design system.

🤖 Prompt for AI Agents
In @src/components/common/StatusBadge.vue at line 15, The danger variant
currently appends the hardcoded class "text-white" (see the return that builds
`${baseClasses} bg-destructive-background text-white`), which is inconsistent
with other variants that use semantic tokens; change that token to the semantic
foreground token (e.g., "text-destructive-foreground") if it exists in the
design system, or to the appropriate semantic token used elsewhere like
"text-base-foreground"; if no semantic token exists, add one to the theme and
use it in the same return expression so the danger badge uses a theme token
instead of a hardcoded color.

case 'contrast':
return `${baseClasses} bg-base-foreground text-base-background`
case 'warn':
return `${baseClasses} bg-warning-background text-base-background`
case 'secondary':
return `${baseClasses} bg-secondary-background text-base-foreground`
default:
return `${baseClasses} bg-primary-background text-base-foreground`
}
}
Comment on lines 9 to 25
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Use cn() utility and computed property for Tailwind classes.

The component concatenates Tailwind classes with template strings, which bypasses tailwind-merge conflict resolution. Additionally, the function is re-executed on every render.

♻️ Refactor to use cn() and computed
 <script setup lang="ts">
+import { computed } from 'vue'
+
+import { cn } from '@/utils/tailwindUtil'
+
 type Severity = 'default' | 'secondary' | 'warn' | 'danger' | 'contrast'
 
 const { label, severity = 'default' } = defineProps<{
   label: string
   severity?: Severity
 }>()
 
-function badgeClasses(sev: Severity): string {
+const badgeClasses = computed(() => {
   const baseClasses =
-    'inline-flex h-3.5 items-center justify-center rounded-full px-1 text-xxxs font-semibold uppercase'
+    'inline-flex h-3.5 items-center justify-center rounded-full px-1 text-xxxs font-semibold uppercase'
 
-  switch (sev) {
+  switch (severity) {
     case 'danger':
-      return `${baseClasses} bg-destructive-background text-white`
+      return cn(baseClasses, 'bg-destructive-background text-white')
     case 'contrast':
-      return `${baseClasses} bg-base-foreground text-base-background`
+      return cn(baseClasses, 'bg-base-foreground text-base-background')
     case 'warn':
-      return `${baseClasses} bg-warning-background text-base-background`
+      return cn(baseClasses, 'bg-warning-background text-base-background')
     case 'secondary':
-      return `${baseClasses} bg-secondary-background text-base-foreground`
+      return cn(baseClasses, 'bg-secondary-background text-base-foreground')
     default:
-      return `${baseClasses} bg-primary-background text-white`
+      return cn(baseClasses, 'bg-primary-background text-white')
   }
-}
+})
 </script>
 
 <template>
-  <span :class="badgeClasses(severity)">{{ label }}</span>
+  <span :class="badgeClasses">{{ label }}</span>
 </template>

Benefits:

  • Proper Tailwind class merging via cn()
  • Computed property caches result
  • Follows repository conventions

Based on coding guidelines and learnings from the repository.

🤖 Prompt for AI Agents
In @src/components/common/StatusBadge.vue around lines 9 - 25, The badgeClasses
function currently builds Tailwind class strings via template literals and runs
on every render; change it to return a computed cached class string and use the
repository cn() utility to merge classes to avoid tailwind-merge conflicts.
Replace the badgeClasses function with a computed property (e.g., const
badgeClass = computed(() => ...)) that switches on the Severity values and
returns cn(baseClasses, conditionalClasses) instead of string interpolation;
update places that call badgeClasses() to use badgeClass.value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make an issue to follow up with a fix later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue for the StatusBadge refactoring

gh issue create \
  --title "Refactor StatusBadge component to use cn() utility and computed property" \
  --body "## Context

This issue tracks a follow-up refactoring task for the StatusBadge component introduced in PR #7897.

## Description

The \`StatusBadge.vue\` component currently uses template string concatenation for building Tailwind class names and re-executes the class-building function on every render. This should be refactored to:

1. Use the \`cn()\` utility from \`@/utils/tailwindUtil\` for proper Tailwind class merging
2. Use a computed property to cache the class string and avoid unnecessary re-computation

## References

- PR: https://github.com/Comfy-Org/ComfyUI_frontend/pull/7897
- Review comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/7897#discussion_r2673493348
- File: \`src/components/common/StatusBadge.vue\`
- Requested by: @DrJKL

## Benefits

- Proper Tailwind class conflict resolution via tailwind-merge
- Performance improvement through computed caching
- Consistency with repository conventions" \
  --assignee DrJKL

Length of output: 57


@DrJKL, I've created issue #7910 to track this refactoring. It's assigned to you and includes references to this PR and review comment.


🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7893
File: src/components/button/IconGroup.vue:5-6
Timestamp: 2026-01-08T02:26:18.357Z
Learning: In Comfy-Org/ComfyUI_frontend, the `cn` utility function from `@/utils/tailwindUtil` uses `tailwind-merge`, which intelligently resolves conflicting Tailwind classes by keeping the last one. When a component uses `cn('base-classes bg-default', propClass)`, if `propClass` contains a conflicting background class, `tailwind-merge` will correctly override `bg-default` with the value from `propClass`. This additive pattern is correct and intentional, following the shadcn-ui convention.
</learning]

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to **/*.vue : Use `cn()` utility function from `@/utils/tailwindUtil` for merging Tailwind class names; do not use `:class="[]"` syntax

Learnt from: benceruleanlu
Repo: Comfy-Org/ComfyUI_frontend PR: 7297
File: src/components/actionbar/ComfyActionbar.vue:33-43
Timestamp: 2025-12-09T21:40:19.792Z
Learning: In the Comfy-Org/ComfyUI_frontend repository, inline Tailwind CSS class strings, even when long, are acceptable and preferred over extracting them to computed properties when the classes are static. This is a common Tailwind pattern and doesn't need to be flagged as a readability issue.

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to **/*.vue : Do not use `!important` or the `!` important prefix for Tailwind classes; find and correct interfering `!important` classes instead

Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7598
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:131-131
Timestamp: 2025-12-18T02:07:44.374Z
Learning: Comfy-Org/ComfyUI_frontend uses Tailwind CSS v4 utilities, including the new “safe” overflow-alignment classes. Do not flag items-center-safe, justify-*-safe, or place-*-safe utilities as invalid in src/**/*.vue or stories.

Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7649
File: src/platform/cloud/subscription/components/PricingTable.vue:185-201
Timestamp: 2025-12-22T21:36:16.031Z
Learning: In the Comfy-Org/ComfyUI_frontend repository, avoid creating single-use variants for the Button component (and other UI components). Variants should be reusable across multiple use cases. It's acceptable to use a mix of variant props and manual Tailwind classes temporarily when a specific styling need exists in only one place, with consolidation deferred to a later phase when patterns emerge.

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to **/*.vue : Do not use the `dark:` Tailwind variant; use semantic values from the `style.css` theme instead (e.g., `bg-node-component-surface`)

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/components/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:45.616Z
Learning: Applies to src/components/**/*.vue : Extract complex conditionals to computed properties

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T19:47:02.860Z
Learning: Applies to src/**/*.vue : Implement computed properties with computed()

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to **/*.vue : Use Tailwind 4 for styling in Vue components; avoid `<style>` blocks

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:34.324Z
Learning: Applies to src/**/*.{vue,ts,tsx} : Follow Vue 3 composition API style guide

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to **/*.vue : Follow Vue 3 style guide and naming conventions

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T19:47:02.860Z
Learning: Applies to src/**/*.vue : Follow Vue 3 style guide and naming conventions

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to **/*.vue : Implement proper props and emits definitions in Vue components

Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-21T06:04:12.562Z
Learning: Applies to **/*.vue : Use Vue 3.5 TypeScript-style default prop declaration with reactive props destructuring; do not use `withDefaults` or runtime props declaration

Comment on lines +9 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Use the cn() utility for merging Tailwind classes.

Per coding guidelines, use the cn() utility function from @/utils/tailwindUtil for merging Tailwind class names instead of manual string concatenation. This ensures proper handling of class conflicts and follows the project's established patterns.

♻️ Proposed refactor using cn() utility
+import { cn } from '@/utils/tailwindUtil'
+
 type Severity = 'default' | 'secondary' | 'warn' | 'danger' | 'contrast'

 const { label, severity = 'default' } = defineProps<{
   label: string
   severity?: Severity
 }>()

 function badgeClasses(sev: Severity): string {
   const baseClasses =
     'inline-flex h-3.5 items-center justify-center rounded-full px-1 text-xxxs font-semibold uppercase'

   switch (sev) {
     case 'danger':
-      return `${baseClasses} bg-destructive-background text-white`
+      return cn(baseClasses, 'bg-destructive-background text-white')
     case 'contrast':
-      return `${baseClasses} bg-base-foreground text-base-background`
+      return cn(baseClasses, 'bg-base-foreground text-base-background')
     case 'warn':
-      return `${baseClasses} bg-warning-background text-base-background`
+      return cn(baseClasses, 'bg-warning-background text-base-background')
     case 'secondary':
-      return `${baseClasses} bg-secondary-background text-base-foreground`
+      return cn(baseClasses, 'bg-secondary-background text-base-foreground')
     default:
-      return `${baseClasses} bg-primary-background text-base-foreground`
+      return cn(baseClasses, 'bg-primary-background text-base-foreground')
   }
 }

As per coding guidelines for **/*.vue: Use cn() utility function from @/utils/tailwindUtil for merging Tailwind class names.

🤖 Prompt for AI Agents
In @src/components/common/StatusBadge.vue around lines 9 - 25, The badgeClasses
function is concatenating Tailwind class strings manually; import and use the
cn() utility from '@/utils/tailwindUtil' and replace the template-string returns
with cn(baseClasses, '<case-specific classes>') for each Severity branch (e.g.,
case 'danger' -> cn(baseClasses, 'bg-destructive-background text-white')),
keeping the same return type and behavior and removing direct string
interpolation.

</script>

<template>
<span :class="badgeClasses(severity)">{{ label }}</span>
</template>
66 changes: 66 additions & 0 deletions src/components/toast/ProgressToastItem.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<script setup lang="ts">
import { computed } from 'vue'
import { useI18n } from 'vue-i18n'

import StatusBadge from '@/components/common/StatusBadge.vue'
import type { AssetDownload } from '@/stores/assetDownloadStore'
import { cn } from '@/utils/tailwindUtil'

const { job } = defineProps<{
job: AssetDownload
}>()

const { t } = useI18n()

const progressPercent = computed(() => Math.round(job.progress * 100))
const isCompleted = computed(() => job.status === 'completed')
const isFailed = computed(() => job.status === 'failed')
const isRunning = computed(() => job.status === 'running')
const isPending = computed(() => job.status === 'created')
</script>

<template>
<div
:class="
cn(
'flex items-center justify-between rounded-lg bg-modal-card-background px-4 py-3',
isCompleted && 'opacity-50'
)
"
>
<div class="flex flex-col">
<span class="text-sm text-base-foreground">{{ job.assetName }}</span>
<span v-if="isRunning" class="text-xs text-muted-foreground">
{{ progressPercent }}%
</span>
</div>

<div class="flex items-center gap-2">
<template v-if="isFailed">
<i
class="icon-[lucide--circle-alert] size-4 text-destructive-background"
/>
<StatusBadge :label="t('progressToast.failed')" severity="danger" />
</template>

<template v-else-if="isCompleted">
<StatusBadge :label="t('progressToast.finished')" severity="contrast" />
</template>

<template v-else-if="isRunning">
<i
class="icon-[lucide--loader-circle] size-4 animate-spin text-primary-background"
/>
<span class="text-xs text-primary-background">
{{ progressPercent }}%
</span>
</template>

<template v-else-if="isPending">
<span class="text-xs text-muted-foreground">
{{ t('progressToast.pending') }}
</span>
</template>
</div>
</div>
</template>
16 changes: 16 additions & 0 deletions src/locales/en/main.json
Original file line number Diff line number Diff line change
Expand Up @@ -2474,5 +2474,21 @@
"help": {
"recentReleases": "Recent releases",
"helpCenterMenu": "Help Center Menu"
},
"progressToast": {
"importingModels": "Importing Models",
"downloadingModel": "Downloading model...",
"downloadsFailed": "{count} downloads failed | {count} download failed | {count} downloads failed",
"allDownloadsCompleted": "All downloads completed",
"noImportsInQueue": "No {filter} in queue",
"failed": "Failed",
"finished": "Finished",
"pending": "Pending",
"progressCount": "{completed} of {total}",
"filter": {
"all": "All",
"completed": "Completed",
"failed": "Failed"
}
}
}
Loading