refactor: centralize all download utils across app and apply special cloud-specific behavior #6188
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
dosubot
bot
added
the
size:L
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 10/23/2025, 06:45:22 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Test Results⏰ Completed at: 10/23/2025, 07:01:00 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) • 🟢 -880 B_Main entry bundles and manifests_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | ---------------------------------------- | ------- | ------- | ----------------------- | ---------------------- | ----------------------- | | **assets/index-BYRUTPvl.js** _(new)_ | — | 2.67 MB | 🔴 +2.67 MB | 🔴 +555 kB | 🔴 +420 kB | | ~~assets/index-CRYH80bd.js~~ _(removed)_ | 2.67 MB | — | 🟢 -2.67 MB | 🟢 -555 kB | 🟢 -420 kB | | ~~assets/index-DBuBwzBD.js~~ _(removed)_ | 616 kB | — | 🟢 -616 kB | 🟢 -114 kB | 🟢 -90.2 kB | | **assets/index-BPmv6bUz.js** _(new)_ | — | 614 kB | 🔴 +614 kB | 🔴 +114 kB | 🔴 +90.2 kB |Status: 2 added / 2 removed Graph Workspace — 707 kB (baseline 707 kB) • 🟢 -711 B_Graph editor runtime, canvas, workflow orchestration_ | File | Before | After | Δ Raw | Δ Gzip | Δ Brotli | | -------------------------------------------- | ------ | ------ | ---------------------- | ---------------------- | ---------------------- | | ~~assets/GraphView-LmMSSolF.js~~ _(removed)_ | 707 kB | — | 🟢 -707 kB | 🟢 -139 kB | 🟢 -107 kB | | **assets/GraphView-s6lnqTmB.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-DYFq62qY.js** _(new)_ | — | 8.15 kB | 🔴 +8.15 kB | 🔴 +2.47 kB | 🔴 +2.16 kB | | ~~assets/UserSelectView-sTWXaTT5.js~~ _(removed)_ | 8.15 kB | — | 🟢 -8.15 kB | 🟢 -2.47 kB | 🟢 -2.16 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-CoO99l_Z.js~~ _(removed)_ | 22.1 kB | — | 🟢 -22.1 kB | 🟢 -5.28 kB | 🟢 -4.6 kB | | **assets/CreditsPanel-DQJvElqF.js** _(new)_ | — | 22.1 kB | 🔴 +22.1 kB | 🔴 +5.28 kB | 🔴 +4.61 kB | | **assets/KeybindingPanel-CAtO-JLd.js** _(new)_ | — | 15.2 kB | 🔴 +15.2 kB | 🔴 +3.76 kB | 🔴 +3.3 kB | | ~~assets/KeybindingPanel-SoC0U1-Y.js~~ _(removed)_ | 15.2 kB | — | 🟢 -15.2 kB | 🟢 -3.76 kB | 🟢 -3.31 kB | | ~~assets/ExtensionPanel-NdIp4oI3.js~~ _(removed)_ | 12.1 kB | — | 🟢 -12.1 kB | 🟢 -2.83 kB | 🟢 -2.47 kB | | **assets/ExtensionPanel-nsWe6PZg.js** _(new)_ | — | 12.1 kB | 🔴 +12.1 kB | 🔴 +2.83 kB | 🔴 +2.48 kB | | **assets/AboutPanel-BSw_IDwa.js** _(new)_ | — | 10.3 kB | 🔴 +10.3 kB | 🔴 +2.67 kB | 🔴 +2.35 kB | | ~~assets/AboutPanel-DgmkUkZY.js~~ _(removed)_ | 10.3 kB | — | 🟢 -10.3 kB | 🟢 -2.67 kB | 🟢 -2.33 kB | | ~~assets/ServerConfigPanel-CqcVfEBb.js~~ _(removed)_ | 8.2 kB | — | 🟢 -8.2 kB | 🟢 -2.16 kB | 🟢 -1.9 kB | | **assets/ServerConfigPanel-JiKOioJi.js** _(new)_ | — | 8.2 kB | 🔴 +8.2 kB | 🔴 +2.16 kB | 🔴 +1.9 kB | | **assets/UserPanel-BwTRIeka.js** _(new)_ | — | 7.91 kB | 🔴 +7.91 kB | 🔴 +2.06 kB | 🔴 +1.79 kB | | ~~assets/UserPanel-JBub_028.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-BQP9cjBl.js~~ _(removed)_ | 11.1 kB | — | 🟢 -11.1 kB | 🟢 -2.76 kB | 🟢 -2.44 kB | | **assets/ComfyQueueButton-CBy7vel_.js** _(new)_ | — | 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-Bc-tMbzY.js~~ _(removed)_ | 7.21 kB | — | 🟢 -7.21 kB | 🟢 -1.75 kB | 🟢 -1.5 kB | | **assets/keybindingService-DIiT77T3.js** _(new)_ | — | 7.21 kB | 🔴 +7.21 kB | 🔴 +1.75 kB | 🔴 +1.51 kB | | assets/serverConfigStore-BE22gWlb.js | 2.79 kB | 2.79 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |Status: 1 added / 1 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-DUFQbqPR.js | 3.22 MB | 3.22 MB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-primevue-PESgPnbc.js | 517 B | 517 B | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-tiptap-DrFxugC4.js | 232 kB | 232 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-visualization-BEfdbjRw.js | 1.82 MB | 1.82 MB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B | | assets/vendor-vue-DJFUVoPA.js | 92.4 kB | 92.4 kB | ⚪ 0 B | ⚪ 0 B | ⚪ 0 B |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 | |
christian-byrne
added
needs-backport
DrJKL
previously approved these changes
Oct 22, 2025
| a.remove() | ||
| window.URL.revokeObjectURL(url) | ||
| }, 0) | ||
| // @ts-expect-error fixme ts strict error |
There was a problem hiding this comment.
Can you fix the type issue while you're here?
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Comprehensive PR Review
This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.
Review Summary
PR: refactor: centralize all download utils across app and apply special cloud-specific behavior (#6188)
Impact: 155 additions, 83 deletions across 9 files
Issue Distribution
- Critical: 0
- High: 5
- Medium: 4
- Low: 2
Category Breakdown
- Architecture: 5 issues
- Security: 0 issues
- Performance: 1 issues
- Code Quality: 5 issues
Key Findings
Architecture & Design
This PR demonstrates excellent architectural refactoring by centralizing download functionality that was previously duplicated across multiple files. The centralization eliminates code duplication in ModelExporter, RecordingManager, and nodeTemplates classes while introducing cloud-specific behavior for cross-origin downloads. The backward compatibility approach using re-exports with knip annotations shows strong consideration for external dependencies.
Security Considerations
No security vulnerabilities identified. The cross-origin download handling using blob fetches actually improves security by preventing navigation away from the current page.
Performance Impact
Minor performance considerations around URL parsing optimization, but the centralized approach with proper memory management (using queueMicrotask for URL cleanup) is well-implemented. The cloud-specific behavior prevents unnecessary double transfers.
Integration Points
The refactoring maintains backward compatibility through re-exports while consolidating implementation. This pattern should be applied consistently across other download operations in the codebase.
Positive Observations
- Excellent comprehensive test coverage including both cloud and non-cloud scenarios
- Proper error handling and memory management with queueMicrotask
- Good TypeScript practices with improved type safety
- Clean separation of concerns between desktop and cloud download behaviors
- Maintains backward compatibility for external custom nodes
References
Next Steps
- Consider addressing the TypeScript error suppression in nodeTemplates.ts
- Apply this centralization pattern to any remaining download operations in the codebase
- Consider documenting the URL cleanup pattern for other developers
- Monitor cloud-specific behavior for any edge cases in production
This is a comprehensive automated review. The refactoring demonstrates excellent architectural practices with proper testing and backward compatibility.
github-actions
bot
removed
the
claude-review
…- addresses review feedback - Extract common triggerLinkDownload() helper to eliminate code duplication - Convert arrow functions to function declarations to prevent HMR issues - Make link hiding consistent by always using display: none
- Replace wildcard import with named import for better clarity - Move URL spy stubs to module level for better performance - Reset mocks in beforeEach instead of recreating them
christian-byrne
force-pushed
the
refactor/centralize-downloads
branch
from
0f77b46 to
642e287
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Oct 23, 2025
…cloud-specific behavior (#6188) ## Summary Centralized all download functionalities across app. Then changed downloadFile on the cloud distribution to stream assets via blob fetches while desktop/local retains direct anchor downloads. This fixes issue where trying to download cross-origin resources opens them in the window, potentially losing the user's unsaved changes. ## Changes - **What**: Moved `downloadBlob` into `downloadUtil`, routed all callers (3D exporter, recording manager, node template export, workflow/palette export, Litegraph save, ~~`useDownload` consumers~~) through shared helpers, and changed `downloadFile` to `fetch` first when `isCloud` so cross-origin URLs download reliably - `useDownload` is the exception since we simply cannot do model downloads through blob (forcing user to transfer the entire model data twice is bad). Fortunately on cloud, the user doesn't need to download models locally anyway. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6188-refactor-centralize-all-download-utils-across-app-and-apply-special-cloud-specific-behav-2946d73d365081de9f27f0994950511d) by [Unito](https://www.unito.io)
|
@christian-byrne Successfully backported to #6230 |
christian-byrne
added a commit
that referenced
this pull request
Oct 23, 2025
… and apply special cloud-specific behavior (#6230) Backport of #6188 to `rh-test` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-6230-backport-rh-test-refactor-centralize-all-download-utils-across-app-and-apply-special-c-2956d73d3650810d980fc30dfea02cc5) by [Unito](https://www.unito.io) Co-authored-by: Christian Byrne <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Centralized all download functionalities across app. Then changed downloadFile on the cloud distribution to stream assets via blob fetches while desktop/local retains direct anchor downloads. This fixes issue where trying to download cross-origin resources opens them in the window, potentially losing the user's unsaved changes.
Changes
downloadBlobintodownloadUtil, routed all callers (3D exporter, recording manager, node template export, workflow/palette export, Litegraph save,useDownloadconsumersdownloadFiletofetchfirst whenisCloudso cross-origin URLs download reliablyuseDownloadis the exception since we simply cannot do model downloads through blob (forcing user to transfer the entire model data twice is bad). Fortunately on cloud, the user doesn't need to download models locally anyway.┆Issue is synchronized with this Notion page by Unito