Skip to content

Conversation

christian-byrne
Copy link
Contributor

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

Summary

Implemented comprehensive node resizing functionality for Vue-rendered nodes with layout synchronization and performance optimizations.

Changes

  • What: Added interactive resize handles to Vue nodes with real-time visual feedback and layout store integration
  • Dependencies: New composable useNodeResize.ts for resize interaction handling

Review Focus

Coordinate system conversions between screen and canvas space, pointer capture handling, and layout synchronization timing to prevent reactive loops.

graph TD
    A[Resize Handle Interaction] --> B{Drag Started?}
    B -->|Yes| C[Pause Auto Tracking]
    C --> D[Update Local Size State]
    D --> E[Visual Feedback]
    E --> F{Drag Continuing?}
    F -->|Yes| D
    F -->|No| G[Resume Auto Tracking]
    G --> H[Sync to Layout Store]
    H --> I[Force Sync to LiteGraph]

    style A fill:#f9f9f9,stroke:#333,color:#000
    style I fill:#f9f9f9,stroke:#333,color:#000
Loading

Key implementation details:

  • Resize composable with pointer capture and constraint handling
  • Tracking pause/resume system to prevent conflicts during manual resize
  • Canvas coordinate conversion for proper scaling with zoom levels
  • Workflow persistence integration with forceSyncAll() method

┆Issue is synchronized with this Notion page by Unito

Add comprehensive node resizing support that integrates with the existing
layout system and persists to workflow state.

Features:
- Transform-aware resizing that works correctly at any zoom level
- Cooperative tracking system that pauses automatic resize detection during manual interactions
- Clean cursor-based resize handle (subtle hover effect)
- Proper integration with layout store and workflow serialization
- Size constraints (min/max width/height) to prevent unusable node sizes

Implementation:
- New useNodeResize composable for resize logic with coordinate conversion
- Enhanced useVueElementTracking with pause/resume functionality
- Layout sync improvements to ensure changes persist to workflow JSON
- Integration with existing layout mutations and state management

The resize handle appears as a small area in the bottom-right corner of nodes
with a subtle hover effect and se-resize cursor. Resizing updates the layout
store and properly syncs to LiteGraph for workflow persistence.
@christian-byrne christian-byrne changed the title feat: Vue node resizing functionality [WIP] Vue node resizing functionality Sep 15, 2025
Resolve workflow persistence import conflict and fix test async issue.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Copy link

github-actions bot commented Sep 15, 2025

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 09/15/2025, 10:23:47 AM UTC

📈 Summary

  • Total Tests: 449
  • Passed: 419 ✅
  • Failed: 0
  • Flaky: 1 ⚠️
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 412 / ❌ 0 / ⚠️ 1 / ⏭️ 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.

@comfyui-wiki comfyui-wiki linked an issue Sep 21, 2025 that may be closed by this pull request
@christian-byrne christian-byrne marked this pull request as ready for review September 22, 2025 14:30
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 22, 2025
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't pause the RO tracking while resizing, the node "slides" back into its original height/width after you finish resizing. I need to debug it a bit further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to find a better approach for this module.

Copy link
Contributor

Choose a reason for hiding this comment

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

useElementSize?

https://vueuse.org/core/useElementSize/

  export function useNodeResize(nodeId) {
    const { batchSync } = useLayoutBatching()
    const nodeElement = useTemplateRef<HTMLElement>('nodeElementRef')

    const { width, height } = useElementSize(nodeElement)

    watch([width, height], ([newWidth, newHeight]: [number, number]) => {
      if (!newWidth || !newHeight) return

      batchSync(nodeId, { width: newWidth, height: newHeight })
    })

    // Separate resize handle logic (not using useDraggable)
    function attachResizeHandles() {
      // Implement custom resize handles with mouse events
      // This directly modifies DOM element size
      // useElementSize will detect the changes and trigger batchSync
    }

    return { width, height, nodeElement }
  }

Copy link
Contributor Author

@christian-byrne christian-byrne Sep 22, 2025

Choose a reason for hiding this comment

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

This could be moved into a separate PR, as it's not necessary for resizing to work - just ensures the newly sized nodes' dimensions are serialized/saved.

/**
* Get all node layouts for syncing to external systems
*/
getAllNodeLayouts(): Map<NodeId, NodeLayout | null> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't use a function like this as it just creates the potential for de-sync and race conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to other file, this can be moved to separate PR

This could be moved into a separate PR, as it's not necessary for resizing to work - just ensures the newly sized nodes' dimensions are serialized/saved.

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.

Functionality and direction both look great for a WIP

The forceSyncAll in persistCurrentWorkflow is a acceptable, but a little ick. It's a slippery slope to O(n^2) performance where every type of action creates a persistence call which causes every possible action to sync. From my testing,

  • Syncing resizes back to litegraph is working fine.
  • execution of persistCurrentWorkflow after a resize has strange behaviour currently. A resize seems to set state as dirty. but doesn't initiate a persist itself, so the dirty state persists until the next option that can resolve dirty state, like the end of a canvas pan

) {
const {
minWidth = 200,
minHeight = 100,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we have some of these as constants somewhere?

Copy link
Contributor

@arjansingh arjansingh left a comment

Choose a reason for hiding this comment

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

So this was a fun problem, I rubber ducked with Claude for a bit on it and came up with the following suggestions:

  1. I think a batching approach where we just flush updates every RAF could be a good middle ground. That way canvas isn't being set dirty too frequently.
  2. I think we can simplify the code with vue-use stuff. I don't think it would be a perf hit but feel free to push back and explain if you think it is.

The sequence in a nutshell:

  sequenceDiagram
      participant User
      participant ResizeHandle
      participant useElementSize
      participant useLayoutBatching
      participant VueStore
      participant Canvas
      participant LiteGraph

      User->>ResizeHandle: Start resize drag
      ResizeHandle->>ResizeHandle: Modify DOM element size directly

      Note over useElementSize: Single source of truth - watches DOM changes
      loop During resize (60fps max)
          useElementSize->>useElementSize: Detect DOM size change
          useElementSize->>useLayoutBatching: batchSync(nodeId, {width, height})
          useLayoutBatching->>useLayoutBatching: pendingUpdates.set(nodeId, layout)

          alt First update this frame
              useLayoutBatching->>useLayoutBatching: Schedule RAF callback
          else Subsequent updates same frame
              useLayoutBatching->>useLayoutBatching: Just update pending map
          end
      end

      Note over useLayoutBatching: RAF executes once per frame maximum
      useLayoutBatching->>useLayoutBatching: processAllUpdates()
      useLayoutBatching->>VueStore: flushAllUpdates() - batch update reactive state
      useLayoutBatching->>Canvas: markCanvasDirty() - ONCE per frame
      Canvas->>LiteGraph: Schedule canvas redraw

      User->>ResizeHandle: End resize
      useElementSize->>useLayoutBatching: Final batchSync()
      useLayoutBatching->>VueStore: Final layout update
      useLayoutBatching->>Canvas: Final setDirty()
Loading

Copy link
Contributor

Choose a reason for hiding this comment

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

useElementSize?

https://vueuse.org/core/useElementSize/

  export function useNodeResize(nodeId) {
    const { batchSync } = useLayoutBatching()
    const nodeElement = useTemplateRef<HTMLElement>('nodeElementRef')

    const { width, height } = useElementSize(nodeElement)

    watch([width, height], ([newWidth, newHeight]: [number, number]) => {
      if (!newWidth || !newHeight) return

      batchSync(nodeId, { width: newWidth, height: newHeight })
    })

    // Separate resize handle logic (not using useDraggable)
    function attachResizeHandles() {
      // Implement custom resize handles with mouse events
      // This directly modifies DOM element size
      // useElementSize will detect the changes and trigger batchSync
    }

    return { width, height, nodeElement }
  }


// Force sync all layout changes to LiteGraph before serialization
if (comfyApp.canvas) {
forceSyncAll(comfyApp.canvas)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe instead of a forced sync, we can try to just update once a frame?

something like:

  // useLayoutBatching.ts
  import { reactive, onScopeDispose } from 'vue'

  export function useLayoutBatching() {
    const pendingUpdates = reactive(new Map<NodeId, NodeLayout>())
    let rafId: number | null = null

    function batchSync(nodeId: NodeId, layout: NodeLayout) {
      // Always accumulate updates in reactive map
      pendingUpdates.set(nodeId, layout)

      // Schedule RAF if not already scheduled
      if (rafId) return
      rafId = requestAnimationFrame(processAllUpdates)
    }

    function processAllUpdates() {
      // Create snapshot of updates to process
      const updatesToProcess = new Map(pendingUpdates)
      pendingUpdates.clear()
      flushAllUpdates(updatesToProcess)

      // Single canvas dirty call per frame
      canvas.setDirty()
      rafId = null
    }

    function flushAllUpdates(updates: Map<NodeId, NodeLayout>) {
      updates.forEach((layout, nodeId) => {
        updateNodeLayout(nodeId, layout)
      })
    }

    function updateNodeLayout(nodeId: NodeId, layout: NodeLayout) {
      // Update the Vue reactive store with new node dimensions
      // Should call layoutStore.updateNodeLayout(nodeId, layout) or similar
      // This keeps Vue state in sync with DOM changes
    }

    function cleanup() {
      if (rafId !== null) {
        cancelAnimationFrame(rafId)
        rafId = null
      }
      pendingUpdates.clear()
    }

    // Auto-cleanup when scope is disposed
    onScopeDispose(cleanup)

    return {
      batchSync,
      cleanup // Expose for manual cleanup if needed
    }
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that might be heavier than we need, since litegraph already does batching in its draw.

v-if="!readonly"
class="absolute bottom-0 right-0 w-3 h-3 cursor-se-resize opacity-0 hover:opacity-20 hover:bg-white transition-opacity duration-200"
@pointerdown.stop="startResize"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Proposed solution would be something like:

  <script setup lang="ts">
  import { useNodeResize } from './composables/useNodeResize'

  interface Props {
    nodeId: string
    title: string
  }

  const props = defineProps<Props>()
  const { nodeElement, width, height } = useNodeResize(props.nodeId)

  function handleNodeClick() {
    // Handle node selection, focus, etc.
    // Should update node state without triggering resize
  }

  function handleContextMenu(event: MouseEvent) {
    // Handle right-click menu for node operations
    // Should prevent default browser context menu
    event.preventDefault()
  }
  </script>

  <template>
    <div
      ref="nodeElementRef"
      :data-node-id="nodeId"
      class="graph-node"
      @click="handleNodeClick"
      @contextmenu="handleContextMenu"
    >
      <!-- Node Content -->
      <!-- Resize Handle -->
      <div class="resize-handle" />
    </div>
  </template>

@christian-byrne
Copy link
Contributor Author

Replaced by #5936

@christian-byrne christian-byrne deleted the feat/node-resize branch October 6, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:node-interaction 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.

Can't resize the node size
6 participants