feat: centralize link topology state and compatibility boundaries#9247
Draft
feat: centralize link topology state and compatibility boundaries#9247
Conversation
- add passive LinkStore for links, floating links, and reroutes - rehydrate LinkStore on graph clear and configure lifecycle paths - add unit and browser tests for ChangeTracker/link topology undo-redo parity Amp-Thread-ID: https://ampcode.com/threads/T-019c9840-2f49-701c-bdf2-ae7f2f50acef Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c9886-c7b7-73f0-b85a-8a046e556ca8 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c9896-0c38-760a-a51c-3750734becc1 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c989e-444b-73fd-b60d-9d9e447d1212 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c98a5-85ea-75ec-9594-aea357107cf6 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c98b1-8cb2-704f-b7ac-7accee5228d0 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c98bb-ebfb-77fc-b105-1d04e023db7a Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c98c7-4fff-75cc-8950-0db38ec987fd Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c98d1-7608-73e5-9da5-3ae48178b28b Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019c98dc-6f6a-7125-b046-418a6ccdffb1 Co-authored-by: Amp <amp@ampcode.com>
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ❌ 543 passed, 3 failed · 6 flaky❌ Failed Tests📊 Browser Reports
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes link topology in a Pinia LinkStore, adds GraphLifecycleEventDispatcher and GraphPersistenceAdapter, introduces subgraph boundary adapters and canonical slot naming, shifts connect/disconnect flows to dispatcher-driven LGraph APIs, delegates clipboard remapping to the persistence adapter, and expands tests across link projection, undo/redo, subgraph IO, and lifecycle ordering. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LGraph
participant LinkStore
participant Dispatcher as GraphLifecycleEventDispatcher
participant SourceNode
participant TargetNode
Client->>LGraph: connectSlots(source,outIdx,target,inIdx,afterReroute?)
LGraph->>LinkStore: create/register link (rehydrate topology)
LGraph->>Dispatcher: dispatchConnectNodePair({source,outIdx,target,inIdx,link})
Dispatcher->>SourceNode: onConnectionsChange(OUTPUT,outIdx,link)
Dispatcher->>TargetNode: onConnectionsChange(INPUT,inIdx,link)
Dispatcher->>Dispatcher: dispatchSlotLinkChanged (emit node:slot-links:changed if widget)
LGraph->>LGraph: finalizeConnectedLink(link) (remove floating links / propagate reroutes)
LGraph->>LinkStore: rehydrate/post-change sync
LGraph->>Client: return link
sequenceDiagram
participant User
participant SubgraphInput
participant LGraph
participant LinkStore
participant Dispatcher as GraphLifecycleEventDispatcher
participant Node
User->>SubgraphInput: connect(node, slotIndex)
SubgraphInput->>LGraph: connectSubgraphInputSlot(...)
LGraph->>LinkStore: create/register link
LGraph->>Dispatcher: dispatchNodeConnectionChange({node, INPUT, slotIndex, connected:true, link})
Dispatcher->>Node: onConnectionsChange(INPUT, slotIndex, link)
SubgraphInput->>User: return link
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
📦 Bundle: 4.49 MB gzip 🔴 +2.1 kBDetailsSummary
Category Glance App Entry Points — 17.8 kB (baseline 17.8 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 913 kB (baseline 913 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.4 kB (baseline 72.4 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 10 added / 10 removed Panels & Settings — 436 kB (baseline 436 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16.1 kB (baseline 16.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 779 B (baseline 779 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 47 kB (baseline 47 kB) • ⚪ 0 BReusable component library chunks
Status: 11 added / 11 removed Data & Services — 2.74 MB (baseline 2.73 MB) • 🔴 +8.68 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 56.6 kB (baseline 56.6 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 12 added / 12 removed Vendor & Third-Party — 8.86 MB (baseline 8.86 MB) • 🟢 -12 BExternal libraries and shared vendor chunks
Status: 1 added / 1 removed Other — 7.92 MB (baseline 7.92 MB) • ⚪ 0 BBundles that do not match a named category
Status: 67 added / 67 removed |
⚡ Performance ReportNo regressions detected. All metrics
Historical variance (last 5 runs)
Raw data{
"timestamp": "2026-03-05T02:40:44.568Z",
"gitSha": "4d102ade8fe741fb7fe52f6fb1bd80991931a84b",
"branch": "drjkl/he-come-to-town",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2030.7390000000396,
"styleRecalcs": 124,
"styleRecalcDurationMs": 25.776999999999997,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 430.58700000000005,
"heapDeltaBytes": -1484924
},
{
"name": "canvas-idle",
"durationMs": 2024.39099999998,
"styleRecalcs": 121,
"styleRecalcDurationMs": 20.304,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 415.5639999999999,
"heapDeltaBytes": -1678768
},
{
"name": "canvas-idle",
"durationMs": 2075.220999999999,
"styleRecalcs": 128,
"styleRecalcDurationMs": 29.883999999999997,
"layouts": 1,
"layoutDurationMs": 0.27599999999999997,
"taskDurationMs": 450.918,
"heapDeltaBytes": -1229416
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2125.7699999999886,
"styleRecalcs": 191,
"styleRecalcDurationMs": 63.238,
"layouts": 12,
"layoutDurationMs": 3.81,
"taskDurationMs": 1180.2440000000001,
"heapDeltaBytes": -1649400
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1834.1500000000224,
"styleRecalcs": 166,
"styleRecalcDurationMs": 50.956,
"layouts": 12,
"layoutDurationMs": 3.306,
"taskDurationMs": 811.5969999999999,
"heapDeltaBytes": -3926836
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1841.7239999999993,
"styleRecalcs": 168,
"styleRecalcDurationMs": 49.93300000000001,
"layouts": 12,
"layoutDurationMs": 3.1119999999999997,
"taskDurationMs": 818.1709999999999,
"heapDeltaBytes": -3596604
},
{
"name": "dom-widget-clipping",
"durationMs": 582.1189999999774,
"styleRecalcs": 43,
"styleRecalcDurationMs": 13.069,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 350.99,
"heapDeltaBytes": 6709460
},
{
"name": "dom-widget-clipping",
"durationMs": 550.0049999999987,
"styleRecalcs": 41,
"styleRecalcDurationMs": 13.603,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 345.729,
"heapDeltaBytes": 6253184
},
{
"name": "dom-widget-clipping",
"durationMs": 582.4649999999565,
"styleRecalcs": 42,
"styleRecalcDurationMs": 13.781999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 360.39500000000004,
"heapDeltaBytes": 7872124
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 616.1430000000223,
"styleRecalcs": 77,
"styleRecalcDurationMs": 19.238,
"layouts": 1,
"layoutDurationMs": 0.174,
"taskDurationMs": 436.824,
"heapDeltaBytes": -7025736
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 624.7599999999807,
"styleRecalcs": 75,
"styleRecalcDurationMs": 16.041,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 427.74700000000007,
"heapDeltaBytes": 14874456
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 589.6789999999896,
"styleRecalcs": 73,
"styleRecalcDurationMs": 17.043,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 436.81199999999995,
"heapDeltaBytes": -7723868
},
{
"name": "subgraph-idle",
"durationMs": 2004.0099999999939,
"styleRecalcs": 121,
"styleRecalcDurationMs": 23.595000000000006,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 412.399,
"heapDeltaBytes": -1843336
},
{
"name": "subgraph-idle",
"durationMs": 1993.7930000000392,
"styleRecalcs": 120,
"styleRecalcDurationMs": 19.720000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 382.28000000000003,
"heapDeltaBytes": -1846388
},
{
"name": "subgraph-idle",
"durationMs": 2031.8870000000402,
"styleRecalcs": 125,
"styleRecalcDurationMs": 25.794999999999998,
"layouts": 1,
"layoutDurationMs": 0.20699999999999993,
"taskDurationMs": 414.15500000000003,
"heapDeltaBytes": -1407688
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1776.8949999999677,
"styleRecalcs": 159,
"styleRecalcDurationMs": 50.136,
"layouts": 16,
"layoutDurationMs": 4.357,
"taskDurationMs": 767.7969999999999,
"heapDeltaBytes": -5192208
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 2007.2460000000092,
"styleRecalcs": 173,
"styleRecalcDurationMs": 62.847000000000016,
"layouts": 16,
"layoutDurationMs": 4.229,
"taskDurationMs": 1009.849,
"heapDeltaBytes": -3824472
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1967.0219999999858,
"styleRecalcs": 171,
"styleRecalcDurationMs": 59.21,
"layouts": 16,
"layoutDurationMs": 4.923,
"taskDurationMs": 1022.5889999999999,
"heapDeltaBytes": -4081232
}
]
} |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/litegraph/src/subgraph/SubgraphOutput.ts (1)
54-68:⚠️ Potential issue | 🟠 MajorEmit disconnect lifecycle event when replacing an existing subgraph-output link.
Line 62 removes the previous link but does not dispatch a
connected: falsenode-connection change for the old output side, so disconnect callbacks can be skipped.🔧 Proposed fix
if (existingLink != null) { subgraph.beforeChange() const { outputNode } = existingLink.resolve(subgraph) if (!outputNode) throw new Error('Expected output node for existing link') + const previousOutput = outputNode.outputs[existingLink.origin_slot] subgraph.disconnectSubgraphOutputLink( this, outputNode, existingLink.origin_slot, existingLink ) + + if (previousOutput) { + graphLifecycleEventDispatcher.dispatchNodeConnectionChange({ + node: outputNode, + slotType: NodeSlotType.OUTPUT, + slotIndex: existingLink.origin_slot, + connected: false, + link: existingLink, + slot: previousOutput + }) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphOutput.ts` around lines 54 - 68, Existing code disconnects the old link but does not notify the graph that the output node lost a connection; after calling subgraph.disconnectSubgraphOutputLink(this, outputNode, existingLink.origin_slot, existingLink) emit the same node-connection change used elsewhere to indicate the output side was disconnected (i.e., dispatch a "connected: false" change for the old output node). Locate the helper/event used for node connection changes in this codebase (for example any method named nodeConnectionChanged, dispatchNodeConnectionChange, emitConnectionChange, or similar) and call it with the outputNode, this output slot, and connected: false immediately after disconnectSubgraphOutputLink (keeping the existing subgraph.beforeChange() call).
🧹 Nitpick comments (1)
src/lib/litegraph/src/LGraphCanvas.ts (1)
8860-8865: Guard proxy-widget remap calls to mapped subgraph types.Calling the adapter for every string-typed node relies on adapter no-op handling for
undefined. An explicit guard makes caller-side behavior clearer and keeps the remap boundary more deterministic.♻️ Suggested refactor
for (const nodeInfo of allNodeInfo) { if (typeof nodeInfo.type !== 'string') continue - graphPersistenceAdapter.remapProxyWidgets( - nodeInfo, - subgraphNodeIdMap.get(nodeInfo.type) - ) + const remappedIds = subgraphNodeIdMap.get(nodeInfo.type) + if (!remappedIds) continue + graphPersistenceAdapter.remapProxyWidgets(nodeInfo, remappedIds) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/LGraphCanvas.ts` around lines 8860 - 8865, The loop calls graphPersistenceAdapter.remapProxyWidgets for any nodeInfo whose type is a string, but passes subgraphNodeIdMap.get(nodeInfo.type) which may be undefined; add an explicit guard so remapProxyWidgets is only invoked when subgraphNodeIdMap.has(nodeInfo.type) (or the result of get(...) is non-undefined). Update the block iterating allNodeInfo to check subgraphNodeIdMap.has(nodeInfo.type) (or store const mapped = subgraphNodeIdMap.get(nodeInfo.type) and check mapped !== undefined) before calling graphPersistenceAdapter.remapProxyWidgets(nodeInfo, mapped) to make the boundary deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/litegraph/src/LGraphNode.ts`:
- Around line 3236-3239: The disconnect dispatch is using the loop index into
output.links (i) instead of the actual source slot index, causing wrong-slot
notifications; update the call to
graphLifecycleEventDispatcher.dispatchDisconnectNodePair so sourceSlotIndex is
set to the origin slot index from the link object (e.g., use link.origin or
link.slot or link.origin_slotIndex depending on the link shape) rather than the
array index i, keeping sourceNode: target_node and sourceSlot: output unchanged;
locate the code around graphLifecycleEventDispatcher.dispatchDisconnectNodePair
and replace the i argument with the correct property from the link object
referenced when iterating output.links.
In `@src/lib/litegraph/src/subgraph/SubgraphInputNode.ts`:
- Around line 193-200: The dispatch payload on disconnect is using the wrong
slot object (slot: subgraphInput) for
graphLifecycleEventDispatcher.dispatchNodeConnectionChange with
NodeSlotType.INPUT; change the slot value to the actual target node input (e.g.,
node.inputs[slotIndex] or the node's getter such as node.getInputSlot(slotIndex)
if available) so the payload reflects the node's input slot, keeping slotIndex
and link unchanged.
In `@src/lib/litegraph/src/subgraph/SubgraphIO.test.ts`:
- Line 10: The import line for LinkDirection has a typo with a double slash in
the module path; update the import that references LinkDirection (the statement
importing from '@/lib/litegraph/src//types/globalEnums') to remove the extra
slash so it imports from '@/lib/litegraph/src/types/globalEnums'.
In `@src/scripts/app.test.ts`:
- Around line 161-173: Replace the unsafe casts: instead of "(app as
any).addAfterConfigureHandler(graph)" cast app to the narrow interface that
exposes addAfterConfigureHandler (e.g., an AppWithAfterConfigureHandler type)
and call addAfterConfigureHandler directly; also replace "graph.onConfigure?.({}
as never)" with a properly typed mock configuration value that matches the
expected parameter type of graph.onConfigure (or create a minimal stub object
typed to that config interface) so both type assertions are removed and type
safety is preserved for addAfterConfigureHandler, graph, and onConfigure.
---
Outside diff comments:
In `@src/lib/litegraph/src/subgraph/SubgraphOutput.ts`:
- Around line 54-68: Existing code disconnects the old link but does not notify
the graph that the output node lost a connection; after calling
subgraph.disconnectSubgraphOutputLink(this, outputNode,
existingLink.origin_slot, existingLink) emit the same node-connection change
used elsewhere to indicate the output side was disconnected (i.e., dispatch a
"connected: false" change for the old output node). Locate the helper/event used
for node connection changes in this codebase (for example any method named
nodeConnectionChanged, dispatchNodeConnectionChange, emitConnectionChange, or
similar) and call it with the outputNode, this output slot, and connected: false
immediately after disconnectSubgraphOutputLink (keeping the existing
subgraph.beforeChange() call).
---
Nitpick comments:
In `@src/lib/litegraph/src/LGraphCanvas.ts`:
- Around line 8860-8865: The loop calls
graphPersistenceAdapter.remapProxyWidgets for any nodeInfo whose type is a
string, but passes subgraphNodeIdMap.get(nodeInfo.type) which may be undefined;
add an explicit guard so remapProxyWidgets is only invoked when
subgraphNodeIdMap.has(nodeInfo.type) (or the result of get(...) is
non-undefined). Update the block iterating allNodeInfo to check
subgraphNodeIdMap.has(nodeInfo.type) (or store const mapped =
subgraphNodeIdMap.get(nodeInfo.type) and check mapped !== undefined) before
calling graphPersistenceAdapter.remapProxyWidgets(nodeInfo, mapped) to make the
boundary deterministic.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
browser_tests/tests/changeTracker.spec.tssrc/lib/litegraph/src/LGraph.test.tssrc/lib/litegraph/src/LGraph.tssrc/lib/litegraph/src/LGraphCanvas.clipboard.test.tssrc/lib/litegraph/src/LGraphCanvas.tssrc/lib/litegraph/src/LGraphNode.test.tssrc/lib/litegraph/src/LGraphNode.tssrc/lib/litegraph/src/LLink.test.tssrc/lib/litegraph/src/LLink.tssrc/lib/litegraph/src/Reroute.tssrc/lib/litegraph/src/infrastructure/GraphLifecycleEventDispatcher.tssrc/lib/litegraph/src/infrastructure/GraphPersistenceAdapter.tssrc/lib/litegraph/src/infrastructure/SubgraphEventMap.tssrc/lib/litegraph/src/subgraph/SubgraphIO.test.tssrc/lib/litegraph/src/subgraph/SubgraphInput.tssrc/lib/litegraph/src/subgraph/SubgraphInputNode.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/lib/litegraph/src/subgraph/SubgraphOutput.tssrc/lib/litegraph/src/subgraph/subgraphUtils.tssrc/scripts/app.test.tssrc/scripts/app.tssrc/scripts/changeTracker.test.tssrc/stores/linkStore.tssrc/utils/litegraphUtil.test.tssrc/utils/litegraphUtil.ts
src/scripts/app.test.ts
Outdated
Comment on lines
+161
to
+173
| ;(app as any).addAfterConfigureHandler(graph) | ||
| graph.onConfigure?.({} as never) | ||
|
|
||
| expect(hasLegacyLinkInputSlotMismatch).toHaveBeenCalledWith(graph) | ||
| expect(fixLinkInputSlots).toHaveBeenCalledWith(graph) | ||
| }) | ||
|
|
||
| it('skips legacy slot repair when no mismatch is present', () => { | ||
| vi.mocked(hasLegacyLinkInputSlotMismatch).mockReturnValue(false) | ||
| const graph = createConfigureGraph() | ||
|
|
||
| ;(app as any).addAfterConfigureHandler(graph) | ||
| graph.onConfigure?.({} as never) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the test file around lines 161-173
sed -n '155,180p' src/scripts/app.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 952
🏁 Script executed:
#!/bin/bash
# Check the type of app variable and its definition in the test file
head -50 src/scripts/app.test.ts | grep -A5 -B5 'app\|import'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 698
🏁 Script executed:
#!/bin/bash
# Find where addAfterConfigureHandler is defined
rg 'addAfterConfigureHandler' --type ts -B2 -A2 | head -50Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1230
🏁 Script executed:
#!/bin/bash
# Check the full implementation of addAfterConfigureHandler to understand why it's private
sed -n '1,50p' src/scripts/app.ts | grep -A20 'class ComfyApp'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Look at the full addAfterConfigureHandler method implementation
rg -A10 'private addAfterConfigureHandler' src/scripts/app.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 458
🏁 Script executed:
#!/bin/bash
# Check if there are any comments in the test explaining the private method call
sed -n '140,180p' src/scripts/app.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1363
🏁 Script executed:
#!/bin/bash
# Check if Partial<ComfyApp> or a proper type is used elsewhere in tests for similar scenarios
rg 'Partial<' src/scripts/app.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 161
🏁 Script executed:
#!/bin/bash
# Check class definition and how addAfterConfigureHandler is used internally
rg 'class ComfyApp' src/scripts/app.ts -A5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Verify if addAfterConfigureHandler is called publicly elsewhere or if it's only internal
rg 'addAfterConfigureHandler' src/scripts/app.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern for accessing private methods in other tests in the codebase
rg '\(.*as unknown as' src/**/*.test.ts -A1 | head -30Repository: Comfy-Org/ComfyUI_frontend
Length of output: 932
🏁 Script executed:
#!/bin/bash
# Check if there are other private method tests that use similar patterns
rg 'private.*graph.*LGraph' src/scripts/app.ts -B2 -A8Repository: Comfy-Org/ComfyUI_frontend
Length of output: 388
🏁 Script executed:
#!/bin/bash
# Verify the exact nature of the cast - check if the method should potentially be protected/public or if it's intentionally private
git log -p --all -S 'addAfterConfigureHandler' -- src/scripts/app.ts | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Verify the exact signature of addAfterConfigureHandler to ensure the suggested interface is correct
rg 'private addAfterConfigureHandler' src/scripts/app.ts -A3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 281
🏁 Script executed:
#!/bin/bash
# Check what type is passed to graph.onConfigure call to understand the 'as never' cast
rg 'onConfigure.*=.*function' src/scripts/app.ts -A5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 285
🏁 Script executed:
#!/bin/bash
# Check LGraph type definition to understand what onConfigure expects
rg 'interface LGraph|type LGraph' src/lib/litegraph/src/litegraph.ts -A15 | grep -A15 'onConfigure'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Verify if there are other similar private method test patterns we can learn from
rg '\(.*as unknown as.*\)' src/scripts/app.test.ts -B3 -A3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Find LGraph type definition
fd 'litegraph' -t f src/lib/litegraph/src/ | head -5Repository: Comfy-Org/ComfyUI_frontend
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Search for onConfigure property in LGraph
rg 'onConfigure' src/lib/litegraph/ -B2 -A2 | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2008
🏁 Script executed:
#!/bin/bash
# Check the actual test helper function to understand the graph mock structure
sed -n '145,155p' src/scripts/app.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Check what ISerialisedGraph and SerialisableGraph types are
rg 'type ISerialisedGraph|interface ISerialisedGraph' src/lib/litegraph/ -A3Repository: Comfy-Org/ComfyUI_frontend
Length of output: 384
🏁 Script executed:
#!/bin/bash
# Get a complete view of both test cases to check all casts
sed -n '150,180p' src/scripts/app.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1082
🏁 Script executed:
#!/bin/bash
# Check if there are any other 'as any' or problematic casts in the entire test file
rg 'as any|as never' src/scripts/app.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 251
🏁 Script executed:
#!/bin/bash
# Verify the suggested interface in the fix matches what the method actually expects
cat > /tmp/test_interface.ts << 'EOF'
type AppWithAfterConfigureHandler = {
addAfterConfigureHandler(graph: any): void
}
EOF
cat /tmp/test_interface.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern for mocking empty graph data in other tests
rg '{} as' src/scripts/*.test.ts -B2 -A2 | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check patterns of mock data with type casts
rg 'as never\|as unknown' src/scripts/app.test.ts -B1 -A1Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check if there's a proper type for the empty graph configuration data
rg 'ISerialisedGraph\|SerialisableGraph' src/scripts/app.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Review the full context - what the handler actually does with the onConfigure argument
sed -n '1,30p' src/scripts/app.ts | grep -A15 'addAfterConfigureHandler'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Final verification: Check the complete method implementation to ensure our understanding is correct
rg -A20 'private addAfterConfigureHandler\(graph: LGraph\)' src/scripts/app.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 870
🏁 Script executed:
#!/bin/bash
# Verify that ComfyApp is imported correctly in the test and understand the app instance type
sed -n '1,30p' src/scripts/app.test.tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 787
Fix both type assertions: replace as any with a proper type cast and address the problematic as never cast.
The (app as any) bypasses type safety. Use a narrow interface type cast instead. Additionally, graph.onConfigure?.({} as never) is problematic—the empty object should match the expected configuration data type.
🔧 Suggested fix
+ type AppWithAfterConfigureHandler = {
+ addAfterConfigureHandler(graph: LGraph): void
+ }
it('runs legacy slot repair when mismatch is detected', () => {
vi.mocked(hasLegacyLinkInputSlotMismatch).mockReturnValue(true)
const graph = createConfigureGraph()
- ;(app as any).addAfterConfigureHandler(graph)
- graph.onConfigure?.({} as never)
+ ;(app as unknown as AppWithAfterConfigureHandler)
+ .addAfterConfigureHandler(graph)
+ graph.onConfigure?.({} as any as Parameters<typeof graph.onConfigure>[0])
expect(hasLegacyLinkInputSlotMismatch).toHaveBeenCalledWith(graph)
expect(fixLinkInputSlots).toHaveBeenCalledWith(graph)
})
it('skips legacy slot repair when no mismatch is present', () => {
vi.mocked(hasLegacyLinkInputSlotMismatch).mockReturnValue(false)
const graph = createConfigureGraph()
- ;(app as any).addAfterConfigureHandler(graph)
- graph.onConfigure?.({} as never)
+ ;(app as unknown as AppWithAfterConfigureHandler)
+ .addAfterConfigureHandler(graph)
+ graph.onConfigure?.({} as any as Parameters<typeof graph.onConfigure>[0])
expect(hasLegacyLinkInputSlotMismatch).toHaveBeenCalledWith(graph)
expect(fixLinkInputSlots).not.toHaveBeenCalled()
})Per coding guidelines src/**/*.{ts,vue}: Never use as any type; use proper TypeScript types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/app.test.ts` around lines 161 - 173, Replace the unsafe casts:
instead of "(app as any).addAfterConfigureHandler(graph)" cast app to the narrow
interface that exposes addAfterConfigureHandler (e.g., an
AppWithAfterConfigureHandler type) and call addAfterConfigureHandler directly;
also replace "graph.onConfigure?.({} as never)" with a properly typed mock
configuration value that matches the expected parameter type of
graph.onConfigure (or create a minimal stub object typed to that config
interface) so both type assertions are removed and type safety is preserved for
addAfterConfigureHandler, graph, and onConfigure.
Amp-Thread-ID: https://ampcode.com/threads/T-019c98dc-6f6a-7125-b046-418a6ccdffb1 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/platform/surveys/NightlySurveyPopover.test.ts (1)
93-93: Remove the 15000ms timeout; it's unnecessary with fake timers and can mask async hangs.The test uses
vi.useFakeTimers()and advances time deterministically by 100ms withvi.advanceTimersByTimeAsync(100), so this long timeout serves no purpose and only slows failure feedback in CI. The other tests in this suite don't use explicit timeouts—keep the default instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/surveys/NightlySurveyPopover.test.ts` at line 93, Remove the explicit 15000ms Jest timeout on the test that uses vi.useFakeTimers() and vi.advanceTimersByTimeAsync(100); locate the test block that ends with "}, 15000)" in NightlySurveyPopover.test.ts and change it to simply "}" (no timeout argument) so the test uses the default timeout and relies on fake timers for deterministic timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/platform/surveys/NightlySurveyPopover.test.ts`:
- Line 93: Remove the explicit 15000ms Jest timeout on the test that uses
vi.useFakeTimers() and vi.advanceTimersByTimeAsync(100); locate the test block
that ends with "}, 15000)" in NightlySurveyPopover.test.ts and change it to
simply "}" (no timeout argument) so the test uses the default timeout and relies
on fake timers for deterministic timing.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/litegraph/src/canvas/LinkConnector.core.test.tssrc/platform/surveys/NightlySurveyPopover.test.ts
…happens on manual rename
Amp-Thread-ID: https://ampcode.com/threads/T-019c9bb5-a77c-72ef-9b51-5394c20ee671 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/litegraph/src/subgraph/SubgraphInputNode.ts (1)
206-213:⚠️ Potential issue | 🟠 MajorDisconnect payload should reference the node input slot, not subgraph slot.
With
slotType: NodeSlotType.INPUTandnode, Line 212 should pass the actual
inputslot object. UsingsubgraphInputbreaks payload parity for listeners
expecting the node-input-side slot.🔧 Suggested fix
graphLifecycleEventDispatcher.dispatchNodeConnectionChange({ node, slotType: NodeSlotType.INPUT, slotIndex, connected: false, link, - slot: subgraphInput + slot: input })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphInputNode.ts` around lines 206 - 213, The dispatch call in graphLifecycleEventDispatcher.dispatchNodeConnectionChange is using the wrong slot object: when sending slotType: NodeSlotType.INPUT with the node, pass the node-side input slot (variable input) instead of subgraphInput so listeners receive the node input slot; update the payload property slot: subgraphInput to slot: input while keeping node, slotIndex, connected, and link as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/litegraph/src/subgraph/SubgraphInputNode.ts`:
- Around line 181-200: The stale-link fallback in disconnectNodeInput mutates
the graph (calls subgraph.disconnectLink, removes floating links, updates
_version and setDirtyCanvas) but returns before emitting the lifecycle event;
update the fallback to call
graphLifecycleEventDispatcher.dispatchNodeConnectionChange(...) with the same
parameters used in the normal disconnect path (use the current subgraph, node
id, input id/name and the new/old connection state as appropriate) immediately
after performing subgraph.disconnectLink (and before return) so consumers
receive the node connection change; ensure you reference disconnectNodeInput,
subgraph.disconnectLink and
graphLifecycleEventDispatcher.dispatchNodeConnectionChange when making the
change.
---
Duplicate comments:
In `@src/lib/litegraph/src/subgraph/SubgraphInputNode.ts`:
- Around line 206-213: The dispatch call in
graphLifecycleEventDispatcher.dispatchNodeConnectionChange is using the wrong
slot object: when sending slotType: NodeSlotType.INPUT with the node, pass the
node-side input slot (variable input) instead of subgraphInput so listeners
receive the node input slot; update the payload property slot: subgraphInput to
slot: input while keeping node, slotIndex, connected, and link as-is.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/litegraph/src/subgraph/SubgraphIO.test.tssrc/lib/litegraph/src/subgraph/SubgraphInputNode.tssrc/lib/litegraph/src/subgraph/SubgraphOutput.ts
- LGraphNode: use link_info.origin_slot instead of array index for sourceSlotIndex in dispatchDisconnectNodePair - SubgraphInputNode: pass node input slot instead of subgraphInput for INPUT disconnect dispatch - SubgraphIO.test: fix double slash in import path - NightlySurveyPopover.test: remove unnecessary 15000ms timeout on fake-timer test Amp-Thread-ID: https://ampcode.com/threads/T-019c9bcb-2c95-712d-8978-ab8e9e688e4d Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/litegraph/src/subgraph/SubgraphInputNode.ts (1)
181-200:⚠️ Potential issue | 🟠 MajorStale-link fallback mutates topology without lifecycle disconnect event.
The fallback path (when
subgraphInputis not found) performs cleanup operations includingsubgraph.disconnectLink(), version increment, andsetDirtyCanvas(), but returns before dispatchingdispatchNodeConnectionChange. This can desync store/listener consumers that rely on lifecycle events.Consider dispatching the lifecycle event before returning:
🔧 Suggested fix
input.link = null if (link) { subgraph.disconnectLink(link, 'output') subgraph._version++ } subgraph.setDirtyCanvas(false, true) + graphLifecycleEventDispatcher.dispatchNodeConnectionChange({ + node, + slotType: NodeSlotType.INPUT, + slotIndex, + connected: false, + link, + slot: input + }) return },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphInputNode.ts` around lines 181 - 200, The fallback branch where subgraphInput is missing mutates topology (removing floating links, nulling input.link, calling subgraph.disconnectLink, bumping subgraph._version and setDirtyCanvas) but returns without emitting the lifecycle event; update the branch in SubgraphInputNode (the block that checks if (!subgraphInput)) to call dispatchNodeConnectionChange with the same arguments/semantics used elsewhere in this file (after performing the disconnect/remove operations and before returning) so listeners see the connection change; ensure you reference input._floatingLinks, subgraph.removeFloatingLink, subgraph.disconnectLink, and subgraph.setDirtyCanvas as in the existing code and emit dispatchNodeConnectionChange prior to the early return.
🧹 Nitpick comments (2)
src/lib/litegraph/src/LGraphNode.ts (1)
3111-3140: Unify non-target disconnects through the node-pair dispatcher for parity.
disconnectOutput(slot)currently uses manual per-node dispatch in this branch, while other disconnect flows usedispatchDisconnectNodePair. Using the pair dispatcher here too would reduce ordering drift risk across disconnect variants.♻️ Proposed refactor
- graphLifecycleEventDispatcher.dispatchNodeConnectionChange({ - node: target, - slotType: NodeSlotType.INPUT, - slotIndex: link_info.target_slot, - connected: false, - link: link_info, - slot: input - }) - } - // remove the link from the links pool - graph.disconnectLink(link_info, 'input') - - graphLifecycleEventDispatcher.dispatchNodeConnectionChange({ - node: this, - slotType: NodeSlotType.OUTPUT, - slotIndex: slot, - connected: false, - link: link_info, - slot: output - }) + } + graph.disconnectLink(link_info, 'input') + if (target) { + graphLifecycleEventDispatcher.dispatchDisconnectNodePair({ + sourceNode: this, + sourceSlotIndex: slot, + sourceSlot: output, + targetNode: target, + targetSlotIndex: link_info.target_slot, + targetSlot: input, + link: link_info + }) + } else { + graphLifecycleEventDispatcher.dispatchNodeConnectionChange({ + node: this, + slotType: NodeSlotType.OUTPUT, + slotIndex: slot, + connected: false, + link: link_info, + slot: output + }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/LGraphNode.ts` around lines 3111 - 3140, disconnectOutput(slot) is dispatching a single-node connection change (graphLifecycleEventDispatcher.dispatchNodeConnectionChange) for the output side after graph.disconnectLink instead of using the existing dispatchDisconnectNodePair helper, which other paths use; replace the manual per-node dispatch in disconnectOutput with a call to dispatchDisconnectNodePair (providing this node, the target node/link info, NodeSlotType.OUTPUT, slot index, connected:false and the output slot object) so both nodes are dispatched together and ordering matches other disconnect flows (ensure you remove the lone dispatchNodeConnectionChange and use dispatchDisconnectNodePair consistently).src/lib/litegraph/src/subgraph/SubgraphIO.test.ts (1)
384-452: Consider adding explicit output lifecycle ordering assertions.You already assert input ordering (
eventvs callback). Mirroring that for output connect/disconnect would lock parity guarantees across both sides and reduce regressions in dispatcher ordering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphIO.test.ts` around lines 384 - 452, Add explicit assertions that verify the ordering of output connect/disconnect lifecycle events (mirror the input ordering tests): after creating the link and after calling subgraphOutput.disconnect(), assert the call order between subgraph.outputNode's event and internalNode.onConnectionsChange by inspecting outputCallback.mock.invocationCallOrder (or using toHaveBeenNthCalledWith) so you assert which handler ran first for connect and which ran first for disconnect; reference the symbols subgraphOutput, internalNode, outputCallback, subgraph.outputNode, and the created link to locate where to insert these ordering checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/litegraph/src/subgraph/SubgraphInputNode.ts`:
- Around line 181-200: The fallback branch where subgraphInput is missing
mutates topology (removing floating links, nulling input.link, calling
subgraph.disconnectLink, bumping subgraph._version and setDirtyCanvas) but
returns without emitting the lifecycle event; update the branch in
SubgraphInputNode (the block that checks if (!subgraphInput)) to call
dispatchNodeConnectionChange with the same arguments/semantics used elsewhere in
this file (after performing the disconnect/remove operations and before
returning) so listeners see the connection change; ensure you reference
input._floatingLinks, subgraph.removeFloatingLink, subgraph.disconnectLink, and
subgraph.setDirtyCanvas as in the existing code and emit
dispatchNodeConnectionChange prior to the early return.
---
Nitpick comments:
In `@src/lib/litegraph/src/LGraphNode.ts`:
- Around line 3111-3140: disconnectOutput(slot) is dispatching a single-node
connection change (graphLifecycleEventDispatcher.dispatchNodeConnectionChange)
for the output side after graph.disconnectLink instead of using the existing
dispatchDisconnectNodePair helper, which other paths use; replace the manual
per-node dispatch in disconnectOutput with a call to dispatchDisconnectNodePair
(providing this node, the target node/link info, NodeSlotType.OUTPUT, slot
index, connected:false and the output slot object) so both nodes are dispatched
together and ordering matches other disconnect flows (ensure you remove the lone
dispatchNodeConnectionChange and use dispatchDisconnectNodePair consistently).
In `@src/lib/litegraph/src/subgraph/SubgraphIO.test.ts`:
- Around line 384-452: Add explicit assertions that verify the ordering of
output connect/disconnect lifecycle events (mirror the input ordering tests):
after creating the link and after calling subgraphOutput.disconnect(), assert
the call order between subgraph.outputNode's event and
internalNode.onConnectionsChange by inspecting
outputCallback.mock.invocationCallOrder (or using toHaveBeenNthCalledWith) so
you assert which handler ran first for connect and which ran first for
disconnect; reference the symbols subgraphOutput, internalNode, outputCallback,
subgraph.outputNode, and the created link to locate where to insert these
ordering checks.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/litegraph/src/LGraphNode.tssrc/lib/litegraph/src/subgraph/SubgraphIO.test.tssrc/lib/litegraph/src/subgraph/SubgraphInputNode.ts
Move private method from ComfyApp to a standalone exported utility function. Eliminates unsafe 'as any' cast in tests by making the function directly importable. Amp-Thread-ID: https://ampcode.com/threads/T-019c9bda-0293-7528-9b0a-f72444199cbe Co-authored-by: Amp <amp@ampcode.com>
- switch LGraph link topology lookups and rehydration to linkStoreKey - update litegraph tests to use graph-scoped linkStoreKey instead of graph.id - add legacy compatibility warnings for slot dedupe, fixLinkInputSlots narrowing, and subgraph output disconnect callback parity - initialize testing Pinia globally in vitest setup and document non-reactive link store intent Amp-Thread-ID: https://ampcode.com/threads/T-019c9c03-dca7-757a-af61-5f41ae8d2dec Co-authored-by: Amp <amp@ampcode.com>
- remove unused exported LinkStoreTopology type to satisfy knip pre-push checks Amp-Thread-ID: https://ampcode.com/threads/T-019c9c03-dca7-757a-af61-5f41ae8d2dec Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/litegraph/src/subgraph/SubgraphOutput.ts (1)
55-89:⚠️ Potential issue | 🟠 MajorFix connect-path lifecycle parity and transaction safety.
Line 78 dispatches a
connected: trueevent even whenconnectSubgraphOutputSlot(...)can returnundefined(Line 38), and it passesslotinstead of the peer slot (this), which is inconsistent with Line 158 indisconnect().
Also, when replacing an existing link (Line 63), no disconnected event is emitted, so callback/event parity diverges from explicit disconnect flow.
Finally,beforeChange/afterChangeis not safely balanced across all paths (Line 58 vs Line 87, plus throw paths).Proposed fix
override connect( slot: INodeOutputSlot, node: LGraphNode, afterRerouteId?: RerouteId ): LLink | undefined { const { subgraph } = this.parent @@ - // Link should not be present, but just in case, disconnect it - const existingLink = this.getLinks().at(0) - if (existingLink != null) { - subgraph.beforeChange() - - const { outputNode } = existingLink.resolve(subgraph) - if (!outputNode) throw new Error('Expected output node for existing link') - - subgraph.disconnectSubgraphOutputLink( - this, - outputNode, - existingLink.origin_slot, - existingLink - ) - } - - const link = subgraph.connectSubgraphOutputSlot( - node, - outputIndex, - this, - afterRerouteId - ) - - graphLifecycleEventDispatcher.dispatchNodeConnectionChange({ - node, - slotType: NodeSlotType.OUTPUT, - slotIndex: outputIndex, - connected: true, - link, - slot - }) - - subgraph.afterChange() - - return link + subgraph.beforeChange() + try { + // Link should not be present, but just in case, disconnect it + const existingLink = this.getLinks().at(0) + if (existingLink != null) { + const { outputNode } = existingLink.resolve(subgraph) + if (!outputNode) throw new Error('Expected output node for existing link') + + subgraph.disconnectSubgraphOutputLink( + this, + outputNode, + existingLink.origin_slot, + existingLink + ) + + graphLifecycleEventDispatcher.dispatchNodeConnectionChange({ + node: outputNode, + slotType: NodeSlotType.OUTPUT, + slotIndex: existingLink.origin_slot, + connected: false, + link: existingLink, + slot: this + }) + } + + const link = subgraph.connectSubgraphOutputSlot( + node, + outputIndex, + this, + afterRerouteId + ) + if (!link) return + + graphLifecycleEventDispatcher.dispatchNodeConnectionChange({ + node, + slotType: NodeSlotType.OUTPUT, + slotIndex: outputIndex, + connected: true, + link, + slot: this + }) + + return link + } finally { + subgraph.afterChange() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphOutput.ts` around lines 55 - 89, The connect path must mirror disconnect behavior and be transaction-safe: when replacing an existing link (found via getLinks().at(0) and resolved with existingLink.resolve), call subgraph.beforeChange(), then perform disconnectSubgraphOutputLink and emit a disconnected graphLifecycleEventDispatcher.dispatchNodeConnectionChange (matching disconnect() semantics), and ensure subgraph.afterChange() is always called in a finally so before/after are balanced even on throws; when calling connectSubgraphOutputSlot only emit a connected event if it returned a truthy link, and pass the correct peer slot reference (use this as the peer slot argument to graphLifecycleEventDispatcher.dispatchNodeConnectionChange rather than slot); ensure all dispatches use NodeSlotType.OUTPUT and the proper slotIndex/connected flags to maintain parity with disconnect().
🧹 Nitpick comments (2)
src/lib/litegraph/src/LGraph.test.ts (1)
345-346: UsecreateTestSubgraph()for subgraph setup in these new testsThese new setup paths instantiate subgraphs manually. Please switch to the shared helper to keep fixtures consistent and avoid setup drift across subgraph tests.
As per coding guidelines:
src/lib/litegraph/**/*.test.{ts,tsx}: “Use test helper functionscreateTestSubgraph()andcreateTestSubgraphNode()from./__fixtures__/subgraphHelperswhen setting up subgraph tests”.Also applies to: 1177-1179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/LGraph.test.ts` around lines 345 - 346, Replace the manual subgraph setup that calls root.createSubgraph(...) and constructs nodes like createNumberNode('subgraph-source') with the shared test helpers: use createTestSubgraph() to create the subgraph fixture and createTestSubgraphNode() to create subgraph nodes; update references to subgraph and subgraphSource to use the helper-returned objects so the tests use the canonical fixture; apply the same change at the other occurrences around the referenced block (also at the later occurrence near lines 1177-1179) to keep subgraph tests consistent.src/utils/graphConfigureUtil.test.ts (1)
31-35: Prefersatisfiesover double-cast in the test fixture.Line 35 uses
as Partial<LGraph> as LGraph, which can mask shape drift in this helper.Suggested refactor
function createConfigureGraph(): LGraph { - return { + const graph = { nodes: [], onConfigure: vi.fn() - } as Partial<LGraph> as LGraph + } satisfies Pick<LGraph, 'nodes' | 'onConfigure'> + + return graph as LGraph }Based on learnings: In test files matching **/*.test.ts under src, when creating mock objects implementing an interface, prefer
satisfies InterfaceTypeover double type assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/graphConfigureUtil.test.ts` around lines 31 - 35, The test helper createConfigureGraph currently uses a double-cast ("as Partial<LGraph> as LGraph") which can hide shape drift; replace that double-cast with TypeScript's satisfies operator so the returned object is validated against LGraph (e.g., make the literal object satisfy LGraph) while still allowing a partial literal in the test. Update the return expression in createConfigureGraph to use "satisfies LGraph" on the object literal instead of the two as-casts, keeping nodes: [] and onConfigure: vi.fn() intact and ensuring the file remains under src/**/*.test.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/litegraph/src/LGraph.test.ts`:
- Around line 304-318: The assertion at the end is brittle because both
linkedInput?.link and linkedInputLink can be undefined and still make the expect
pass; instead first assert the link ID exists on the restored node (call
restored.getNodeById(linkedNodeId) and ensure its inputs[0].link is defined),
then fetch linkedInputLink via restored.getLink(...) and assert the IDs match or
that linkStore.getLink(restored.linkStoreKey, linkId) equals linkedInputLink;
update the test to first expect a defined link ID from restored.getNodeById,
then compare that ID to the retrieved link instance from restored.getLink and to
linkStore.getLink to avoid false positives.
In `@src/lib/litegraph/src/LGraph.ts`:
- Around line 1704-1738: The connectSubgraphInputSlot and
connectSubgraphOutputSlot functions currently mutate slot.link and slot.linkIds
but do not call the same lifecycle event used in connectSlots; add a call to
dispatchSlotLinkChanged (the same helper used by connectSlots) after you update
subgraphInput.linkIds/targetInput.link (and similarly for the output variant) so
listeners receive the slot-link change; place the dispatch before
finalizeConnectedLink (or in the same spot connectSlots calls it) and ensure you
pass the correct node, slot index and new link id as connectSlots does.
- Around line 442-448: rehydrateLinkStore() registers graph-scoped topology
entries under linkStoreKey but never unregisters them; add a teardown path that
removes the linkStoreKey entries when the graph/subgraph is disposed. Implement
a teardown method (e.g., teardownLinkStore) that calls the inverse API on
useLinkStore() to delete/unregister this.linkStoreKey (removing
links/floatingLinks/reroutes), and invoke that method from the LGraph disposal
lifecycle (the class's dispose/destroy/close method or subgraph removal handler)
so the store does not retain stale topology entries.
In `@src/utils/graphConfigureUtil.ts`:
- Around line 25-35: The onAfterGraphConfigured callback must run even if
onConfigure throws: move the call to triggerCallbackOnAllNodes(this,
'onAfterGraphConfigured') into a finally block that always executes after
attempting onConfigure. Keep the pre-configuration steps
(hasLegacyLinkInputSlotMismatch and fixLinkInputSlots) and
triggerCallbackOnAllNodes(this, 'onGraphConfigured') before calling onConfigure;
capture the return value of onConfigure (onConfigure?.apply(this, args)) and
return it after the finally, and rethrow any exception from onConfigure so
behavior is preserved while guaranteeing triggerCallbackOnAllNodes(this,
'onAfterGraphConfigured') runs. Reference: hasLegacyLinkInputSlotMismatch,
fixLinkInputSlots, triggerCallbackOnAllNodes, onConfigure.
In `@src/utils/litegraphUtil.ts`:
- Around line 221-223: The deprecation warning is being logged inside the
recursive repair routine causing duplicate messages; modify the repair API so
warnDeprecated is emitted only once per top-level repair pass by either moving
the warnDeprecated call out of the recursive function into a non-recursive
wrapper or by adding a boolean flag param to fixLinkInputSlots (e.g., hasWarned
or warnedRef) that defaults to false, check-and-set it before calling
warnDeprecated, and pass the flag through recursive calls; update the same
pattern around the other warning sites noted (lines ~240-242) so they use the
same wrapper/flag approach.
---
Outside diff comments:
In `@src/lib/litegraph/src/subgraph/SubgraphOutput.ts`:
- Around line 55-89: The connect path must mirror disconnect behavior and be
transaction-safe: when replacing an existing link (found via getLinks().at(0)
and resolved with existingLink.resolve), call subgraph.beforeChange(), then
perform disconnectSubgraphOutputLink and emit a disconnected
graphLifecycleEventDispatcher.dispatchNodeConnectionChange (matching
disconnect() semantics), and ensure subgraph.afterChange() is always called in a
finally so before/after are balanced even on throws; when calling
connectSubgraphOutputSlot only emit a connected event if it returned a truthy
link, and pass the correct peer slot reference (use this as the peer slot
argument to graphLifecycleEventDispatcher.dispatchNodeConnectionChange rather
than slot); ensure all dispatches use NodeSlotType.OUTPUT and the proper
slotIndex/connected flags to maintain parity with disconnect().
---
Nitpick comments:
In `@src/lib/litegraph/src/LGraph.test.ts`:
- Around line 345-346: Replace the manual subgraph setup that calls
root.createSubgraph(...) and constructs nodes like
createNumberNode('subgraph-source') with the shared test helpers: use
createTestSubgraph() to create the subgraph fixture and createTestSubgraphNode()
to create subgraph nodes; update references to subgraph and subgraphSource to
use the helper-returned objects so the tests use the canonical fixture; apply
the same change at the other occurrences around the referenced block (also at
the later occurrence near lines 1177-1179) to keep subgraph tests consistent.
In `@src/utils/graphConfigureUtil.test.ts`:
- Around line 31-35: The test helper createConfigureGraph currently uses a
double-cast ("as Partial<LGraph> as LGraph") which can hide shape drift; replace
that double-cast with TypeScript's satisfies operator so the returned object is
validated against LGraph (e.g., make the literal object satisfy LGraph) while
still allowing a partial literal in the test. Update the return expression in
createConfigureGraph to use "satisfies LGraph" on the object literal instead of
the two as-casts, keeping nodes: [] and onConfigure: vi.fn() intact and ensuring
the file remains under src/**/*.test.ts.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/lib/litegraph/src/LGraph.test.tssrc/lib/litegraph/src/LGraph.tssrc/lib/litegraph/src/LLink.test.tssrc/lib/litegraph/src/subgraph/SubgraphOutput.tssrc/scripts/app.test.tssrc/scripts/app.tssrc/stores/linkStore.tssrc/utils/graphConfigureUtil.test.tssrc/utils/graphConfigureUtil.tssrc/utils/litegraphUtil.tsvitest.setup.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/scripts/app.ts
- src/scripts/app.test.ts
## Summary Adds app builder in arrange/preview mode with re-orderable widgets, maintaining size (as much as possible) between the select + preview steps ## Changes - **What**: - Extract sidebar size constants for sharing between canvas splitter + app mode - Add widget list using DraggableList and render inert WidgetItems ## Screenshots (if applicable) <img width="1391" height="923" alt="image" src="https://github.com/user-attachments/assets/3e17eafe-db1e-40a3-83b5-15a7d0672909" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9260-Render-app-builder-in-arrange-mode-3136d73d365081ef875acab683d01d9e) by [Unito](https://www.unito.io)
…t image (#9261) ## Summary Prevent latent previews received after the job/node has already finished processing overwriting the actual output display ## Changes - **What**: - updates job preview store to also track which node the preview was for - updates linear progress tracking to store executed nodes enabling skipping previews of these ## Review Focus <!-- Critical design decisions or edge cases that need attention --> <!-- If this PR fixes an issue, uncomment and update the line below --> <!-- Fixes #ISSUE_NUMBER --> ## Screenshots (if applicable) <!-- Add screenshots or video recording to help explain your changes --> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9261-App-mode-discard-slow-preview-messages-to-prevent-overwriting-output-image-3136d73d3650817884c2ce2ff5993b9e) by [Unito](https://www.unito.io)
## Summary - remove exit builder button from right panel - add builder exit button to bottom of canvas - add builder menu with save & exit in top left ## Screenshots (if applicable) <img width="1544" height="998" alt="image" src="https://github.com/user-attachments/assets/f5deadc5-2bf5-4729-b644-2b6a815b9975" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9218-App-builder-exit-updates-3126d73d365081a0bf1adf92e1171060) by [Unito](https://www.unito.io)
In adding an essentials cateogory for nodes, #8987 introduced a regression where core nodes which are also essential are marked as being from a `nodes` custom node instead of being marked core. Since the essentials designation should pre-empt core and custom nodes can choose to mark themself as essential, the getter for `isCoreNode` is updated to instead repeat the existing check for if a node is core. | Before | After | | ------ | ----- | | <img width="360" alt="before" src="https://github.com/user-attachments/assets/f1b8bf80-d072-409a-a0f9-4837e1d11767" /> | <img width="360" alt="after" src="https://github.com/user-attachments/assets/14ff525b-9833-4e73-888f-791aff6cf531"/>| ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9287-Fix-essentials-nodes-not-being-marked-core-3146d73d365081fca2a0f8bdc2baf01a) by [Unito](https://www.unito.io)
## Summary
Pre-rasterize the SubgraphNode SVG icon to a bitmap canvas to eliminate
Firefox's per-frame SVG style processing.
## Changes
- **What**: Add `getWorkflowBitmap()` that lazily rasterizes the
`data:image/svg+xml` workflow icon to an `HTMLCanvasElement` (16×16) on
first use. `SubgraphNode.drawTitleBox()` draws the cached bitmap instead
of the raw SVG.
## Review Focus
- Firefox re-processes SVG internal stylesheets (`stroke`,
`stroke-linecap`, `stroke-width`) every time `ctx.drawImage(svgImage)`
is called. Chrome caches the rasterization. This happens on every frame
for every visible SubgraphNode.
- Reporter confirmed strong subgraph correlation: "it may be happening
in the default workflow with subgraph" / "didn't seem to happen just
using manually wired up diffusion loader, clip, sampler, etc."
- Falls back to the raw SVG Image if not yet loaded or if
`getContext('2d')` returns null.
## Stack
3 of 4 in Firefox perf fix stack. Depends on #9170.
<!-- Fixes #ISSUE_NUMBER -->
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-9172-fix-pre-rasterize-SubgraphNode-SVG-icon-to-bitmap-canvas-3116d73d365081babf17cf0848d37269)
by [Unito](https://www.unito.io)
---------
Co-authored-by: GitHub Action <action@github.com>
Patch version increment to 1.41.8 **Base branch:** `main` ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9288-1-41-8-3156d73d3650817ca737ced3e08d8c86) by [Unito](https://www.unito.io) --------- Co-authored-by: christian-byrne <72887196+christian-byrne@users.noreply.github.com> Co-authored-by: github-actions <github-actions@github.com>
## Summary Fixes two bugs in the node replacement flow: placeholder detection failing after workflow execution or pack reinstallation, and missing UI sync in the Errors Tab when replacements are applied from the modal dialog. ## Changes - **Placeholder detection**: Node placeholder detection now matches against `targetTypes` (derived from the replaceable node list built at workflow load time) instead of relying on `has_errors` flag or `registered_node_types` lookup. This ensures replacement works reliably after execution (where `has_errors` gets cleared) and after pack reinstallation (where the type becomes registered). - **Modal → Errors Tab sync**: Added `executionErrorStore.removeMissingNodesByType()` call in `MissingNodesContent.vue` after replacement, so the Errors Tab reflects changes immediately without requiring a page reload. ## Review Focus - `collectAllNodes` predicate change in `useNodeReplacement.ts`: now uses `targetTypes.has(originalType)` to find nodes by their original serialized type. This is independent of runtime state like `has_errors` or `registered_node_types`. - `executionErrorStore.removeMissingNodesByType` call timing in `MissingNodesContent.vue` — runs synchronously after `replaceNodesInPlace` resolves, before auto-close logic. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9269-fix-node-replacement-fails-after-execution-and-modal-sync-3146d73d365081218398c961639b450f) by [Unito](https://www.unito.io)
…omprehensive test suite (#9301) ## Summary Resolves six open issues by reorganizing node replacement components into a domain-driven folder structure, refactoring event handling to follow the emit pattern, and adding comprehensive test coverage across all affected modules. ## Changes - **What**: - Moved `SwapNodeGroupRow.vue` and `SwapNodesCard.vue` from `src/components/rightSidePanel/errors/` to `src/platform/nodeReplacement/components/` (Issues #9255) - Moved `useMissingNodeScan.ts` from `src/composables/` to `src/platform/nodeReplacement/missingNodeScan.ts`, renamed to reflect it is a plain function not a Vue composable (Issues #9254) - Refactored `SwapNodeGroupRow.vue` to emit a `'replace'` event instead of calling `useNodeReplacement()` and `useExecutionErrorStore()` directly; replacement logic now handled in `TabErrors.vue` (Issue #9267) - Added unit tests for `removeMissingNodesByType` (`executionErrorStore.test.ts`), `scanMissingNodes` (`missingNodeScan.test.ts`), and `swapNodeGroups` computed (`swapNodeGroups.test.ts`, `useErrorGroups.test.ts`) (Issue #9270) - Added placeholder detection tests covering unregistered-type detection when `has_errors` is false, and exclusion of registered types (`useNodeReplacement.test.ts`) (Issue #9271) - Added component tests for `MissingNodeCard` and `MissingPackGroupRow` covering rendering, expand/collapse, events, install states, and edge cases (Issue #9231) - Added component tests for `SwapNodeGroupRow` and `SwapNodesCard` (Issues #9255, #9267) ## Additional Changes (Post-Review) - **Edge case guard in placeholder detection** (`useNodeReplacement.ts`): When `last_serialization.type` is absent (old serialization format), the predicate falls back to `n.type`, which the app may have already run through `sanitizeNodeName` — stripping HTML special characters (`& < > " ' \` =`). In that case, a `Set.has()` lookup against the original unsanitized type name would silently miss, causing replacement to be skipped. Fixed by including sanitized variants of each target type in the `targetTypes` Set at construction time. For the overwhelmingly common case (no special characters in type names), the Set deduplicates the entries and runtime behavior is identical to before. A regression test was added to cover the specific scenario: `last_serialization.type` absent + live `n.type` already sanitized. ## Review Focus - `TabErrors.vue`: confirm the new `@replace` event handler correctly replaces nodes and removes them from missing nodes list (mirrors the old inline logic in `SwapNodeGroupRow`) - `missingNodeScan.ts`: filename/export name change from `useMissingNodeScan` — verify all call sites updated via `app.ts` - Test mocking strategy: module-level `vi.mock()` factories use closures over `ref`/plain objects to allow per-test overrides without global mutable state - Fixes #9231 - Fixes #9254 - Fixes #9255 - Fixes #9267 - Fixes #9270 - Fixes #9271
## Summary Implement 11 Figma design discrepancies for the Node Library sidebar and V2 Node Search dialog, aligning the UI with the [Toolbox Figma design](https://www.figma.com/design/xMFxCziXJe6Denz4dpDGTq/Toolbox?node-id=2074-21394&m=dev). ## Changes - **What**: Sidebar: reorder tabs (All/Essentials/Blueprints), rename Custom→Blueprints, uppercase section headers, chevron-left of folder icon, bookmark-on-hover for node rows, filter dropdown with checkbox items, sort labels (Categorized/A-Z) with label-left/check-right layout, hide section headers in A-Z mode. Search dialog: expand filter chips from 3→6, add Recents and source categories to sidebar, remove "Filter by" label. Pull foundation V2 components from merged PR #8548. - **Dependencies**: Depends on #8987 (V2 Node Search) and #8548 (NodeLibrarySidebarTabV2) ## Review Focus - Filter dropdown (`filterOptions`) is UI-scaffolded but not yet wired to filtering logic (pending V2 integration) - "Recents" category currently returns frequency-based results as placeholder until a usage-tracking store is implemented - Pre-existing type errors from V2 PR dependencies not in the base commit (SearchBoxV2, usePerTabState, TextTicker, getProviderIcon, getLinkTypeColor, SidebarContainerKey) are expected and will resolve when rebased onto main after parent PRs land ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9085-feat-Node-Library-sidebar-and-V2-Search-dialog-Figma-design-improvements-30f6d73d36508175bf72d716f5904476) by [Unito](https://www.unito.io) --------- Co-authored-by: Yourz <crazilou@vip.qq.com> Co-authored-by: github-actions <github-actions@github.com>
## Summary
Send CURVE values as { __type: 'CURVE', value: [...] } instead of {
__value__: [...] } to avoid ambiguity with link detection and enable
external tools to identify the data type.
change requested by @guill
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-9294-feat-wrap-CURVE-widget-value-with-typed-format-3156d73d365081bf8e5de59527e2d3ce)
by [Unito](https://www.unito.io)
## Summary Adds a little indicator circle when new workflow menu items are added that the user has not seen ## Changes - **What**: Adds a hidden setting to track menu items flagged as new that have been seen ## Screenshots (if applicable) <img width="164" height="120" alt="image" src="https://github.com/user-attachments/assets/ac36673d-fbf1-42ff-9a9e-1371eb96115b" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9220-Add-indicator-circle-when-new-unseen-menu-items-are-available-3126d73d3650819cb8cde854d6b6510b) by [Unito](https://www.unito.io)
Fix multiple issues with promoted widget resolution in nested subgraphs, ensuring correct value propagation, slot matching, and rendering for deeply nested promoted widgets. - **What**: Stabilize nested subgraph promoted widget resolution chain - Use deep source keys for promoted widget values in Vue rendering mode - Resolve effective widget options from the source widget instead of the promoted view - Stabilize slot resolution for nested promoted widgets - Preserve combo value rendering for promoted subgraph widgets - Prevent subgraph definition deletion while other nodes still reference the same type - Clean up unused exported resolution types - `resolveConcretePromotedWidget.ts` — new recursive resolution logic for deeply nested promoted widgets - `useGraphNodeManager.ts` — option extraction now uses `effectiveWidget` for promoted widgets - `SubgraphNode.ts` — unpack no longer force-deletes definitions referenced by other nodes ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9282-fix-stabilize-nested-subgraph-promoted-widget-resolution-3146d73d365081208a4fe931bb7569cf) by [Unito](https://www.unito.io) --------- Co-authored-by: Amp <amp@ampcode.com> Co-authored-by: GitHub Action <action@github.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019ca639-cde7-744d-b426-6ea21b2159e6 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/lib/litegraph/src/LGraph.test.ts (1)
303-318:⚠️ Potential issue | 🟡 MinorHarden this restore assertion to avoid
undefined === undefinedfalse positives.The final expectation can still pass when restore failed because both
linkedInput?.linkandlinkedInputLinkareundefined. Assert existence first, then compare.✅ Suggested fix
const restored = new LGraph(serialised) const linkedInput = restored.getNodeById(linkedNodeId)?.inputs[0] - const linkedInputLink = restored.getLink(linkedInput?.link) + if (!linkedInput || linkedInput.link == null) + throw new Error('Expected linked input after restore') + const linkedInputLink = restored.getLink(linkedInput.link) + if (!linkedInputLink) throw new Error('Expected restored linked input link') @@ - expect(linkStore.getLink(restored.linkStoreKey, linkedInput?.link)).toBe( + expect(linkStore.getLink(restored.linkStoreKey, linkedInput.link)).toBe( linkedInputLink )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/LGraph.test.ts` around lines 303 - 318, The test currently allows a false positive when both linkedInput?.link and linkedInputLink are undefined; first assert that linkedInput and linkedInput?.link are defined (e.g., expect(restored.getNodeById(linkedNodeId)).toBeDefined() and expect(linkedInput?.link).toBeDefined()), then fetch linkedInputLink via restored.getLink(linkedInput!.link) and finally assert equality with linkStore.getLink(restored.linkStoreKey, linkedInput!.link); use the LGraph, linkedInput, linkedInputLink, getNodeById and getLink symbols to locate and update the assertions.src/lib/litegraph/src/LGraph.ts (1)
1714-1787:⚠️ Potential issue | 🟠 MajorSubgraph connect helpers still skip slot-link lifecycle dispatch.
Line 1744 and Line 1782 update slot link state, but neither path emits
dispatchSlotLinkChangedlikeconnectSlotsdoes. This keeps a parity gap for slot-link listeners on subgraph boundary connects.🧩 Parity patch
connectSubgraphInputSlot( @@ subgraphInput.linkIds.push(link.id) targetInput.link = link.id + graphLifecycleEventDispatcher.dispatchSlotLinkChanged({ + graph: this, + nodeId: targetNode.id, + slotType: NodeSlotType.INPUT, + slotIndex: targetSlotIndex, + connected: true, + linkId: link.id, + hasWidget: !!targetInput.widget + }) this.finalizeConnectedLink(link) return link } @@ connectSubgraphOutputSlot( @@ subgraphOutput.linkIds[0] = link.id sourceOutput.links ??= [] sourceOutput.links.push(link.id) + graphLifecycleEventDispatcher.dispatchSlotLinkChanged({ + graph: this, + nodeId: sourceNode.id, + slotType: NodeSlotType.OUTPUT, + slotIndex: sourceSlotIndex, + connected: true, + linkId: link.id, + hasWidget: false + }) this.finalizeConnectedLink(link) return link }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/LGraph.ts` around lines 1714 - 1787, connectSubgraphInputSlot and connectSubgraphOutputSlot set link ids on subgraph slots and node slots but never emit the slot-link lifecycle event; mirror connectSlots by invoking dispatchSlotLinkChanged after mutating the slot link state. Specifically, in connectSubgraphInputSlot call dispatchSlotLinkChanged for both the subgraph input (subgraphInput.parent.id, subgraphInputIndex, 'input', link.id) and the target node input (targetNode.id, targetSlotIndex, 'input', link.id) after pushing/setting link ids; similarly, in connectSubgraphOutputSlot call dispatchSlotLinkChanged for the source node output (sourceNode.id, sourceSlotIndex, 'output', link.id) and the subgraph output (subgraphOutput.parent.id, subgraphOutputIndex, 'output', link.id) after updating their link arrays—use the same dispatchSlotLinkChanged method used by connectSlots to ensure parity.
🧹 Nitpick comments (1)
src/lib/litegraph/src/LGraph.test.ts (1)
26-57: Scope layout mutation mocks to avoid shared file-level mutable state.The mock functions are created once and shared across all tests in this file. This can couple suites and hide ordering/leakage issues.
♻️ Suggested refactor
+const layoutMutationsMock = vi.hoisted(() => ({ + createLink: vi.fn(), + deleteLink: vi.fn(), + createNode: vi.fn(), + deleteNode: vi.fn(), + moveNode: vi.fn(), + resizeNode: vi.fn(), + setNodeZIndex: vi.fn(), + createReroute: vi.fn(), + deleteReroute: vi.fn(), + moveReroute: vi.fn(), + bringNodeToFront: vi.fn(), + setSource: vi.fn(), + setActor: vi.fn() +})) vi.mock('@/renderer/core/layout/operations/layoutMutations', () => { - const createLink = vi.fn() - const deleteLink = vi.fn() - const createNode = vi.fn() - const deleteNode = vi.fn() - const moveNode = vi.fn() - const resizeNode = vi.fn() - const setNodeZIndex = vi.fn() - const createReroute = vi.fn() - const deleteReroute = vi.fn() - const moveReroute = vi.fn() - const bringNodeToFront = vi.fn() - const setSource = vi.fn() - const setActor = vi.fn() return { - useLayoutMutations: () => ({ - createLink, - deleteLink, - createNode, - deleteNode, - moveNode, - resizeNode, - setNodeZIndex, - createReroute, - deleteReroute, - moveReroute, - bringNodeToFront, - setSource, - setActor - }) + useLayoutMutations: () => layoutMutationsMock } }) + +beforeEach(() => { + vi.clearAllMocks() +})As per coding guidelines, "For mocking in Vitest, leverage Vitest's utilities where possible; keep module mocks contained without global mutable state; use
vi.hoisted()if necessary for per-test mock state manipulation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/LGraph.test.ts` around lines 26 - 57, The current mock creates module-level vi.fn() instances that are shared across tests (createLink, deleteLink, createNode, etc.), causing cross-test leakage; change the mock to produce per-test mocks by using vi.hoisted(() => vi.fn()) for each mock function or by creating the vi.fn() instances inside a factory invoked per test and exposing them via useLayoutMutations; update the mock returned by vi.mock to reference those hoisted factories (or reset each mock in a beforeEach using mockReset) so useLayoutMutations, createLink, deleteLink, createNode, deleteNode, moveNode, resizeNode, setNodeZIndex, createReroute, deleteReroute, moveReroute, bringNodeToFront, setSource and setActor are fresh for each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/litegraph/src/LGraph.test.ts`:
- Around line 303-318: The test currently allows a false positive when both
linkedInput?.link and linkedInputLink are undefined; first assert that
linkedInput and linkedInput?.link are defined (e.g.,
expect(restored.getNodeById(linkedNodeId)).toBeDefined() and
expect(linkedInput?.link).toBeDefined()), then fetch linkedInputLink via
restored.getLink(linkedInput!.link) and finally assert equality with
linkStore.getLink(restored.linkStoreKey, linkedInput!.link); use the LGraph,
linkedInput, linkedInputLink, getNodeById and getLink symbols to locate and
update the assertions.
In `@src/lib/litegraph/src/LGraph.ts`:
- Around line 1714-1787: connectSubgraphInputSlot and connectSubgraphOutputSlot
set link ids on subgraph slots and node slots but never emit the slot-link
lifecycle event; mirror connectSlots by invoking dispatchSlotLinkChanged after
mutating the slot link state. Specifically, in connectSubgraphInputSlot call
dispatchSlotLinkChanged for both the subgraph input (subgraphInput.parent.id,
subgraphInputIndex, 'input', link.id) and the target node input (targetNode.id,
targetSlotIndex, 'input', link.id) after pushing/setting link ids; similarly, in
connectSubgraphOutputSlot call dispatchSlotLinkChanged for the source node
output (sourceNode.id, sourceSlotIndex, 'output', link.id) and the subgraph
output (subgraphOutput.parent.id, subgraphOutputIndex, 'output', link.id) after
updating their link arrays—use the same dispatchSlotLinkChanged method used by
connectSlots to ensure parity.
---
Nitpick comments:
In `@src/lib/litegraph/src/LGraph.test.ts`:
- Around line 26-57: The current mock creates module-level vi.fn() instances
that are shared across tests (createLink, deleteLink, createNode, etc.), causing
cross-test leakage; change the mock to produce per-test mocks by using
vi.hoisted(() => vi.fn()) for each mock function or by creating the vi.fn()
instances inside a factory invoked per test and exposing them via
useLayoutMutations; update the mock returned by vi.mock to reference those
hoisted factories (or reset each mock in a beforeEach using mockReset) so
useLayoutMutations, createLink, deleteLink, createNode, deleteNode, moveNode,
resizeNode, setNodeZIndex, createReroute, deleteReroute, moveReroute,
bringNodeToFront, setSource and setActor are fresh for each test.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/litegraph/src/LGraph.test.tssrc/lib/litegraph/src/LGraph.tssrc/lib/litegraph/src/subgraph/SubgraphInput.tssrc/lib/litegraph/src/subgraph/SubgraphNode.tssrc/scripts/app.ts
- Replace deprecated indexed link access with Map#get in SubgraphOutput.disconnect - Add behavior-focused regression coverage for idempotent output disconnect Amp-Thread-ID: https://ampcode.com/threads/T-019ca639-cde7-744d-b426-6ea21b2159e6 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/litegraph/src/subgraph/SubgraphOutput.ts (1)
133-135: Consider controlling deprecation warning emission.The
warnDeprecatedcall executes on everydisconnect()invocation, which may be noisy in production or during automated workflows. Consider gating this behind a development/debug flag or emitting only once per session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/subgraph/SubgraphOutput.ts` around lines 133 - 135, The warnDeprecated call inside SubgraphOutput.disconnect should not fire on every call; modify the disconnect method to emit that deprecation warning only once per session or only in development by introducing a guard (e.g., a module-level or static flag like warnedSubgraphOutputDisconnect or SubgraphOutput._warnedDisconnect) and/or check a debug/dev flag before calling warnDeprecated; update SubgraphOutput.disconnect to check the guard/flag, call warnDeprecated only the first time (or when process.env.NODE_ENV !== 'production' / a provided debug flag is true), and set the guard so subsequent disconnect calls are silent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/litegraph/src/subgraph/SubgraphOutput.ts`:
- Around line 133-135: The warnDeprecated call inside SubgraphOutput.disconnect
should not fire on every call; modify the disconnect method to emit that
deprecation warning only once per session or only in development by introducing
a guard (e.g., a module-level or static flag like warnedSubgraphOutputDisconnect
or SubgraphOutput._warnedDisconnect) and/or check a debug/dev flag before
calling warnDeprecated; update SubgraphOutput.disconnect to check the
guard/flag, call warnDeprecated only the first time (or when
process.env.NODE_ENV !== 'production' / a provided debug flag is true), and set
the guard so subsequent disconnect calls are silent.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/litegraph/src/subgraph/SubgraphIO.test.tssrc/lib/litegraph/src/subgraph/SubgraphOutput.ts
graph.linkStore was removed; use graph.links, graph.floatingLinks, and graph.reroutes maps directly to verify store-backed accessor parity. Amp-Thread-ID: https://ampcode.com/threads/T-019ca6ac-9a9e-7139-8b31-dee2bec82341 Co-authored-by: Amp <amp@ampcode.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
browser_tests/tests/changeTracker.spec.ts (2)
105-125: Use retrying assertions after programmatic undo/redo.These post-undo/redo snapshot checks are immediate; switching to
expect.pollmakes the test less timing-sensitive if topology projection settles on a later tick.Suggested patch
- const afterUndo = await readTopology() - expect(afterUndo.links).toBe(baseline.links) - expect(afterUndo.floatingLinks).toBe(baseline.floatingLinks) - expect(afterUndo.reroutes).toBe(baseline.reroutes) - expect(afterUndo.serialised).toEqual(baseline.serialised) + await expect.poll(readTopology).toEqual(baseline) @@ - const afterRedo = await readTopology() - expect(afterRedo.links).toBe(mutated.links) - expect(afterRedo.floatingLinks).toBe(mutated.floatingLinks) - expect(afterRedo.reroutes).toBe(mutated.reroutes) - expect(afterRedo.serialised).toEqual(mutated.serialised) + await expect.poll(readTopology).toEqual(mutated)As per coding guidelines,
browser_tests/**/*.spec.tsshould follow Playwright best practices, including retrying assertions for async behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/changeTracker.spec.ts` around lines 105 - 125, The immediate assertions after calling changeTracker.undo()/redo() are brittle; wrap the topology checks in Playwright's retrying assertions (expect.poll) so the test waits for projection to settle. Specifically, after calling comfyPage.page.evaluate that invokes (window.app!.extensionManager as WorkspaceStore).workflow.activeWorkflow?.changeTracker.undo() or .redo(), replace the direct readTopology() + expect(...) checks with expect.poll(() => readTopology()).toHaveProperty/... or a poll that returns the topology and assert links, floatingLinks, reroutes and serialised against baseline/mutated; keep the same identifiers (comfyPage.page.evaluate, WorkspaceStore, changeTracker.undo/redo, readTopology) so the logic location is obvious.
86-99: Consider asserting before/after callback ordering in this path.Given the PR’s compatibility goal around lifecycle ordering, adding an explicit sequence assertion here would lock in the
beforeChange -> mutation -> afterChangecontract beyond queue-size checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@browser_tests/tests/changeTracker.spec.ts` around lines 86 - 99, Add an explicit ordering assertion around the beforeChange -> mutation -> afterChange lifecycle: in the test that calls beforeChange(comfyPage), runs comfyPage.page.evaluate(...) (which uses window.app!.rootGraph.createReroute, addFloatingLink, toFloating) and then afterChange(comfyPage), record a simple ordering flag or timestamp in the page context (e.g. set window.__changeSequence before/after and during the mutation) and assert in the test that the flag/timestamps show beforeChange ran first, the mutation ran second, and afterChange ran last; update the assertion near the existing calls to beforeChange, comfyPage.page.evaluate, and afterChange to fail if the sequence is violated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@browser_tests/tests/changeTracker.spec.ts`:
- Around line 105-125: The immediate assertions after calling
changeTracker.undo()/redo() are brittle; wrap the topology checks in
Playwright's retrying assertions (expect.poll) so the test waits for projection
to settle. Specifically, after calling comfyPage.page.evaluate that invokes
(window.app!.extensionManager as
WorkspaceStore).workflow.activeWorkflow?.changeTracker.undo() or .redo(),
replace the direct readTopology() + expect(...) checks with expect.poll(() =>
readTopology()).toHaveProperty/... or a poll that returns the topology and
assert links, floatingLinks, reroutes and serialised against baseline/mutated;
keep the same identifiers (comfyPage.page.evaluate, WorkspaceStore,
changeTracker.undo/redo, readTopology) so the logic location is obvious.
- Around line 86-99: Add an explicit ordering assertion around the beforeChange
-> mutation -> afterChange lifecycle: in the test that calls
beforeChange(comfyPage), runs comfyPage.page.evaluate(...) (which uses
window.app!.rootGraph.createReroute, addFloatingLink, toFloating) and then
afterChange(comfyPage), record a simple ordering flag or timestamp in the page
context (e.g. set window.__changeSequence before/after and during the mutation)
and assert in the test that the flag/timestamps show beforeChange ran first, the
mutation ran second, and afterChange ran last; update the assertion near the
existing calls to beforeChange, comfyPage.page.evaluate, and afterChange to fail
if the sequence is violated.
- Fix copy-paste error in SubgraphOutput type error message - Add linkStore.clearGraph() call in LGraph.clear() to prevent memory leak - Cache useLinkStore() reference on LGraph instance for hot-path perf - Use frozen EMPTY_TOPOLOGY sentinel instead of allocating per miss - Make canonicalName required in SubgraphEventMap (always provided) - Replace deprecated links[linkId] with links.get() in SubgraphNode - Document connectSlots non-nullable return contract Amp-Thread-ID: https://ampcode.com/threads/T-019ca6ba-5e48-707b-8c81-3eda7f6bc9e2 Co-authored-by: Amp <amp@ampcode.com>
- Remove redundant storeNodeId and storeName from SafeWidgetData - Extract buildSlotMetadata helper to deduplicate slot metadata logic - Simplify normalizeWidgetValue to collapse identity branches - Collapse duplicated flags.* property handlers into single case - Merge color/bgcolor handlers with computed property key - Remove redundant syncWithGraph (existing-nodes loop covers it) - Inline trigger dispatch, remove intermediate triggerHandlers map - Avoid duplicate extractWidgetDisplayOptions calls in common path - Remove unused LGraphTriggerAction import Amp-Thread-ID: https://ampcode.com/threads/T-019ca721-4ac5-730e-a526-6c71e474e766 Co-authored-by: Amp <amp@ampcode.com>
- S1: Extract _createAndRegisterLink helper from 3 connect methods - S2: Remove dead SubgraphInputNode.connectSlots (unregistered LLinks) - S3: Document that LGraph.connectSlots callers dispatch callbacks - S4: Add dispatchSlotLinkChanged to connectSubgraphInputSlot - S5: Document _version increment contract on disconnect paths - S6: Add backwards-compat comments at 3 behavioral change sites - S7: Gate SubgraphOutput.disconnect deprecation warning with once flag - S8: Fix splitPositionables instanceof ordering (subclass before base) - S9: Add explicit return type to mapReroutes - S10: Add JSDoc to LinkStoreTopology about mutable backing maps - S11: Document dispatch ordering asymmetry in event dispatcher Amp-Thread-ID: https://ampcode.com/threads/T-019ca787-c4d4-755f-b63a-4d814ff46e2c Co-authored-by: Amp <amp@ampcode.com>
- L7: Extract resolveCanonicalSlotName and normalizeLegacySlotIdentity to utils/slotIdentity.ts (-45 lines from LGraph.ts) - L3: Inline 4 trivial subgraphBoundaryAdapter wrappers using existing LLink getters (-26 lines from subgraphUtils.ts) - L4: Merge hasLegacyLinkInputSlotMismatch into fixLinkInputSlots single-pass (-17 lines, eliminates double traversal) Amp-Thread-ID: https://ampcode.com/threads/T-019ca83b-1182-77df-b270-4703bb00cf45 Co-authored-by: Amp <amp@ampcode.com>
- Emit dispatchNodeConnectionChange in SubgraphInputNode stale-link fallback - Balance beforeChange/afterChange with try/finally in SubgraphOutput.connect - Emit disconnect event when SubgraphOutput.connect replaces existing link - Guard connectSubgraphOutputSlot return and skip dispatch on falsy link - Move onAfterGraphConfigured to finally block in graphConfigureUtil - Add dispatchSlotLinkChanged to connectSubgraphOutputSlot for parity - Guard remapProxyWidgets call with explicit map-entry check - Harden LinkStore restore test assertions with existence checks - Use dispatchDisconnectNodePair in disconnectOutput for consistency Amp-Thread-ID: https://ampcode.com/threads/T-019cb1e7-a18e-712a-b8c7-4448f603c8b1 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019cbbd4-2ada-7557-8a34-23a174f34f9a Co-authored-by: Amp <amp@ampcode.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Centralizes link/slot topology behavior behind store-driven read/write and compatibility adapters, then narrows legacy slot-repair to migration-only mismatch cases.
Changes
fixLinkInputSlotsruntime usage to explicit legacy mismatch detection.Review Focus
hasLegacyLinkInputSlotMismatch+ guarded repair invoke).Validation
pnpm exec vitest runfor:src/lib/litegraph/src/LGraph.test.tssrc/lib/litegraph/src/subgraph/SubgraphIO.test.tssrc/lib/litegraph/src/LGraphNode.test.tssrc/lib/litegraph/src/canvas/LinkConnector.integration.test.tssrc/lib/litegraph/src/LGraphCanvas.clipboard.test.tssrc/utils/litegraphUtil.test.tssrc/scripts/app.test.tspnpm exec eslint --no-warn-ignoredon touched litegraph + app filespnpm exec oxlinton touched litegraph + app filespnpm typecheckScreenshots (if applicable)
N/A