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
12 changes: 10 additions & 2 deletions src/renderer/extensions/vueNodes/VideoPreview.vue
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,15 @@ const hasMultipleVideos = computed(() => props.imageUrls.length > 1)
// Watch for URL changes and reset state
watch(
() => props.imageUrls,
(newUrls) => {
(newUrls, oldUrls) => {
// Only reset state if URLs actually changed (not just array reference)
const urlsChanged =
!oldUrls ||
newUrls.length !== oldUrls.length ||
newUrls.some((url, i) => url !== oldUrls[i])

if (!urlsChanged) return

// Reset current index if it's out of bounds
if (currentIndex.value >= newUrls.length) {
currentIndex.value = 0
Expand All @@ -169,7 +177,7 @@ watch(
videoError.value = false
showLoader.value = newUrls.length > 0
},
{ deep: true, immediate: true }
{ immediate: true }
)

// Event handlers
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,4 +308,80 @@ describe('ImagePreview', () => {
expect(imgElement.exists()).toBe(true)
expect(imgElement.attributes('alt')).toBe('Node output 2')
})

describe('URL change detection', () => {
it('should NOT reset loading state when imageUrls prop is reassigned with identical URLs', async () => {
vi.useFakeTimers()
try {
const urls = ['/api/view?filename=test.png&type=output']
const wrapper = mountImagePreview({ imageUrls: urls })

// Simulate image load completing
const img = wrapper.find('img')
await img.trigger('load')
await nextTick()

// Verify loader is hidden after load
expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false)

// Reassign with new array reference but same content
await wrapper.setProps({ imageUrls: [...urls] })
await nextTick()

// Advance past the 250ms delayed loader timeout
await vi.advanceTimersByTimeAsync(300)
await nextTick()

// Loading state should NOT have been reset - aria-busy should still be false
// because the URLs are identical (just a new array reference)
expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false)
} finally {
vi.useRealTimers()
}
})

it('should reset loading state when imageUrls prop changes to different URLs', async () => {
const urls = ['/api/view?filename=test.png&type=output']
const wrapper = mountImagePreview({ imageUrls: urls })

// Simulate image load completing
const img = wrapper.find('img')
await img.trigger('load')
await nextTick()

// Verify loader is hidden
expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false)

// Change to different URL
await wrapper.setProps({
imageUrls: ['/api/view?filename=different.png&type=output']
})
await nextTick()

// After 250ms timeout, loading state should be reset (aria-busy="true")
// We can check the internal state via the Skeleton appearing
// or wait for the timeout
await new Promise((resolve) => setTimeout(resolve, 300))
await nextTick()

expect(wrapper.find('[aria-busy="true"]').exists()).toBe(true)
})
Comment on lines +343 to +368
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n src/renderer/extensions/vueNodes/components/ImagePreview.test.ts | head -400 | tail -100

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 4177


🏁 Script executed:

# Search for other uses of vi.useFakeTimers in the test file
rg -n "useFakeTimers|advanceTimersByTime|setTimeout" src/renderer/extensions/vueNodes/components/ImagePreview.test.ts -A 2 -B 2

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 708


🏁 Script executed:

# Check the full context of the test section around lines 343-368
sed -n '310,380p' src/renderer/extensions/vueNodes/components/ImagePreview.test.ts | cat -n

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 3069


Use fake timers to match the adjacent test and avoid real 300ms waits.

The previous test in this suite (lines 313–341) already demonstrates the proper pattern using vi.useFakeTimers() and vi.advanceTimersByTimeAsync(). Apply the same approach here for consistency and to keep the test suite fast and deterministic.

♻️ Suggested update
   it('should reset loading state when imageUrls prop changes to different URLs', async () => {
+    vi.useFakeTimers()
+    try {
       const urls = ['/api/view?filename=test.png&type=output']
       const wrapper = mountImagePreview({ imageUrls: urls })

       // Simulate image load completing
       const img = wrapper.find('img')
       await img.trigger('load')
       await nextTick()

       // Verify loader is hidden
       expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false)

       // Change to different URL
       await wrapper.setProps({
         imageUrls: ['/api/view?filename=different.png&type=output']
       })
       await nextTick()

       // After 250ms timeout, loading state should be reset (aria-busy="true")
-      await new Promise((resolve) => setTimeout(resolve, 300))
+      await vi.advanceTimersByTimeAsync(300)
       await nextTick()

       expect(wrapper.find('[aria-busy="true"]').exists()).toBe(true)
+    } finally {
+      vi.useRealTimers()
+    }
   })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('should reset loading state when imageUrls prop changes to different URLs', async () => {
const urls = ['/api/view?filename=test.png&type=output']
const wrapper = mountImagePreview({ imageUrls: urls })
// Simulate image load completing
const img = wrapper.find('img')
await img.trigger('load')
await nextTick()
// Verify loader is hidden
expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false)
// Change to different URL
await wrapper.setProps({
imageUrls: ['/api/view?filename=different.png&type=output']
})
await nextTick()
// After 250ms timeout, loading state should be reset (aria-busy="true")
// We can check the internal state via the Skeleton appearing
// or wait for the timeout
await new Promise((resolve) => setTimeout(resolve, 300))
await nextTick()
expect(wrapper.find('[aria-busy="true"]').exists()).toBe(true)
})
it('should reset loading state when imageUrls prop changes to different URLs', async () => {
vi.useFakeTimers()
try {
const urls = ['/api/view?filename=test.png&type=output']
const wrapper = mountImagePreview({ imageUrls: urls })
// Simulate image load completing
const img = wrapper.find('img')
await img.trigger('load')
await nextTick()
// Verify loader is hidden
expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false)
// Change to different URL
await wrapper.setProps({
imageUrls: ['/api/view?filename=different.png&type=output']
})
await nextTick()
// After 250ms timeout, loading state should be reset (aria-busy="true")
// We can check the internal state via the Skeleton appearing
// or wait for the timeout
await vi.advanceTimersByTimeAsync(300)
await nextTick()
expect(wrapper.find('[aria-busy="true"]').exists()).toBe(true)
} finally {
vi.useRealTimers()
}
})
🤖 Prompt for AI Agents
In `@src/renderer/extensions/vueNodes/components/ImagePreview.test.ts` around
lines 343 - 368, Replace the real 300ms sleep with fake timers: call
vi.useFakeTimers() before triggering the prop change, then after await
wrapper.setProps({...}) and await nextTick(), advance the timers with await
vi.advanceTimersByTimeAsync(250) (or 300 to match existing expectation), await
nextTick(), and finally call vi.useRealTimers() to restore. Update the test
around wrapper.setProps/await nextTick() (the test function in
ImagePreview.test.ts that uses wrapper.setProps and img.trigger('load')) to
remove the new Promise(setTimeout...) and use
vi.useFakeTimers()/vi.advanceTimersByTimeAsync() instead so it matches the
adjacent test pattern.


it('should handle empty to non-empty URL transitions correctly', async () => {
const wrapper = mountImagePreview({ imageUrls: [] })

// No preview initially
expect(wrapper.find('.image-preview').exists()).toBe(false)

// Add URLs
await wrapper.setProps({
imageUrls: ['/api/view?filename=test.png&type=output']
})
await nextTick()

// Preview should appear
expect(wrapper.find('.image-preview').exists()).toBe(true)
expect(wrapper.find('img').exists()).toBe(true)
})
})
})
12 changes: 10 additions & 2 deletions src/renderer/extensions/vueNodes/components/ImagePreview.vue
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,15 @@ const imageAltText = computed(() => `Node output ${currentIndex.value + 1}`)
// Watch for URL changes and reset state
watch(
() => props.imageUrls,
(newUrls) => {
(newUrls, oldUrls) => {
// Only reset state if URLs actually changed (not just array reference)
const urlsChanged =
!oldUrls ||
newUrls.length !== oldUrls.length ||
newUrls.some((url, i) => url !== oldUrls[i])

if (!urlsChanged) return

// Reset current index if it's out of bounds
if (currentIndex.value >= newUrls.length) {
currentIndex.value = 0
Expand All @@ -188,7 +196,7 @@ watch(
imageError.value = false
if (newUrls.length > 0) startDelayedLoader()
},
{ deep: true, immediate: true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: deep is redundant on string arrays -- adding, removing, reassigning all trigger reactivity. Proof: Vue Playground Link

{ immediate: true }
)

// Event handlers
Expand Down
12 changes: 7 additions & 5 deletions src/renderer/extensions/vueNodes/components/LGraphNode.vue
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,12 @@ const showAdvancedState = customRef((track, trigger) => {
}
})

const hasVideoInput = computed(() => {
return (
lgraphNode.value?.inputs?.some((input) => input.type === 'VIDEO') ?? false
)
})

const nodeMedia = computed(() => {
const newOutputs = nodeOutputs.nodeOutputs[nodeOutputLocatorId.value]
const node = lgraphNode.value
Expand All @@ -558,13 +564,9 @@ const nodeMedia = computed(() => {
const urls = nodeOutputs.getNodeImageUrls(node)
if (!urls?.length) return undefined

// Determine media type from previewMediaType or fallback to input slot types
// Note: Despite the field name "images", videos are also included in outputs
// TODO: fix the backend to return videos using the videos key instead of the images key
const hasVideoInput = node.inputs?.some((input) => input.type === 'VIDEO')
const type =
node.previewMediaType === 'video' ||
(!node.previewMediaType && hasVideoInput)
(!node.previewMediaType && hasVideoInput.value)
? 'video'
: 'image'

Expand Down