Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/components/graph/GraphCanvas.vue
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@
/>
</TransformPane>

<!-- Selection rectangle overlay for Vue nodes mode -->
<SelectionRectangle v-if="shouldRenderVueNodes && comfyAppReady" />

<NodeTooltip v-if="tooltipEnabled" />
<NodeSearchboxPopover ref="nodeSearchboxPopoverRef" />

Expand Down Expand Up @@ -157,6 +160,7 @@ import { useWorkspaceStore } from '@/stores/workspaceStore'
import { isNativeWindow } from '@/utils/envUtil'

import TryVueNodeBanner from '../topbar/TryVueNodeBanner.vue'
import SelectionRectangle from './SelectionRectangle.vue'

const emit = defineEmits<{
ready: []
Expand Down
63 changes: 63 additions & 0 deletions src/components/graph/SelectionRectangle.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<template>
<div
v-if="isVisible"
class="pointer-events-none absolute border border-blue-400 bg-blue-500/20"
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: Want to use theme tokens instead?

:style="rectangleStyle"
/>
</template>

<script setup lang="ts">
import { useRafFn } from '@vueuse/core'
import { computed, ref } from 'vue'
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
const canvasStore = useCanvasStore()
const selectionRect = ref<{
Copy link

Choose a reason for hiding this comment

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

[quality] low Priority

Issue: Inline type definition instead of reusing existing types
Context: The selection rectangle type is defined inline, but ComfyUI likely has existing Rectangle or Bounds types that could be reused
Suggestion: Consider extracting this to a shared type or importing an existing type from the litegraph types to maintain consistency with the codebase

x: number
y: number
w: number
h: number
} | null>(null)
useRafFn(() => {
const canvas = canvasStore.canvas
if (!canvas) {
selectionRect.value = null
return
}
const { pointer, dragging_rectangle } = canvas
Copy link

Choose a reason for hiding this comment

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

[quality] medium Priority

Issue: Missing null safety checks for canvas properties access
Context: The code directly accesses canvas.pointer without checking if these properties exist, which could cause runtime errors if the canvas state is unexpected
Suggestion: Add null checks: if (!canvas || !canvas.pointer) { selectionRect.value = null; return }

if (dragging_rectangle && pointer.eDown && pointer.eMove) {
Copy link

Choose a reason for hiding this comment

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

[quality] medium Priority

Issue: Missing null checks for pointer.eDown and pointer.eMove properties
Context: The code assumes eDown and eMove are always present when dragging_rectangle is true, but these could be undefined in edge cases
Suggestion: Add checks: if (dragging_rectangle && pointer.eDown && pointer.eMove && pointer.eDown.safeOffsetX !== undefined)

const x = pointer.eDown.safeOffsetX
const y = pointer.eDown.safeOffsetY
const w = pointer.eMove.safeOffsetX - x
const h = pointer.eMove.safeOffsetY - y
selectionRect.value = { x, y, w, h }
} else {
selectionRect.value = null
}
})
Copy link

Choose a reason for hiding this comment

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

[quality] low Priority

Issue: Missing explicit cleanup in SelectionRectangle component
Context: While Vue will clean up automatically, the RAF callback could continue running briefly after unmount if the component is destroyed while dragging
Suggestion: Add onUnmounted(() => { pause() }) using the pause function returned by useRafFn for explicit cleanup

const isVisible = computed(() => selectionRect.value !== null)
const rectangleStyle = computed(() => {
const rect = selectionRect.value
if (!rect) return {}
const left = rect.w >= 0 ? rect.x : rect.x + rect.w
const top = rect.h >= 0 ? rect.y : rect.y + rect.h
const width = Math.abs(rect.w)
const height = Math.abs(rect.h)
return {
left: `${left}px`,
top: `${top}px`,
width: `${width}px`,
height: `${height}px`
}
})
</script>
3 changes: 2 additions & 1 deletion src/lib/litegraph/src/LGraphCanvas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4799,7 +4799,8 @@ export class LGraphCanvas
}

// Area-selection rectangle
if (this.dragging_rectangle) {
// In Vue nodes mode, selection rectangle is rendered in DOM layer
if (this.dragging_rectangle && !LiteGraph.vueNodesMode) {
Copy link

Choose a reason for hiding this comment

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

[architecture] medium Priority

Issue: Direct dependency on global LiteGraph.vueNodesMode creates tight coupling
Context: The canvas rendering logic now depends on a global flag, which breaks separation of concerns and makes testing harder
Suggestion: Consider injecting this dependency or using a callback/strategy pattern instead of global state access

Copy link
Contributor

Choose a reason for hiding this comment

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

We really need to fix this app-wide...

const { eDown, eMove } = this.pointer
ctx.strokeStyle = '#FFF'

Expand Down