-
Notifications
You must be signed in to change notification settings - Fork 490
fix RightSidePanel title not updating when litegraph node title changes #7860
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| import { mount } from '@vue/test-utils' | ||
| import { createPinia, setActivePinia } from 'pinia' | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { nextTick, shallowRef } from 'vue' | ||
| import { createI18n } from 'vue-i18n' | ||
|
|
||
| import { useGraphNodeManager } from '@/composables/graph/useGraphNodeManager' | ||
| import type { GraphNodeManager } from '@/composables/graph/useGraphNodeManager' | ||
| import enMessages from '@/locales/en/main.json' | ||
| import { LGraph, LGraphNode } from '@/lib/litegraph/src/litegraph' | ||
| import { useCanvasStore } from '@/renderer/core/canvas/canvasStore' | ||
|
|
||
| import RightSidePanel from './RightSidePanel.vue' | ||
|
|
||
| const mockNodeManager = shallowRef<GraphNodeManager | null>(null) | ||
|
|
||
| vi.mock('@/composables/graph/useVueNodeLifecycle', () => ({ | ||
| useVueNodeLifecycle: () => ({ | ||
| nodeManager: mockNodeManager | ||
| }) | ||
| })) | ||
|
|
||
| // Mock useLayoutMutations (used by useGraphNodeManager) | ||
| vi.mock('@/renderer/core/layout/operations/layoutMutations', () => ({ | ||
| useLayoutMutations: () => ({ | ||
| createNode: vi.fn(), | ||
| setSource: vi.fn() | ||
| }) | ||
| })) | ||
|
|
||
| const createMountConfig = () => { | ||
| return { | ||
| global: { | ||
| stubs: { | ||
| TabParameters: true | ||
| }, | ||
| plugins: [ | ||
| createI18n({ | ||
| legacy: false, | ||
| locale: 'en', | ||
| messages: { en: enMessages } | ||
| }) | ||
| ] | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const setupGraph = () => { | ||
| const graph = new LGraph() | ||
| mockNodeManager.value = useGraphNodeManager(graph) | ||
| return { graph, canvasStore: useCanvasStore() } | ||
| } | ||
|
|
||
| describe('RightSidePanel', () => { | ||
| beforeEach(() => { | ||
| setActivePinia(createPinia()) | ||
| mockNodeManager.value = null | ||
| }) | ||
|
|
||
| describe('title reactivity', () => { | ||
| it('updates panel title when node.title changes', async () => { | ||
| const { graph, canvasStore } = setupGraph() | ||
|
|
||
| const node = new LGraphNode('Original Title', 'TestNode') | ||
| graph.add(node) | ||
| canvasStore.selectedItems = [node] | ||
|
|
||
| const wrapper = mount(RightSidePanel, createMountConfig()) | ||
| await nextTick() | ||
| expect(wrapper.text()).toContain('Original Title') | ||
|
|
||
| node.title = 'Updated Title' | ||
|
|
||
| await nextTick() | ||
| expect(wrapper.text()).toContain('Updated Title') | ||
| }) | ||
|
|
||
| it('shows selection count message when multiple nodes are selected', async () => { | ||
| const { graph, canvasStore } = setupGraph() | ||
|
|
||
| const node1 = new LGraphNode('Node 1', 'TestNode') | ||
| const node2 = new LGraphNode('Node 2', 'TestNode') | ||
| graph.add(node1) | ||
| graph.add(node2) | ||
| canvasStore.selectedItems = [node1, node2] | ||
|
|
||
| const wrapper = mount(RightSidePanel, createMountConfig()) | ||
| await nextTick() | ||
|
|
||
| expect(wrapper.text()).toContain('2') | ||
| expect(wrapper.text()).not.toContain('Node 1') | ||
| expect(wrapper.text()).not.toContain('Node 2') | ||
| }) | ||
|
Comment on lines
+60
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider adding edge case tests for fallback logic. The current tests cover happy path scenarios (title updates, multi-selection). Consider adding tests for:
📋 Suggested additional test casesit('falls back to node.type when title is empty', async () => {
const { graph, canvasStore } = setupGraph()
const node = new LGraphNode('', 'CustomNodeType')
graph.add(node)
canvasStore.selectedItems = [node]
const wrapper = mount(RightSidePanel, createMountConfig())
await nextTick()
expect(wrapper.text()).toContain('CustomNodeType')
})
it('handles null nodeManager gracefully', async () => {
mockNodeManager.value = null
const canvasStore = useCanvasStore()
const node = new LGraphNode('Test Node', 'TestType')
canvasStore.selectedItems = [node]
const wrapper = mount(RightSidePanel, createMountConfig())
await nextTick()
// Should fallback to node.title when nodeManager is unavailable
expect(wrapper.text()).toContain('Test Node')
})🤖 Prompt for AI Agents |
||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ import EditableText from '@/components/common/EditableText.vue' | |||||||||||||||||||||||||||
| import Tab from '@/components/tab/Tab.vue' | ||||||||||||||||||||||||||||
| import TabList from '@/components/tab/TabList.vue' | ||||||||||||||||||||||||||||
| import Button from '@/components/ui/button/Button.vue' | ||||||||||||||||||||||||||||
| import { useVueNodeLifecycle } from '@/composables/graph/useVueNodeLifecycle' | ||||||||||||||||||||||||||||
| import { SubgraphNode } from '@/lib/litegraph/src/litegraph' | ||||||||||||||||||||||||||||
| import type { LGraphNode } from '@/lib/litegraph/src/litegraph' | ||||||||||||||||||||||||||||
| import { useSettingStore } from '@/platform/settings/settingStore' | ||||||||||||||||||||||||||||
|
|
@@ -24,6 +25,7 @@ import SubgraphEditor from './subgraph/SubgraphEditor.vue' | |||||||||||||||||||||||||||
| const canvasStore = useCanvasStore() | ||||||||||||||||||||||||||||
| const rightSidePanelStore = useRightSidePanelStore() | ||||||||||||||||||||||||||||
| const settingStore = useSettingStore() | ||||||||||||||||||||||||||||
| const { nodeManager } = useVueNodeLifecycle() | ||||||||||||||||||||||||||||
| const { t } = useI18n() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const { selectedItems } = storeToRefs(canvasStore) | ||||||||||||||||||||||||||||
|
|
@@ -58,9 +60,16 @@ const selectedNode = computed(() => { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const selectionCount = computed(() => selectedItems.value.length) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const selectedNodeTitle = computed(() => { | ||||||||||||||||||||||||||||
| if (!selectedNode.value) return null | ||||||||||||||||||||||||||||
| const nodeId = String(selectedNode.value.id) | ||||||||||||||||||||||||||||
| const nodeData = nodeManager.value?.vueNodeData.get(nodeId) | ||||||||||||||||||||||||||||
| return nodeData?.title || selectedNode.value.title || selectedNode.value.type | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
Comment on lines
+63
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Optional: Add clarifying comment for the fallback chain. The fallback logic is sound, but a brief comment explaining the priority would improve maintainability: 💡 Suggested clarifying comment const selectedNodeTitle = computed(() => {
if (!selectedNode.value) return null
const nodeId = String(selectedNode.value.id)
const nodeData = nodeManager.value?.vueNodeData.get(nodeId)
+ // Priority: reactive lifecycle data → node title → node type
return nodeData?.title || selectedNode.value.title || selectedNode.value.type
})📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const panelTitle = computed(() => { | ||||||||||||||||||||||||||||
| if (isSingleNodeSelected.value && selectedNode.value) { | ||||||||||||||||||||||||||||
| return selectedNode.value.title || selectedNode.value.type || 'Node' | ||||||||||||||||||||||||||||
| return selectedNodeTitle.value || 'Node' | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| return t('rightSidePanel.title', { count: selectionCount.value }) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
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.
🧩 Analysis chain
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 125
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4143
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 141
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3498
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 733
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 124
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5766
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 396
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1891
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 534
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 149
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3596
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4947
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4848
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4715
Direct property assignment does not trigger LiteGraph property change events for the title field.
The test assumes
node.title = 'Updated Title'will emit a'node:property:changed'event thatuseGraphNodeManagerlistens to, but the implementation shows event triggering is only explicitly implemented for specific properties (e.g.,shape). Thetitleproperty is a plain string with no setter interceptor, so direct assignment won't trigger the event handler that updatesvueNodeData.title. This test will fail.The title property must be updated through a method that explicitly calls
graph.trigger('node:property:changed', { property: 'title', ... }), or the test needs to mock the event emission, or a property setter needs to be added toLGraphNode.title.🤖 Prompt for AI Agents