mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-19 22:09:37 +00:00
Follow-up to #10856. Four correctness issues and their regression tests. ## Bugs fixed ### 1. ErrorOverlay model count reflected node selection `useErrorGroups` exposed `filteredMissingModelGroups` under the public name `missingModelGroups`. `ErrorOverlay.vue` read that alias to compute its model count label, so selecting a node shrank the overlay total. The overlay must always show the whole workflow's errors. Exposed both shapes explicitly: `missingModelGroups` / `missingMediaGroups` (unfiltered totals) and `filteredMissingModelGroups` / `filteredMissingMediaGroups` (selection-scoped). `TabErrors.vue` destructures the filtered variant with an alias. Before https://github.com/user-attachments/assets/eb848c5f-d092-4a4f-b86f-d22bb4408003 After https://github.com/user-attachments/assets/75e67819-c9f2-45ec-9241-74023eca6120 ### 2. Bypass → un-bypass dropped url/hash metadata Realtime `scanNodeModelCandidates` only reads widget values, so un-bypass produced a fresh candidate without the url that `enrichWithEmbeddedMetadata` had previously attached from `graphData.models`. `MissingModelRow`'s download/copy-url buttons disappeared after a bypass/un-bypass cycle. Added `enrichCandidateFromNodeProperties` that copies `url`/`hash`/`directory` from the node's own `properties.models` — which persists across mode toggles — into each scanned candidate. Applied to every call site of the per-node scan. A later fix in the same branch also enforces directory agreement to prevent a same-name / different-directory collision from stamping the wrong metadata. Before https://github.com/user-attachments/assets/39039d83-4d55-41a9-9d01-dec40843741b After https://github.com/user-attachments/assets/047a603b-fb52-4320-886d-dfeed457d833 ### 3. Initial full scan surfaced interior errors of a muted/bypassed subgraph container `scanAllModelCandidates`, `scanAllMediaCandidates`, and the JSON-based missing-node scan only check each node's own mode. Interior nodes whose parent container was bypassed passed the filter. Added `isAncestorPathActive(rootGraph, executionId)` to `graphTraversalUtil` and post-filter the three pipelines in `app.ts` after the live rootGraph is configured. The filter uses the execution-ID path (`"65:63"` → check node 65's mode) so it handles both live-scan-produced and JSON-enrichment-produced candidates. Before https://github.com/user-attachments/assets/3032d46b-81cd-420e-ab8e-f58392267602 After https://github.com/user-attachments/assets/02a01931-951d-4a48-986c-06424044fbf8 ### 4. Bypassed subgraph entry re-surfaced interior errors `useGraphNodeManager` replays `graph.onNodeAdded` for each existing interior node when the Vue node manager initializes on subgraph entry. That chain reached `scanSingleNodeErrors` via `installErrorClearingHooks`' `onNodeAdded` override. Each interior node's own mode was active, so the caller guards passed and the scan re-introduced the error that the initial pipeline had correctly suppressed. Added an ancestor-activity gate at the top of `scanSingleNodeErrors`, the single entry point shared by paste, un-bypass, subgraph entry, and subgraph container activation. A later commit also hardens this guard against detached nodes (null execution ID → skip) and applies the same ancestor check to `isCandidateStillActive` in the realtime verification callback. Before https://github.com/user-attachments/assets/fe44862d-f1d6-41ed-982d-614a7e83d441 After https://github.com/user-attachments/assets/497a76ce-3caa-479f-9024-4cd0f7bd20a4 ## Tests - 6 unit tests for `isAncestorPathActive` (root, active, immediate-bypass, deep-nested mute, unresolvable ancestor, null rootGraph) - 4 unit tests for `enrichCandidateFromNodeProperties` (enrichment, no-overwrite, name mismatch, directory mismatch) - 1 unit test for `scanSingleNodeErrors` ancestor guard (subgraph entry replaying onNodeAdded) - 2 unit tests for `useErrorGroups` dual export + ErrorOverlay contract - 4 E2E tests: - ErrorOverlay model count stays constant when a node is selected (new fixture `missing_models_distinct.json`) - Bypass/un-bypass cycle preserves Copy URL button (uses `missing_models_from_node_properties`) - Loading a workflow with bypassed subgraph suppresses interior missing model error (new fixture `missing_models_in_bypassed_subgraph.json`) - Entering a bypassed subgraph does not resurface interior missing model error (shares the above fixture) `pnpm typecheck`, `pnpm lint`, 206 related unit tests passing. ## Follow-up Several items raised by code review are deferred as pre-existing tech debt or scope-avoided refactors. Tracked via comments on #11215 and #11216. --- Follows up on #10856.
248 lines
7.9 KiB
TypeScript
248 lines
7.9 KiB
TypeScript
import type { Page } from '@playwright/test'
|
|
|
|
import {
|
|
comfyPageFixture as test,
|
|
comfyExpect as expect
|
|
} from '@e2e/fixtures/ComfyPage'
|
|
import { TestIds } from '@e2e/fixtures/selectors'
|
|
import { cleanupFakeModel } from '@e2e/tests/propertiesPanel/ErrorsTabHelper'
|
|
|
|
test.describe('Error overlay', { tag: '@ui' }, () => {
|
|
test.beforeEach(async ({ comfyPage }) => {
|
|
await comfyPage.settings.setSetting(
|
|
'Comfy.RightSidePanel.ShowErrorsTab',
|
|
true
|
|
)
|
|
})
|
|
|
|
function getOverlay(page: Page) {
|
|
return page.getByTestId(TestIds.dialogs.errorOverlay)
|
|
}
|
|
|
|
function getSeeErrorsButton(page: Page) {
|
|
return getOverlay(page).getByTestId(TestIds.dialogs.errorOverlaySeeErrors)
|
|
}
|
|
|
|
test.describe('Labels', () => {
|
|
test('Should display singular error count label for single error', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.workflow.loadWorkflow('missing/missing_nodes')
|
|
|
|
await expect(getOverlay(comfyPage.page)).toBeVisible()
|
|
await expect(getOverlay(comfyPage.page)).toContainText(/1 ERROR/i)
|
|
})
|
|
|
|
test('Should display "Show missing nodes" button for missing node errors', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.workflow.loadWorkflow('missing/missing_nodes')
|
|
|
|
await expect(getOverlay(comfyPage.page)).toBeVisible()
|
|
await expect(getSeeErrorsButton(comfyPage.page)).toContainText(
|
|
/Show missing nodes/i
|
|
)
|
|
})
|
|
|
|
test('Should display "Show missing models" button for missing model errors', async ({
|
|
comfyPage
|
|
}) => {
|
|
await cleanupFakeModel(comfyPage)
|
|
|
|
await comfyPage.workflow.loadWorkflow('missing/missing_models')
|
|
|
|
await expect(getOverlay(comfyPage.page)).toBeVisible()
|
|
await expect(getSeeErrorsButton(comfyPage.page)).toContainText(
|
|
/Show missing models/i
|
|
)
|
|
})
|
|
|
|
test('Should display "Show missing inputs" button for missing media errors', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.workflow.loadWorkflow('missing/missing_media_single')
|
|
|
|
await expect(getOverlay(comfyPage.page)).toBeVisible()
|
|
await expect(getSeeErrorsButton(comfyPage.page)).toContainText(
|
|
/Show missing inputs/i
|
|
)
|
|
})
|
|
|
|
test('Should display generic "See Errors" button for multiple error types', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.workflow.loadWorkflow('missing/missing_nodes_and_media')
|
|
|
|
await expect(getOverlay(comfyPage.page)).toBeVisible()
|
|
await expect(getSeeErrorsButton(comfyPage.page)).toContainText(
|
|
/See Errors/i
|
|
)
|
|
})
|
|
})
|
|
|
|
test.describe('Persistence', () => {
|
|
test('Does not resurface missing nodes on undo/redo', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.workflow.loadWorkflow('missing/missing_nodes')
|
|
|
|
const errorOverlay = getOverlay(comfyPage.page)
|
|
await expect(errorOverlay).toBeVisible()
|
|
await errorOverlay
|
|
.getByTestId(TestIds.dialogs.errorOverlayDismiss)
|
|
.click()
|
|
await expect(errorOverlay).toBeHidden()
|
|
|
|
await comfyPage.canvas.click()
|
|
await comfyPage.nextFrame()
|
|
await comfyPage.page.keyboard.press('Control+a')
|
|
await comfyPage.page.mouse.move(400, 300)
|
|
await comfyPage.page.mouse.down()
|
|
await comfyPage.page.mouse.move(450, 350, { steps: 5 })
|
|
await comfyPage.page.mouse.up()
|
|
await comfyPage.nextFrame()
|
|
|
|
await comfyPage.keyboard.undo()
|
|
await expect(errorOverlay).toBeHidden()
|
|
|
|
await comfyPage.keyboard.redo()
|
|
await expect(errorOverlay).toBeHidden()
|
|
})
|
|
|
|
test('Does not resurface error overlay when switching back to workflow with missing nodes', async ({
|
|
comfyPage
|
|
}) => {
|
|
await comfyPage.settings.setSetting(
|
|
'Comfy.Workflow.WorkflowTabsPosition',
|
|
'Sidebar'
|
|
)
|
|
await comfyPage.menu.workflowsTab.open()
|
|
|
|
await comfyPage.workflow.loadWorkflow('missing/missing_nodes')
|
|
|
|
const errorOverlay = getOverlay(comfyPage.page)
|
|
await expect(errorOverlay).toBeVisible()
|
|
|
|
await errorOverlay
|
|
.getByTestId(TestIds.dialogs.errorOverlayDismiss)
|
|
.click()
|
|
await expect(errorOverlay).toBeHidden()
|
|
|
|
await comfyPage.menu.workflowsTab.open()
|
|
await comfyPage.command.executeCommand('Comfy.NewBlankWorkflow')
|
|
|
|
await comfyPage.menu.workflowsTab.switchToWorkflow('missing_nodes')
|
|
|
|
await expect(errorOverlay).toBeHidden()
|
|
})
|
|
})
|
|
|
|
test.describe('See Errors flow', () => {
|
|
test.beforeEach(async ({ comfyPage }) => {
|
|
await comfyPage.setup()
|
|
})
|
|
|
|
async function triggerExecutionError(comfyPage: {
|
|
canvasOps: { disconnectEdge: () => Promise<void> }
|
|
page: Page
|
|
command: { executeCommand: (cmd: string) => Promise<void> }
|
|
}) {
|
|
await comfyPage.canvasOps.disconnectEdge()
|
|
await comfyPage.page.keyboard.press('Escape')
|
|
await comfyPage.command.executeCommand('Comfy.QueuePrompt')
|
|
}
|
|
|
|
test('Error overlay appears on execution error', async ({ comfyPage }) => {
|
|
await triggerExecutionError(comfyPage)
|
|
|
|
await expect(getOverlay(comfyPage.page)).toBeVisible()
|
|
})
|
|
|
|
test('Error overlay shows error message', async ({ comfyPage }) => {
|
|
await triggerExecutionError(comfyPage)
|
|
|
|
const overlay = getOverlay(comfyPage.page)
|
|
await expect(overlay).toBeVisible()
|
|
await expect(overlay).toHaveText(/\S/)
|
|
})
|
|
|
|
test('"See Errors" opens right side panel', async ({ comfyPage }) => {
|
|
await triggerExecutionError(comfyPage)
|
|
|
|
const overlay = getOverlay(comfyPage.page)
|
|
await expect(overlay).toBeVisible()
|
|
|
|
await overlay.getByTestId(TestIds.dialogs.errorOverlaySeeErrors).click()
|
|
|
|
await expect(overlay).toBeHidden()
|
|
await expect(comfyPage.page.getByTestId('properties-panel')).toBeVisible()
|
|
})
|
|
|
|
test('"See Errors" dismisses the overlay', async ({ comfyPage }) => {
|
|
await triggerExecutionError(comfyPage)
|
|
|
|
const overlay = getOverlay(comfyPage.page)
|
|
await expect(overlay).toBeVisible()
|
|
|
|
await overlay.getByTestId(TestIds.dialogs.errorOverlaySeeErrors).click()
|
|
|
|
await expect(overlay).toBeHidden()
|
|
})
|
|
|
|
test('"Dismiss" closes overlay without opening panel', async ({
|
|
comfyPage
|
|
}) => {
|
|
await triggerExecutionError(comfyPage)
|
|
|
|
const overlay = getOverlay(comfyPage.page)
|
|
await expect(overlay).toBeVisible()
|
|
|
|
await overlay.getByTestId(TestIds.dialogs.errorOverlayDismiss).click()
|
|
|
|
await expect(overlay).toBeHidden()
|
|
await expect(comfyPage.page.getByTestId('properties-panel')).toBeHidden()
|
|
})
|
|
|
|
test('Close button (X) dismisses overlay', async ({ comfyPage }) => {
|
|
await triggerExecutionError(comfyPage)
|
|
|
|
const overlay = getOverlay(comfyPage.page)
|
|
await expect(overlay).toBeVisible()
|
|
|
|
await overlay.getByRole('button', { name: /close/i }).click()
|
|
|
|
await expect(overlay).toBeHidden()
|
|
})
|
|
})
|
|
|
|
test.describe('Count independence from node selection', () => {
|
|
test.beforeEach(async ({ comfyPage }) => {
|
|
await cleanupFakeModel(comfyPage)
|
|
})
|
|
|
|
test.afterEach(async ({ comfyPage }) => {
|
|
await cleanupFakeModel(comfyPage)
|
|
})
|
|
|
|
test('missing model count stays constant when a node is selected', async ({
|
|
comfyPage
|
|
}) => {
|
|
// Regression: ErrorOverlay previously read the selection-filtered
|
|
// missingModelGroups from useErrorGroups, so selecting one of two
|
|
// missing-model nodes would shrink the overlay label from
|
|
// "2 required models are missing" to "1". The overlay must show
|
|
// the workflow total regardless of canvas selection.
|
|
await comfyPage.workflow.loadWorkflow('missing/missing_models_distinct')
|
|
|
|
const overlay = getOverlay(comfyPage.page)
|
|
await expect(overlay).toBeVisible()
|
|
await expect(overlay).toContainText(/2 required models are missing/i)
|
|
|
|
const node = await comfyPage.nodeOps.getNodeRefById('1')
|
|
await node.click('title')
|
|
|
|
await expect(overlay).toContainText(/2 required models are missing/i)
|
|
})
|
|
})
|
|
})
|