Skip to content

Fix reactivity washing in refreshNodeSlots #7802

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.

Sign up for GitHub

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

Merged
AustinMroz merged 4 commits into from
Jan 11, 2026
Merged

Fix reactivity washing in refreshNodeSlots #7802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d1529b
Fix reactivity washing in refreshNodeSlots
AustinMroz Dec 30, 2025
2888c7c
Add test
AustinMroz Dec 30, 2025
f2980b1
Test nits
AustinMroz Dec 30, 2025
d361c94
Merge branch 'main' into austin/fix-reactivity-washing
DrJKL Jan 11, 2026
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
13 changes: 3 additions & 10 deletions src/composables/graph/useGraphNodeManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,17 +245,10 @@ export function useGraphNodeManager(graph: LGraph): GraphNodeManager {
})

// Update only widgets with new slot metadata, keeping other widget data intact
const updatedWidgets = currentData.widgets?.map((widget) => {
for (const widget of currentData.widgets ?? []) {
const slotInfo = slotMetadata.get(widget.name)
return slotInfo ? { ...widget, slotMetadata: slotInfo } : widget
})

vueNodeData.set(nodeId, {
...currentData,
widgets: updatedWidgets,
inputs: nodeRef.inputs ? [...nodeRef.inputs] : undefined,
outputs: nodeRef.outputs ? [...nodeRef.outputs] : undefined
})
if (slotInfo) widget.slotMetadata = slotInfo
Copy link
Contributor

@DrJKL DrJKL Jan 11, 2026

Choose a reason for hiding this comment

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

Just noting for the future, but this is the kind of thing that could be done PRN in the context of a WidgetDataStore.

}
Comment on lines +248 to +251
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 11, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

LGTM - Reactivity fix is correct!

The in-place mutation correctly preserves Vue reactivity by avoiding the spread operator's object reconstruction. This ensures widgets remain reactive after connections are made.

💡 Optional: Consider clearing stale slot metadata

The current code only updates widgets that have slot info. If a widget previously had a slot that was later removed (rare edge case), the old slotMetadata would persist. Consider:

 for (const widget of currentData.widgets ?? []) {
-  const slotInfo = slotMetadata.get(widget.name)
-  if (slotInfo) widget.slotMetadata = slotInfo
+  widget.slotMetadata = slotMetadata.get(widget.name)
 }

This assigns undefined to widgets without slots, ensuring stale metadata is cleared. However, this may not be necessary if structural changes are handled elsewhere via extractVueNodeData, and could have minor performance implications.

📝 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
for (const widget of currentData.widgets ?? []) {
const slotInfo = slotMetadata.get(widget.name)
return slotInfo ? { ...widget, slotMetadata: slotInfo } : widget
})
vueNodeData.set(nodeId, {
...currentData,
widgets: updatedWidgets,
inputs: nodeRef.inputs ? [...nodeRef.inputs] : undefined,
outputs: nodeRef.outputs ? [...nodeRef.outputs] : undefined
})
if (slotInfo) widget.slotMetadata = slotInfo
}
for (const widget of currentData.widgets ?? []) {
widget.slotMetadata = slotMetadata.get(widget.name)
}
🤖 Prompt for AI Agents 
In @src/composables/graph/useGraphNodeManager.ts around lines 248 - 251, The
loop that assigns slot metadata to widgets only sets widget.slotMetadata when
slotInfo exists, which can leave stale metadata on widgets whose slots were
removed; update the loop over currentData.widgets to assign widget.slotMetadata
= slotInfo ?? undefined (or explicitly set to undefined when no slotInfo) so
stale metadata is cleared, referencing the currentData.widgets iteration and
slotMetadata lookup and ensuring this change aligns with extractVueNodeData
expectations.

}

// Extract safe data from LiteGraph node for Vue consumption
Expand Down
49 changes: 49 additions & 0 deletions tests-ui/tests/composables/graph/useGraphNodeManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { setActivePinia } from 'pinia'
import { createTestingPinia } from '@pinia/testing'
import { describe, expect, it, vi } from 'vitest'
import { nextTick, watch } from 'vue'

import { useGraphNodeManager } from '@/composables/graph/useGraphNodeManager'
import { LGraph, LGraphNode } from '@/lib/litegraph/src/litegraph'
import { NodeSlotType } from '@/lib/litegraph/src/types/globalEnums'

setActivePinia(createTestingPinia())

function createTestGraph() {
const graph = new LGraph()
const node = new LGraphNode('test')
node.addInput('input', 'INT')
node.addWidget('number', 'testnum', 2, () => undefined, {})
graph.add(node)

const { vueNodeData } = useGraphNodeManager(graph)
const onReactivityUpdate = vi.fn()
watch(vueNodeData, onReactivityUpdate)

return [node, graph, onReactivityUpdate] as const
}
Comment on lines +12 to +24
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 30, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider capturing and calling cleanup for proper teardown.

The useGraphNodeManager returns a cleanup function that should be invoked to properly tear down event listeners and internal state. While the current tests work because each creates a new graph instance, capturing and calling cleanup is best practice for test hygiene.

🔎 Proposed refactor 
 function createTestGraph() {
   const graph = new LGraph()
   const node = new LGraphNode('test')
   node.addInput('input', 'INT')
   node.addWidget('number', 'testnum', 2, () => undefined, {})
   graph.add(node)
 
-  const { vueNodeData } = useGraphNodeManager(graph)
+  const { vueNodeData, cleanup } = useGraphNodeManager(graph)
   const onReactivityUpdate = vi.fn()
   watch(vueNodeData, onReactivityUpdate)
 
-  return [node, graph, onReactivityUpdate] as const
+  return [node, graph, onReactivityUpdate, cleanup] as const
 }
 
 describe('Node Reactivity', () => {
+  let cleanup: (() => void) | undefined
+
+  afterEach(() => {
+    cleanup?.()
+  })
+
   it('should trigger on callback', async () => {
-    const [node, , onReactivityUpdate] = createTestGraph()
+    const [node, , onReactivityUpdate, cleanupFn] = createTestGraph()
+    cleanup = cleanupFn

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In tests-ui/tests/composables/graph/useGraphNodeManager.test.ts around lines 12
to 24, the helper createTestGraph calls useGraphNodeManager but does not capture
or invoke the returned cleanup function; update createTestGraph to destructure
and return the cleanup function (or call cleanup in each test/afterEach) and
ensure tests invoke cleanup() after use to teardown event listeners and internal
state properly.


describe('Node Reactivity', () => {
it('should trigger on callback', async () => {
const [node, , onReactivityUpdate] = createTestGraph()

node.widgets![0].callback!(2)
await nextTick()
expect(onReactivityUpdate).toHaveBeenCalledTimes(1)
})

it('should remain reactive after a connection is made', async () => {
const [node, graph, onReactivityUpdate] = createTestGraph()

graph.trigger('node:slot-links:changed', {
nodeId: '1',
slotType: NodeSlotType.INPUT
})
await nextTick()
onReactivityUpdate.mockClear()

node.widgets![0].callback!(2)
await nextTick()
expect(onReactivityUpdate).toHaveBeenCalledTimes(1)
})
})
Loading