Implement drop-on-canvas + linkconnectoradapter consolidation #5898

benceruleanlu · 2025-10-03T01:59:04Z

Implements droponcanvas functionality and a linkconnectoradapter refactor.

Drop on canvas (Shift and default) integrated via LinkConnector ‘dropped-on-canvas’ with proper CanvasPointerEvent.
LinkConnector adapter: now wraps the live canvas linkConnector (no duplicate state); added dropOnCanvas() helper.
Tests: Playwright scenarios for Shift-drop context menu/searchbox, pinned endpoint, type prefilter, and post-selection auto-connect (browser_tests/tests/vueNodes/interactions/links/linkInteraction.spec.ts).

There are some followup PRs that will fix/refactor some more noncritical things, like the terrible slotid, the number/string nodeid confusion, etc.

#5898 <-- #5903

github-actions · 2025-10-03T01:59:32Z

🎭 Playwright Test Results

❌ Some tests failed

⏰ Completed at: 10/03/2025, 11:35:16 AM UTC

📈 Summary

Total Tests: 483
Passed: 442 ✅
Failed: 6 ❌
Flaky: 5 ⚠️
Skipped: 30 ⏭️

📊 Test Reports by Browser

✅ chromium: View Report • ✅ 433 / ❌ 6 / ⚠️ 5 / ⏭️ 30
✅ chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
✅ mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

claude · 2025-10-03T20:28:38Z

src/renderer/core/canvas/interaction/canvasPointerEvent.ts

+  y: number
+}
+
+const pointerHistory = new Map<number, PointerOffsets>()


[performance] medium Priority

Issue: Pointer history map grows unbounded without cleanup of stale entries
Context: pointerHistory Map on line 13 accumulates entries for all pointer IDs but only explicitly cleans up when clearCanvasPointerHistory is called, which may not happen for all pointers
Suggestion: Add periodic cleanup or use WeakMap if possible, or implement automatic cleanup in pointerup/pointercancel events

claude · 2025-10-03T20:28:47Z

src/renderer/core/canvas/interaction/canvasPointerEvent.ts

+    event.clientY
+  ])
+
+  const canvas = useCanvasStore().getCanvas()


[quality] low Priority

Issue: Potential undefined canvas access without null check
Context: useCanvasStore().getCanvas() could return undefined, then accessing .ds property would throw
Suggestion: Add null check: const canvas = useCanvasStore().getCanvas(); if (!canvas) return defaultEnhancement;

claude · 2025-10-03T20:28:55Z

src/renderer/core/canvas/links/linkDropOrchestrator.ts

+    if (cached != null) {
+      candidate.compatible = cached
+    } else {
+      const nodeId = Number(layout.nodeId)


[quality] medium Priority

Issue: Inconsistent nodeId types - mixing string and number conversions
Context: Layout.nodeId is string (line 34) but converted to number for graph.getNodeById calls throughout the codebase
Suggestion: Establish consistent type discipline - either use NodeId type consistently or create utility functions for safe conversion

this is something I'm working on to fix, it's scuffed right now but works, it's because of the terrible group node code that forces it to be number or string type

claude · 2025-10-03T20:29:02Z

src/renderer/core/canvas/links/linkDropOrchestrator.ts

+
+  if (!adapter || !graph) return null
+
+  const nodeId = Number(nodeIdStr)


[quality] medium Priority

Issue: No validation that Number() conversion succeeded before using nodeId
Context: Number(layout.nodeId) could return NaN if layout.nodeId contains non-numeric data, which would cause graph.getNodeById to fail silently
Suggestion: Add validation: const nodeId = Number(layout.nodeId); if (isNaN(nodeId)) return null;

We technically support string-based node IDs (like "nodeName") if you look at the backend code - esp. for subgraphs or virtual nodes on the backend IIRC.

claude · 2025-10-03T20:29:39Z

src/renderer/core/canvas/links/linkConnectorAdapter.ts

+import type { CanvasPointerEvent } from '@/lib/litegraph/src/types/events'
 import { app } from '@/scripts/app'

 // Keep one adapter per graph so rendering and interaction share state.


[architecture] low Priority

Issue: WeakMap usage might cause adapter recreation if LGraph is temporarily dereferenced
Context: WeakMap allows garbage collection which could cause adapter recreation, potentially breaking the singleton pattern intended by the comment above
Suggestion: Consider if explicit cleanup on graph destruction would be more predictable than relying on GC

if that happens i think that should be fixed and not making this more resilient

claude · 2025-10-03T20:29:58Z

src/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.ts

+
+    app.canvas?.setDirty(true, true)
  }
  const raf = createRafBatch(processPointerMoveFrame)


[performance] medium Priority

Issue: Heavy computation in RAF callback could cause frame drops
Context: processPointerMoveFrame performs expensive DOM queries, layout lookups, and cache operations on every animation frame
Suggestion: Consider debouncing DOM queries or pre-caching hover targets to reduce per-frame work

true, let me recheck perf

src/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.ts

claude

Comprehensive PR Review

This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.

Review Summary

PR: Implement drop-on-canvas + linkconnectoradapter consolidation (#5898)
Impact: 469 additions, 240 deletions across 15 files

Issue Distribution

Critical: 0
High: 0
Medium: 4
Low: 3

Category Breakdown

Architecture: 1 issues
Security: 0 issues
Performance: 2 issues
Code Quality: 4 issues

Key Findings

Architecture & Design

This PR implements a significant refactor of the link interaction system with good architectural principles:

LinkConnectorAdapter consolidation: Good move to eliminate duplicate state by wrapping the live canvas linkConnector rather than creating new instances
Drop orchestrator pattern: Clean separation of concerns with specialized functions for slot and node surface candidate resolution
Canvas pointer event abstraction: Well-designed abstraction for canvas coordinate conversion and pointer history

The WeakMap usage for adapter caching follows a reasonable pattern, though the garbage collection behavior might cause unexpected adapter recreation.

Performance Impact

The RAF-based pointer movement processing is well-architected but could benefit from optimization:

Heavy DOM querying operations on every animation frame
Unbounded pointer history growth without cleanup mechanisms
Consider debouncing expensive operations or pre-caching hover targets

Bundle impact appears minimal with mostly refactoring existing functionality.

Integration Points

Backward compatibility: Preserved existing Vue composable behavior patterns
Extension compatibility: Maintained existing public APIs that extensions depend on
Test coverage: Excellent Playwright test coverage for the new shift-drop functionality

Positive Observations

Comprehensive E2E test coverage including edge cases and error scenarios
Clean TypeScript interfaces and proper type safety throughout
Good use of caching patterns to avoid repeated expensive operations
Proper cleanup patterns with pointer history management
Well-structured composable following Vue 3 best practices

References

Repository Architecture Guide
Vue 3 Composition API patterns
ComfyUI Frontend Guidelines (CLAUDE.md)

Next Steps

Address medium priority issues for better robustness (pointer history cleanup, number validation)
Consider performance optimizations for RAF callbacks
Validate comprehensive test coverage passes
Consider documenting the event propagation patterns

This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.

benceruleanlu · 2025-10-03T20:49:56Z

@codex review

chatgpt-codex-connector · 2025-10-03T20:55:06Z

Codex Review: Didn't find any major issues. 👍

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

@codex fix this CI failure
@codex address that feedback

benceruleanlu added 10 commits October 2, 2025 13:21

Add more ts files to allowDefaultProject

47781c7

Temporarily increase playwright timeout

f198e13

Temporarily regenerate screenshot expectations

6cc0a1e

Export interface SlotLinkDragSession

634dec5

Remove slotLinkPreviewRenderer

bae44be

Add more test cases

f271fed

Make linkconnector use the canonical linkconnector

e04bf8a

add canvasPointerEvent helper

d2f3051

Add linkDropOrchestrator

fafd374

Add canvas drop to useSlotLinkInteraction.ts

d9a283b

benceruleanlu changed the title ~~Vue Nodes: drop-on-canvas + pinned endpoint; adapter consolidation; tests~~ Implement drop-on-canvas + linkconnectoradapter consolidation Oct 3, 2025

Revert temporary playwright timeout increase

b00e429

benceruleanlu mentioned this pull request Oct 3, 2025

feat: handle empty link drop in Vue #5850

Closed

benceruleanlu marked this pull request as ready for review October 3, 2025 03:15

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Oct 3, 2025

benceruleanlu added area:links area:vue-migration labels Oct 3, 2025

benceruleanlu mentioned this pull request Oct 3, 2025

Fix reroute links not snapping correctly #5903

Open

Merge remote-tracking branch 'origin/bl-snap' into bl-linkslot-refactor

2ddb8f3

benceruleanlu added the New Browser Test Expectations New browser test screenshot should be set by github action label Oct 3, 2025

[automated] Update test expectations

89a2315

christian-byrne added the claude-review Add to trigger a PR code review from Claude Code label Oct 3, 2025

claude bot reviewed Oct 3, 2025

View reviewed changes

src/renderer/extensions/vueNodes/composables/useSlotLinkInteraction.ts Show resolved Hide resolved

claude bot reviewed Oct 3, 2025

View reviewed changes


		if (!adapter \|\| !graph) return null

		const nodeId = Number(nodeIdStr)

Implement drop-on-canvas + linkconnectoradapter consolidation #5898

Are you sure you want to change the base?

Implement drop-on-canvas + linkconnectoradapter consolidation #5898

Uh oh!

Conversation

benceruleanlu commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎭 Playwright Test Results

📈 Summary

📊 Test Reports by Browser

Uh oh!

claude bot Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

benceruleanlu Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

benceruleanlu Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

christian-byrne Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

benceruleanlu Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

benceruleanlu Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Comprehensive PR Review

Review Summary

Issue Distribution

Category Breakdown

Key Findings

Architecture & Design

Performance Impact

Integration Points

Positive Observations

References

Next Steps

Uh oh!

benceruleanlu commented Oct 3, 2025

Uh oh!

chatgpt-codex-connector bot commented Oct 3, 2025

Uh oh!

Uh oh!

benceruleanlu commented Oct 3, 2025 •

edited

Loading

github-actions bot commented Oct 3, 2025 •

edited

Loading

christian-byrne Oct 3, 2025 •

edited

Loading