fix: prevent image/video preview reset on dynamic widget addition #8366
+103
−9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThis PR adds URL change detection guards to image and video preview component watchers to prevent unnecessary state resets when array references change without content changes. It removes deep watching and adds test coverage for the new behavior, while also refactoring media type detection in LGraphNode. Changes
Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/30/2026, 07:04:52 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests: ✅ PassedResults: 507 passed, 0 failed, 0 flaky, 8 skipped (Total: 515) 📊 Browser Reports
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 26 kB (baseline 26 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 974 kB (baseline 974 kB) • 🔴 +262 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 471 kB (baseline 471 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.89 kB (baseline 2.89 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 4 added / 4 removed Data & Services — 2.71 MB (baseline 2.71 MB) • 🔴 +1 BStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 25.3 kB (baseline 25.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.1 MB (baseline 7.1 MB) • 🟢 -198 BBundles that do not match a named category
Status: 34 added / 34 removed |
When widgets are added dynamically to a node, the watch on imageUrls/ imageUrls would trigger even when URL content is identical (just a new array reference). This caused the preview to unnecessarily reset to loading state. Fix: Compare URL arrays by content, not reference. Only reset loading state when URLs actually change. - Add urlsChanged check comparing array length and element equality - Remove deep: true since we compare manually - Add unit tests for URL change detection behavior - Add E2E test for dynamic widget scenario Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c03a2-2acb-74cc-94ef-993894df9b22
christian-byrne
force-pushed
the
dynamic-widget-rerender-fix
branch
from
391d159 to
115f788
Compare
dosubot
bot
added
the
size:L
christian-byrne
commented
Jan 28, 2026
| imageError.value = false | ||
| if (newUrls.length > 0) startDelayedLoader() | ||
| }, | ||
| { deep: true, immediate: true } |
Contributor
Author
There was a problem hiding this comment.
Note for reviewers: deep is redundant on string arrays -- adding, removing, reassigning all trigger reactivity. Proof: Vue Playground Link
christian-byrne
added
needs-backport
AustinMroz
reviewed
Jan 29, 2026
Collaborator
AustinMroz
left a comment
AustinMroz
left a comment
There was a problem hiding this comment.
Change to watches look correct, but when manually testing, it seems that Videos are still reloading anytime the dynamicCombo is changed because the rand parameter in the url is changed.
Will keep investigating.
Collaborator
|
In If we move |
Contributor
Author
This was just to fix the previews breaking entirely when dynamic widget changes. The |
Collaborator
I'm pretty sure that's the part I'm having issues with. I'm running this branch with a single added
I've got a slight leaning towards "not handing video differently than images". The ideal case is removing the cache busting entirely and trusting |
Amp-Thread-ID: https://ampcode.com/threads/T-019c0685-bab7-7566-8e7a-432ee7face71 Co-authored-by: Amp <amp@ampcode.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/renderer/extensions/vueNodes/components/ImagePreview.test.ts`:
- Around line 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.
Comment on lines
+343
to
+368
| 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) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/extensions/vueNodes/components/ImagePreview.test.ts | head -400 | tail -100Repository: 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 2Repository: 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 -nRepository: 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.
Contributor
Author
Done in 9013914 |
Contributor
Author
|
Tested and works better now (doesn't re-render when widgets are added). |
AustinMroz
approved these changes
Jan 30, 2026
github-actions bot
pushed a commit
that referenced
this pull request
Jan 30, 2026
) ## 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>
github-actions bot
pushed a commit
that referenced
this pull request
Jan 30, 2026
) ## 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>
Member
|
@christian-byrne Successfully backported to #8492 |
Member
|
@christian-byrne Successfully backported to #8493 |
github-actions
bot
removed
the
needs-backport
AustinMroz
pushed a commit
that referenced
this pull request
Jan 30, 2026
…c widget addition (#8492) Backport of #8366 to `core/1.38` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8492-backport-core-1-38-fix-prevent-image-video-preview-reset-on-dynamic-widget-addition-2f86d73d36508123acfdfac61554da7e) by [Unito](https://www.unito.io) Co-authored-by: Christian Byrne <cbyrne@comfy.org> Co-authored-by: Subagent 5 <subagent@example.com> Co-authored-by: Amp <amp@ampcode.com>
AustinMroz
pushed a commit
that referenced
this pull request
Jan 30, 2026
…ic widget addition (#8493) Backport of #8366 to `cloud/1.38` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8493-backport-cloud-1-38-fix-prevent-image-video-preview-reset-on-dynamic-widget-addition-2f86d73d36508192b27fc55a29d6d02e) by [Unito](https://www.unito.io) Co-authored-by: Christian Byrne <cbyrne@comfy.org> Co-authored-by: Subagent 5 <subagent@example.com> Co-authored-by: Amp <amp@ampcode.com>
christian-byrne
added a commit
that referenced
this pull request
Jan 31, 2026
## Summary Fix for COM-14110: Preview image does not display new outputs in vue-nodes. ## Problem The merge logic in `setOutputsByLocatorId` updated `app.nodeOutputs` but returned early without updating the reactive `nodeOutputs.value` ref. This caused Vue components to never receive merged output updates because only the non-reactive `app.nodeOutputs` was being updated. ## Solution Added `nodeOutputs.value[nodeLocatorId] = existingOutput` after the merge loop, before the return statement. ## Testing - Added 2 unit tests covering the merge behavior - All 4076 existing unit tests pass - Typechecks pass - Lint passes ## Notes - Related open PRs touching same files: #8143, #8366 - potential minor conflicts possible Fixes COM-14110 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8479-fix-update-reactive-ref-after-merge-in-imagePreviewStore-2f86d73d365081f1a145fa5a9782515f) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
github-actions bot
pushed a commit
that referenced
this pull request
Jan 31, 2026
## Summary Fix for COM-14110: Preview image does not display new outputs in vue-nodes. ## Problem The merge logic in `setOutputsByLocatorId` updated `app.nodeOutputs` but returned early without updating the reactive `nodeOutputs.value` ref. This caused Vue components to never receive merged output updates because only the non-reactive `app.nodeOutputs` was being updated. ## Solution Added `nodeOutputs.value[nodeLocatorId] = existingOutput` after the merge loop, before the return statement. ## Testing - Added 2 unit tests covering the merge behavior - All 4076 existing unit tests pass - Typechecks pass - Lint passes ## Notes - Related open PRs touching same files: #8143, #8366 - potential minor conflicts possible Fixes COM-14110 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8479-fix-update-reactive-ref-after-merge-in-imagePreviewStore-2f86d73d365081f1a145fa5a9782515f) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
github-actions bot
pushed a commit
that referenced
this pull request
Jan 31, 2026
## Summary Fix for COM-14110: Preview image does not display new outputs in vue-nodes. ## Problem The merge logic in `setOutputsByLocatorId` updated `app.nodeOutputs` but returned early without updating the reactive `nodeOutputs.value` ref. This caused Vue components to never receive merged output updates because only the non-reactive `app.nodeOutputs` was being updated. ## Solution Added `nodeOutputs.value[nodeLocatorId] = existingOutput` after the merge loop, before the return statement. ## Testing - Added 2 unit tests covering the merge behavior - All 4076 existing unit tests pass - Typechecks pass - Lint passes ## Notes - Related open PRs touching same files: #8143, #8366 - potential minor conflicts possible Fixes COM-14110 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8479-fix-update-reactive-ref-after-merge-in-imagePreviewStore-2f86d73d365081f1a145fa5a9782515f) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
DrJKL
pushed a commit
that referenced
this pull request
Jan 31, 2026
) ## 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>
DrJKL
pushed a commit
that referenced
this pull request
Jan 31, 2026
## Summary Fix for COM-14110: Preview image does not display new outputs in vue-nodes. ## Problem The merge logic in `setOutputsByLocatorId` updated `app.nodeOutputs` but returned early without updating the reactive `nodeOutputs.value` ref. This caused Vue components to never receive merged output updates because only the non-reactive `app.nodeOutputs` was being updated. ## Solution Added `nodeOutputs.value[nodeLocatorId] = existingOutput` after the merge loop, before the return statement. ## Testing - Added 2 unit tests covering the merge behavior - All 4076 existing unit tests pass - Typechecks pass - Lint passes ## Notes - Related open PRs touching same files: #8143, #8366 - potential minor conflicts possible Fixes COM-14110 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8479-fix-update-reactive-ref-after-merge-in-imagePreviewStore-2f86d73d365081f1a145fa5a9782515f) by [Unito](https://www.unito.io) Co-authored-by: Amp <amp@ampcode.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
imageUrlsprop even when the URL content is identical—the array has a new reference but the same values. This caused:startDelayedLoader()to reset loading state to pending@loadbefore the 250ms timeout, the loader shows and stays stuckSolution
Compare URL arrays by content, not reference. Only reset loading state when URLs actually change:
deep: truesince we compare manuallyScreenshots
┆Issue is synchronized with this Notion page by Unito