Skip to content

Commit 5c5466d

Browse files
Jaewon Yoonampagent
andcommitted
test: verify generated output deletion
Amp-Thread-ID: https://ampcode.com/threads/T-019fb9b3-7fcf-705f-93b9-c7ff0b98a7ee Co-authored-by: Amp <amp@ampcode.com>
1 parent 709b190 commit 5c5466d

5 files changed

Lines changed: 252 additions & 9 deletions

File tree

browser_tests/tests/sidebar/assetsSidebarTab.spec.ts

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { expect, mergeTests } from '@playwright/test'
22
import type { Page, Response } from '@playwright/test'
33

4+
import type { Asset, ListAssetsResponse } from '@comfyorg/ingest-types'
45
import { comfyPageFixture } from '@e2e/fixtures/ComfyPage'
56
import { expectNoErrorUiAfterVerification } from '@e2e/fixtures/helpers/ErrorsTabHelper'
67
import {
@@ -10,11 +11,14 @@ import {
1011
routeMockJobTimestamp
1112
} from '@e2e/fixtures/jobsRouteFixture'
1213
import { TestIds } from '@e2e/fixtures/selectors'
14+
import { mockBilling } from '@e2e/fixtures/utils/cloudBillingMocks'
15+
import { mockCloudBoot } from '@e2e/fixtures/utils/cloudBootMocks'
1316
import { PropertiesPanelHelper } from '@e2e/tests/propertiesPanel/PropertiesPanelHelper'
1417
import type {
1518
JobDetail,
1619
RawJobListItem
1720
} from '@/platform/remote/comfyui/jobs/jobTypes'
21+
import type { ComfyNodeDef } from '@/schemas/nodeDefSchema'
1822

1923
const test = mergeTests(comfyPageFixture, jobsRouteFixture)
2024

@@ -103,6 +107,19 @@ const viewFiles = {
103107
'multi-output-b.png': {}
104108
}
105109

110+
const alphaOutputAsset: Asset = {
111+
id: 'alpha-asset-id',
112+
name: 'ComfyUI_alpha.png',
113+
hash: 'alpha.png',
114+
mime_type: 'image/png',
115+
tags: ['output'],
116+
created_at: '2026-01-01T12:00:00Z',
117+
updated_at: '2026-01-01T12:00:00Z'
118+
}
119+
120+
const outputAssetsByPage = new WeakMap<Page, Asset[]>()
121+
const generatedJobsByPage = new WeakMap<Page, RawJobListItem[]>()
122+
106123
async function mockInputFiles(page: Page, files: readonly string[]) {
107124
await page.route('**/internal/files/input**', async (route) => {
108125
if (route.request().method().toUpperCase() !== 'GET') {
@@ -170,6 +187,89 @@ const bulkInsertionTest = comfyPageFixture.extend({
170187
}
171188
})
172189

190+
const loadImageNodeDef: ComfyNodeDef = {
191+
name: 'LoadImage',
192+
display_name: 'Load Image',
193+
description: '',
194+
category: 'image',
195+
input: {
196+
required: {
197+
image: [['alpha.png [output]'], { image_upload: true }]
198+
}
199+
},
200+
output: ['IMAGE', 'MASK'],
201+
output_is_list: [false, false],
202+
output_name: ['IMAGE', 'MASK'],
203+
output_node: false,
204+
python_module: 'nodes',
205+
deprecated: false,
206+
experimental: false
207+
}
208+
209+
const cloudAssetDeletionTest = test.extend({
210+
page: async ({ page }, use) => {
211+
outputAssetsByPage.set(page, [alphaOutputAsset])
212+
generatedJobsByPage.set(page, [alphaJob])
213+
await mockCloudBoot(page, {
214+
features: {},
215+
settings: {
216+
'Comfy.Queue.QPOV2': false,
217+
'Comfy.RightSidePanel.ShowErrorsTab': false,
218+
'Comfy.TutorialCompleted': true,
219+
'Comfy.UseNewMenu': 'Top',
220+
'Comfy.VersionCompatibility.DisableWarnings': true,
221+
'Comfy.VueNodes.Enabled': true
222+
}
223+
})
224+
await mockBilling(page)
225+
await page.route('**/api/devtools/set_settings', (route) =>
226+
route.fulfill({ json: {} })
227+
)
228+
await page.route(/\/api\/assets(?:\?.*)?$/, (route) => {
229+
const assets = new URL(route.request().url()).searchParams
230+
.get('include_tags')
231+
?.split(',')
232+
.includes('output')
233+
? (outputAssetsByPage.get(page) ?? [])
234+
: []
235+
return route.fulfill({
236+
json: {
237+
assets,
238+
total: assets.length,
239+
has_more: false
240+
} satisfies ListAssetsResponse
241+
})
242+
})
243+
await page.route(/\/api\/jobs(?:\?.*)?$/, (route) => {
244+
const url = new URL(route.request().url())
245+
const isHistory = url.searchParams
246+
.get('status')
247+
?.split(',')
248+
.includes('completed')
249+
const jobs = isHistory ? (generatedJobsByPage.get(page) ?? []) : []
250+
const limit = Number(url.searchParams.get('limit') ?? 200)
251+
const offset = Number(url.searchParams.get('offset') ?? 0)
252+
return route.fulfill({
253+
json: {
254+
jobs,
255+
pagination: {
256+
offset,
257+
limit,
258+
total: jobs.length,
259+
has_more: false
260+
}
261+
}
262+
})
263+
})
264+
await page.route('**/api/object_info', (route) =>
265+
route.fulfill({ json: { LoadImage: loadImageNodeDef } })
266+
)
267+
await use(page)
268+
outputAssetsByPage.delete(page)
269+
generatedJobsByPage.delete(page)
270+
}
271+
})
272+
173273
test.describe('FE-130 assets sidebar route mocks', () => {
174274
test.beforeEach(async ({ jobsRoutes, page }) => {
175275
await jobsRoutes.mockJobsQueue([])
@@ -309,6 +409,79 @@ test.describe('FE-130 assets sidebar route mocks', () => {
309409
})
310410
})
311411

412+
cloudAssetDeletionTest.describe(
413+
'IR-91 generated output deletion',
414+
{ tag: '@cloud' },
415+
() => {
416+
cloudAssetDeletionTest.beforeEach(async ({ page }) => {
417+
await mockInputFiles(page, [])
418+
await mockViewFiles(page, viewFiles)
419+
})
420+
421+
cloudAssetDeletionTest(
422+
'removes a deleted generated output from Load Image options',
423+
async ({ comfyPage, page }) => {
424+
const deletedAssetIds: string[] = []
425+
await page.route('**/api/jobs/alpha/assets?*', async (route) => {
426+
await route.fulfill({
427+
json: {
428+
assets: [{ id: 'alpha-asset-id' }],
429+
pagination: {
430+
offset: 0,
431+
limit: 500,
432+
total: 1,
433+
has_more: false
434+
}
435+
}
436+
})
437+
})
438+
await page.route('**/api/assets/alpha-asset-id', async (route) => {
439+
deletedAssetIds.push('alpha-asset-id')
440+
outputAssetsByPage.set(page, [])
441+
await route.fulfill({ status: 204, body: '' })
442+
})
443+
444+
await comfyPage.workflow.loadWorkflow('widgets/load_image_widget')
445+
await comfyPage.vueNodes.waitForNodes(1)
446+
const imageWidgetButton = comfyPage.vueNodes
447+
.getNodeByTitle('Load Image')
448+
.getByRole('button', { name: 'Select image...' })
449+
await imageWidgetButton.click()
450+
const imagePicker = page.getByRole('dialog')
451+
await expect(
452+
imagePicker.getByText('ComfyUI_alpha.png [output]', { exact: true })
453+
).toBeVisible()
454+
await page.keyboard.press('Escape')
455+
456+
const tab = comfyPage.menu.assetsTab
457+
await tab.open()
458+
await expect(tab.getAssetCardByName('alpha')).toBeVisible()
459+
const historyDeleteRequests: { delete: string[] }[] = []
460+
await page.route('**/api/history', async (route) => {
461+
historyDeleteRequests.push(route.request().postDataJSON())
462+
generatedJobsByPage.set(page, [])
463+
await route.fulfill({ json: {} })
464+
})
465+
await tab.getAssetCardByName('alpha').click({ button: 'right' })
466+
await tab.contextMenuItem('Delete').click()
467+
await comfyPage.confirmDialog.delete.click()
468+
469+
await expect.poll(() => deletedAssetIds).toEqual(['alpha-asset-id'])
470+
await expect
471+
.poll(() => historyDeleteRequests)
472+
.toEqual([{ delete: ['alpha'] }])
473+
await expect(tab.getAssetCardByName('alpha')).toHaveCount(0)
474+
await tab.dismissToasts()
475+
await tab.close()
476+
await imageWidgetButton.click()
477+
await expect(
478+
imagePicker.getByText('ComfyUI_alpha.png [output]', { exact: true })
479+
).toHaveCount(0)
480+
}
481+
)
482+
}
483+
)
484+
312485
bulkInsertionTest.describe(
313486
'Assets sidebar - bulk insert as nodes',
314487
{ tag: ['@vue-nodes', '@ui', '@node', '@widget'] },

src/platform/assets/composables/useMediaAssetActions.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1350,6 +1350,31 @@ describe('useMediaAssetActions', () => {
13501350
expect(mockUpdateFlatOutputs).toHaveBeenCalled()
13511351
})
13521352

1353+
it('cleans up a selected output job only once', async () => {
1354+
mockIsCloud.value = true
1355+
mockGetOutputAssetMetadata.mockReturnValue({ jobId: 'job-1' })
1356+
mockGetJobAssetIds.mockResolvedValue(['output-uuid-1', 'output-uuid-2'])
1357+
mockDeleteAsset.mockResolvedValue(undefined)
1358+
const actions = useMediaAssetActions()
1359+
1360+
await actions.deleteAssets([
1361+
createMockAsset({
1362+
id: 'generated-card-1',
1363+
name: 'generated-1.png',
1364+
tags: ['output']
1365+
}),
1366+
createMockAsset({
1367+
id: 'generated-card-2',
1368+
name: 'generated-2.png',
1369+
tags: ['output']
1370+
})
1371+
])
1372+
1373+
expect(mockGetJobAssetIds).toHaveBeenCalledOnce()
1374+
expect(mockDeleteAsset).toHaveBeenCalledTimes(2)
1375+
expect(vi.mocked(api.fetchApi)).toHaveBeenCalledOnce()
1376+
})
1377+
13531378
it('keeps history when linked asset lookup fails', async () => {
13541379
mockIsCloud.value = true
13551380
mockGetJobAssetIds.mockRejectedValue(new Error('lookup failed'))

src/platform/assets/composables/useMediaAssetActions.ts

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,8 @@ export function useMediaAssetActions() {
107107

108108
const deleteAssetApi = async (
109109
asset: AssetItem,
110-
assetType: string
110+
assetType: string,
111+
outputDeletionByJob: Map<string, Promise<void>>
111112
): Promise<void> => {
112113
if (assetType === 'output' || assetType === 'temp') {
113114
const jobId =
@@ -116,13 +117,21 @@ export function useMediaAssetActions() {
116117
throw new Error('Unable to extract job ID from asset')
117118
}
118119

119-
const assetIds = isCloud ? await assetService.getJobAssetIds(jobId) : []
120-
const results = await Promise.allSettled(
121-
assetIds.map((id) => assetService.deleteAsset(id))
122-
)
123-
const failure = results.find((result) => result.status === 'rejected')
124-
if (failure?.status === 'rejected') throw failure.reason
125-
await deleteHistoryJob(jobId)
120+
const pendingDeletion =
121+
outputDeletionByJob.get(jobId) ??
122+
(async () => {
123+
const assetIds = isCloud
124+
? await assetService.getJobAssetIds(jobId)
125+
: []
126+
const results = await Promise.allSettled(
127+
assetIds.map((id) => assetService.deleteAsset(id))
128+
)
129+
const failure = results.find((result) => result.status === 'rejected')
130+
if (failure?.status === 'rejected') throw failure.reason
131+
await deleteHistoryJob(jobId)
132+
})()
133+
outputDeletionByJob.set(jobId, pendingDeletion)
134+
await pendingDeletion
126135
} else {
127136
// Input assets can only be deleted in cloud environment
128137
if (!isCloud) {
@@ -703,10 +712,15 @@ export function useMediaAssetActions() {
703712
)
704713

705714
try {
715+
const outputDeletionByJob = new Map<string, Promise<void>>()
706716
// Delete all assets using Promise.allSettled to track individual results
707717
const results = await Promise.allSettled(
708718
assetArray.map((asset) =>
709-
deleteAssetApi(asset, getAssetType(asset))
719+
deleteAssetApi(
720+
asset,
721+
getAssetType(asset),
722+
outputDeletionByJob
723+
)
710724
)
711725
)
712726

src/platform/assets/services/assetService.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,30 @@ describe(assetService.getJobAssetIds, () => {
513513
'Invalid job assets pagination offset'
514514
)
515515
})
516+
517+
it('throws instead of deleting from an unbounded asset listing', async () => {
518+
fetchApiMock.mockImplementation((input) => {
519+
const offset = Number(
520+
new URL(String(input), 'http://localhost').searchParams.get('offset')
521+
)
522+
return Promise.resolve(
523+
buildResponse({
524+
assets: [{ id: `asset-${offset}` }],
525+
pagination: {
526+
offset,
527+
limit: 500,
528+
total: Number.MAX_SAFE_INTEGER,
529+
has_more: true
530+
}
531+
})
532+
)
533+
})
534+
535+
await expect(assetService.getJobAssetIds('job-1')).rejects.toThrow(
536+
'exceeded 1000 batches'
537+
)
538+
expect(fetchApiMock).toHaveBeenCalledTimes(1000)
539+
})
516540
})
517541

518542
describe(assetService.getAssetModels, () => {

src/platform/assets/services/assetService.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -882,8 +882,15 @@ function createAssetService() {
882882
async function getJobAssetIds(jobId: string): Promise<AssetId[]> {
883883
const assetIds: AssetId[] = []
884884
let offset = 0
885+
let batchCount = 0
885886

886887
while (true) {
888+
if (batchCount++ >= MAX_PAGINATION_BATCHES) {
889+
throw new Error(
890+
`Job assets pagination exceeded ${MAX_PAGINATION_BATCHES} batches`
891+
)
892+
}
893+
887894
const query = new URLSearchParams({
888895
limit: DEFAULT_LIMIT.toString(),
889896
offset: offset.toString()

0 commit comments

Comments
 (0)