From 4593ceaf6800afb6d7ac54e030567768011c7ac0 Mon Sep 17 00:00:00 2001 From: bymyself Date: Thu, 19 Feb 2026 21:02:28 -0800 Subject: [PATCH 1/2] fix: prevent persistent loading state when cycling batches with identical URLs In setCurrentIndex(), only start the loader when the target URL differs from the current URL. When batch images/videos share the same URL (common on Cloud), the browser doesn't fire a new load event since src didn't change, leaving the skeleton loader visible permanently. Also fix VideoPreview navigation dots using bg-white instead of semantic bg-base-foreground tokens. Amp-Thread-ID: https://ampcode.com/threads/T-019c796c-8895-74a2-b0d8-c9052a8772f3 --- .../extensions/vueNodes/VideoPreview.test.ts | 107 ++++++++++++++++++ .../extensions/vueNodes/VideoPreview.vue | 12 +- .../vueNodes/components/ImagePreview.test.ts | 72 ++++++++---- .../vueNodes/components/ImagePreview.vue | 3 +- 4 files changed, 171 insertions(+), 23 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 00000000000..9fef2c23de6 --- /dev/null +++ b/src/renderer/extensions/vueNodes/VideoPreview.test.ts @@ -0,0 +1,107 @@ +import { createTestingPinia } from '@pinia/testing' +import type { VueWrapper } from '@vue/test-utils' +import { mount } from '@vue/test-utils' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { nextTick } from 'vue' +import { createI18n } from 'vue-i18n' + +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 = { + imageUrls: [ + '/api/view?filename=test1.mp4&type=output', + '/api/view?filename=test2.mp4&type=output' + ] + } + + const wrapperRegistry = new Set() + + const mountVideoPreview = (props = {}) => { + const wrapper = mount(VideoPreview, { + props: { ...defaultProps, ...props }, + global: { + plugins: [createTestingPinia({ createSpy: vi.fn }), i18n], + stubs: { + Skeleton: true + } + } + }) + wrapperRegistry.add(wrapper) + return wrapper + } + + afterEach(() => { + wrapperRegistry.forEach((w) => w.unmount()) + wrapperRegistry.clear() + }) + + 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 6b308a1b374..aab26662536 100644 --- a/src/renderer/extensions/vueNodes/VideoPreview.vue +++ b/src/renderer/extensions/vueNodes/VideoPreview.vue @@ -217,11 +217,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 + } } } @@ -246,7 +250,9 @@ const handleFocusOut = (event: FocusEvent) => { const getNavigationDotClass = (index: number) => { return [ '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' ] } diff --git a/src/renderer/extensions/vueNodes/components/ImagePreview.test.ts b/src/renderer/extensions/vueNodes/components/ImagePreview.test.ts index bb0f3fd0c0e..b781dc6fda8 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 5d1e5ad8eb7..80b301205e6 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() } } From 90dc92bc02284c258082ca8c8390a915891be98a Mon Sep 17 00:00:00 2001 From: bymyself Date: Thu, 19 Feb 2026 21:57:37 -0800 Subject: [PATCH 2/2] fix: address code review - use cn() for class merging, improve test typing Amp-Thread-ID: https://ampcode.com/threads/T-019c796c-8895-74a2-b0d8-c9052a8772f3 --- .../extensions/vueNodes/VideoPreview.test.ts | 25 +++++++++---------- .../extensions/vueNodes/VideoPreview.vue | 7 +++--- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/src/renderer/extensions/vueNodes/VideoPreview.test.ts b/src/renderer/extensions/vueNodes/VideoPreview.test.ts index 9fef2c23de6..eca76f8a775 100644 --- a/src/renderer/extensions/vueNodes/VideoPreview.test.ts +++ b/src/renderer/extensions/vueNodes/VideoPreview.test.ts @@ -1,9 +1,9 @@ import { createTestingPinia } from '@pinia/testing' -import type { VueWrapper } from '@vue/test-utils' 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' @@ -33,18 +33,24 @@ const i18n = createI18n({ }) describe('VideoPreview', () => { - const defaultProps = { + const defaultProps: ComponentProps = { imageUrls: [ '/api/view?filename=test1.mp4&type=output', '/api/view?filename=test2.mp4&type=output' ] } - const wrapperRegistry = new Set() + afterEach(() => { + vi.clearAllMocks() + }) - const mountVideoPreview = (props = {}) => { - const wrapper = mount(VideoPreview, { - props: { ...defaultProps, ...props }, + function mountVideoPreview( + props: Partial> = {} + ) { + return mount(VideoPreview, { + props: { ...defaultProps, ...props } as ComponentProps< + typeof VideoPreview + >, global: { plugins: [createTestingPinia({ createSpy: vi.fn }), i18n], stubs: { @@ -52,15 +58,8 @@ describe('VideoPreview', () => { } } }) - wrapperRegistry.add(wrapper) - return wrapper } - afterEach(() => { - wrapperRegistry.forEach((w) => w.unmount()) - wrapperRegistry.clear() - }) - 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' diff --git a/src/renderer/extensions/vueNodes/VideoPreview.vue b/src/renderer/extensions/vueNodes/VideoPreview.vue index aab26662536..a8532865b28 100644 --- a/src/renderer/extensions/vueNodes/VideoPreview.vue +++ b/src/renderer/extensions/vueNodes/VideoPreview.vue @@ -247,14 +247,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-base-foreground' : 'bg-base-foreground/50 hover:bg-base-foreground/80' - ] -} + ) const handleKeyDown = (event: KeyboardEvent) => { if (props.imageUrls.length <= 1) return