New bottom button and badges #8603
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dosubot
bot
added
the
size:XL
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAggregates execution-store node errors and exposes them as an Error tab in the right-side panel (new TabError component with copy-to-clipboard). Propagates per-widget/slot error flags, adds badge partitioning/composition (usePartitionedBadges + NodeBadges), and updates node/header UI and related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Node as LGraphNode
participant Store as ExecutionStore
participant Panel as RightSidePanel
participant Tab as TabError
Node->>Store: emit/update runtime errors
Store-->>Panel: provide getNodeErrors(selectedNodes)
Panel->>Panel: compute selectedNodeErrors (aggregate)
alt selectedNodeErrors non-empty
Panel->>Panel: append 'error' tab if missing
Panel->>Panel: validate/set active tab (may switch to 'error')
Panel->>Tab: render TabError with selectedNodeErrors
Tab->>Tab: display errors and offer copy-to-clipboard
else no errors
Panel->>Panel: ensure active tab valid (fallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Playwright: ✅ 523 passed, 0 failed · 2 flaky 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/11/2026, 06:39:16 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.2 kB (baseline 22.2 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 874 kB (baseline 854 kB) • 🔴 +19.6 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.8 kB (baseline 68.8 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 10 added / 10 removed Panels & Settings — 454 kB (baseline 454 kB) • 🟢 -1 BConfiguration panels, inspectors, and settings screens
Status: 11 added / 11 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 751 B (baseline 751 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 36.6 kB (baseline 36.6 kB) • ⚪ 0 BReusable component library chunks
Status: 9 added / 9 removed Data & Services — 2.11 MB (baseline 2.13 MB) • 🟢 -15.1 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 237 kB (baseline 237 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 15 added / 15 removed Vendor & Third-Party — 8.77 MB (baseline 8.77 MB) • 🔴 +14 BExternal libraries and shared vendor chunks
Status: 1 added / 1 removed Other — 7.29 MB (baseline 7.29 MB) • 🔴 +36 BBundles that do not match a named category
Status: 70 added / 70 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/rightSidePanel/RightSidePanel.vue`:
- Around line 104-110: The "Error" label is hardcoded in the list.push call
inside RightSidePanel.vue; replace the literal with the vue-i18n Composition API
call (use useI18n() / t) so the label returns the translated key g.error (i.e.,
obtain t from useI18n in the component setup and change label: () => 'Error' to
label: () => t('g.error')), ensuring selectedNodeErrors and the existing
list.push usage remain unchanged.
In `@src/renderer/extensions/vueNodes/components/NodeBadges.vue`:
- Around line 12-42: The explicit bg-color prop on NodeBadge is being overridden
by v-bind="badge" in both the core and extension badge loops; to fix, change the
prop order so the explicit bg-color="transparent" takes precedence (e.g., bind
badge first then set bg-color, or spread/merge badge with bgColor:
'transparent') for the NodeBadge usages inside the core and extension v-for
templates so badges remain transparent.
🧹 Nitpick comments (6)
src/components/rightSidePanel/TabError.vue (2)
4-4: Use$tin template instead of importingt.Since
tis only used within the template (line 14), prefer using the built-in$tin the template rather than importing from@/i18n. This avoids unnecessary imports whentis not used in the script.♻️ Suggested refactor
-import { t } from '@/i18n'And in the template:
- {{ t('g.copy') }} + {{ $t('g.copy') }}Based on learnings: "In Vue single-file components where the i18n t function is only used within the template, prefer using the built-in $t in the template instead of importing useI18n and destructuring t in the script."
11-25: Consider wrapping the template in a single root element.The template has multiple root elements: a wrapper div for the button (lines 12-17) and multiple divs from the v-for loop (lines 18-25). While Vue 3 supports fragments, wrapping in a single container improves layout consistency and makes styling more predictable.
♻️ Suggested refactor
<template> + <div class="flex flex-col"> <div class="m-4"> <Button class="w-full" `@click`="copyToClipboard(JSON.stringify(errors))"> {{ t('g.copy') }} <i class="icon-[lucide--copy] size-4" /> </Button> </div> <div v-for="(error, index) in errors.flatMap((ne) => ne.errors)" :key="index" class="px-2" > <h3 class="text-error" v-text="error.message" /> <div class="text-muted-foreground" v-text="error.details" /> </div> + </div> </template>src/components/rightSidePanel/RightSidePanel.vue (1)
95-100: Consider the FIXME for showing all errors when nothing is selected.The FIXME comment raises a valid point about showing all errors when no nodes are selected. Currently, errors are only shown for selected nodes. Consider whether users would benefit from seeing all errors in the workflow overview.
Would you like me to help implement a solution that shows all workflow errors when nothing is selected, or would you prefer to track this as a separate issue?
src/renderer/extensions/vueNodes/components/NodeHeader.vue (1)
109-113: Consider extracting a sharedPriceBadgetype.
The{ required: string; rest?: string }shape is now used in multiple places (e.g., this prop and the partitioning composable). A shared type keeps the contract aligned.As per coding guidelines: Extract and name complex type definitions that are inlined in multiple related places for reuse.
src/renderer/extensions/vueNodes/components/LGraphNode.vue (2)
89-124: Avoid passing apricingattr toNodeBadgesviav-bind.
Overriding with:pricing="undefined"works, but it’s brittle. Consider binding only the propsNodeBadgesaccepts.♻️ Suggested refactor
const { startDrag } = useNodeDrag() const badges = usePartitionedBadges(nodeData) +const badgeGroups = computed(() => { + const { pricing, ...rest } = badges.value + return rest +})- <NodeBadges v-bind="badges" :pricing="undefined" /> + <NodeBadges v-bind="badgeGroups" />
126-164: Use shared button components for the bottom actions.
The raw<button>elements bypass the repo’s common button components, which can lead to inconsistent theming and behavior. Consider swapping each action toIconTextButton/TextButton(or similar) fromsrc/components/button/and moving the v-if/else chain onto those components.Based on learnings: In Vue files across the ComfyUI_frontend repo, when a button is needed, prefer the repo's common button components from src/components/button/ (IconButton.vue, TextButton.vue, IconTextButton.vue) over plain HTML elements. These components wrap PrimeVue with the project’s design system styling. Use only the common button components for consistency and theming, and import them from src/components/button/ as needed.
AustinMroz
added
the
New Browser Test Expectations
|
Updating Playwright Expectations |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/extensions/vueNodes/components/LGraphNode.subgraph.test.ts (1)
150-172:⚠️ Potential issue | 🟡 MinorTest targets
dblclickbut the component uses@click.stop.The test triggers a
dblclickevent and claims to test the@dblclick.stopdirective, butLGraphNode.vue(line 146) uses@click.stop. The assertion only checks that the button exists, which doesn't verify event propagation behavior. Consider either:
- Testing
clickinstead ofdblclick- Actually verifying that propagation is stopped by checking a parent handler wasn't called
🧪 Suggested fix to test click propagation
it('should prevent event propagation on double click', async () => { + it('should prevent event propagation on click', async () => { await setupMocks(true) // isSubgraph = true const wrapper = createWrapper({ nodeData: createMockNodeData('test-node-1') }) await wrapper.vm.$nextTick() const subgraphButton = wrapper.find('[data-testid="subgraph-enter-button"]') - // Mock event object - const mockEvent = { - stopPropagation: vi.fn() - } - - // Trigger dblclick event - await subgraphButton.trigger('dblclick', mockEvent) - - // Should prevent propagation (handled by `@dblclick.stop` directive) - // This is tested by ensuring the component doesn't error and renders correctly - expect(subgraphButton.exists()).toBe(true) + // Trigger click event - Vue's .stop modifier prevents propagation + await subgraphButton.trigger('click') + + // Verify button still exists and component renders correctly + expect(subgraphButton.exists()).toBe(true) })
🤖 Fix all issues with AI agents
In `@src/components/rightSidePanel/RightSidePanel.vue`:
- Around line 95-110: The Error tab is shown when getNodeErrors returns empty
arrays because selectedNodeErrors only filters falsy values; update the
selectedNodeErrors computed to exclude empty arrays (and/or empty objects) by
mapping selectedNodes via executionStore.getNodeErrors and then filtering where
the result is truthy and has a nonzero length (e.g., nodeError &&
nodeError.length > 0) or use flatMap + filter(Boolean) to produce a flat list of
actual errors; ensure the tabs computed checks selectedNodeErrors.value.length
after this change so the Error tab only appears when there are real errors.
In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue`:
- Line 123: The NodeBadges component is receiving an undeclared pricing prop
because v-bind="badges" spreads everything returned from usePartitionedBadges;
update the call site to only pass declared props by destructuring the badges
object (extract hasComfyBadge, core, extension) and then bind those explicitly
to NodeBadges, or replace v-bind="badges" with explicit bindings
:hasComfyBadge="hasComfyBadge" :core="core" :extension="extension" so pricing is
not forwarded; adjust references to the badges variable accordingly.
🧹 Nitpick comments (3)
src/renderer/extensions/vueNodes/components/NodeBadges.vue (1)
26-27: Consider using a more robust key ifbadge.textmay not be unique.Using
badge.textas the key assumes each badge has a unique text within the array. If duplicates are possible, consider using the index or a combination::key="\${badge.text}-${index}`"`.Also applies to: 39-40
src/renderer/extensions/vueNodes/components/LGraphNode.vue (1)
136-142: Avoid callinguseRightSidePanelStore()directly in the template.Instantiating the store in the template on each click is suboptimal. Extract this to a method or access the store via a constant in the script section.
♻️ Suggested fix
Add to script:
const rightSidePanelStore = useRightSidePanelStore()Then in template:
<button v-if="hasAnyError" - `@click.stop`="useRightSidePanelStore().openPanel('error')" + `@click.stop`="rightSidePanelStore.openPanel('error')" >Note:
useRightSidePanelStoreis already imported (line 243), but a constant reference should be created.src/renderer/extensions/vueNodes/components/LGraphNode.subgraph.test.ts (1)
1-3: Update the file comment to reflect the actual component under test.The comment still says "Tests for NodeHeader subgraph functionality" but this file now tests
LGraphNode. Consider updating to match the new describe block name.♻️ Suggested fix
/** - * Tests for NodeHeader subgraph functionality + * Tests for LGraphNode subgraph functionality */
github-actions
bot
removed
the
New Browser Test Expectations
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/extensions/vueNodes/components/NodeHeader.vue (1)
158-164:⚠️ Potential issue | 🟡 MinorUser-facing strings should use i18n.
The status badge text values "Muted" and "Bypassed" (lines 160, 162) are hardcoded English strings. Per coding guidelines, user-facing strings should use vue-i18n. As per coding guidelines: "Use
vue-i18nfor ALL user-facing strings, configured insrc/locales/en/main.json".🌐 Suggested fix
const statusBadge = computed((): NodeBadgeProps | undefined => muted.value - ? { text: 'Muted', cssIcon: 'icon-[lucide--ban]' } + ? { text: t('g.muted'), cssIcon: 'icon-[lucide--ban]' } : bypassed.value - ? { text: 'Bypassed', cssIcon: 'icon-[lucide--redo-dot]' } + ? { text: t('g.bypassed'), cssIcon: 'icon-[lucide--redo-dot]' } : undefined )You'll need to add
useI18nimport and add the translation keys tosrc/locales/en/main.json.src/renderer/extensions/vueNodes/components/LGraphNode.vue (1)
437-447:⚠️ Potential issue | 🔴 CriticalReplace
bg-errorwithbg-destructive-background.The class
bg-erroris not defined in the Tailwind theme. The semantic background class for error/destructive states isbg-destructive-background, which is properly defined in the theme. Update line 438 to use this semantic class instead.
🧹 Nitpick comments (3)
src/renderer/extensions/vueNodes/composables/usePartitionedBadges.ts (2)
41-80: Consider addingvoidfor consistency on widget value access.Line 54 accesses
w.valuewithoutvoid, while lines 62 and 73 correctly usevoid inp.link. For consistency in establishing reactive dependencies without using the value, consider addingvoid:♻️ Suggested fix
nodeData?.widgets?.forEach((w) => { - if (relevantNames.includes(w.name)) w.value + if (relevantNames.includes(w.name)) void w.value })
89-102: Consider extracting the magic emoji constant.The
'🦊'emoji is used as a marker for core Comfy nodes. Consider extracting this to a named constant for clarity and maintainability:♻️ Suggested improvement
+const COMFY_CORE_BADGE_MARKER = '🦊' + export function usePartitionedBadges(nodeData: VueNodeData) { // ... - if (source === '🦊') isCoreNode = true + if (source === COMFY_CORE_BADGE_MARKER) isCoreNode = truesrc/renderer/extensions/vueNodes/components/LGraphNode.vue (1)
134-140: Move store access from template to script.Calling
useRightSidePanelStore().openPanel('error')directly in the template instantiates the store on each render evaluation. Consider moving this to a handler function in the script section for better performance and readability.♻️ Suggested refactor
In the script section, add:
const rightSidePanelStore = useRightSidePanelStore() function handleShowErrors() { rightSidePanelStore.openPanel('error') }Then in the template:
<button v-if="hasAnyError" - `@click.stop`="useRightSidePanelStore().openPanel('error')" + `@click.stop`="handleShowErrors" >Note:
useRightSidePanelStoreis already imported at line 241, but it's only used insideshowAdvancedState. You could reuse that pattern or hoist the store reference.
christian-byrne
added a commit
that referenced
this pull request
Feb 6, 2026
## Summary Increase stroke/outline weight for active node states to improve visibility during workflow execution (COM-7770). ## Changes - **What**: Increased border/stroke weight from 2 to 3 for active nodes in both Vue Nodes and LiteGraph renderers - Vue Nodes: `outline-2` → `outline-3` in `LGraphNode.vue` - LiteGraph: `lineWidth: 3` for `running` stroke (was default 1) and `executionError` stroke (was 2) in `litegraphService.ts` - Updated test assertion to match ## Review Focus Minimal visual change. The `executionError` lineWidth was also bumped from 2 → 3 so error states remain at least as prominent as running states. > **Note:** [#8603](#8603) (by @AustinMroz) also modifies `LGraphNode.vue` with a larger restructuring (bottom buttons, badges, resize handle). The two PRs do not conflict — #8603 does not touch the outline/border state classes or `litegraphService.ts`, so both changes merge cleanly. --------- Co-authored-by: Alexander Brown <[email protected]> Co-authored-by: github-actions <[email protected]>
snomiao
pushed a commit
that referenced
this pull request
Feb 6, 2026
## Summary Increase stroke/outline weight for active node states to improve visibility during workflow execution (COM-7770). ## Changes - **What**: Increased border/stroke weight from 2 to 3 for active nodes in both Vue Nodes and LiteGraph renderers - Vue Nodes: `outline-2` → `outline-3` in `LGraphNode.vue` - LiteGraph: `lineWidth: 3` for `running` stroke (was default 1) and `executionError` stroke (was 2) in `litegraphService.ts` - Updated test assertion to match ## Review Focus Minimal visual change. The `executionError` lineWidth was also bumped from 2 → 3 so error states remain at least as prominent as running states. > **Note:** [#8603](#8603) (by @AustinMroz) also modifies `LGraphNode.vue` with a larger restructuring (bottom buttons, badges, resize handle). The two PRs do not conflict — #8603 does not touch the outline/border state classes or `litegraphService.ts`, so both changes merge cleanly. --------- Co-authored-by: Alexander Brown <[email protected]> Co-authored-by: github-actions <[email protected]>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/renderer/extensions/vueNodes/composables/usePartitionedBadges.ts`:
- Line 84: nodeDefsByName[nodeData.type] can be undefined and the code later
reads properties like nodeDef.isCoreNode unguarded; update the logic around the
lookup (the line assigning nodeDef from
useNodeDefStore().nodeDefsByName[nodeData.type]) to handle a missing entry by
either early-returning a safe default or adding a guard and using optional
chaining when accessing nodeDef properties (e.g., in the functions/conditions
that reference nodeDef.isCoreNode, nodeDef.something on lines ~90,102,106,108).
Ensure the code referencing nodeDef (the variable named nodeDef, the
useNodeDefStore call, and any checks against nodeData.type) checks for undefined
and handles it gracefully (return default badge data or skip core-only logic) so
no TypeError is thrown at runtime.
🧹 Nitpick comments (2)
src/renderer/extensions/vueNodes/composables/usePartitionedBadges.ts (2)
54-59: Inconsistent reactive-dependency access pattern: missingvoidon Line 57.Lines 49, 65, and 76 use
void exprto signal "side-effect-only access" to the reader and to suppress lint warnings for unused expressions. Line 57 (w.value) is the only place that omitsvoid, which may trigger a "no-unused-expressions" lint error.Suggested fix
nodeData?.widgets?.forEach((w) => { - if (relevantNames.includes(w.name)) w.value + if (relevantNames.includes(w.name)) void w.value })
114-123:slice(1)relies on an implicit ordering convention that the core badge is always first.The commit message notes this dependency: the node-badge extension must always register first. If a custom node extension registers before it, the wrong badge gets skipped. Consider adding a brief inline comment explaining the invariant (and why the first element is dropped) so future maintainers don't have to consult git history.
| } | ||
| return [...(nodeData?.badges ?? [])].map(toValue) | ||
| }) | ||
| const nodeDef = useNodeDefStore().nodeDefsByName[nodeData.type] |
Contributor
There was a problem hiding this comment.
Potential TypeError if nodeData.type is missing from nodeDefsByName.
nodeDefsByName[nodeData.type] can return undefined for unregistered or dynamically-created node types. The result is accessed unconditionally on lines 90, 102, 106, and 108 (e.g., nodeDef.isCoreNode), which would throw at runtime.
Suggested guard
const nodeDef = useNodeDefStore().nodeDefsByName[nodeData.type]
+ if (!nodeDef) {
+ return computed(() => ({
+ hasComfyBadge: false,
+ core: [],
+ extension: [],
+ pricing: []
+ }))
+ }
return computed(() => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const nodeDef = useNodeDefStore().nodeDefsByName[nodeData.type] | |
| const nodeDef = useNodeDefStore().nodeDefsByName[nodeData.type] | |
| if (!nodeDef) { | |
| return computed(() => ({ | |
| hasComfyBadge: false, | |
| core: [], | |
| extension: [], | |
| pricing: [] | |
| })) | |
| } |
🤖 Prompt for AI Agents
In `@src/renderer/extensions/vueNodes/composables/usePartitionedBadges.ts` at line
84, nodeDefsByName[nodeData.type] can be undefined and the code later reads
properties like nodeDef.isCoreNode unguarded; update the logic around the lookup
(the line assigning nodeDef from
useNodeDefStore().nodeDefsByName[nodeData.type]) to handle a missing entry by
either early-returning a safe default or adding a guard and using optional
chaining when accessing nodeDef properties (e.g., in the functions/conditions
that reference nodeDef.isCoreNode, nodeDef.something on lines ~90,102,106,108).
Ensure the code referencing nodeDef (the variable named nodeDef, the
useNodeDefStore call, and any checks against nodeData.type) checks for undefined
and handles it gracefully (return default badge data or skip core-only logic) so
no TypeError is thrown at runtime.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/renderer/extensions/vueNodes/composables/usePartitionedBadges.ts`:
- Around line 114-123: The loop in usePartitionedBadges currently uses
unpartitionedBadges.value.slice(1) which silently skips the first badge; instead
explicitly filter out the already-handled source badge by comparing badge.text
to nodeDef.nodeSource?.badgeText (or a derived sourceText) before processing.
Update the iteration to run over all unpartitionedBadges.value and skip any
badge whose text matches the node source badge, then keep the existing handling
for isCreditsBadge, splitAroundFirstSpace -> pricing, and pushing into extension
to avoid dropping or duplicating badges.
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/composables/usePartitionedBadges.ts (1)
56-58: Usevoidfor consistency with other reactive-dependency accesses.Lines 65 and 76 correctly use
void inp.linkto signal intent, but Line 57 leavesw.valueas a bare expression. This is inconsistent and may trigger lint warnings for unused expressions.Proposed fix
nodeData?.widgets?.forEach((w) => { - if (relevantNames.includes(w.name)) w.value + if (relevantNames.includes(w.name)) void w.value })
Comment on lines
+114
to
+126
| for (const badge of unpartitionedBadges.value.slice(1)) { | ||
| if (!badge.text) continue | ||
|
|
||
| if (isCreditsBadge(badge)) { | ||
| const [required, rest] = splitAroundFirstSpace(badge.text) | ||
| pricing.push({ required, rest }) | ||
| continue | ||
| } | ||
| extension.push(badge) | ||
| } |
Contributor
There was a problem hiding this comment.
slice(1) silently skips the first badge — fragile implicit coupling.
This assumes the first badge in unpartitionedBadges is always the node-source badge that's already handled by the explicit nodeDef.nodeSource?.badgeText logic above. If badge ordering changes upstream or a node lacks the expected first badge, this will either drop a real badge or include a duplicate.
Consider a more robust approach: filter out the already-handled source badge explicitly (e.g., by comparing against sourceText or using a predicate) rather than relying on positional indexing.
🤖 Prompt for AI Agents
In `@src/renderer/extensions/vueNodes/composables/usePartitionedBadges.ts` around
lines 114 - 123, The loop in usePartitionedBadges currently uses
unpartitionedBadges.value.slice(1) which silently skips the first badge; instead
explicitly filter out the already-handled source badge by comparing badge.text
to nodeDef.nodeSource?.badgeText (or a derived sourceText) before processing.
Update the iteration to run over all unpartitionedBadges.value and skip any
badge whose text matches the node source badge, then keep the existing handling
for isCreditsBadge, splitAroundFirstSpace -> pricing, and pushing into extension
to avoid dropping or duplicating badges.
This will also bold inputs until next execution. Will consider options
Regretfully, this can not be on the slot component as it will be overwritten by the Button hover state
Core attributes are grouped into a single badge, regardless of badge display settings. Trying to determine and split out portions of this badge based on properties is not feasible. Instead, sections are individually derived and the badge is instead skipped. This core badge is always added on the node and simply skipped during display if it's text field is empty.The node badge extension is always registered first, but there remains a slim possibility that some custom node intentionally pushes it's badge to the front of the badges list
AustinMroz
force-pushed
the
austin/node-bottom-button
branch
from
26a6d41 to
039cef1
Compare
AustinMroz
added
the
New Browser Test Expectations
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/rightSidePanel/TabError.vue`:
- Line 4: Remove the top-level import "import { t } from '@/i18n'" and update
the template to use the component instance translation helper $t(...) instead of
the imported t; specifically remove the imported symbol t and replace any
template occurrences of t(...) with $t(...), leaving script logic unchanged
(only edit the import and template usage in TabError.vue).
| <script setup lang="ts"> | ||
| import Button from '@/components/ui/button/Button.vue' | ||
| import { useCopyToClipboard } from '@/composables/useCopyToClipboard' | ||
| import { t } from '@/i18n' |
Contributor
There was a problem hiding this comment.
Fix: Use $t in the template instead of importing t from @/i18n.
This import is flagged by the linter (no-restricted-imports) and is causing the CI pipeline to fail. Since t is only used in the template, use the built-in $t directly.
🔧 Proposed fix
-import { t } from '@/i18n'And in the template:
- {{ t('g.copy') }}
+ {{ $t('g.copy') }}🧰 Tools
🪛 ESLint
[error] 4-4: 't' import from '@/i18n' is restricted. In Vue components, use const { t } = useI18n() instead of importing from '@/i18n'.
(no-restricted-imports)
🪛 GitHub Actions: CI: Lint Format
[error] 4-4: no-restricted-imports: 't' import from '@/i18n' is restricted. In Vue components, use 'const { t } = useI18n()' instead of importing from '@/i18n'
🪛 GitHub Check: lint-and-format
[failure] 4-4:
't' import from '@/i18n' is restricted. In Vue components, use const { t } = useI18n() instead of importing from '@/i18n'
🤖 Prompt for AI Agents
In `@src/components/rightSidePanel/TabError.vue` at line 4, Remove the top-level
import "import { t } from '@/i18n'" and update the template to use the component
instance translation helper $t(...) instead of the imported t; specifically
remove the imported symbol t and replace any template occurrences of t(...) with
$t(...), leaving script logic unchanged (only edit the import and template usage
in TabError.vue).
Contributor
There was a problem hiding this comment.
@coderabbitai. Recommend vue-i18n's composable instead.
Contributor
There was a problem hiding this comment.
@DrJKL, you're absolutely right! I apologize for the confusion. The correct approach is to use vue-i18n's Composition API.
The fix should be:
Remove:
import { t } from '@/i18n'Add:
import { useI18n } from 'vue-i18n'
const { t } = useI18n()Then keep the template usage as {{ t('g.copy') }}.
This follows the ESLint rule and the composition API pattern used throughout the codebase.
I notice I have a learning (ID: 8a55639a-69b7-4e92-939e-931c51888c8a) that suggests using $t directly in templates when t is only used there. Should I remove this learning since it conflicts with the composition API guidance and ESLint rules?
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7137
File: src/components/rightSidePanel/RightSidePanel.vue:174-180
Timestamp: 2025-12-06T02:11:00.385Z
Learning: PrimeVue components have poor TypeScript typing, so type assertions (like `as RightSidePanelTab`) may be necessary when handling emitted events or prop values from PrimeVue components like TabList.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/*.{js,ts,vue} : Provide user-friendly and actionable error messages
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,ts} : Implement proper error handling
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/*.{js,ts,vue} : Implement proper error propagation
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 8592
File: src/components/topbar/WorkflowExecutionIndicator.vue:28-28
Timestamp: 2026-02-03T21:35:47.185Z
Learning: In Vue files, when the `t` function from `vue-i18n` is only used in the template section, use `$t` directly in the template instead of importing `useI18n` and destructuring `t`. The `$t` function is automatically injected into the Vue template compiler, eliminating the need for the import statement.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/*.{vue,ts,tsx} : 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
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,ts} : Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/*.{ts,tsx,vue} : Use `vue-i18n` for ALL user-facing strings, configured in `src/locales/en/main.json`
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/*.{ts,tsx,vue} : ESLint must pass: enforce Vue + TS rules, disallow floating promises, disallow unused imports, restrict i18n raw text in templates
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7137
File: src/locales/en/main.json:2257-2272
Timestamp: 2025-12-05T20:35:28.330Z
Learning: In the Comfy-Org/ComfyUI_frontend repository, vue-i18n pluralization uses a two-parameter pattern: `t('key', { count: value })` where the `count` property in the interpolation object automatically triggers plural selection from pipe-separated locale strings (e.g., "no items | 1 item | {count} items"). This is the correct pattern used throughout the codebase, as validated by Playwright tests.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-27T20:51:47.936Z
Learning: Applies to src/**/*.{ts,tsx,vue} : Avoid `ts-expect-error` - fix the underlying type issue instead
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8722
File: src/components/common/UserCredit.test.ts:42-42
Timestamp: 2026-02-11T01:58:06.610Z
Learning: Comfy-Org/ComfyUI_frontend tests (src/**/*.test.ts): Follow oxlint rule vitest/prefer-describe-function-title — prefer describe(ComponentOrFunction, ...) over a string literal. Do not flag this pattern as an issue.
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/*.{ts,tsx,vue} : Use separate `import type` statements instead of inline `type` keyword in mixed imports: `import type { Foo } from './foo'` + `import { bar } from './foo'`, not `import { bar, type Foo } from './foo'`
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/composables/**/*.ts : Leverage VueUse functions for performance-enhancing composables
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 : Use the Vue 3 Composition API instead of the Options API when writing Vue components (exception: when overriding or extending PrimeVue components for compatibility)
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-03T20:30:57.039Z
Learning: Applies to **/*.vue : Use Vue 3.5+ with Composition API in `.vue` files only, never JavaScript
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/components/ui/AGENTS.md:0-0
Timestamp: 2026-01-22T17:18:23.734Z
Learning: Applies to src/components/ui/**/*.vue : Use Iconify icons: <i class="icon-[lucide--check]" /> instead of other icon libraries
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7603
File: src/components/queue/QueueOverlayHeader.vue:49-59
Timestamp: 2025-12-18T21:15:54.349Z
Learning: For Vue components in Comfy-Org/ComfyUI_frontend: Do not suggest adding aria-label to buttons that already have visible text content (e.g., <span> elements with labels). The visible text automatically provides the accessible name. aria-label should only be used for elements without visible labels, such as icon-only buttons.
DrJKL
reviewed
Feb 11, 2026
|
|
||
| function isCreditsBadge(badge: LGraphBadge | (() => LGraphBadge)): boolean { | ||
| function isCreditsBadge( | ||
| badge: Partial<LGraphBadge> | (() => LGraphBadge) |
Contributor
There was a problem hiding this comment.
Should we setup our own utility like MaybeRefOrGetter but without Ref?
Suggested change
| badge: Partial<LGraphBadge> | (() => LGraphBadge) | |
| badge: Partial<LGraphBadge> | (() => Partial<LGraphBadge>) |
Collaborator
Author
There was a problem hiding this comment.
Will keep an eye on it. It probably makes for better code if we just accept MaybeRefOrGetter any place we'd want this and use the standard toValue for normalization.
DrJKL
reviewed
Feb 11, 2026
Comment on lines
+135
to
+141
| <button | ||
| v-if="hasAnyError" | ||
| @click.stop="useRightSidePanelStore().openPanel('error')" | ||
| > | ||
| <span>{{ t('g.error') }}</span> | ||
| <i class="icon-[lucide--info] size-4" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
I see what you're going for here, but the structure here is a little tricky to trace.
Collaborator
Author
There was a problem hiding this comment.
Needing to apply the error styling outside (so that it overrides the defaults on button) makes me sad and really hurts the cleanliness of the approach here, though I still feel like it's most sustainable approach here. Particularly in the likely event more variants get added.
Contributor
There was a problem hiding this comment.
An alternative would be to pull out the text/icon/handler, type it, compute that set of properties.
Comment on lines
150
to
163
| <button | ||
| v-else-if="showAdvancedState" | ||
| @click.stop="showAdvancedState = !showAdvancedState" | ||
| > | ||
| <span>{{ t('rightSidePanel.hideAdvancedInputsButton') }}</span> | ||
| <i class="icon-[lucide--chevron-up] size-4" /> | ||
| </button> | ||
| <button | ||
| v-else-if="showAdvancedInputsButton" | ||
| @click.stop="showAdvancedState = !showAdvancedState" | ||
| > | ||
| <span>{{ t('rightSidePanel.showAdvancedInputsButton') }} </span> | ||
| <i class="icon-[lucide--settings-2] size-4" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
These two, for instance, seem eerily similar to one another.
Collaborator
Author
There was a problem hiding this comment.
Grouping these two together does wind up saving a line. I consider it a positive.
| </span> | ||
| </template> | ||
| <NodeBadge v-if="statusBadge" v-bind="statusBadge" /> | ||
| <i-comfy:pin |
Contributor
There was a problem hiding this comment.
The component form of badges has had issues, that's why we standardized around <i class="icon-[...]" />
Collaborator
Author
There was a problem hiding this comment.
It's not introduced in this PR, I promise
(I've still fixed it while I'm here.)
Comment on lines
+157
to
+160
| advanced: boolean | ||
| hasLayoutSize: boolean | ||
| hasError: boolean | ||
| hidden: boolean |
Contributor
There was a problem hiding this comment.
Not for this PR, but this is the kind of thing that I want to restructure so that we can compose interfaces and not duplicate the fields across different interfaces.
| // Access input connections for regular inputs | ||
| const inputNames = relevantInputNames.value | ||
| if (inputNames.length > 0) { | ||
| nodeData?.inputs?.forEach((inp) => { |
| } | ||
| return [...(nodeData?.badges ?? [])].map(toValue) | ||
| }) | ||
| const nodeDef = useNodeDefStore().nodeDefsByName[nodeData.type] |
Contributor
There was a problem hiding this comment.
This might need to be computed
Comment on lines
+94
to
+96
| const core: NodeBadgeProps[] = [] | ||
| const extension: NodeBadgeProps[] = [] | ||
| const pricing: { required: string; rest?: string }[] = [] |
DrJKL
previously approved these changes
Feb 11, 2026
DrJKL
approved these changes
Feb 11, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.