From 47f2b636288e4f1bf6d530b04c3538fd9b06afb4 Mon Sep 17 00:00:00 2001 From: Christian Byrne Date: Thu, 5 Mar 2026 17:14:40 -0800 Subject: [PATCH] fix: prevent persistent loading state when cycling batches with identical URLs (#8999) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Fix persistent loading/skeleton state when cycling through batch images or videos that share the same URL (common on Cloud). ## Changes - **What**: In `setCurrentIndex()` for both `ImagePreview.vue` and `VideoPreview.vue`, only start the loader when the target URL differs from the current URL. When batch items share the same URL, the browser doesn't fire a new `load`/`loadeddata` event since `src` didn't change, so the loader was never dismissed. - Also fixes `VideoPreview.vue` navigation dots using hardcoded `bg-white` instead of semantic `bg-base-foreground` tokens. ## Review Focus This bug has regressed 3+ times (PRs #6521, #7094, #8366). The regression tests specifically target the root cause — cycling through identical URLs — to prevent future reintroduction. Fixes https://www.notion.so/comfy-org/Bug-Cycling-through-image-batches-results-in-persistent-loading-state-on-Cloud-30c6d73d3650816e9738d5dbea52c47d ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8999-fix-prevent-persistent-loading-state-when-cycling-batches-with-identical-URLs-30d6d73d36508180831edbaf8ad8ad48) by [Unito](https://www.unito.io) --------- Co-authored-by: Simula_r <18093452+simula-r@users.noreply.github.com> --- .../extensions/vueNodes/VideoPreview.test.ts | 106 ++++++++++++++++++ .../extensions/vueNodes/VideoPreview.vue | 19 ++-- .../vueNodes/components/ImagePreview.test.ts | 72 ++++++++---- .../vueNodes/components/ImagePreview.vue | 3 +- 4 files changed, 173 insertions(+), 27 deletions(-) create mode 100644 src/renderer/extensions/vueNodes/VideoPreview.test.ts diff --git a/src/renderer/extensions/vueNodes/VideoPreview.test.ts b/src/renderer/extensions/vueNodes/VideoPreview.test.ts new file mode 100644 index 0000000000..eca76f8a77 --- /dev/null +++ b/src/renderer/extensions/vueNodes/VideoPreview.test.ts @@ -0,0 +1,106 @@ +import { createTestingPinia } from '@pinia/testing' +import { mount } from '@vue/test-utils' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { nextTick } from 'vue' +import { createI18n } from 'vue-i18n' +import type { ComponentProps } from 'vue-component-type-helpers' + +import VideoPreview from '@/renderer/extensions/vueNodes/VideoPreview.vue' + +vi.mock('@/base/common/downloadUtil', () => ({ + downloadFile: vi.fn() +})) + +const i18n = createI18n({ + legacy: false, + locale: 'en', + messages: { + en: { + g: { + downloadVideo: 'Download video', + removeVideo: 'Remove video', + viewVideoOfTotal: 'View video {index} of {total}', + videoPreview: + 'Video preview - Use arrow keys to navigate between videos', + errorLoadingVideo: 'Error loading video', + failedToDownloadVideo: 'Failed to download video', + calculatingDimensions: 'Calculating dimensions', + videoFailedToLoad: 'Video failed to load', + loading: 'Loading' + } + } + } +}) + +describe('VideoPreview', () => { + const defaultProps: ComponentProps = { + imageUrls: [ + '/api/view?filename=test1.mp4&type=output', + '/api/view?filename=test2.mp4&type=output' + ] + } + + afterEach(() => { + vi.clearAllMocks() + }) + + function mountVideoPreview( + props: Partial> = {} + ) { + return mount(VideoPreview, { + props: { ...defaultProps, ...props } as ComponentProps< + typeof VideoPreview + >, + global: { + plugins: [createTestingPinia({ createSpy: vi.fn }), i18n], + stubs: { + Skeleton: true + } + } + }) + } + + describe('batch cycling with identical URLs', () => { + it('should not enter persistent loading state when cycling through identical videos', async () => { + const sameUrl = '/api/view?filename=test.mp4&type=output' + const wrapper = mountVideoPreview({ + imageUrls: [sameUrl, sameUrl, sameUrl] + }) + + // Simulate initial video load + await wrapper.find('video').trigger('loadeddata') + await nextTick() + expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false) + + // Click second navigation dot to cycle to identical URL + const dots = wrapper.findAll('[aria-label^="View video"]') + await dots[1].trigger('click') + await nextTick() + + // Should NOT be in loading state since URL didn't change + expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false) + }) + + it('should show loader when cycling to a different URL', async () => { + const wrapper = mountVideoPreview({ + imageUrls: [ + '/api/view?filename=a.mp4&type=output', + '/api/view?filename=b.mp4&type=output' + ] + }) + + // Simulate initial video load + await wrapper.find('video').trigger('loadeddata') + await nextTick() + expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false) + + // Click second dot — different URL + const dots = wrapper.findAll('[aria-label^="View video"]') + await dots[1].trigger('click') + await nextTick() + + // Should be in loading state since URL changed + expect(wrapper.find('[aria-busy="true"]').exists()).toBe(true) + }) + }) +}) diff --git a/src/renderer/extensions/vueNodes/VideoPreview.vue b/src/renderer/extensions/vueNodes/VideoPreview.vue index ace611d744..b85b9e1883 100644 --- a/src/renderer/extensions/vueNodes/VideoPreview.vue +++ b/src/renderer/extensions/vueNodes/VideoPreview.vue @@ -215,11 +215,15 @@ const handleRemove = () => { } const setCurrentIndex = (index: number) => { + if (currentIndex.value === index) return if (index >= 0 && index < props.imageUrls.length) { + const urlChanged = props.imageUrls[index] !== currentVideoUrl.value currentIndex.value = index - actualDimensions.value = null - showLoader.value = true videoError.value = false + if (urlChanged) { + actualDimensions.value = null + showLoader.value = true + } } } @@ -241,12 +245,13 @@ const handleFocusOut = (event: FocusEvent) => { } } -const getNavigationDotClass = (index: number) => { - return [ +const getNavigationDotClass = (index: number) => + cn( 'w-2 h-2 rounded-full transition-all duration-200 border-0 cursor-pointer', - index === currentIndex.value ? 'bg-white' : 'bg-white/50 hover:bg-white/80' - ] -} + index === currentIndex.value + ? 'bg-base-foreground' + : 'bg-base-foreground/50 hover:bg-base-foreground/80' + ) const handleKeyDown = (event: KeyboardEvent) => { if (props.imageUrls.length <= 1) return diff --git a/src/renderer/extensions/vueNodes/components/ImagePreview.test.ts b/src/renderer/extensions/vueNodes/components/ImagePreview.test.ts index bb0f3fd0c0..b781dc6fda 100644 --- a/src/renderer/extensions/vueNodes/components/ImagePreview.test.ts +++ b/src/renderer/extensions/vueNodes/components/ImagePreview.test.ts @@ -311,6 +311,37 @@ describe('ImagePreview', () => { expect(imgElement.attributes('alt')).toBe('Node output 2') }) + describe('batch cycling with identical URLs', () => { + it('should not enter persistent loading state when cycling through identical images', async () => { + vi.useFakeTimers() + try { + const sameUrl = '/api/view?filename=test.png&type=output' + const wrapper = mountImagePreview({ + imageUrls: [sameUrl, sameUrl, sameUrl] + }) + + // Simulate initial image load + await wrapper.find('img').trigger('load') + await nextTick() + expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false) + + // Click second navigation dot to cycle + const dots = wrapper.findAll('.w-2.h-2.rounded-full') + await dots[1].trigger('click') + await nextTick() + + // Advance past the delayed loader timeout + await vi.advanceTimersByTimeAsync(300) + await nextTick() + + // Should NOT be in loading state since URL didn't change + expect(wrapper.find('[aria-busy="true"]').exists()).toBe(false) + } finally { + vi.useRealTimers() + } + }) + }) + describe('URL change detection', () => { it('should NOT reset loading state when imageUrls prop is reassigned with identical URLs', async () => { vi.useFakeTimers() @@ -343,30 +374,33 @@ describe('ImagePreview', () => { }) 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 }) + 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() + // 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) + // 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() + // 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() + // Advance past the 250ms delayed loader timeout + await vi.advanceTimersByTimeAsync(300) + await nextTick() - expect(wrapper.find('[aria-busy="true"]').exists()).toBe(true) + expect(wrapper.find('[aria-busy="true"]').exists()).toBe(true) + } finally { + vi.useRealTimers() + } }) it('should handle empty to non-empty URL transitions correctly', async () => { diff --git a/src/renderer/extensions/vueNodes/components/ImagePreview.vue b/src/renderer/extensions/vueNodes/components/ImagePreview.vue index 8f840b585f..9898c8b6af 100644 --- a/src/renderer/extensions/vueNodes/components/ImagePreview.vue +++ b/src/renderer/extensions/vueNodes/components/ImagePreview.vue @@ -255,9 +255,10 @@ const handleRemove = () => { const setCurrentIndex = (index: number) => { if (currentIndex.value === index) return if (index >= 0 && index < props.imageUrls.length) { + const urlChanged = props.imageUrls[index] !== currentImageUrl.value currentIndex.value = index - startDelayedLoader() imageError.value = false + if (urlChanged) startDelayedLoader() } }