mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-05-23 06:10:32 +00:00
## Summary Import published media assets for shared workflows before loading the graph so the first missing-media scan sees the user's newly imported references instead of surfacing a false missing asset error. cc FE-773 ## Changes - **What**: Moves the shared workflow import step ahead of `loadGraphData` for the copy-and-open flow, while still allowing the workflow to open with a warning path if asset import fails. - **What**: Clears the shared workflow URL intent consistently on failure paths, including graph load failure after an import attempt, so reloads do not repeatedly replay the same shared workflow side effects. - **What**: Invalidates the input asset cache after published asset import so graph loading and missing-media resolution can observe the refreshed media state. - **What**: Adds a global loading spinner while shared workflow asset import and graph load are in progress, with `role="status"`, `aria-live`, reduced-motion-safe animation, and body teleporting so it stays visible above blocking UI. - **What**: Adds stable TestIds for the shared workflow dialog and updates existing shared workflow E2E selectors away from copy-dependent role text. - **What**: Adds a cloud E2E regression fixture and spec covering the critical flow: shared URL opens the dialog, the user confirms asset import, published media is imported before the public-inclusive input asset scan, the workflow loads, the share query is removed, and missing media UI is not surfaced. - **Breaking**: None. - **Dependencies**: None. ## Root Cause Shared workflow graph loading triggered the missing-media pipeline before the user-selected published media import had completed. Because `include_public=true` does not include published assets, the pre-import scan could classify shared media as missing even when the user was about to import those assets into their own library. ## Review Focus - The ordering in `useSharedWorkflowUrlLoader`: import published assets first, then load the graph, while keeping import failure non-fatal for workflow opening. - The failure cleanup behavior: the shared URL/preserved query intent is now cleared for graph load failures too, avoiding repeated reload-triggered imports. - The spinner behavior in `App.vue`: it uses the existing `workspaceStore.spinner` boolean and intentionally keeps broader ref-counted spinner ownership as follow-up work. - The E2E sentinel in `sharedWorkflowMissingMedia.spec.ts`: it asserts no public-inclusive input asset scan occurs before `/api/assets/import`, then waits for a settling window to ensure the missing-media overlay does not appear. ## Validation - `pnpm format` - `pnpm lint` (passed with existing unrelated warnings only) - `pnpm typecheck` - `pnpm test:unit` - Commit hook: lint-staged formatting/linting, `pnpm typecheck`, `pnpm typecheck:browser` - Push hook: `pnpm knip --cache` (passed with existing tag hint only) ## Follow-Up - Consider a ref-counted or scoped global spinner API so long-running flows do not directly toggle `workspaceStore.spinner`. - Consider separating shared workflow load status into orthogonal result fields instead of encoding partial success in a single string union. - Consider moving published asset import/cache invalidation behind an asset-service-owned API boundary. - Backend follow-up remains needed for `include_public=true` not including published assets; this PR only removes the frontend false positive when the user explicitly imports the shared media. ## Screenshots Before https://github.com/user-attachments/assets/dc790046-237c-4dd8-b773-2507f9a66650 After https://github.com/user-attachments/assets/6517cd38-2c3d-4bfe-a990-35892b7e50ae https://github.com/user-attachments/assets/d89dc3d3-75d9-4251-998b-0c354414e25b ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-12333-fix-avoid-false-missing-media-errors-after-importing-shared-workflow-assets-3656d73d365081b38634dcb7625cfc32) by [Unito](https://www.unito.io)
148 lines
4.6 KiB
TypeScript
148 lines
4.6 KiB
TypeScript
import { expect, mergeTests } from '@playwright/test'
|
|
|
|
import { comfyPageFixture } from '@e2e/fixtures/ComfyPage'
|
|
import type { ComfyPage } from '@e2e/fixtures/ComfyPage'
|
|
import { TestIds } from '@e2e/fixtures/selectors'
|
|
import {
|
|
sharedWorkflowImportFixture,
|
|
sharedWorkflowImportScenario
|
|
} from '@e2e/fixtures/sharedWorkflowImportFixture'
|
|
import type { SharedWorkflowImportMocks } from '@e2e/fixtures/sharedWorkflowImportFixture'
|
|
import { PropertiesPanelHelper } from '@e2e/tests/propertiesPanel/PropertiesPanelHelper'
|
|
import type { WorkspaceStore } from '@e2e/types/globals'
|
|
|
|
const IMPORT_ORDER_TIMEOUT_MS = 5_000
|
|
|
|
async function expectImportPrecedesPublicInclusiveInputAssetScan(
|
|
mocks: SharedWorkflowImportMocks
|
|
): Promise<void> {
|
|
await expect(async () => {
|
|
const events = mocks.getRequestEvents()
|
|
const importIndex = events.indexOf('import')
|
|
const afterImportIndex = events.indexOf(
|
|
'input-assets-including-public-after-import'
|
|
)
|
|
|
|
expect(
|
|
events,
|
|
'public-inclusive input assets must not be scanned before import'
|
|
).not.toContain('input-assets-including-public-before-import')
|
|
expect(importIndex, `events: ${events.join(',')}`).toBeGreaterThanOrEqual(0)
|
|
expect(afterImportIndex, `events: ${events.join(',')}`).toBeGreaterThan(
|
|
importIndex
|
|
)
|
|
}).toPass({ timeout: IMPORT_ORDER_TIMEOUT_MS })
|
|
}
|
|
|
|
async function getCachedMissingMediaWarningNames(
|
|
comfyPage: ComfyPage
|
|
): Promise<string[] | null> {
|
|
return await comfyPage.page.evaluate(() => {
|
|
const workflow = (window.app!.extensionManager as WorkspaceStore).workflow
|
|
.activeWorkflow
|
|
if (!workflow) return null
|
|
|
|
return (
|
|
workflow.pendingWarnings?.missingMediaCandidates?.map(
|
|
(candidate) => candidate.name
|
|
) ?? []
|
|
)
|
|
})
|
|
}
|
|
|
|
async function expectNoMissingMediaAfterPublicInclusiveAssetScan(
|
|
comfyPage: ComfyPage,
|
|
mocks: SharedWorkflowImportMocks
|
|
): Promise<void> {
|
|
await mocks.waitForPublicInclusiveInputAssetResponseAfterImport()
|
|
await comfyPage.nextFrame()
|
|
|
|
await expect(
|
|
comfyPage.page.getByTestId(TestIds.dialogs.errorOverlay)
|
|
).toBeHidden()
|
|
await expect
|
|
.poll(() => getCachedMissingMediaWarningNames(comfyPage))
|
|
.toEqual([])
|
|
}
|
|
|
|
async function openPanelAndExpectNoMissingMedia(
|
|
comfyPage: ComfyPage
|
|
): Promise<void> {
|
|
const page = comfyPage.page
|
|
const errorOverlay = page.getByTestId(TestIds.dialogs.errorOverlay)
|
|
await expect(errorOverlay).toBeHidden()
|
|
|
|
const panel = new PropertiesPanelHelper(page)
|
|
await panel.open(comfyPage.actionbar.propertiesButton)
|
|
await expect(
|
|
panel.root.getByTestId(TestIds.propertiesPanel.errorsTab)
|
|
).toBeHidden()
|
|
await expect(page.getByTestId(TestIds.dialogs.missingMediaGroup)).toHaveCount(
|
|
0
|
|
)
|
|
}
|
|
|
|
const test = mergeTests(comfyPageFixture, sharedWorkflowImportFixture)
|
|
|
|
test.describe('Shared workflow missing media', { tag: '@cloud' }, () => {
|
|
// Missing media only surfaces the overlay when the Errors tab is enabled
|
|
// (src/stores/executionErrorStore.ts).
|
|
test.use({
|
|
initialSettings: {
|
|
'Comfy.RightSidePanel.ShowErrorsTab': true
|
|
}
|
|
})
|
|
|
|
test.beforeEach(async ({ comfyPage, sharedWorkflowImportMocks }) => {
|
|
sharedWorkflowImportMocks.resetAndStartRecording()
|
|
await comfyPage.setup({
|
|
clearStorage: false,
|
|
url: `/?share=${sharedWorkflowImportScenario.shareId}`
|
|
})
|
|
})
|
|
|
|
test('imports shared media before loading workflow so missing media is not surfaced', async ({
|
|
comfyPage,
|
|
sharedWorkflowImportMocks
|
|
}) => {
|
|
const { page } = comfyPage
|
|
|
|
const dialog = page.getByTestId(TestIds.dialogs.openSharedWorkflow)
|
|
await expect(
|
|
dialog.getByTestId(TestIds.dialogs.openSharedWorkflowTitle)
|
|
).toBeVisible()
|
|
|
|
await dialog.getByTestId(TestIds.dialogs.openSharedWorkflowConfirm).click()
|
|
|
|
await expect
|
|
.poll(() =>
|
|
page.evaluate(() =>
|
|
window.app!.graph.nodes.map((node) => ({
|
|
type: node.type,
|
|
value: node.widgets?.[0]?.value
|
|
}))
|
|
)
|
|
)
|
|
.toEqual([
|
|
{
|
|
type: 'LoadImage',
|
|
value: sharedWorkflowImportScenario.inputFileName
|
|
}
|
|
])
|
|
await expectImportPrecedesPublicInclusiveInputAssetScan(
|
|
sharedWorkflowImportMocks
|
|
)
|
|
await expectNoMissingMediaAfterPublicInclusiveAssetScan(
|
|
comfyPage,
|
|
sharedWorkflowImportMocks
|
|
)
|
|
|
|
expect(sharedWorkflowImportMocks.getImportBody()).toEqual({
|
|
published_asset_ids: [sharedWorkflowImportScenario.publishedAssetId],
|
|
share_id: sharedWorkflowImportScenario.shareId
|
|
})
|
|
expect(new URL(page.url()).searchParams.has('share')).toBe(false)
|
|
await openPanelAndExpectNoMissingMedia(comfyPage)
|
|
})
|
|
})
|