-
Notifications
You must be signed in to change notification settings - Fork 491
New bottom button and badges #8603
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
base: main
Are you sure you want to change the base?
Conversation
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
📝 WalkthroughWalkthroughAggregates execution-store errors for selected nodes and surfaces 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 node runtime errors
Store-->>Panel: executionStore.getNodeErrors(selectedNodes)
Panel->>Panel: compute selectedNodeErrors (aggregate)
alt selectedNodeErrors non-empty
Panel->>Panel: append 'error' tab if missing
Panel->>Panel: set/validate active tab (may switch to 'error')
Panel->>Tab: render TabError with selectedNodeErrors
Tab->>Tab: display errors and provide copy-to-clipboard
else no errors
Panel->>Panel: ensure active tab valid (fallback to first tab)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ 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 Tests:
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/05/2026, 03:07:31 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.5 kB (baseline 22.5 kB) • 🔴 +32 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 859 kB (baseline 840 kB) • 🔴 +19 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 69 kB (baseline 69 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 410 kB (baseline 410 kB) • 🔴 +150 BConfiguration panels, inspectors, and settings screens
Status: 24 added / 24 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 6 added / 6 removed Editors & Dialogs — 3.47 kB (baseline 3.47 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 37.8 kB (baseline 37.8 kB) • ⚪ 0 BReusable component library chunks
Status: 10 added / 10 removed Data & Services — 2.08 MB (baseline 2.1 MB) • 🟢 -14.1 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 234 kB (baseline 234 kB) • 🔴 +44 BHelpers, composables, and utility bundles
Status: 15 added / 15 removed Vendor & Third-Party — 9.37 MB (baseline 9.37 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 2 added / 2 removed Other — 7.08 MB (baseline 7.08 MB) • 🔴 +1.46 kBBundles that do not match a named category
Status: 119 added / 118 removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
Updating Playwright Expectations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
## 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 <drjkl@comfy.org> Co-authored-by: github-actions <github-actions@github.com>
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 })
| 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) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.