mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-28 18:22:40 +00:00
fix: prevent persistent loading state when cycling batches with identical URLs (#8999)
## 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>
This commit is contained in:
@@ -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 () => {
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user