Skip to content

Commit 2842bc4

Browse files
authored
fix: preserve annotations for bulk asset insertion (#14126)
## ELI5 When ComfyUI inserts or drops a generated asset, it must keep the marker that says where the file lives. This change makes structured assets own that decision, preventing valid output files from being reported as missing. ## Summary - Fix false missing-media errors after bulk-inserting temporary or output assets from the Asset Panel. - Fix dropped output `ResultItem`s on output-folder image loaders so their serialized widget values retain `[output]`. - Make the structured path API reject caller-supplied root/subfolder options, while preserving the explicit string-path contract. - Preserve the Job Queue insertion behavior merged in #14081. ## Changes - **Path contract**: Separate string and `ResultItem` overloads. `ResultItem` paths derive annotations from `item.type`, preserve existing annotations, and reject caller options; string roots are limited to `ResultItemType`. - **Callers**: Share one Asset Panel asset-to-widget-path conversion across single and bulk insertion. Keep string uploads driven by `image_folder`, while dropped `ResultItem`s use their own type. Narrow the Mask Editor string boundary safely. - **Unit coverage**: Guard the forbidden `ResultItem` options call at runtime and typecheck time; cover input/temp/output, pre-annotated paths, missing types, single/bulk Asset Panel insertion, Job Queue media types, and image-upload string/`ResultItem` controls. - **E2E coverage**: From a blank workflow, bulk-insert exactly two generated assets through the Asset Panel, confirm two nodes, and finish at the user-visible no-error assertion without inspecting annotation strings. - **E2E synchronization**: The shared helper races the exact verification response against surfaced error UI. Job Queue synchronization now requires the output-tagged `/api/assets` request, so a regression fails at `sawErrorUi` instead of an unrelated response or network timeout. ## RED → GREEN proof - **🔴 RED**: CodeRabbit reviewed the tests-only [bca10b0 commit](bca10b0) with no remaining findings. - [Unit test job](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/30234592564/job/89879872146): the new test alone failed because `hash1.jpeg` and `hash2.jpeg` were missing `[temp]` and `[output]`; the other 13,467 tests passed. - [Chromium E2E shard 11/16](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/30234592612/job/89880156060): setup, two-asset selection, and two-node creation passed, then every attempt failed only at the final no-error-UI observation with `observed: true, sawErrorUi: true`. [Playwright Chromium report](https://2e6947aa.comfyui-playwright-chromium.pages.dev). - **🟢 GREEN**: The historical proof remains at the [production implementation](06d27f9) and [GREEN HEAD](de4c593). - [Unit test job](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/30236104670/job/89885801805): all 1,020 test files and 13,476 tests passed, including string/`ResultItem` controls, Asset Panel input/temp/output widget values, and Job Queue image/video/audio annotations. - [Chromium E2E shard 11/16](https://github.com/Comfy-Org/ComfyUI_frontend/actions/runs/30236104668/job/89884452924): the regression passed through blank-workflow setup, two-asset selection, two-node creation, and the final no-error-UI observation. [Playwright Chromium report](https://cc695aff.comfyui-playwright-chromium.pages.dev). - **Current review HEAD**: [eef7a9c](eef7a9c), including the [review-response commit b2d171c](b2d171c) after integrating merged parent #14081 in [b20d562](b20d562). - Local verification: 99 focused Unit tests, app/browser typechecks, full lint, format check, knip, and the focused Asset Panel Playwright E2E pass. - Fresh CI is in progress for this HEAD. ## Follow-up - #14356 tracks `[temp]` paths in missing-media detection. The annotation is correct, but the detector lacks an authoritative temp-asset source; simply widening its regex would not safely resolve the end-to-end false positive. ## Review Focus - Confirm the overloads make `ResultItem` ownership unambiguous while preserving string-path behavior. - Confirm single, bulk, drop, Job Queue, and input/temp/output controls cover both the fixed paths and preserved behavior. - Confirm both E2E paths protect the user-visible no-error outcome with positive synchronization signals. ## Screenshot Before https://github.com/user-attachments/assets/1d309b94-6dda-4441-974f-5d69ff32beb8 After https://github.com/user-attachments/assets/8cf5320f-6dc0-4552-86a3-98ef5d39aa1a
1 parent fb34a8e commit 2842bc4

14 files changed

Lines changed: 380 additions & 95 deletions

File tree

apps/website/src/components/home/GetStartedSection.vue

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,17 +37,17 @@ const steps = [
3737

3838
<template>
3939
<section
40-
class="bg-primary-comfy-ink max-w-9xl mx-auto px-4 py-20 lg:px-20 lg:py-24"
40+
class="max-w-9xl mx-auto bg-primary-comfy-ink px-4 py-20 lg:px-20 lg:py-24"
4141
>
4242
<div class="flex flex-col gap-12 lg:flex-row lg:gap-8">
4343
<!-- Left heading -->
4444
<div
45-
class="bg-primary-comfy-ink sticky top-20 shrink-0 py-2 lg:top-28 lg:w-115 lg:self-start"
45+
class="sticky top-20 shrink-0 bg-primary-comfy-ink py-2 lg:top-28 lg:w-115 lg:self-start"
4646
>
47-
<h2 class="text-primary-comfy-canvas text-5xl font-light">
47+
<h2 class="text-5xl font-light text-primary-comfy-canvas">
4848
{{ t('getStarted.heading', locale) }}
4949
</h2>
50-
<p class="text-primary-comfy-canvas mt-8 text-base">
50+
<p class="mt-8 text-base text-primary-comfy-canvas">
5151
{{ t('getStarted.subheading', locale) }}
5252
</p>
5353
</div>
@@ -57,27 +57,27 @@ const steps = [
5757
<div
5858
v-for="step in steps"
5959
:key="step.number"
60-
class="border-primary-comfy-canvas flex flex-col gap-4 border-b py-12 first:pt-0 last:border-b lg:flex-row lg:items-start lg:gap-8"
60+
class="flex flex-col gap-4 border-b border-primary-comfy-canvas py-12 first:pt-0 last:border-b lg:flex-row lg:items-start lg:gap-8"
6161
>
6262
<span
63-
class="text-primary-comfy-canvas w-16 shrink-0 text-6xl font-light lg:text-7xl"
63+
class="w-16 shrink-0 text-6xl font-light text-primary-comfy-canvas lg:text-7xl"
6464
>
6565
{{ step.number }}
6666
</span>
6767
<h3
68-
class="text-primary-comfy-canvas shrink-0 text-2xl font-light lg:w-84"
68+
class="shrink-0 text-2xl font-light text-primary-comfy-canvas lg:w-84"
6969
>
7070
{{ step.title }}
7171
</h3>
7272
<p
7373
v-if="step.description"
74-
class="text-primary-comfy-canvas flex-1 text-sm"
74+
class="flex-1 text-sm text-primary-comfy-canvas"
7575
>
7676
{{ step.description }}
7777
</p>
7878
<p
7979
v-else-if="step.number === '2'"
80-
class="text-primary-comfy-canvas flex-1 text-sm"
80+
class="flex-1 text-sm text-primary-comfy-canvas"
8181
>
8282
{{ t('getStarted.step2.descriptionPrefix', locale)
8383
}}<a

apps/website/src/components/home/HeroSection.vue

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,13 @@ const { loaded: logoLoaded } = useHeroLogo(logoContainer)
3131

3232
<div class="flex-1 px-6 py-12 lg:px-16">
3333
<h1
34-
class="text-primary-comfy-canvas text-4xl font-light whitespace-pre-line lg:text-6xl"
34+
class="text-4xl font-light whitespace-pre-line text-primary-comfy-canvas lg:text-6xl"
3535
>
3636
{{ t('hero.title', locale) }}
3737
</h1>
3838

3939
<p
40-
class="text-primary-comfy-canvas mt-8 max-w-lg text-sm/relaxed lg:text-base"
40+
class="mt-8 max-w-lg text-sm/relaxed text-primary-comfy-canvas lg:text-base"
4141
>
4242
{{ t('hero.subtitle', locale) }}
4343
</p>

browser_tests/fixtures/helpers/ErrorsTabHelper.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,15 @@ export async function expectNoErrorUiAfterVerification(
5050
verificationResponse: Promise<Response>,
5151
observationMs = 2_000
5252
): Promise<void> {
53-
await verificationResponse
54-
5553
const overlay = comfyPage.page.getByTestId(TestIds.dialogs.errorOverlay)
56-
let sawErrorUi = false
54+
const readiness = await Promise.race([
55+
verificationResponse.then(() => 'verification' as const),
56+
overlay.waitFor({ state: 'visible' }).then(() => 'error-ui' as const),
57+
panel.errorsTab
58+
.waitFor({ state: 'visible' })
59+
.then(() => 'error-ui' as const)
60+
])
61+
let sawErrorUi = readiness === 'error-ui'
5762
const startedAt = Date.now()
5863

5964
await expect

browser_tests/tests/queue/jobOutputInsertion.spec.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { expect } from '@playwright/test'
22

3+
import { assetRequestIncludesTag } from '@e2e/fixtures/assetApiFixture'
34
import { jobOutputInsertionCases } from '@e2e/fixtures/data/jobOutputInsertion'
45
import { expectNoErrorUiAfterVerification } from '@e2e/fixtures/helpers/ErrorsTabHelper'
56
import { jobOutputInsertionTest as test } from '@e2e/fixtures/jobOutputInsertionFixture'
@@ -36,7 +37,8 @@ test.describe(
3637
(response) =>
3738
response.request().method().toUpperCase() === 'GET' &&
3839
response.status() === 200 &&
39-
new URL(response.url()).pathname.endsWith('/api/assets')
40+
new URL(response.url()).pathname.endsWith('/api/assets') &&
41+
assetRequestIncludesTag(response.url(), 'output')
4042
)
4143

4244
await comfyPage.queuePanel.addOutputToCurrentWorkflow(scenario.job.id)

browser_tests/tests/sidebar/assetsSidebarTab.spec.ts

Lines changed: 88 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,16 @@
11
import { expect, mergeTests } from '@playwright/test'
2-
import type { Page } from '@playwright/test'
2+
import type { Page, Response } from '@playwright/test'
33

44
import { comfyPageFixture } from '@e2e/fixtures/ComfyPage'
5+
import { expectNoErrorUiAfterVerification } from '@e2e/fixtures/helpers/ErrorsTabHelper'
56
import {
67
createRouteMockJob,
8+
JobsRouteMocker,
79
jobsRouteFixture,
810
routeMockJobTimestamp
911
} from '@e2e/fixtures/jobsRouteFixture'
12+
import { TestIds } from '@e2e/fixtures/selectors'
13+
import { PropertiesPanelHelper } from '@e2e/tests/propertiesPanel/PropertiesPanelHelper'
1014
import type {
1115
JobDetail,
1216
RawJobListItem
@@ -145,6 +149,27 @@ async function mockViewFiles(page: Page, filesByName: ViewFilesByName) {
145149
})
146150
}
147151

152+
function isGeneratedAssetVerificationResponse(response: Response): boolean {
153+
const url = new URL(response.url())
154+
return (
155+
response.request().method().toUpperCase() === 'GET' &&
156+
response.status() === 200 &&
157+
url.pathname.endsWith('/api/jobs') &&
158+
url.searchParams.get('status')?.split(',').includes('completed') === true
159+
)
160+
}
161+
162+
const bulkInsertionTest = comfyPageFixture.extend({
163+
page: async ({ page }, use) => {
164+
const jobsRoutes = new JobsRouteMocker(page)
165+
await jobsRoutes.mockJobsQueue([])
166+
await jobsRoutes.mockJobsHistory(generatedJobs)
167+
await mockInputFiles(page, [])
168+
await mockViewFiles(page, viewFiles)
169+
await use(page)
170+
}
171+
})
172+
148173
test.describe('FE-130 assets sidebar route mocks', () => {
149174
test.beforeEach(async ({ jobsRoutes, page }) => {
150175
await jobsRoutes.mockJobsQueue([])
@@ -284,6 +309,68 @@ test.describe('FE-130 assets sidebar route mocks', () => {
284309
})
285310
})
286311

312+
bulkInsertionTest.describe(
313+
'Assets sidebar - bulk insert as nodes',
314+
{ tag: ['@vue-nodes', '@ui', '@node', '@widget'] },
315+
() => {
316+
bulkInsertionTest.use({
317+
initialSettings: {
318+
'Comfy.RightSidePanel.ShowErrorsTab': true
319+
}
320+
})
321+
322+
bulkInsertionTest.beforeEach(async ({ comfyPage }) => {
323+
await comfyPage.command.executeCommand('Comfy.NewBlankWorkflow')
324+
await expect.poll(() => comfyPage.nodeOps.getGraphNodesCount()).toBe(0)
325+
await comfyPage.toast.closeToasts()
326+
const panel = new PropertiesPanelHelper(comfyPage.page)
327+
await panel.open(comfyPage.actionbar.propertiesButton)
328+
await expect(
329+
comfyPage.page.getByTestId(TestIds.dialogs.errorOverlay)
330+
).toBeHidden()
331+
await expect(panel.errorsTab).toBeHidden()
332+
333+
const tab = comfyPage.menu.assetsTab
334+
await tab.open()
335+
await expect(tab.assetCards).toHaveCount(2)
336+
})
337+
338+
bulkInsertionTest(
339+
'does not surface errors for inserted output assets',
340+
async ({ comfyPage }) => {
341+
const tab = comfyPage.menu.assetsTab
342+
const panel = new PropertiesPanelHelper(comfyPage.page)
343+
await expect(panel.root).toBeVisible()
344+
345+
await tab.getAssetCardByName('alpha').click()
346+
await comfyPage.page.keyboard.down('ControlOrMeta')
347+
await tab.getAssetCardByName('beta').click()
348+
await comfyPage.page.keyboard.up('ControlOrMeta')
349+
await expect(tab.selectedCards).toHaveCount(2)
350+
351+
const generatedAssetVerificationResponse =
352+
comfyPage.page.waitForResponse(isGeneratedAssetVerificationResponse)
353+
354+
await tab.getAssetCardByName('alpha').dispatchEvent('contextmenu', {
355+
bubbles: true,
356+
cancelable: true,
357+
button: 2
358+
})
359+
await expect(comfyPage.contextMenu.primeVueMenu).toBeVisible()
360+
await tab.contextMenuItem('Insert all assets as nodes').click()
361+
362+
await expect.poll(() => comfyPage.vueNodes.getNodeCount()).toBe(2)
363+
364+
await expectNoErrorUiAfterVerification(
365+
comfyPage,
366+
panel,
367+
generatedAssetVerificationResponse
368+
)
369+
}
370+
)
371+
}
372+
)
373+
287374
test.describe('FE-910 marquee selection and select all', () => {
288375
test.beforeEach(async ({ jobsRoutes, page, comfyPage }) => {
289376
await jobsRoutes.mockJobsQueue([])

src/composables/maskeditor/useMaskEditorSaver.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { api } from '@/scripts/api'
1212
import { app } from '@/scripts/app'
1313
import { createAnnotatedPath } from '@/utils/createAnnotatedPath'
1414
import { encodeRgbaAsPng } from '@/utils/pngEncodeUtil'
15+
import { isResultItemType } from '@/utils/typeGuardUtil'
1516
import type { LGraphNode } from '@/lib/litegraph/src/litegraph'
1617

1718
// Private layer filename functions
@@ -302,7 +303,7 @@ export function useMaskEditorSaver() {
302303
node.imgs = undefined
303304
const annotatedPath = createAnnotatedPath(mainRef.filename, {
304305
subfolder: mainRef.subfolder,
305-
rootFolder: mainRef.type
306+
rootFolder: isResultItemType(mainRef.type) ? mainRef.type : undefined
306307
})
307308
nodeOutputStore.setNodeOutputs(node, annotatedPath, { folder: 'input' })
308309
node.graph?.setDirtyCanvas(true)

src/composables/queue/useJobMenu.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -441,6 +441,7 @@ describe('useJobMenu', () => {
441441
filename: 'foo.png',
442442
widget: 'image',
443443
type: 'temp',
444+
// TODO(#14356): Missing-media detection cannot yet verify [temp] paths.
444445
expectedWidgetValue: 'bar/foo.png [temp]'
445446
},
446447
{

src/composables/queue/useJobMenu.ts

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { useSettingStore } from '@/platform/settings/settingStore'
1010
import { withNodeAddSource } from '@/platform/telemetry/nodeAdded/nodeAddSource'
1111
import { useWorkflowService } from '@/platform/workflow/core/services/workflowService'
1212
import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore'
13-
import type { ResultItem, ResultItemType } from '@/schemas/apiSchema'
13+
import type { ResultItem } from '@/schemas/apiSchema'
1414
import { api } from '@/scripts/api'
1515
import { downloadBlob } from '@/scripts/utils'
1616
import { useDialogService } from '@/services/dialogService'
@@ -22,6 +22,7 @@ import { useQueueStore } from '@/stores/queueStore'
2222
import type { ResultItemImpl, TaskItemImpl } from '@/stores/queueStore'
2323
import { createAnnotatedPath } from '@/utils/createAnnotatedPath'
2424
import { appendJsonExt } from '@/utils/formatUtil'
25+
import { isResultItemType } from '@/utils/typeGuardUtil'
2526

2627
export type MenuEntry =
2728
| {
@@ -153,18 +154,13 @@ export function useJobMenu(
153154

154155
if (!node) return
155156

156-
const isResultItemType = (v: string | undefined): v is ResultItemType =>
157-
v === 'input' || v === 'output' || v === 'temp'
158-
159157
const apiItem: ResultItem = {
160158
filename: result.filename,
161159
subfolder: result.subfolder,
162160
type: isResultItemType(result.type) ? result.type : 'output'
163161
}
164162

165-
const annotated = createAnnotatedPath(apiItem, {
166-
rootFolder: 'input'
167-
})
163+
const annotated = createAnnotatedPath(apiItem)
168164
const widget = node.widgets?.find((w) => w.name === widgetName)
169165
if (widget) {
170166
widget.value = annotated

0 commit comments

Comments
 (0)