fix Vue node widgets should be in disabled state if their slots are connected with a link #5834
Conversation
dosubot
bot
added
the
size:M
🎭 Playwright Test Results⏰ Completed at: 10/08/2025, 12:36:12 AM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
christian-byrne
requested review from
benceruleanlu and
DrJKL
and removed request for
benceruleanlu
| graph.trigger('node:slot-links:changed', { | ||
| nodeId: this.node.id, | ||
| slotType: NodeSlotType.INPUT, | ||
| slotIndex: index, | ||
| connected: this._link != null, | ||
| linkId: this._link ?? null | ||
| }) |
There was a problem hiding this comment.
This feels like something that we should be doing in a store, if not now, then soon. I don't like complex decisions being made in components.
| nodeId: this.node.id, | ||
| slotType: NodeSlotType.INPUT, | ||
| slotIndex: index, | ||
| connected: this._link != null, |
There was a problem hiding this comment.
| connected: this._link != null, | |
| connected: this.isConnected, |
| const index = this.node.inputs.indexOf(this) | ||
| if (index === -1) return | ||
|
|
||
| graph.trigger('node:slot-links:changed', { | ||
| nodeId: this.node.id, | ||
| slotType: NodeSlotType.INPUT, | ||
| slotIndex: index, |
There was a problem hiding this comment.
option:
| const index = this.node.inputs.indexOf(this) | |
| if (index === -1) return | |
| graph.trigger('node:slot-links:changed', { | |
| nodeId: this.node.id, | |
| slotType: NodeSlotType.INPUT, | |
| slotIndex: index, | |
| const slotIndex = this.node.inputs.indexOf(this) | |
| if (slotIndex === -1) return | |
| graph.trigger('node:slot-links:changed', { | |
| nodeId: this.node.id, | |
| slotType: NodeSlotType.INPUT, | |
| slotIndex, |
|
|
||
| vueNodeData.set(nodeId, { | ||
| ...currentData, | ||
| inputs: node.inputs ? [...node.inputs] : undefined, |
There was a problem hiding this comment.
Not for this PR, but what if we defined inputs and outputs as being required, so that a lack of them would just be an empty array? Would that simplify some of the logic?
christian-byrne
requested review from
viva-jinyi,
ltdrdata,
arjansingh and
webfiltered
as code owners
dosubot
bot
added
size:XXL
christian-byrne
force-pushed
the
vue-node/instrument-widget-links
branch
from
5ab2ac6 to
e0b9095
Compare
dosubot
bot
added
size:L
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/08/2025, 12:25:55 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
There was a problem hiding this comment.
Most importantly: LGTM!
I'm still a little confused about why we're using the LGraph "trigger" system - the reason I mention that is just because it feels like we have added a few iterations of complexity and workarounds.
If we need to revisit again, maybe we should go more nuclear on the fix, and remove any possibility of decade-old design decisions haunting us any further?
| const refreshedData = extractVueNodeData(nodeRef) | ||
|
|
||
| vueNodeData.set(nodeId, { | ||
| ...currentData, | ||
| widgets: refreshedData.widgets, | ||
| inputs: refreshedData.inputs, | ||
| outputs: refreshedData.outputs | ||
| }) |
There was a problem hiding this comment.
| const refreshedData = extractVueNodeData(nodeRef) | |
| vueNodeData.set(nodeId, { | |
| ...currentData, | |
| widgets: refreshedData.widgets, | |
| inputs: refreshedData.inputs, | |
| outputs: refreshedData.outputs | |
| }) | |
| const { widgets, inputs, outputs } = extractVueNodeData(nodeRef) | |
| vueNodeData.set(nodeId, { ...currentData, widgets, inputs, outputs }) |
This is a sidegrade at best, but... I wanted to see if it would fit. After I checked, the "work" was already done, so please take / discard as you like.
| const vueComponent = getComponent(widget.type) || WidgetInputText | ||
| const slotMetadata = widget.slotMetadata |
There was a problem hiding this comment.
| const slotMetadata = widget.slotMetadata | |
| const { slotMetadata } = widget |
nit
There was a problem hiding this comment.
Could pull off options at the same time
| graph.trigger('node:slot-links:changed', { | ||
| nodeId: target.id, | ||
| slotType: NodeSlotType.INPUT, | ||
| slotIndex: link_info.target_slot, | ||
| connected: false, | ||
| linkId: link_info.id | ||
| }) |
There was a problem hiding this comment.
If this is the same in all three locations, how would you feel about extracting something like maybeTriggerLinkChange(...)?
There was a problem hiding this comment.
I think it would only be simplified if it were to be made a class instance property of some sort, which would start to go backwards a bit. If it's moved to a more pure helper, then it becomes even more code, I think. Don't see a way to simplify/refactor cleanly.
- Use discriminated union pattern for type-safe events without assertions
- Preserve monkey-patching/chaining for multiple system compatibility
- Convert old (action, param) format to new {type, ...} format in trigger()
- All handlers receive properly typed LGraphTriggerEvent objects
I think everyone who reads this all wants to go nuclear on the fix, but it doesn't fully align with business goals to dedicate a lot of time to doing it. We will gradually replace the system with signals-based implementation. |
|
Planning on making changes:
|
christian-byrne
added
the
claude-review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: fix Vue node widgets should be in disabled state if their slots are connected with a link (#5834)
Impact: 226 additions, 82 deletions across 8 files
Purpose: Implements proper reactive event system for Vue widgets to disable when their input slots are connected
Issue Distribution
- Critical: 0
- High: 3
- Medium: 4
- Low: 0
Category Breakdown
- Architecture: 2 issues
- Security: 1 issue
- Performance: 2 issues
- Code Quality: 2 issues
Key Findings
Architecture & Design
Event System Implementation: The PR introduces a new typed event system (graphTriggers.ts) for tracking slot link changes, which is a solid architectural choice. However, the implementation has some concerns:
- Event Triggering Scatter: The event triggering logic is duplicated across 4 locations in LGraphNode.ts, creating maintenance overhead
- Type Inconsistency: The event interfaces use string|number for nodeId instead of the established NodeId type
Vue Integration: The integration with Vue's reactivity system through useGraphNodeManager is well-designed, providing proper encapsulation of LiteGraph state for Vue consumption.
Security Considerations
Event Parameter Validation: The trigger method spreads event parameters directly without validation, which could potentially allow property injection. While this is internal code, defensive programming practices should still apply.
Performance Impact
Event Handler Efficiency: Two performance concerns identified:
- Full Re-extraction: The refreshNodeSlots function re-extracts all node data when only slot data has changed
- Event Frequency: Slot link change events could fire frequently during complex operations, triggering expensive Vue reactivity updates
Bundle Impact: The changes are relatively contained and shouldn't significantly impact bundle size.
Integration Points
Vue Nodes Mode: The feature properly integrates with existing Vue node infrastructure and respects the LiteGraph.vueNodesMode flag. The slot layout sync correctly handles event handler coordination between different systems.
Widget State Management: The core feature - automatically disabling Vue widgets when their slots are connected - addresses a real UX issue where users could have conflicting input sources.
Positive Observations
- Addresses Real User Problem: Fixes issue #5692 where Vue widgets remained enabled despite being converted to inputs
- Type Safety: Good use of TypeScript discriminated unions for event handling
- Backwards Compatibility: Changes maintain compatibility with existing non-Vue workflow
- Clean Event Architecture: The event system follows established patterns and uses proper TypeScript typing
- Proper Vue Integration: Correctly integrates with Vue's reactivity system without causing memory leaks
- Test Coverage: Includes updated test snapshots indicating proper validation
References
Next Steps
- Address architectural concerns around event triggering duplication (high priority)
- Consider performance optimizations for high-frequency event scenarios
- Add parameter validation to event spreading for defensive programming
- Consider centralizing the event triggering logic in NodeInputSlot setters
- Update type definitions to use consistent NodeId typing
This is a comprehensive automated review focusing on architectural implications, performance considerations, and code quality. The core functionality addresses a legitimate user experience issue and the implementation follows established patterns in the codebase.
Summary
Fixes #5692 by making widget link connection status trigger on change so Vue widgets with connected links could properly switch to the
disabledstate when they are implicitly converted to inputs.Changes
node:slot-links:changedevent tracking and reactive slot data synchronization for Vue widgetsgraph TD A[Widget Link Change] --> B[NodeInputSlot.link setter] B --> C{Is Widget Input?} C -->|Yes| D[Trigger slot-links:changed] C -->|No| E[End] D --> F[Graph Event Handler] F --> G[syncNodeSlotData] G --> H[Update Vue Reactive Data] H --> I[Widget Re-render] style A fill:#f9f9f9,stroke:#333,color:#000 style I fill:#f9f9f9,stroke:#333,color:#000Review Focus
Widget reactivity performance with frequent link changes and event handler memory management in graph operations.
┆Issue is synchronized with this Notion page by Unito