mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-10 18:10:08 +00:00
fix: prevent image/video preview reset on dynamic widget addition (#8366)
## Summary Fixes image/video previews getting stuck in loading state when widgets are added dynamically to a node. ## Problem When dynamic widgets are added to a node (e.g., by extensions), Vue reactivity triggers the watch on `imageUrls` prop even when the URL content is identical—the array has a new reference but the same values. This caused: 1. `startDelayedLoader()` to reset loading state to pending 2. If the cached image doesn't trigger `@load` before the 250ms timeout, the loader shows and stays stuck ## Solution Compare URL arrays by content, not reference. Only reset loading state when URLs actually change: - Check array length and element-by-element equality - Return early if URLs are identical (just a new array reference) - Remove `deep: true` since we compare manually ## Screenshots <img width="749" height="647" alt="image" src="https://github.com/user-attachments/assets/3a1ff656-59ed-467a-a121-b70b91423a50" /> <img width="749" height="647" alt="Screenshot from 2026-01-28 15-24-18" src="https://github.com/user-attachments/assets/28265dad-1d79-47c8-9fd4-5a82b94e72cd" /> <img width="749" height="647" alt="Screenshot from 2026-01-28 15-24-05" src="https://github.com/user-attachments/assets/c7af93b7-c898-405f-860b-0f82abe5af6d" /> ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8366-fix-prevent-image-video-preview-reset-on-dynamic-widget-addition-2f66d73d3650819483b2d5cbfb78187f) by [Unito](https://www.unito.io) --------- Co-authored-by: Subagent 5 <subagent@example.com> Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
@@ -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
|
||||
@@ -169,7 +177,7 @@ watch(
|
||||
videoError.value = false
|
||||
showLoader.value = newUrls.length > 0
|
||||
},
|
||||
{ deep: true, immediate: true }
|
||||
{ immediate: true }
|
||||
)
|
||||
|
||||
// Event handlers
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
|
||||
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)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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
|
||||
@@ -188,7 +196,7 @@ watch(
|
||||
imageError.value = false
|
||||
if (newUrls.length > 0) startDelayedLoader()
|
||||
},
|
||||
{ deep: true, immediate: true }
|
||||
{ immediate: true }
|
||||
)
|
||||
|
||||
// Event handlers
|
||||
|
||||
@@ -551,6 +551,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
|
||||
@@ -560,13 +566,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'
|
||||
|
||||
|
||||
Reference in New Issue
Block a user