mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-20 20:39:30 +00:00
refactor: unify media asset downloads (#11717)
## Summary Unifies media asset download actions behind a single `downloadAssets(assets?)` API to avoid single and multi asset download path drift. ## Changes - **What**: Replaces `downloadAsset` and `downloadMultipleAssets` with `downloadAssets`, preserving no-arg media context fallback and explicit asset arrays. - **Dependencies**: None. ## Review Focus Download behavior for single-card, context-menu, and sidebar bulk actions should continue to use the same ZIP-export path for cloud multi-output jobs. Fixes #11715 ## Screenshots (if applicable) N/A ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-11717-refactor-unify-media-asset-downloads-3506d73d3650810d8bcec9c0194e743d) by [Unito](https://www.unito.io) --------- Co-authored-by: Alexander Brown <drjkl@comfy.org>
This commit is contained in:
@@ -327,7 +327,7 @@ const {
|
||||
} = useAssetSelection()
|
||||
|
||||
const {
|
||||
downloadMultipleAssets,
|
||||
downloadAssets,
|
||||
deleteAssets,
|
||||
addMultipleToWorkflow,
|
||||
openMultipleWorkflows,
|
||||
@@ -533,7 +533,7 @@ function handleContextMenuHide() {
|
||||
}
|
||||
|
||||
const handleBulkDownload = (assets: AssetItem[]) => {
|
||||
downloadMultipleAssets(assets)
|
||||
downloadAssets(assets)
|
||||
clearSelection()
|
||||
}
|
||||
|
||||
@@ -559,7 +559,7 @@ const handleBulkExportWorkflow = async (assets: AssetItem[]) => {
|
||||
}
|
||||
|
||||
const handleDownloadSelected = () => {
|
||||
downloadMultipleAssets(selectedAssets.value)
|
||||
downloadAssets(selectedAssets.value)
|
||||
clearSelection()
|
||||
}
|
||||
|
||||
|
||||
@@ -44,7 +44,7 @@
|
||||
:context="{ type: assetType }"
|
||||
class="absolute inset-0"
|
||||
@view="handleZoomClick"
|
||||
@download="actions.downloadAsset()"
|
||||
@download="asset && actions.downloadAssets([asset])"
|
||||
@video-playing-state-changed="isVideoPlaying = $event"
|
||||
@video-controls-changed="showVideoControls = $event"
|
||||
@image-loaded="handleImageLoaded"
|
||||
|
||||
@@ -31,8 +31,7 @@ vi.mock('@/utils/loaderNodeUtil', () => ({
|
||||
|
||||
const mediaAssetActions = {
|
||||
addWorkflow: vi.fn(),
|
||||
downloadAsset: vi.fn(),
|
||||
downloadMultipleAssets: vi.fn(),
|
||||
downloadAssets: vi.fn(),
|
||||
openWorkflow: vi.fn(),
|
||||
exportWorkflow: vi.fn(),
|
||||
copyJobId: vi.fn(),
|
||||
@@ -185,7 +184,7 @@ describe('MediaAssetContextMenu', () => {
|
||||
unmount()
|
||||
})
|
||||
|
||||
it('routes Download through downloadMultipleAssets so multi-output jobs zip', async () => {
|
||||
it('routes Download through downloadAssets so multi-output jobs zip', async () => {
|
||||
const { container, unmount } = mountComponent()
|
||||
await showMenu(container)
|
||||
|
||||
@@ -195,10 +194,7 @@ describe('MediaAssetContextMenu', () => {
|
||||
item: downloadItem
|
||||
})
|
||||
|
||||
expect(mediaAssetActions.downloadMultipleAssets).toHaveBeenCalledWith([
|
||||
asset
|
||||
])
|
||||
expect(mediaAssetActions.downloadAsset).not.toHaveBeenCalled()
|
||||
expect(mediaAssetActions.downloadAssets).toHaveBeenCalledWith([asset])
|
||||
|
||||
unmount()
|
||||
})
|
||||
|
||||
@@ -217,7 +217,7 @@ const contextMenuItems = computed<MenuItem[]>(() => {
|
||||
items.push({
|
||||
label: t('mediaAsset.actions.download'),
|
||||
icon: 'icon-[lucide--download]',
|
||||
command: () => actions.downloadMultipleAssets([asset])
|
||||
command: () => actions.downloadAssets([asset])
|
||||
})
|
||||
|
||||
// Separator before workflow actions (only if there are workflow actions)
|
||||
|
||||
@@ -2,9 +2,12 @@ import { createTestingPinia } from '@pinia/testing'
|
||||
import { fromAny } from '@total-typescript/shoehorn'
|
||||
import { setActivePinia } from 'pinia'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { createApp, defineComponent, h, provide, ref } from 'vue'
|
||||
|
||||
import type { LGraphNode } from '@/lib/litegraph/src/litegraph'
|
||||
import { MediaAssetKey } from '@/platform/assets/schemas/mediaAssetSchema'
|
||||
import type { AssetItem } from '@/platform/assets/schemas/assetSchema'
|
||||
import type { AssetMeta } from '@/platform/assets/schemas/mediaAssetSchema'
|
||||
import { useMediaAssetActions } from './useMediaAssetActions'
|
||||
|
||||
// Use vi.hoisted to create a mutable reference for isCloud
|
||||
@@ -13,6 +16,11 @@ const mockIsCloud = vi.hoisted(() => ({ value: false }))
|
||||
// Track the filename passed to createAnnotatedPath
|
||||
const capturedFilenames = vi.hoisted(() => ({ values: [] as string[] }))
|
||||
|
||||
const mockDownloadFile = vi.hoisted(() => vi.fn())
|
||||
vi.mock('@/base/common/downloadUtil', () => ({
|
||||
downloadFile: mockDownloadFile
|
||||
}))
|
||||
|
||||
vi.mock('@/platform/distribution/types', () => ({
|
||||
get isCloud() {
|
||||
return mockIsCloud.value
|
||||
@@ -168,13 +176,58 @@ function createMockAsset(overrides: Partial<AssetItem> = {}): AssetItem {
|
||||
}
|
||||
}
|
||||
|
||||
function createMockMediaAsset(overrides: Partial<AssetMeta> = {}): AssetMeta {
|
||||
return {
|
||||
...createMockAsset(),
|
||||
kind: 'image',
|
||||
src: 'https://example.com/default-preview.png',
|
||||
...overrides
|
||||
}
|
||||
}
|
||||
|
||||
function mountMediaActions(asset?: AssetMeta) {
|
||||
let actions: ReturnType<typeof useMediaAssetActions> | undefined
|
||||
|
||||
const ChildComponent = defineComponent({
|
||||
setup() {
|
||||
actions = useMediaAssetActions()
|
||||
return () => null
|
||||
}
|
||||
})
|
||||
|
||||
const HostComponent = defineComponent({
|
||||
setup() {
|
||||
provide(MediaAssetKey, {
|
||||
asset: ref(asset),
|
||||
context: ref({ type: 'input' as const }),
|
||||
isVideoPlaying: ref(false),
|
||||
showVideoControls: ref(false)
|
||||
})
|
||||
return () => h(ChildComponent)
|
||||
}
|
||||
})
|
||||
|
||||
const host = document.createElement('div')
|
||||
const app = createApp(HostComponent)
|
||||
app.mount(host)
|
||||
|
||||
if (!actions) throw new Error('media asset actions not initialized')
|
||||
|
||||
return {
|
||||
actions,
|
||||
unmount: () => app.unmount()
|
||||
}
|
||||
}
|
||||
|
||||
describe('useMediaAssetActions', () => {
|
||||
beforeEach(() => {
|
||||
vi.resetModules()
|
||||
setActivePinia(createTestingPinia({ stubActions: false }))
|
||||
vi.clearAllMocks()
|
||||
capturedFilenames.values = []
|
||||
mockIsCloud.value = false
|
||||
mockGetOutputAssetMetadata.mockReset()
|
||||
mockGetOutputAssetMetadata.mockReturnValue(null)
|
||||
mockGetAssetType.mockReset()
|
||||
})
|
||||
|
||||
describe('addWorkflow', () => {
|
||||
@@ -275,7 +328,102 @@ describe('useMediaAssetActions', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('downloadMultipleAssets - job_asset_name_filters', () => {
|
||||
describe('downloadAssets', () => {
|
||||
it('downloads the injected media asset when called without explicit assets', () => {
|
||||
const mediaAsset = createMockMediaAsset({
|
||||
id: 'context-asset',
|
||||
name: 'context-name.png',
|
||||
display_name: 'Context image.png',
|
||||
preview_url: 'https://example.com/context-preview.png'
|
||||
})
|
||||
|
||||
const { actions, unmount } = mountMediaActions(mediaAsset)
|
||||
actions.downloadAssets()
|
||||
|
||||
expect(mockDownloadFile).toHaveBeenCalledOnce()
|
||||
expect(mockDownloadFile).toHaveBeenCalledWith(
|
||||
'https://example.com/context-preview.png',
|
||||
'Context image.png'
|
||||
)
|
||||
expect(mockCreateAssetExport).not.toHaveBeenCalled()
|
||||
expect(mockTrackExport).not.toHaveBeenCalled()
|
||||
|
||||
unmount()
|
||||
})
|
||||
|
||||
it('does nothing when called without explicit assets and no media context asset', () => {
|
||||
const { actions, unmount } = mountMediaActions()
|
||||
actions.downloadAssets()
|
||||
|
||||
expect(mockDownloadFile).not.toHaveBeenCalled()
|
||||
expect(mockCreateAssetExport).not.toHaveBeenCalled()
|
||||
expect(mockTrackExport).not.toHaveBeenCalled()
|
||||
|
||||
unmount()
|
||||
})
|
||||
|
||||
it('keeps single explicit assets on the direct download path in cloud', () => {
|
||||
mockIsCloud.value = true
|
||||
mockGetOutputAssetMetadata.mockReturnValue({
|
||||
jobId: 'job1',
|
||||
outputCount: 1
|
||||
})
|
||||
|
||||
const asset = createMockAsset({
|
||||
id: 'single-output',
|
||||
name: 'single-output.png',
|
||||
preview_url: 'https://example.com/single-output.png',
|
||||
tags: ['output'],
|
||||
user_metadata: { jobId: 'job1', outputCount: 1 }
|
||||
})
|
||||
|
||||
const actions = useMediaAssetActions()
|
||||
actions.downloadAssets([asset])
|
||||
|
||||
expect(mockDownloadFile).toHaveBeenCalledOnce()
|
||||
expect(mockDownloadFile).toHaveBeenCalledWith(
|
||||
'https://example.com/single-output.png',
|
||||
'single-output.png'
|
||||
)
|
||||
expect(mockCreateAssetExport).not.toHaveBeenCalled()
|
||||
expect(mockTrackExport).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('uses ZIP export for an injected single multi-output asset in cloud', async () => {
|
||||
mockIsCloud.value = true
|
||||
mockGetAssetType.mockReturnValue('output')
|
||||
mockGetOutputAssetMetadata.mockReturnValue({
|
||||
jobId: 'job1',
|
||||
outputCount: 3
|
||||
})
|
||||
|
||||
const mediaAsset = createMockMediaAsset({
|
||||
id: 'multi-output',
|
||||
name: 'multi-output.png',
|
||||
preview_url: 'https://example.com/multi-output.png',
|
||||
tags: ['output'],
|
||||
user_metadata: { jobId: 'job1', outputCount: 3 }
|
||||
})
|
||||
|
||||
const { actions, unmount } = mountMediaActions(mediaAsset)
|
||||
actions.downloadAssets()
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(mockCreateAssetExport).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
expect(mockDownloadFile).not.toHaveBeenCalled()
|
||||
expect(mockCreateAssetExport).toHaveBeenCalledWith({
|
||||
job_ids: ['job1'],
|
||||
naming_strategy: 'preserve'
|
||||
})
|
||||
expect(mockTrackExport).toHaveBeenCalledWith('test-task-id')
|
||||
|
||||
unmount()
|
||||
})
|
||||
})
|
||||
|
||||
describe('downloadAssets - cloud zip filters', () => {
|
||||
beforeEach(() => {
|
||||
mockIsCloud.value = true
|
||||
mockCreateAssetExport.mockClear()
|
||||
@@ -305,7 +453,7 @@ describe('useMediaAssetActions', () => {
|
||||
const assets = [createOutputAsset('a1', 'img1.png', 'job1', 3)]
|
||||
|
||||
const actions = useMediaAssetActions()
|
||||
actions.downloadMultipleAssets(assets)
|
||||
actions.downloadAssets(assets)
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(mockCreateAssetExport).toHaveBeenCalledTimes(1)
|
||||
@@ -323,7 +471,7 @@ describe('useMediaAssetActions', () => {
|
||||
const j2 = createOutputAsset('a3', 'out2.png', 'job2', 1)
|
||||
|
||||
const actions = useMediaAssetActions()
|
||||
actions.downloadMultipleAssets([j1a, j1b, j2])
|
||||
actions.downloadAssets([j1a, j1b, j2])
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(mockCreateAssetExport).toHaveBeenCalledTimes(1)
|
||||
@@ -340,7 +488,7 @@ describe('useMediaAssetActions', () => {
|
||||
const asset2 = createOutputAsset('a2', 'img2.png', 'job2')
|
||||
|
||||
const actions = useMediaAssetActions()
|
||||
actions.downloadMultipleAssets([asset1, asset2])
|
||||
actions.downloadAssets([asset1, asset2])
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(mockCreateAssetExport).toHaveBeenCalledTimes(1)
|
||||
@@ -360,7 +508,7 @@ describe('useMediaAssetActions', () => {
|
||||
const j2 = createOutputAsset('a3', 'img2.png', 'job2')
|
||||
|
||||
const actions = useMediaAssetActions()
|
||||
actions.downloadMultipleAssets([j1a, j1b, j2])
|
||||
actions.downloadAssets([j1a, j1b, j2])
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(mockCreateAssetExport).toHaveBeenCalledTimes(1)
|
||||
@@ -379,7 +527,7 @@ describe('useMediaAssetActions', () => {
|
||||
const asset2 = createOutputAsset('a2', 'img2.png', 'job1')
|
||||
|
||||
const actions = useMediaAssetActions()
|
||||
actions.downloadMultipleAssets([asset1, asset2])
|
||||
actions.downloadAssets([asset1, asset2])
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(mockCreateAssetExport).toHaveBeenCalledTimes(1)
|
||||
|
||||
@@ -64,52 +64,30 @@ export function useMediaAssetActions() {
|
||||
}
|
||||
}
|
||||
|
||||
const downloadAsset = (asset?: AssetItem) => {
|
||||
const targetAsset = asset ?? mediaContext?.asset.value
|
||||
if (!targetAsset) return
|
||||
|
||||
try {
|
||||
const filename = getAssetDisplayName(targetAsset)
|
||||
// Prefer preview_url (already includes subfolder) with getAssetUrl as fallback
|
||||
const downloadUrl = targetAsset.preview_url || getAssetUrl(targetAsset)
|
||||
|
||||
downloadFile(downloadUrl, filename)
|
||||
|
||||
toast.add({
|
||||
severity: 'success',
|
||||
summary: t('g.success'),
|
||||
detail: t('mediaAsset.selection.downloadsStarted', 1),
|
||||
life: 2000
|
||||
})
|
||||
} catch (error) {
|
||||
toast.add({
|
||||
severity: 'error',
|
||||
summary: t('g.error'),
|
||||
detail: t('g.failedToDownloadImage')
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Download multiple assets at once.
|
||||
* In cloud mode with 2+ assets, creates a ZIP export via the backend.
|
||||
* Falls back to individual downloads in OSS mode or for single assets.
|
||||
* Download one or more assets.
|
||||
* In cloud mode, creates a ZIP export via the backend when called with
|
||||
* 2+ assets or with any asset whose job has `outputCount > 1`.
|
||||
* Falls back to direct downloads in OSS mode and for single single-output
|
||||
* assets. With no argument, uses the asset from `MediaAssetKey` context.
|
||||
*/
|
||||
const downloadMultipleAssets = (assets: AssetItem[]) => {
|
||||
if (!assets || assets.length === 0) return
|
||||
const downloadAssets = (assets?: AssetItem[]) => {
|
||||
const targetAssets =
|
||||
assets ?? (mediaContext?.asset.value ? [mediaContext.asset.value] : [])
|
||||
if (targetAssets.length === 0) return
|
||||
|
||||
const hasMultiOutputJobs = assets.some((a) => {
|
||||
const hasMultiOutputJobs = targetAssets.some((a) => {
|
||||
const count = getOutputAssetMetadata(a.user_metadata)?.outputCount
|
||||
return typeof count === 'number' && count > 1
|
||||
})
|
||||
|
||||
if (isCloud && (assets.length > 1 || hasMultiOutputJobs)) {
|
||||
void downloadMultipleAssetsAsZip(assets)
|
||||
if (isCloud && (targetAssets.length > 1 || hasMultiOutputJobs)) {
|
||||
void downloadAssetsAsZip(targetAssets)
|
||||
return
|
||||
}
|
||||
|
||||
try {
|
||||
assets.forEach((asset) => {
|
||||
targetAssets.forEach((asset) => {
|
||||
const filename = getAssetDisplayName(asset)
|
||||
const downloadUrl = asset.preview_url || getAssetUrl(asset)
|
||||
downloadFile(downloadUrl, filename)
|
||||
@@ -118,7 +96,7 @@ export function useMediaAssetActions() {
|
||||
toast.add({
|
||||
severity: 'success',
|
||||
summary: t('g.success'),
|
||||
detail: t('mediaAsset.selection.downloadsStarted', assets.length),
|
||||
detail: t('mediaAsset.selection.downloadsStarted', targetAssets.length),
|
||||
life: 2000
|
||||
})
|
||||
} catch (error) {
|
||||
@@ -131,7 +109,7 @@ export function useMediaAssetActions() {
|
||||
}
|
||||
}
|
||||
|
||||
async function downloadMultipleAssetsAsZip(assets: AssetItem[]) {
|
||||
async function downloadAssetsAsZip(assets: AssetItem[]) {
|
||||
const assetExportStore = useAssetExportStore()
|
||||
|
||||
try {
|
||||
@@ -720,8 +698,7 @@ export function useMediaAssetActions() {
|
||||
}
|
||||
|
||||
return {
|
||||
downloadAsset,
|
||||
downloadMultipleAssets,
|
||||
downloadAssets,
|
||||
deleteAssets,
|
||||
copyJobId,
|
||||
addWorkflow,
|
||||
|
||||
Reference in New Issue
Block a user