-
Notifications
You must be signed in to change notification settings - Fork 498
fix: sync node.imgs for legacy context menu in Vue Nodes mode #8143
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import { expect } from '@playwright/test' | ||
|
|
||
| import { comfyPageFixture as test } from '../../../../fixtures/ComfyPage' | ||
|
|
||
| test.describe('Vue Nodes Image Preview', () => { | ||
| test.beforeEach(async ({ comfyPage }) => { | ||
| await comfyPage.setSetting('Comfy.VueNodes.Enabled', true) | ||
| await comfyPage.loadWorkflow('widgets/load_image_widget') | ||
| await comfyPage.vueNodes.waitForNodes() | ||
| }) | ||
|
|
||
| async function loadImageOnNode( | ||
| comfyPage: Awaited< | ||
| ReturnType<(typeof test)['info']> | ||
| >['fixtures']['comfyPage'] | ||
| ) { | ||
| const loadImageNode = (await comfyPage.getNodeRefsByType('LoadImage'))[0] | ||
| const { x, y } = await loadImageNode.getPosition() | ||
|
|
||
| await comfyPage.dragAndDropFile('image64x64.webp', { | ||
| dropPosition: { x, y } | ||
| }) | ||
|
|
||
| const imagePreview = comfyPage.page.locator('.image-preview') | ||
| await expect(imagePreview).toBeVisible() | ||
| await expect(imagePreview.locator('img')).toBeVisible() | ||
| await expect(imagePreview).toContainText('x') | ||
|
|
||
| return imagePreview | ||
| } | ||
|
|
||
| test('opens mask editor from image preview button', async ({ comfyPage }) => { | ||
| const imagePreview = await loadImageOnNode(comfyPage) | ||
|
|
||
| await imagePreview.locator('[role="img"]').hover() | ||
| await comfyPage.page.getByLabel('Edit or mask image').click() | ||
|
|
||
| await expect(comfyPage.page.locator('.mask-editor-dialog')).toBeVisible() | ||
| }) | ||
|
|
||
| test('shows image context menu options', async ({ comfyPage }) => { | ||
| await loadImageOnNode(comfyPage) | ||
|
|
||
| const nodeHeader = comfyPage.vueNodes.getNodeByTitle('Load Image') | ||
| await nodeHeader.click() | ||
| await nodeHeader.click({ button: 'right' }) | ||
|
|
||
| const contextMenu = comfyPage.page.locator('.p-contextmenu') | ||
| await expect(contextMenu).toBeVisible() | ||
| await expect(contextMenu.getByText('Open Image')).toBeVisible() | ||
| await expect(contextMenu.getByText('Copy Image')).toBeVisible() | ||
| await expect(contextMenu.getByText('Save Image')).toBeVisible() | ||
| await expect(contextMenu.getByText('Open in Mask Editor')).toBeVisible() | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,7 +40,6 @@ | |
| <!-- Main Image --> | ||
| <img | ||
| v-if="!imageError" | ||
| ref="currentImageEl" | ||
| :src="currentImageUrl" | ||
| :alt="imageAltText" | ||
| class="block size-full object-contain pointer-events-none contain-size" | ||
|
|
@@ -128,8 +127,8 @@ import { computed, ref, watch } from 'vue' | |
| import { useI18n } from 'vue-i18n' | ||
|
|
||
| import { downloadFile } from '@/base/common/downloadUtil' | ||
| import { useMaskEditor } from '@/composables/maskeditor/useMaskEditor' | ||
| import { app } from '@/scripts/app' | ||
| import { useCommandStore } from '@/stores/commandStore' | ||
| import { useNodeOutputStore } from '@/stores/imagePreviewStore' | ||
|
|
||
| interface ImagePreviewProps { | ||
|
|
@@ -142,7 +141,6 @@ interface ImagePreviewProps { | |
| const props = defineProps<ImagePreviewProps>() | ||
|
|
||
| const { t } = useI18n() | ||
| const commandStore = useCommandStore() | ||
| const nodeOutputStore = useNodeOutputStore() | ||
|
|
||
| const actionButtonClass = | ||
|
|
@@ -156,7 +154,6 @@ const actualDimensions = ref<string | null>(null) | |
| const imageError = ref(false) | ||
| const showLoader = ref(false) | ||
|
|
||
| const currentImageEl = ref<HTMLImageElement>() | ||
| const imageWrapperEl = ref<HTMLDivElement>() | ||
|
|
||
| const { start: startDelayedLoader, stop: stopDelayedLoader } = useTimeoutFn( | ||
|
|
@@ -209,6 +206,10 @@ const handleImageLoad = (event: Event) => { | |
| if (img.naturalWidth && img.naturalHeight) { | ||
| actualDimensions.value = `${img.naturalWidth} x ${img.naturalHeight}` | ||
| } | ||
|
|
||
| if (props.nodeId) { | ||
| nodeOutputStore.syncLegacyNodeImgs(props.nodeId, img, currentIndex.value) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's just doing what happens in litegraph
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does that not comfort me? |
||
| } | ||
| } | ||
|
|
||
| const handleImageError = () => { | ||
|
|
@@ -218,19 +219,11 @@ const handleImageError = () => { | |
| actualDimensions.value = null | ||
| } | ||
|
|
||
| // In vueNodes mode, we need to set them manually before opening the mask editor. | ||
| const setupNodeForMaskEditor = () => { | ||
| if (!props.nodeId || !currentImageEl.value) return | ||
| const node = app.rootGraph?.getNodeById(props.nodeId) | ||
| if (!node) return | ||
| node.imageIndex = currentIndex.value | ||
| node.imgs = [currentImageEl.value] | ||
| app.canvas?.select(node) | ||
| } | ||
|
|
||
| const handleEditMask = () => { | ||
| setupNodeForMaskEditor() | ||
| void commandStore.execute('Comfy.MaskEditor.OpenMaskEditor') | ||
| if (!props.nodeId) return | ||
| const node = app.rootGraph?.getNodeById(Number(props.nodeId)) | ||
| if (!node) return | ||
| useMaskEditor().openMaskEditor(node) | ||
|
Comment on lines
222
to
+226
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Hoist useMaskEditor() to setup to avoid instance/timing pitfalls. Calling composables inside event handlers can miss the active component instance if the composable uses injection. Safer to instantiate once in setup and reuse. ♻️ Suggested refactor-const nodeOutputStore = useNodeOutputStore()
+const nodeOutputStore = useNodeOutputStore()
+const maskEditor = useMaskEditor()
...
- useMaskEditor().openMaskEditor(node)
+ maskEditor.openMaskEditor(node)🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| const handleDownload = () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { defineStore } from 'pinia' | |
| import { ref } from 'vue' | ||
|
|
||
| import type { LGraphNode, SubgraphNode } from '@/lib/litegraph/src/litegraph' | ||
| import { LiteGraph } from '@/lib/litegraph/src/litegraph' | ||
| import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore' | ||
| import type { | ||
| ExecutedWsMessage, | ||
|
|
@@ -394,6 +395,32 @@ export const useNodeOutputStore = defineStore('nodeOutput', () => { | |
| revokeAllPreviews() | ||
| } | ||
|
|
||
| /** | ||
| * Sync legacy node.imgs property for backwards compatibility. | ||
| * | ||
| * In Vue Nodes mode, legacy systems (Copy Image, Open Image, Save Image, | ||
| * Open in Mask Editor) rely on `node.imgs` containing HTMLImageElement | ||
| * references. Since Vue handles image rendering, we need to sync the | ||
| * already-loaded element from the Vue component to the node. | ||
| * | ||
| * @param nodeId - The node ID | ||
| * @param element - The loaded HTMLImageElement from the Vue component | ||
| * @param activeIndex - The current image index (for multi-image outputs) | ||
| */ | ||
| function syncLegacyNodeImgs( | ||
| nodeId: string | number, | ||
| element: HTMLImageElement, | ||
| activeIndex: number = 0 | ||
| ) { | ||
| if (!LiteGraph.vueNodesMode) return | ||
|
|
||
| const node = app.rootGraph?.getNodeById(Number(nodeId)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We really need to standardize our NodeId type. |
||
| if (!node) return | ||
|
|
||
| node.imgs = [element] | ||
| node.imageIndex = activeIndex | ||
| } | ||
|
|
||
| return { | ||
| // Getters | ||
| getNodeOutputs, | ||
|
|
@@ -407,6 +434,7 @@ export const useNodeOutputStore = defineStore('nodeOutput', () => { | |
| setNodePreviewsByExecutionId, | ||
| setNodePreviewsByNodeId, | ||
| updateNodeImages, | ||
| syncLegacyNodeImgs, | ||
|
|
||
| // Cleanup | ||
| revokePreviewsByExecutionId, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3648
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2765
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 984
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 234
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2356
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1071
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 283
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1163
🏁 Script executed:
Repository: Comfy-Org/ComfyUI_frontend
Length of output: 974
Fixture type derivation is invalid; use the imported
ComfyPagetype directly.Playwright
TestInfo(fromtest.info()) doesn't expose afixturesproperty, making the type path inaccessible. Import and use the concreteComfyPagetype instead.Additionally, use centralized
TestIdsfor element selection instead of class-based selectors (.image-preview,.mask-editor-dialog,.p-contextmenu). IfTestIdsdoesn't have entries for these Vue node components yet, add them tobrowser_tests/fixtures/selectors.tsand update the components to usedata-testidattributes.✅ Proposed fix for fixture type
import { expect } from '@playwright/test' import { comfyPageFixture as test } from '../../../../fixtures/ComfyPage' +import type { ComfyPage } from '../../../../fixtures/ComfyPage' async function loadImageOnNode( - comfyPage: Awaited< - ReturnType<(typeof test)['info']> - >['fixtures']['comfyPage'] + comfyPage: ComfyPage ) {🤖 Prompt for AI Agents