-
Notifications
You must be signed in to change notification settings - Fork 622
fix: stabilize nested subgraph promoted widget resolution #9282
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
Changes from 10 commits
d820cff
42201a2
32c88d3
38419cb
b7ef63d
4c0db3e
25afb54
6ac2cfb
9595fec
4e2c244
8690d82
41907c0
f1eea82
1e57842
9895929
835f5c8
356e726
ce1a32d
38b088d
31c1ead
c12fc97
2dbc101
62f23d0
4fa4642
62654c6
54d117c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,9 @@ import { reactive, shallowReactive } from 'vue' | |
|
|
||
| import { useChainCallback } from '@/composables/functional/useChainCallback' | ||
| import { isPromotedWidgetView } from '@/core/graph/subgraph/promotedWidgetTypes' | ||
| import { resolveConcretePromotedWidget } from '@/core/graph/subgraph/resolveConcretePromotedWidget' | ||
| import { resolvePromotedWidgetSource } from '@/core/graph/subgraph/resolvePromotedWidgetSource' | ||
| import { resolveSubgraphInputLink } from '@/core/graph/subgraph/resolveSubgraphInputLink' | ||
| import type { | ||
| INodeInputSlot, | ||
| INodeOutputSlot | ||
|
|
@@ -46,7 +48,9 @@ export interface WidgetSlotMetadata { | |
| */ | ||
| export interface SafeWidgetData { | ||
| nodeId?: NodeId | ||
| storeNodeId?: NodeId | ||
| name: string | ||
| storeName?: string | ||
| type: string | ||
| /** Callback to invoke when widget value changes (wraps LiteGraph callback + triggerDraw) */ | ||
| callback?: ((value: unknown) => void) | undefined | ||
|
|
@@ -161,7 +165,7 @@ function getSharedWidgetEnhancements( | |
| /** | ||
| * Validates that a value is a valid WidgetValue type | ||
| */ | ||
| const normalizeWidgetValue = (value: unknown): WidgetValue => { | ||
| function normalizeWidgetValue(value: unknown): WidgetValue { | ||
| if (value === null || value === undefined || value === void 0) { | ||
| return undefined | ||
| } | ||
|
|
@@ -193,11 +197,68 @@ function safeWidgetMapper( | |
| node: LGraphNode, | ||
| slotMetadata: Map<string, WidgetSlotMetadata> | ||
| ): (widget: IBaseWidget) => SafeWidgetData { | ||
| function extractWidgetDisplayOptions( | ||
| widget: IBaseWidget | ||
| ): SafeWidgetData['options'] { | ||
| if (!widget.options) return undefined | ||
|
|
||
| return { | ||
| canvasOnly: widget.options.canvasOnly, | ||
| advanced: widget.advanced, | ||
| hidden: widget.options.hidden, | ||
| read_only: widget.options.read_only | ||
| } | ||
| } | ||
|
|
||
| function resolveSourceSeedByInputName(inputName: string): { | ||
|
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. suggestion: Consider extracting a shared resolver function in
Contributor
Author
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. Two resolvers marched side by side, |
||
| sourceNodeId: string | ||
| sourceWidgetName: string | ||
| } | null { | ||
| const resolved = resolveSubgraphInputLink( | ||
| node, | ||
| inputName, | ||
| ({ inputNode, targetInput, getTargetWidget }) => { | ||
| if (inputNode.isSubgraphNode()) { | ||
| return { | ||
| sourceNodeId: String(inputNode.id), | ||
| sourceWidgetName: targetInput.name | ||
| } | ||
| } | ||
|
|
||
| const targetWidget = getTargetWidget() | ||
| if (!targetWidget) return undefined | ||
| return { | ||
| sourceNodeId: String(inputNode.id), | ||
| sourceWidgetName: targetWidget.name | ||
| } | ||
| } | ||
| ) | ||
|
|
||
| return resolved ?? null | ||
| } | ||
|
|
||
| return function (widget) { | ||
| try { | ||
| const promotedInputName = isPromotedWidgetView(widget) | ||
| ? node.inputs?.find((input) => { | ||
| if (input.name === widget.name) return true | ||
| if (input._widget === widget) return true | ||
| return false | ||
| })?.name | ||
| : undefined | ||
| const displayName = promotedInputName ?? widget.name | ||
| const promotedSourceSeed = | ||
|
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. suggestion (non-blocking): Similar to the SubgraphNode comment - as the project moves toward ECS-style centralized state, consider extracting the promoted widget identity resolution into a pure helper (e.g. Also a naming nit:
Contributor
Author
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. One path names the source, |
||
| isPromotedWidgetView(widget) && displayName | ||
| ? (resolveSourceSeedByInputName(displayName) ?? { | ||
| sourceNodeId: widget.sourceNodeId, | ||
| sourceWidgetName: widget.sourceWidgetName | ||
| }) | ||
| : null | ||
|
|
||
| // Get shared enhancements (controlWidget, spec, nodeType) | ||
| const sharedEnhancements = getSharedWidgetEnhancements(node, widget) | ||
| const slotInfo = slotMetadata.get(widget.name) | ||
| const slotInfo = | ||
| slotMetadata.get(displayName) ?? slotMetadata.get(widget.name) | ||
|
|
||
| // Wrapper callback specific to Nodes 2.0 rendering | ||
| const callback = (v: unknown) => { | ||
|
|
@@ -215,36 +276,52 @@ function safeWidgetMapper( | |
| isPromotedWidgetView(widget) && widget.sourceWidgetName.startsWith('$$') | ||
|
|
||
| // Extract only render-critical options (canvasOnly, advanced, read_only) | ||
| const options = widget.options | ||
| ? { | ||
| canvasOnly: widget.options.canvasOnly, | ||
| advanced: widget.advanced, | ||
| hidden: widget.options.hidden, | ||
| read_only: widget.options.read_only | ||
| } | ||
| : undefined | ||
| const options = extractWidgetDisplayOptions(widget) | ||
| const subgraphId = node.isSubgraphNode() && node.subgraph.id | ||
|
|
||
| const resolvedSourceResult = | ||
| isPromotedWidgetView(widget) && promotedSourceSeed | ||
|
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. suggestion (non-blocking): the new nested resolution paths in @coderabbitai Make an Issue to do this in a followup PR on a future date
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. 🧩 Analysis chain🏁 Script executed: Length of output: 57
🧠 Learnings used
Contributor
Author
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. Through two nested graphs it would roam,
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.
|
||
| ? resolveConcretePromotedWidget( | ||
| node, | ||
| promotedSourceSeed.sourceNodeId, | ||
| promotedSourceSeed.sourceWidgetName | ||
| ) | ||
| : null | ||
| const resolvedSource = | ||
| resolvedSourceResult?.status === 'resolved' | ||
| ? resolvedSourceResult.resolved | ||
| : undefined | ||
| const sourceWidget = resolvedSource?.widget | ||
| const sourceNode = resolvedSource?.node | ||
|
|
||
| const effectiveWidget = sourceWidget ?? widget | ||
|
|
||
| const localId = isPromotedWidgetView(widget) | ||
| ? widget.sourceNodeId | ||
| ? String(sourceNode?.id ?? promotedSourceSeed?.sourceNodeId) | ||
| : undefined | ||
| const nodeId = | ||
| subgraphId && localId ? `${subgraphId}:${localId}` : undefined | ||
| const name = isPromotedWidgetView(widget) | ||
| ? widget.sourceWidgetName | ||
| : widget.name | ||
| const storeName = isPromotedWidgetView(widget) | ||
| ? (sourceWidget?.name ?? promotedSourceSeed?.sourceWidgetName) | ||
| : undefined | ||
| const name = storeName ?? displayName | ||
|
|
||
| return { | ||
| nodeId, | ||
| storeNodeId: nodeId, | ||
| name, | ||
| type: widget.type, | ||
| storeName, | ||
| type: effectiveWidget.type, | ||
| ...sharedEnhancements, | ||
| callback, | ||
| hasLayoutSize: typeof widget.computeLayoutSize === 'function', | ||
| hasLayoutSize: typeof effectiveWidget.computeLayoutSize === 'function', | ||
| isDOMWidget: isDOMWidget(widget) || isPromotedDOMWidget(widget), | ||
| options: isPromotedPseudoWidget | ||
| ? { ...options, canvasOnly: true } | ||
| : options, | ||
| ? { | ||
| ...(extractWidgetDisplayOptions(effectiveWidget) ?? options), | ||
| canvasOnly: true | ||
| } | ||
| : (extractWidgetDisplayOptions(effectiveWidget) ?? options), | ||
| slotMetadata: slotInfo, | ||
| slotName: name !== widget.name ? widget.name : undefined | ||
|
DrJKL marked this conversation as resolved.
|
||
| } | ||
|
|
@@ -312,14 +389,17 @@ export function extractVueNodeData(node: LGraphNode): VueNodeData { | |
| }) | ||
|
|
||
| const safeWidgets = reactiveComputed<SafeWidgetData[]>(() => { | ||
| const widgetsSnapshot = node.widgets ?? [] | ||
|
|
||
| node.inputs?.forEach((input, index) => { | ||
| if (!input?.widget?.name) return | ||
| slotMetadata.set(input.widget.name, { | ||
| const slotInfo = { | ||
| index, | ||
| linked: input.link != null | ||
| }) | ||
| } | ||
| if (input.name) slotMetadata.set(input.name, slotInfo) | ||
| if (input.widget?.name) slotMetadata.set(input.widget.name, slotInfo) | ||
| }) | ||
| return node.widgets?.map(safeWidgetMapper(node, slotMetadata)) ?? [] | ||
| return widgetsSnapshot.map(safeWidgetMapper(node, slotMetadata)) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }) | ||
|
|
||
| const nodeType = | ||
|
|
@@ -375,11 +455,12 @@ export function useGraphNodeManager(graph: LGraph): GraphNodeManager { | |
| const slotMetadata = new Map<string, WidgetSlotMetadata>() | ||
|
|
||
| nodeRef.inputs?.forEach((input, index) => { | ||
| if (!input?.widget?.name) return | ||
| slotMetadata.set(input.widget.name, { | ||
| const slotInfo = { | ||
| index, | ||
| linked: input.link != null | ||
| }) | ||
| } | ||
| if (input.name) slotMetadata.set(input.name, slotInfo) | ||
| if (input.widget?.name) slotMetadata.set(input.widget.name, slotInfo) | ||
| }) | ||
|
|
||
| // Update only widgets with new slot metadata, keeping other widget data intact | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.