When moving subgraphInput link, properly disconnect old link #7229
Conversation
dosubot
bot
added
the
size:M
🎭 Playwright Test Results⏰ Completed at: 12/08/2025, 07:48:19 PM UTC 📈 Summary
📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 12/08/2025, 07:38:30 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
📝 WalkthroughWalkthroughWhen moving a link into a subgraph input, the connectToInput flow now resolves the previously connected link, clears its input.link reference (sets it to null), and dispatches an 'input-moved' event. Related tests for disconnection and validation were re-enabled and expanded. Changes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)tests-ui/**/*.test.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (tests-ui/CLAUDE.md)
Files:
**/*.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,vue}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.test.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (10)📓 Common learnings📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
📚 Learning: 2025-12-06T00:52:35.750ZApplied to files:
📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
📚 Learning: 2025-11-24T19:47:56.371ZApplied to files:
📚 Learning: 2025-12-06T00:52:35.750ZApplied to files:
📚 Learning: 2025-11-24T19:48:09.318ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.2 MB (baseline 3.2 MB) • 🔴 +183 BMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 976 kB (baseline 976 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 6.54 kB (baseline 6.54 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 298 kB (baseline 298 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 177 kB (baseline 177 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 2.94 kB (baseline 2.94 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 8.56 MB (baseline 8.56 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.81 MB (baseline 3.81 MB) • ⚪ 0 BBundles that do not match a named category
Status: 17 added / 17 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts (1)
55-72: Resolve and clear the previous input slot before reconnectingThe new cleanup correctly targets the stale
input.link, but doing:const newLink = fromSlot.connect(input, node, fromReroute?.id) if (existingLink) { const { input } = existingLink.resolve(this.network) if (input) input.link = null }relies on
existingLink.resolve(this.network)still pointing at the old input afterconnect. IfSubgraphInput.connectis ever refactored to update the sameLLinkinstance, this would instead clear the new input.To make this robust and avoid name shadowing, consider:
connectToInput( node: LGraphNode, input: INodeInputSlot, events: CustomEventTarget<LinkConnectorEventMap> ) { - const { fromSlot, fromReroute, existingLink } = this - - const newLink = fromSlot.connect(input, node, fromReroute?.id) - - if (existingLink) { - // Moving an existing link - const { input } = existingLink.resolve(this.network) - if (input) input.link = null + const { fromSlot, fromReroute, existingLink } = this + + const previousInput = existingLink + ? existingLink.resolve(this.network).input + : null + + const newLink = fromSlot.connect(input, node, fromReroute?.id) + + if (existingLink) { + // Moving an existing link + if (previousInput && previousInput !== input) previousInput.link = null events.dispatch('input-moved', this) } else { // Creating a new link events.dispatch('link-created', newLink) } }This guarantees we always clear the actual previous target slot, independent of how
connectmutates links, and improves readability.tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts (1)
162-177: ToOutputRenderLink test ensures required API surface existsThe test for
ToOutputRenderLinkconfirms thatcanConnectToSubgraphInputis implemented on the render-link used when connecting to subgraph outputs. This keeps LinkConnector logic safe to call that method without additional guards.If you later expand behavior here, it could be worth adding a second test that exercises a simple valid/invalid compatibility scenario, but this presence check is sufficient for the current change set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts(1 hunks)tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Files:
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
Files:
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using @ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
Files:
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts
src/lib/litegraph/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (src/lib/litegraph/CLAUDE.md)
src/lib/litegraph/**/*.{js,ts,jsx,tsx}: Run ESLint instead of manually figuring out whitespace fixes or other trivial style concerns using thepnpm lint:fixcommand
Take advantage ofTypedArraysubarraywhen appropriate
Thesizeandposproperties ofRectangleshare the same array buffer (subarray); they may be used to set the rectangle's size and position
Prefer single lineifsyntax over adding curly braces, when the statement has a very concise expression and concise, single line statement
Do not replace&&=or||=with=when there is no reason to do so. If you do find a reason to remove either&&=or||=, leave a comment explaining why the removal occurred
When writing methods, prefer returning idiomatic JavaScriptundefinedovernull
Files:
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts
src/lib/litegraph/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/lib/litegraph/CLAUDE.md)
Type assertions are an absolute last resort. In almost all cases, they are a crutch that leads to brittle code
Files:
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript exclusively; no new JavaScript code
Files:
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.tstests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts
**/*.{ts,vue}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,vue}: Use camelCase for variable and function names
Indent with 2 spaces (see.prettierrc)
Use single quotes for strings (see.prettierrc)
No trailing semicolons (see.prettierrc)
Maximum line width of 80 characters (see.prettierrc)
Sort and group imports by plugin (runpnpm formatbefore committing)
Never useanytype; use proper TypeScript types instead
Never useas anytype assertions; fix the underlying type issue instead
Avoid code comments unless absolutely necessary; write expressive, self-documenting code instead
When writing new code, ask if there is a simpler way to introduce the same functionality; if yes, choose the simpler approach
Use refactoring to make complex code simpler
Use es-toolkit for utility functions
Use Vite for fast development and building
Implement proper error handling
Write tests for all changes, especially bug fixes to catch future regressions
Files:
src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.tstests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts
tests-ui/**/*.test.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (tests-ui/CLAUDE.md)
tests-ui/**/*.test.{js,ts,jsx,tsx}: Write tests for new features
Follow existing test patterns in the codebase
Use existing test utilities rather than writing custom utilities
Mock external dependencies in tests
Always prefer vitest mock functions over writing verbose manual mocks
Files:
tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts
**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.ts: Avoid writing change detector tests that just assert default values
Avoid writing tests dependent on non-behavioral features like utility classes or styles
Avoid writing redundant tests
Files:
tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : Use provided test helpers `createTestSubgraph` and `createTestSubgraphNode` from `./fixtures/subgraphHelpers` for consistent subgraph test setup
Applied to files:
tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{ts,tsx} : When writing tests for subgraph-related code, always import from the barrel export at `@/lib/litegraph/src/litegraph` to avoid circular dependency issues
Applied to files:
tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{js,ts,jsx,tsx} : When adding features, always write vitest unit tests using cursor rules in @.cursor
Applied to files:
tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts
🧬 Code graph analysis (1)
tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts (3)
tests-ui/tests/litegraph/subgraph/fixtures/subgraphHelpers.ts (1)
createTestSubgraph(76-165)src/lib/litegraph/src/LGraphCanvas.ts (1)
renderLink(5970-6021)src/lib/litegraph/src/canvas/ToInputFromIoNodeLink.ts (1)
ToInputFromIoNodeLink(20-147)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: setup
- GitHub Check: lint-and-format
- GitHub Check: collect
🔇 Additional comments (4)
tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts (4)
25-64: Regression test accurately covers subgraph input link move cleanupThis test sets up a minimal subgraph, moves a link sourced from
SubgraphInputto a new target viaToInputFromIoNodeLink, and asserts:
- Old target input slot’s
linkisnull.- New target input slot’s
linkis non-null.- Both nodes’
onConnectionsChangecallbacks fire once.That directly exercises the bug described in the PR and should prevent regressions around stale
input.linkreferences when retargeting subgraph input links.
67-160: MovingOutputLink tests give solid coverage of compatibility rulesThe
MovingOutputLinksuite:
- Verifies the presence of
canConnectToSubgraphInput.- Checks strict type compatibility (matching number vs mismatched string).
- Covers wildcard (
*) subgraph inputs accepting any type.The setup via
createTestSubgraphand directLLinkconstruction is clear and focused on behavior, and the expectations align well with typical type-checking semantics for subgraph IO.
179-279: dropOnIoNode tests correctly gate invalid vs valid drops on SubgraphInputNodeThese tests:
- Use
createTestSubgraphandMovingOutputLinkto simulate dragging links.- For invalid types, assert:
console.warnis called with the expected message and types.connectToSubgraphInputis not invoked.- For valid types, assert
connectToSubgraphInputis invoked with the correct subgraph input and event target.The use of spies for both
console.warnandconnectToSubgraphInput, and resettingconnector.renderLinks, makes the behavior under test very explicit and should help catch regressions in drop handling.
281-356: isSubgraphInputValidDrop tests cover key edge casesThe
isSubgraphInputValidDropsuite validates:
- All-invalid render links →
false.- All-valid render links →
true.- Mixed valid/invalid →
true(at least one connectable link).- A render link without
canConnectToSubgraphInput→false.This is a good balance between positive and negative cases and also covers the defensive path for non-conforming render links in
connector.renderLinks.
| // TODO: Fix these tests after migration | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import { LinkConnector } from '@/lib/litegraph/src/litegraph' | ||
| import { MovingOutputLink } from '@/lib/litegraph/src/litegraph' | ||
| import { ToOutputRenderLink } from '@/lib/litegraph/src/litegraph' | ||
| import { LGraphNode, LLink } from '@/lib/litegraph/src/litegraph' | ||
| import { | ||
| LinkConnector, | ||
| MovingOutputLink, | ||
| ToOutputRenderLink, | ||
| LGraphNode, | ||
| LLink | ||
| } from '@/lib/litegraph/src/litegraph' | ||
| import { ToInputFromIoNodeLink } from '@/lib/litegraph/src/canvas/ToInputFromIoNodeLink' | ||
| import type { NodeInputSlot } from '@/lib/litegraph/src/litegraph' | ||
| import { LinkDirection } from '@/lib/litegraph/src/types/globalEnums' | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clean up stale TODO and consider consolidating imports
Now that these tests are fixed and running, the comment on Line 1 (// TODO: Fix these tests after migration) is misleading. It can be removed to avoid confusion.
Also, you’re correctly using the litegraph barrel for most types. If ToInputFromIoNodeLink is (or can be) re-exported from the same barrel, consider importing it from there as well to keep subgraph-related tests on a single import surface and reduce the risk of deep-path coupling.
🤖 Prompt for AI Agents
In tests-ui/tests/litegraph/canvas/LinkConnectorSubgraphInputValidation.test.ts
around lines 1 to 14, remove the stale top-line comment "// TODO: Fix these
tests after migration" and, if ToInputFromIoNodeLink is re-exported by the
litegraph barrel, replace its deep import with a single import from
'@/lib/litegraph/src/litegraph' to consolidate imports; update the import list
accordingly so all litegraph symbols come from the same barrel and delete the
misleading TODO comment.
AustinMroz
force-pushed
the
austin/move-subgraph-input-link-fix
branch
from
7bfe62d to
6d705ef
Compare
christian-byrne
added
needs-backport
When moving an existing link with subgraphInput as source to a new node, the prior link is removed, but the previous target node would not have it's link property cleared. Resolves #7225 Also re-enables several functional skipped tests. It feels like I'm having to play whack-a-mole reimplementing the same fixes on every permutation of subgraphIO links. I'd like to setup up a unified test set that covers them all, but wouldn't want the added work to further delay this fix. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7229-When-moving-subgraphInput-link-properly-disconnect-old-link-2c36d73d36508149aca0ce477fee5c9e) by [Unito](https://www.unito.io)
|
@AustinMroz Successfully backported to #7255 |
github-actions
bot
removed
the
needs-backport
…ect old link (#7255) Backport of #7229 to `core/1.33` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-7255-backport-core-1-33-When-moving-subgraphInput-link-properly-disconnect-old-link-2c46d73d365081cda606e81ec4cbfeec) by [Unito](https://www.unito.io) Co-authored-by: AustinMroz <austin@comfy.org>
When moving an existing link with subgraphInput as source to a new node, the prior link is removed, but the previous target node would not have it's link property cleared.
Resolves #7225
Also re-enables several functional skipped tests.
It feels like I'm having to play whack-a-mole reimplementing the same fixes on every permutation of subgraphIO links. I'd like to setup up a unified test set that covers them all, but wouldn't want the added work to further delay this fix.
┆Issue is synchronized with this Notion page by Unito