Skip to content

fix mask editor bug under vueNodes #5953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+39 −4
Open

fix mask editor bug under vueNodes #5953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e6a759
fix mask editor bug under vueNodes
jtydhr88 Oct 7, 2025
f6d75c7
improve code
jtydhr88 Oct 8, 2025
0d850ba
add img.crossOrigin
jtydhr88 Oct 8, 2025
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
27 changes: 25 additions & 2 deletions src/renderer/extensions/vueNodes/components/ImagePreview.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ import { computed, ref, watch } from 'vue'
import { useI18n } from 'vue-i18n'

import { downloadFile } from '@/base/common/downloadUtil'
import { app } from '@/scripts/app'
import { useCommandStore } from '@/stores/commandStore'
import { useNodeOutputStore } from '@/stores/imagePreviewStore'

Expand Down Expand Up @@ -186,8 +187,30 @@ const handleImageError = () => {
actualDimensions.value = null
}

const handleEditMask = () => {
void commandStore.execute('Comfy.MaskEditor.OpenMaskEditor')
const handleEditMask = async () => {
if (props.nodeId) {
const node = app.rootGraph?.getNodeById(props.nodeId)
if (node) {
node.imageIndex = currentIndex.value
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 mutation of node object properties
Context: Directly setting node.imageIndex and node.imgs bypasses Vue reactivity and violates immutability principles
Suggestion: Use a reactive store or emit events to parent components to handle state changes properly


node.imgs = await Promise.all(
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: Promise rejection not handled - Image loading could fail silently
Context: If an image fails to load in the Promise.all, the entire operation fails but there's no error handling
Suggestion: Add proper error handling with try-catch or Promise.allSettled to handle individual failures gracefully

Copy link

Choose a reason for hiding this comment

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

[performance] medium Priority

Issue: Blocking image loading may freeze UI
Context: Promise.all waits for all images to load synchronously, potentially causing UI freezes with large images or slow networks
Suggestion: Consider loading images asynchronously or showing a loading indicator during this operation

props.imageUrls.map((url) => {
return new Promise<HTMLImageElement>((resolve) => {
const img = new Image()

img.crossOrigin = 'anonymous'
img.onload = () => resolve(img)

img.src = url
})
})
)

app.canvas?.selectNode(node)
}
}

await commandStore.execute('Comfy.MaskEditor.OpenMaskEditor')
}

const handleDownload = () => {
Expand Down
16 changes: 14 additions & 2 deletions src/scripts/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,11 +379,15 @@ export class ComfyApp {
const paintedIndex = selectedIndex + 1
const combinedIndex = selectedIndex + 2

// for vueNodes mode
const images =
node.images ?? useNodeOutputStore().getNodeOutputs(node)?.images

ComfyApp.clipspace = {
widgets: widgets,
imgs: imgs,
original_imgs: orig_imgs,
images: node.images,
images: images,
selectedIndex: selectedIndex,
img_paste_mode: 'selected', // reset to default im_paste_mode state on copy action
paintedIndex: paintedIndex,
Expand All @@ -410,7 +414,8 @@ export class ComfyApp {
ComfyApp.clipspace.imgs[ComfyApp.clipspace.combinedIndex].src
}
if (ComfyApp.clipspace.imgs && node.imgs) {
if (node.images && ComfyApp.clipspace.images) {
// Update node.images even if it's initially undefined (vueNodes mode)
if (ComfyApp.clipspace.images) {
Copy link

Choose a reason for hiding this comment

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

[quality] high Priority

Issue: Logic condition removed may cause null reference errors
Context: Removed check for node.images existence before accessing ClipSpace.images could cause undefined/null references
Suggestion: Restore defensive check or add null safety: if (ComfyApp.clipspace.images && node.images)

if (ComfyApp.clipspace['img_paste_mode'] == 'selected') {
node.images = [
ComfyApp.clipspace.images[ComfyApp.clipspace['selectedIndex']]
Expand Down Expand Up @@ -512,6 +517,13 @@ export class ComfyApp {
}

app.graph.setDirtyCanvas(true)

if (node.images && app.nodeOutputs[node.id]) {
useNodeOutputStore().nodeOutputs[node.id] = {
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: Potential race condition with nodeOutputs access
Context: Accessing app.nodeOutputs[node.id] and useNodeOutputStore().nodeOutputs[node.id] without proper synchronization could lead to race conditions
Suggestion: Add proper locks or use atomic operations when modifying shared state

...app.nodeOutputs[node.id],
images: node.images
}
}
}
}

Expand Down