Skip to content

Conversation

Myestery
Copy link
Collaborator

@Myestery Myestery commented Sep 30, 2025

This pull request refactors the node selection logic in the Vue nodes event handler composable to simplify the function signature and improve single vs. multi-selection behavior. The main change is the removal of the wasDragging parameter from the handleNodeSelect function, with selection logic now determined by the current selection state. Related test code is updated to match the new function signature.

Node selection logic improvements:

  • Refactored the handleNodeSelect function in useNodeEventHandlersIndividual to remove the wasDragging parameter, making the function signature simpler and relying on selection state to handle single vs. multi-selection.
  • Updated the selection logic to check if multiple nodes are already selected using isLGraphNode, and only perform single selection if not.

Code and test updates:

  • Updated all calls to handleNodeSelect in the composable to remove the wasDragging argument, ensuring consistent usage throughout the codebase. [1] [2] [3]
  • Updated all related test cases to use the new handleNodeSelect signature without the third parameter. [1] [2] [3] [4] [5] [6]

Utility import:

  • Added an import for isLGraphNode from @/utils/litegraphUtil to support the updated selection logic.## Summary

Screenshots (if applicable)

Screen.Recording.2025-09-30.at.17.47.18.mov

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 30, 2025
Copy link

github-actions bot commented Sep 30, 2025

🎭 Playwright Test Results

Some tests failed

⏰ Completed at: 09/30/2025, 04:57:38 PM UTC

📈 Summary

  • Total Tests: 481
  • Passed: 445 ✅
  • Failed: 5 ❌
  • Flaky: 2 ⚠️
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

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

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

Copy link

github-actions bot commented Sep 30, 2025

🎨 Storybook Build Status

Build completed successfully!

⏰ Completed at: 09/30/2025, 04:47:43 PM UTC

🔗 Links


🎉 Your Storybook is ready for review!

// If it wasn't a drag: single-select the node
if (!wasDragging) {
const selectedMultipleNodes =
canvasStore.selectedItems.filter((n) => isLGraphNode(n)).length > 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a little weird to me. Can we change the selection logic in useNodePointerInteractions instead? I think we'll have to change the interface there anyway since that's where the dragging logic lives.

I would expect the selection to be apparent as I click and drag, not after I release the click.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - I think we should try to match litegraph behavior exactly (unless it's extremely difficult to do).

Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fairly comfortable to add a test case here, following the existing patterns:

test(`should allow selecting multiple nodes with ${name}+click`, async ({
comfyPage
}) => {
await comfyPage.page.getByText('Load Checkpoint').click()
expect(await comfyPage.vueNodes.getSelectedNodeCount()).toBe(1)
await comfyPage.page.getByText('Empty Latent Image').click({
modifiers: [modifier]
})
expect(await comfyPage.vueNodes.getSelectedNodeCount()).toBe(2)
await comfyPage.page.getByText('KSampler').click({
modifiers: [modifier]
})
expect(await comfyPage.vueNodes.getSelectedNodeCount()).toBe(3)
})

Comment on lines +48 to +49
const selectedMultipleNodes =
canvasStore.selectedItems.filter((n) => isLGraphNode(n)).length > 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can optimize this by just doing a normal for loop and returning early if we hit 2, then move it to existing utils file or into the module scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long term if we're doing this in multiple places, it might be a candidate for a computed value in the store. Something like canvasStore.selectedNodesCount? (Depending on how expensive the filtering ends up being)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last time I checked, canvas.selectedItems is not reactive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants