fix: prevent persistent loading state when cycling batches with identical URLs#8999
Open
christian-byrne wants to merge 2 commits intomainfrom
Open
fix: prevent persistent loading state when cycling batches with identical URLs#8999christian-byrne wants to merge 2 commits intomainfrom
christian-byrne wants to merge 2 commits intomainfrom
Conversation
…ical 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
christian-byrne
added
the
verified bug
dosubot
bot
added
the
size:L
|
Playwright: ✅ 518 passed, 0 failed · 4 flaky 📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/20/2026, 05:59:26 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughUpdates add tests and adjust ImagePreview and VideoPreview to avoid resetting loader/dimensions when cycling to the same URL; loader is triggered only on actual URL changes. VideoPreview navigation dot classes are updated to use base-foreground tokens. New tests cover identical vs different URL cycling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
📦 Bundle: 4.26 MB gzip 🔴 +39 BDetailsSummary
Category Glance App Entry Points — 21.4 kB (baseline 21.4 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 914 kB (baseline 914 kB) • 🔴 +252 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 68.6 kB (baseline 68.6 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Panels & Settings — 430 kB (baseline 430 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Editors & Dialogs — 706 B (baseline 706 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
UI Components — 42.3 kB (baseline 42.3 kB) • ⚪ 0 BReusable component library chunks
Data & Services — 2.4 MB (baseline 2.4 MB) • ⚪ 0 BStores, services, APIs, and repositories
Utilities & Hooks — 57.6 kB (baseline 57.6 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 8.69 MB (baseline 8.69 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.38 MB (baseline 7.38 MB) • ⚪ 0 BBundles that do not match a named category
|
![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
🧹 Nitpick comments (1)
src/renderer/extensions/vueNodes/VideoPreview.vue (1)
250-256: Usecn()to compose navigation-dot classes (avoid array return).
The repo standard requirescn()for class merging and forbids array-based class bindings.♻️ Suggested refactor
-const getNavigationDotClass = (index: number) => { - return [ - '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 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' + )As per coding guidelines: Use
cn()utility from '@/utils/tailwindUtil' to merge class names; never use:class="[]"syntax.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/extensions/vueNodes/VideoPreview.vue` around lines 250 - 256, getNavigationDotClass currently returns an array of class strings which violates the repo standard; instead import and use the cn() utility from '@/utils/tailwindUtil' and return a single string composed by cn(). Specifically, update getNavigationDotClass(index: number) to call 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') so the class merging uses cn and no array is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/extensions/vueNodes/VideoPreview.test.ts`:
- Around line 35-105: The module-level mutable wrapperRegistry should be removed
and mountVideoPreview should be converted to a properly typed function
declaration using Partial<ComponentProps<typeof VideoPreview>> for its props;
stop using a global Set to track wrappers and instead unmount the wrapper
returned by mountVideoPreview inside each test's finally block (and remove the
centralized afterEach cleanup). Update all tests to call mountVideoPreview(...)
and ensure they call wrapper.unmount() in finally to guarantee per-test teardown
while keeping references to VideoPreview and mountVideoPreview to locate the
changes.
---
Nitpick comments:
In `@src/renderer/extensions/vueNodes/VideoPreview.vue`:
- Around line 250-256: getNavigationDotClass currently returns an array of class
strings which violates the repo standard; instead import and use the cn()
utility from '@/utils/tailwindUtil' and return a single string composed by cn().
Specifically, update getNavigationDotClass(index: number) to call 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') so the class merging uses cn and no array is
returned.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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
Fix persistent loading/skeleton state when cycling through batch images or videos that share the same URL (common on Cloud).
Changes
setCurrentIndex()for bothImagePreview.vueandVideoPreview.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 newload/loadeddataevent sincesrcdidn't change, so the loader was never dismissed.VideoPreview.vuenavigation dots using hardcodedbg-whiteinstead of semanticbg-base-foregroundtokens.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 by Unito