- 
                Notifications
    You must be signed in to change notification settings 
- Fork 393
load assets browser before fetch completes and show loading state #6189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/23/2025, 08:21:39 PM UTC 🔗 Links🎉 Your Storybook is ready for review! | 
| 🎭 Playwright Test Results⏰ Completed at: 10/23/2025, 08:35:13 PM UTC 📈 Summary
 📊 Test Reports by Browser
 🎉 Click on the links above to view detailed test results for each browser configuration. | 
| Bundle Size ReportSummary 
 Category Glance Per-category breakdownApp Entry Points — 3.29 MB (baseline 3.29 MB) • 🔴 +1.24 kB_Main entry bundles and manifests_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------- | ------- | ------- | ----------------------- | ---------------------- | ----------------------- | | **assets/index-6FWH0N78.js** _(new)_ | — | 2.67 MB | 🔴 +2.67 MB | 🔴 +556 kB | 🔴 +421 kB | | ~~assets/index-D7En9vgj.js~~ _(removed)_ | 2.67 MB | — | 🟢 -2.67 MB | 🟢 -555 kB | 🟢 -420 kB | | ~~assets/index-BhXY03IZ.js~~ _(removed)_ | 614 kB | — | 🟢 -614 kB | 🟢 -114 kB | 🟢 -90.2 kB | | **assets/index-D1wFFd6h.js** _(new)_ | — | 614 kB | 🔴 +614 kB | 🔴 +114 kB | 🔴 +90.1 kB |Status: 2 added / 2 removed Graph Workspace — 707 kB (baseline 707 kB) • ⚪ 0 B_Graph editor runtime, canvas, workflow orchestration_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | -------------------------------------------- | ------ | ------ | ---------------------- | ---------------------- | ---------------------- | | ~~assets/GraphView-B9_fHy27.js~~ _(removed)_ | 707 kB | — | 🟢 -707 kB | 🟢 -138 kB | 🟢 -107 kB | | **assets/GraphView-BWHbS6c9.js** _(new)_ | — | 707 kB | 🔴 +707 kB | 🔴 +138 kB | 🔴 +107 kB |Status: 1 added / 1 removed Views & Navigation — 8.15 kB (baseline 8.15 kB) • ⚪ 0 B_Top-level views, pages, and routed surfaces_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | ~~assets/UserSelectView-17-kvr3D.js~~ _(removed)_ | 8.15 kB | — | 🟢 -8.15 kB | 🟢 -2.47 kB | 🟢 -2.16 kB | | **assets/UserSelectView-BGTyY_9T.js** _(new)_ | — | 8.15 kB | 🔴 +8.15 kB | 🔴 +2.47 kB | 🔴 +2.15 kB |Status: 1 added / 1 removed Panels & Settings — 294 kB (baseline 294 kB) • ⚪ 0 B_Configuration panels, inspectors, and settings screens_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | ~~assets/CreditsPanel-B_wxsqPZ.js~~ _(removed)_ | 22.1 kB | — | 🟢 -22.1 kB | 🟢 -5.28 kB | 🟢 -4.61 kB | | **assets/CreditsPanel-Ci0xzKI-.js** _(new)_ | — | 22.1 kB | 🔴 +22.1 kB | 🔴 +5.28 kB | 🔴 +4.61 kB | | ~~assets/KeybindingPanel-2kR2LI2I.js~~ _(removed)_ | 15.2 kB | — | 🟢 -15.2 kB | 🟢 -3.76 kB | 🟢 -3.31 kB | | **assets/KeybindingPanel-CH9a59a3.js** _(new)_ | — | 15.2 kB | 🔴 +15.2 kB | 🔴 +3.76 kB | 🔴 +3.31 kB | | **assets/ExtensionPanel-BpwU7muV.js** _(new)_ | — | 12.1 kB | 🔴 +12.1 kB | 🔴 +2.83 kB | 🔴 +2.48 kB | | ~~assets/ExtensionPanel-C0DF4Ude.js~~ _(removed)_ | 12.1 kB | — | 🟢 -12.1 kB | 🟢 -2.83 kB | 🟢 -2.48 kB | | **assets/AboutPanel-DINAfsz8.js** _(new)_ | — | 10.3 kB | 🔴 +10.3 kB | 🔴 +2.67 kB | 🔴 +2.35 kB | | ~~assets/AboutPanel-qv8pJnBI.js~~ _(removed)_ | 10.3 kB | — | 🟢 -10.3 kB | 🟢 -2.67 kB | 🟢 -2.35 kB | | **assets/ServerConfigPanel-Dp3HHM-T.js** _(new)_ | — | 8.2 kB | 🔴 +8.2 kB | 🔴 +2.17 kB | 🔴 +1.89 kB | | ~~assets/ServerConfigPanel-gtbjwyor.js~~ _(removed)_ | 8.2 kB | — | 🟢 -8.2 kB | 🟢 -2.16 kB | 🟢 -1.9 kB | | **assets/UserPanel-D1JK7WXC.js** _(new)_ | — | 7.91 kB | 🔴 +7.91 kB | 🔴 +2.06 kB | 🔴 +1.79 kB | | ~~assets/UserPanel-tRAQMh2x.js~~ _(removed)_ | 7.91 kB | — | 🟢 -7.91 kB | 🟢 -2.06 kB | 🟢 -1.79 kB | | assets/settings-B-df0dZe.js | 20.7 kB | 20.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-CI6OKvJn.js | 22.9 kB | 22.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-CXGVj_nD.js | 24.5 kB | 24.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DfQ6dSJj.js | 31.6 kB | 31.6 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DJ2QgDzm.js | 25.2 kB | 25.2 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DRNLPMG6.js | 23.7 kB | 23.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-DVVycxDc.js | 19.9 kB | 19.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-G6Dybj1b.js | 24.1 kB | 24.1 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/settings-M6_GZccG.js | 26 kB | 26 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Status: 6 added / 6 removed UI Components — 12.3 kB (baseline 12.3 kB) • ⚪ 0 B_Reusable component library chunks_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ----------------------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | **assets/ComfyQueueButton-CHe1G75M.js** _(new)_ | — | 11.1 kB | 🔴 +11.1 kB | 🔴 +2.76 kB | 🔴 +2.43 kB | | ~~assets/ComfyQueueButton-DQwhfLNW.js~~ _(removed)_ | 11.1 kB | — | 🟢 -11.1 kB | 🟢 -2.76 kB | 🟢 -2.44 kB | | assets/UserAvatar.vue_vue_type_script_setup_true_lang-C9bSkTC5.js | 1.12 kB | 1.12 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Status: 1 added / 1 removed Data & Services — 10 kB (baseline 10 kB) • ⚪ 0 B_Stores, services, APIs, and repositories_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------------------- | ------- | ------- | ----------------------- | ----------------------- | ---------------------- | | ~~assets/keybindingService-CV-GuwCp.js~~ _(removed)_ | 7.21 kB | — | 🟢 -7.21 kB | 🟢 -1.75 kB | 🟢 -1.5 kB | | **assets/keybindingService-DqxyKoLL.js** _(new)_ | — | 7.21 kB | 🔴 +7.21 kB | 🔴 +1.75 kB | 🔴 +1.51 kB | | ~~assets/serverConfigStore-BE22gWlb.js~~ _(removed)_ | 2.79 kB | — | 🟢 -2.79 kB | 🟢 -891 B | 🟢 -777 B | | **assets/serverConfigStore-DUtwNk8I.js** _(new)_ | — | 2.79 kB | 🔴 +2.79 kB | 🔴 +890 B | 🔴 +777 B |Status: 2 added / 2 removed Utilities & Hooks — 1.07 kB (baseline 1.07 kB) • ⚪ 0 B_Helpers, composables, and utility bundles_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | --------------------------- | ------- | ------- | ------------------ | ------------------ | ------------------ | | assets/mathUtil-CTARWQ-l.js | 1.07 kB | 1.07 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Vendor & Third-Party — 5.36 MB (baseline 5.36 MB) • ⚪ 0 B_External libraries and shared vendor chunks_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ------------------------------------------------ | ------- | ------- | ----------------------- | ----------------------- | ----------------------- | | **assets/vendor-other-CAGhv92x.js** _(new)_ | — | 3.22 MB | 🔴 +3.22 MB | 🔴 +685 kB | 🔴 +549 kB | | ~~assets/vendor-other-DUFQbqPR.js~~ _(removed)_ | 3.22 MB | — | 🟢 -3.22 MB | 🟢 -685 kB | 🟢 -549 kB | | **assets/vendor-tiptap-CBVOKkrf.js** _(new)_ | — | 232 kB | 🔴 +232 kB | 🔴 +45.7 kB | 🔴 +37.7 kB | | ~~assets/vendor-tiptap-DrFxugC4.js~~ _(removed)_ | 232 kB | — | 🟢 -232 kB | 🟢 -45.7 kB | 🟢 -37.7 kB | | **assets/vendor-vue-DeEYjaLm.js** _(new)_ | — | 92.4 kB | 🔴 +92.4 kB | 🔴 +23.9 kB | 🔴 +20.8 kB | | ~~assets/vendor-vue-DJFUVoPA.js~~ _(removed)_ | 92.4 kB | — | 🟢 -92.4 kB | 🟢 -23.9 kB | 🟢 -20.8 kB | | assets/vendor-primevue-PESgPnbc.js | 517 B | 517 B | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-visualization-BEfdbjRw.js | 1.82 MB | 1.82 MB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Status: 3 added / 3 removed Other — 2.58 MB (baseline 2.58 MB) • ⚪ 0 B_Bundles that do not match a named category_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | --------------------------- | ------- | ------- | ------------------ | ------------------ | ------------------ | | assets/commands-B2KZRBmX.js | 15.1 kB | 15.1 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-Bw-ckyga.js | 13.9 kB | 13.9 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-C_NmM85I.js | 13.8 kB | 13.8 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-CuozCW4W.js | 14 kB | 14 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-DGfVUJCR.js | 16.2 kB | 16.2 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-dOJNDogK.js | 14.5 kB | 14.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-DwiE551e.js | 14.7 kB | 14.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-Fw7mvqSy.js | 13.1 kB | 13.1 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/commands-FXnO1W4Q.js | 13.2 kB | 13.2 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-Bgu6_Hvd.js | 59.5 kB | 59.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-Bv0L0qvp.js | 93 kB | 93 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-C3Doz3n_.js | 67.6 kB | 67.6 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-C7eBl607.js | 70.7 kB | 70.7 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CHiV9ds2.js | 76.4 kB | 76.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-CIc79Nts.js | 68.5 kB | 68.5 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-DK5LmuBm.js | 58.8 kB | 58.8 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-J1nit7cj.js | 66.3 kB | 66.3 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/main-W97XgvAQ.js | 80.4 kB | 80.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-BePSqkA4.js | 195 kB | 195 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-BfT7dJcF.js | 204 kB | 204 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-BiAtoiXc.js | 194 kB | 194 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-CDfbduPY.js | 219 kB | 219 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-CDurg_KW.js | 197 kB | 197 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-CE-vG3RG.js | 182 kB | 182 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-DAwVV156.js | 200 kB | 200 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-DexhCMEi.js | 233 kB | 233 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/nodeDefs-kTrYLFPK.js | 184 kB | 184 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit (not blocking) otherwise we're good.
| watch( | ||
| () => [props.nodeType, props.assetType], | ||
| async () => { | ||
| await execute() | ||
| }, | ||
| { immediate: true } | ||
| ) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: instead of { immediate: true } I think could you do something like this to make the lifecycle more explicit:
onMount(() => execute())
watch(() => [props.nodeType, props.assetType], () => execute())There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you @DrJKL!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! After merging with main, this code has been superseded. The component now accepts assets as a prop instead of fetching internally, so the useAsyncState + watch({immediate: true}) pattern is no longer present.
For context on the original implementation: watch({immediate: true}) runs during component setup (before DOM mount), while onMounted() runs after mount. For data fetching that doesn't need DOM access, starting earlier provides better perceived performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude got slightly confused on this one.
f3ae110    to
    64813fa      
    Compare
  
    …erModal test The vue-i18n mock's t function was not accepting params, causing the displayTitle test to fail. Updated mock to match the @/i18n mock pattern which correctly handles params and formats them for assertions.
d7d9313    to
    f212a0b      
    Compare
  
    | id: z.string(), | ||
| name: z.string(), | ||
| asset_hash: z.string().optional(), | ||
| asset_hash: z.string().nullish(), | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing something from optional to required but nullable can be risky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the OSS, it returns null, on the cloud, it's undefined. Thanks for flagging this out - cc @arjansingh
| import type { AssetItem } from '@/platform/assets/schemas/assetSchema' | ||
|  | ||
| // Mock @/i18n for useAssetBrowser and AssetFilterBar | ||
| const mockAssetService = vi.hoisted(() => ({ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooooh, very nice.
| */ | ||
| export function useAssetBrowser(assets: AssetItem[] = []) { | ||
| export function useAssetBrowser( | ||
| assetsSource: Ref<AssetItem[] | undefined> = ref<AssetItem[] | undefined>([]) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a nit, but I like to make composables more flexible by using MaybeRefOrGetter and toValue.
It'd also have saved you from having to update all the tests that were passing in pure values instead of refs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make composables more flexible by using MaybeRefOrGetter and toValue
YAGNI
| 🔧 Auto-fixes AppliedThis PR has been automatically updated to fix linting and formatting issues. 
 Changes made:
 | 
) ## Summary Moves the fetch and post-fetch logic associated with the asset browser into the component and shows a loading state while fetching. To test, use this branch: comfyanonymous/ComfyUI#10045 https://github.com/user-attachments/assets/718974d5-efc7-46a0-bcd6-e82596d4c389 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6189-load-assets-browser-before-fetch-completes-and-show-loading-state-2946d73d365081879d1bd05d86e8c036) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <[email protected]>
| @christian-byrne Successfully backported to #6236 | 
…w loading state (#6236) Backport of #6189 to `rh-test` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6236-backport-rh-test-load-assets-browser-before-fetch-completes-and-show-loading-state-2956d73d3650817386fad4f54b1c0a89) by [Unito](https://www.unito.io) Co-authored-by: Christian Byrne <[email protected]> Co-authored-by: GitHub Action <[email protected]>
Summary
Moves the fetch and post-fetch logic associated with the asset browser into the component and shows a loading state while fetching.
To test, use this branch: comfyanonymous/ComfyUI#10045
assets-async.mp4
┆Issue is synchronized with this Notion page by Unito