Skip to content

Conversation

christian-byrne
Copy link
Contributor

@christian-byrne christian-byrne commented Sep 25, 2025

Summary

Added pinning functionality to Vue nodes with hotkey support and visual indicators.

Changes

  • What: Added node pinning feature with 'p' hotkey toggle and pin icon indicator
  • Components: Updated LGraphNode.vue and NodeHeader.vue with pin state tracking
  • State Management: Extended useGraphNodeManager to sync pinned flag with Vue components
  • Tests: Added E2E tests for single and multi-node pin toggling

Review Focus

Pin state persistence in graph serialization and visual indicator positioning in node header layout. Verify hotkey doesn't conflict with existing shortcuts.

Technical Details

  • Pin state tracked via flags.pinned property in LGraphNode
  • Uses Vue memoization for efficient header re-rendering
  • Integrates with existing node property change detection system
  • Visual indicator uses Lucide pin icon with theme-aware styling

Screenshots (if applicable)

Screenshot from 2025-09-25 13-02-21 Screenshot from 2025-09-25 13-02-10

Related

┆Issue is synchronized with this Notion page by Unito

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

github-actions bot commented Sep 25, 2025

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 09/27/2025, 05:55:16 PM UTC

📈 Summary

  • Total Tests: 465
  • Passed: 433 ✅
  • Failed: 0
  • Flaky: 3 ⚠️
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

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

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

AustinMroz
AustinMroz previously approved these changes Sep 25, 2025
Copy link
Collaborator

@AustinMroz AustinMroz left a comment

Choose a reason for hiding this comment

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

This is just state tracking and display right? You can still actually drag vue nodes which are pinned.

webfiltered
webfiltered previously approved these changes Sep 25, 2025
// Delete the flag if unpinned, so that we don't get unnecessary
// flags.pinned = false in serialized object.
if (!this.pinned) delete this.flags.pinned
if (!this.pinned) this.flags.pinned = undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Supremely happy that this is no longer necessary~

DrJKL
DrJKL previously approved these changes Sep 25, 2025
@edit="handleTitleEdit"
@cancel="handleTitleCancel"
/>
<i-lucide:pin
Copy link
Contributor

Choose a reason for hiding this comment

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

This could use the class based icon, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better? Or just for consistency?

@christian-byrne
Copy link
Contributor Author

This is just state tracking and display right? You can still actually drag vue nodes which are pinned.

omg I forgot to actually implement the pinning aspect

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 27, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Sep 27, 2025
@christian-byrne
Copy link
Contributor Author

Changes:

  • Prevent node from being dragged while pinned

@christian-byrne christian-byrne merged commit 856eb44 into main Sep 27, 2025
25 checks passed
@christian-byrne christian-byrne deleted the vue-nodes/pinned-state branch September 27, 2025 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:vue-migration size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants