mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-06-29 02:57:18 +00:00
## Summary Fixes a Nodes 2.0 node replacement regression where widgets that only exist on the replacement node were not registered with the widget value store, causing their Vue-rendered controls to fall back to component defaults such as `0` instead of the replacement node's real widget default. The root cause is that `replaceWithMapping()` replaces the placeholder node in-place by writing directly to `graph._nodes` and `graph._nodes_by_id`. That path intentionally preserves the old node id, but it also bypasses the normal `LGraph.add()` flow that binds widgets to their owning node id. As a result, newly introduced bindable widgets on the replacement node could exist on the LiteGraph node object while remaining absent from `useWidgetValueStore`, which is the state Vue Nodes reads from when rendering widget controls. ## Changes - **What**: Bind every bindable widget on the replacement node to the reused node id inside `replaceWithMapping()` after the replacement node is inserted into the graph maps and before widget values are transferred. - **What**: Preserve the existing widget value transfer behavior for mapped widgets. Because widgets are now bound before `newWidget.value = oldValue` runs, transferred values are written through the normal widget store state instead of only mutating the unbound widget object. - **What**: Add a focused unit regression check that verifies replacement-only widgets are bound with the reused node id during node replacement. - **What**: Extend the existing node replacement Playwright coverage to assert the Vue Nodes rendered input for `KSampler.denoise` keeps the expected replacement value after the replacement flow. - **Breaking**: None. - **Dependencies**: None. ## Review Focus Please focus on the placement of the widget binding in `replaceWithMapping()`. The binding happens after the new node has been assigned the reused id and inserted into the graph's node maps, but before mapped widget values are copied over from the old node. This mirrors the important part of the normal graph add flow for widgets while keeping the in-place replacement behavior intact. The tests intentionally avoid asserting replacement-node fixture defaults in isolation. The unit test verifies the actual new side effect that prevents the regression: `setNodeId()` is called for a bindable widget that was not present on the old node. The Playwright assertion then covers the user-visible Nodes 2.0 symptom: the replacement widget is rendered from the widget store instead of falling back to the Vue numeric default. Linear: FE-1070 ## Validation - `pnpm vitest run src/platform/nodeReplacement/useNodeReplacement.test.ts` - `pnpm typecheck:browser` - `PLAYWRIGHT_LOCAL=1 PLAYWRIGHT_TEST_URL=http://localhost:5174 pnpm exec playwright test --project=chromium browser_tests/tests/nodeReplacement.spec.ts -g "Widget values are preserved after replacement"` - `pnpm lint` - `pnpm typecheck` - Commit hook also reran staged formatting/linting and `pnpm typecheck` during the final amend. ## Screenshots (if applicable) Before https://github.com/user-attachments/assets/dc4e8137-d8aa-4a70-9973-5559ed84b90e After https://github.com/user-attachments/assets/4c70b9e4-d971-4e94-8d2f-12b0f2b00a09
260 lines
8.6 KiB
TypeScript
260 lines
8.6 KiB
TypeScript
import {
|
|
comfyPageFixture as test,
|
|
comfyExpect as expect
|
|
} from '@e2e/fixtures/ComfyPage'
|
|
import {
|
|
mockNodeReplacements,
|
|
mockNodeReplacementsSingle
|
|
} from '@e2e/fixtures/data/nodeReplacements'
|
|
import { loadWorkflowAndOpenErrorsTab } from '@e2e/fixtures/helpers/ErrorsTabHelper'
|
|
import {
|
|
getSwapNodesGroup,
|
|
setupNodeReplacement
|
|
} from '@e2e/fixtures/helpers/NodeReplacementHelper'
|
|
import { TestIds } from '@e2e/fixtures/selectors'
|
|
|
|
const renderModes = [
|
|
{ name: 'vue nodes', vueNodesEnabled: true },
|
|
{ name: 'litegraph', vueNodesEnabled: false }
|
|
] as const
|
|
|
|
test.describe('Node replacement', { tag: ['@node', '@ui'] }, () => {
|
|
for (const mode of renderModes) {
|
|
test.describe(`(${mode.name})`, () => {
|
|
test.describe('Single replacement', () => {
|
|
test.beforeEach(async ({ comfyPage }) => {
|
|
await comfyPage.settings.setSetting(
|
|
'Comfy.VueNodes.Enabled',
|
|
mode.vueNodesEnabled
|
|
)
|
|
await setupNodeReplacement(comfyPage, mockNodeReplacementsSingle)
|
|
await loadWorkflowAndOpenErrorsTab(
|
|
comfyPage,
|
|
'missing/node_replacement_simple'
|
|
)
|
|
})
|
|
|
|
test('Swap Nodes group appears in errors tab for replaceable nodes', async ({
|
|
comfyPage
|
|
}) => {
|
|
const swapGroup = getSwapNodesGroup(comfyPage.page)
|
|
await expect(swapGroup).toBeVisible()
|
|
await expect(
|
|
swapGroup.getByTestId(TestIds.dialogs.errorGroupDisplayMessage)
|
|
).toHaveText(/\S/)
|
|
await expect(swapGroup).toContainText('E2E_OldSampler')
|
|
await expect(
|
|
swapGroup.getByRole('button', { name: 'Replace All', exact: true })
|
|
).toBeVisible()
|
|
})
|
|
|
|
test('Shows direct row label and locate action for a single replacement group', async ({
|
|
comfyPage
|
|
}) => {
|
|
const swapGroup = getSwapNodesGroup(comfyPage.page)
|
|
const rowLabel = swapGroup.getByRole('button', {
|
|
name: 'E2E_OldSampler',
|
|
exact: true
|
|
})
|
|
|
|
await expect(rowLabel).toBeVisible()
|
|
await expect(
|
|
swapGroup.getByRole('button', {
|
|
name: 'Locate node on canvas',
|
|
exact: true
|
|
})
|
|
).toBeVisible()
|
|
await expect(
|
|
swapGroup.getByTestId(TestIds.dialogs.swapNodeGroupCount)
|
|
).toHaveCount(0)
|
|
|
|
await comfyPage.canvasOps.pan({ x: -800, y: -800 })
|
|
const offsetBeforeLocate = await comfyPage.canvasOps.getOffset()
|
|
|
|
await rowLabel.click()
|
|
|
|
await expect
|
|
.poll(() => comfyPage.canvasOps.getOffset())
|
|
.not.toEqual(offsetBeforeLocate)
|
|
})
|
|
|
|
test('Replace Node replaces a single group in-place', async ({
|
|
comfyPage
|
|
}) => {
|
|
const swapGroup = getSwapNodesGroup(comfyPage.page)
|
|
await swapGroup.getByRole('button', { name: /replace node/i }).click()
|
|
await expect(swapGroup).toBeHidden()
|
|
|
|
const workflow = await comfyPage.workflow.getExportedWorkflow()
|
|
expect(
|
|
workflow.nodes,
|
|
'Node count should be unchanged after in-place replacement'
|
|
).toHaveLength(2)
|
|
|
|
const nodeTypes = workflow.nodes.map((n) => n.type)
|
|
expect(nodeTypes).not.toContain('E2E_OldSampler')
|
|
expect(nodeTypes).toContain('KSampler')
|
|
|
|
const ksampler = workflow.nodes.find((n) => n.type === 'KSampler')
|
|
expect(
|
|
ksampler?.id,
|
|
'Replaced node should keep the original id'
|
|
).toBe(1)
|
|
|
|
const linkFromReplacedToDecode = workflow.links?.find(
|
|
(l) => l[1] === 1 && l[3] === 2
|
|
)
|
|
expect(
|
|
linkFromReplacedToDecode,
|
|
'Output link from replaced node to VAEDecode should be preserved'
|
|
).toBeDefined()
|
|
})
|
|
|
|
test('Widget values are preserved after replacement', async ({
|
|
comfyPage
|
|
}) => {
|
|
await getSwapNodesGroup(comfyPage.page)
|
|
.getByRole('button', { name: /replace node/i })
|
|
.click()
|
|
|
|
const workflow = await comfyPage.workflow.getExportedWorkflow()
|
|
const ksampler = workflow.nodes.find((n) => n.type === 'KSampler')
|
|
|
|
expect(ksampler?.widgets_values).toBeDefined()
|
|
const widgetValues = ksampler!.widgets_values as unknown[]
|
|
expect(widgetValues).toEqual([
|
|
42,
|
|
'randomize',
|
|
20,
|
|
7,
|
|
'euler',
|
|
'normal',
|
|
1
|
|
])
|
|
|
|
if (mode.vueNodesEnabled) {
|
|
await expect(
|
|
comfyPage.vueNodes
|
|
.getWidgetByName('KSampler', 'denoise')
|
|
.locator('input')
|
|
).toHaveValue(/^1(?:\.0+)?$/)
|
|
}
|
|
})
|
|
|
|
test('Success toast is shown after replacement', async ({
|
|
comfyPage
|
|
}) => {
|
|
await getSwapNodesGroup(comfyPage.page)
|
|
.getByRole('button', { name: /replace node/i })
|
|
.click()
|
|
|
|
await expect(comfyPage.visibleToasts.first()).toContainText(
|
|
/replaced|swapped/i
|
|
)
|
|
})
|
|
})
|
|
|
|
test.describe('Same-type replacement group', () => {
|
|
test.beforeEach(async ({ comfyPage }) => {
|
|
await comfyPage.settings.setSetting(
|
|
'Comfy.VueNodes.Enabled',
|
|
mode.vueNodesEnabled
|
|
)
|
|
await setupNodeReplacement(comfyPage, mockNodeReplacementsSingle)
|
|
await loadWorkflowAndOpenErrorsTab(
|
|
comfyPage,
|
|
'missing/node_replacement_same_type'
|
|
)
|
|
})
|
|
|
|
test('Groups same-type replacement rows behind the title disclosure', async ({
|
|
comfyPage
|
|
}) => {
|
|
const swapGroup = getSwapNodesGroup(comfyPage.page)
|
|
const countBadge = swapGroup.getByTestId(
|
|
TestIds.dialogs.swapNodeGroupCount
|
|
)
|
|
const childRows = swapGroup.getByRole('listitem')
|
|
const expandButton = swapGroup.getByRole('button', {
|
|
name: 'Expand E2E_OldSampler',
|
|
exact: true
|
|
})
|
|
|
|
await expect(expandButton).toBeVisible()
|
|
await expect(countBadge).toHaveText('2')
|
|
await expect(childRows).toHaveCount(0)
|
|
|
|
await expandButton.click()
|
|
await expect(childRows).toHaveCount(2)
|
|
await expect(
|
|
swapGroup.getByRole('button', {
|
|
name: 'E2E_OldSampler',
|
|
exact: true
|
|
})
|
|
).toHaveCount(2)
|
|
|
|
await swapGroup
|
|
.getByRole('button', {
|
|
name: 'Collapse E2E_OldSampler',
|
|
exact: true
|
|
})
|
|
.click()
|
|
await expect(childRows).toHaveCount(0)
|
|
})
|
|
})
|
|
|
|
test.describe('Multi-type replacement', () => {
|
|
test.beforeEach(async ({ comfyPage }) => {
|
|
await comfyPage.settings.setSetting(
|
|
'Comfy.VueNodes.Enabled',
|
|
mode.vueNodesEnabled
|
|
)
|
|
await setupNodeReplacement(comfyPage, mockNodeReplacements)
|
|
await loadWorkflowAndOpenErrorsTab(
|
|
comfyPage,
|
|
'missing/node_replacement_multi'
|
|
)
|
|
})
|
|
|
|
test('Replace All replaces all groups across multiple types', async ({
|
|
comfyPage
|
|
}) => {
|
|
const swapGroup = getSwapNodesGroup(comfyPage.page)
|
|
await expect(swapGroup).toBeVisible()
|
|
await expect(swapGroup).toContainText('E2E_OldSampler')
|
|
await expect(swapGroup).toContainText('E2E_OldUpscaler')
|
|
|
|
await swapGroup
|
|
.getByRole('button', { name: 'Replace All', exact: true })
|
|
.click()
|
|
await expect(swapGroup).toBeHidden()
|
|
|
|
const workflow = await comfyPage.workflow.getExportedWorkflow()
|
|
const nodeTypes = workflow.nodes.map((n) => n.type)
|
|
expect(nodeTypes).not.toContain('E2E_OldSampler')
|
|
expect(nodeTypes).not.toContain('E2E_OldUpscaler')
|
|
expect(nodeTypes).toContain('KSampler')
|
|
expect(nodeTypes).toContain('ImageScaleBy')
|
|
})
|
|
|
|
test('Output connections are preserved across replacement with output mapping', async ({
|
|
comfyPage
|
|
}) => {
|
|
await getSwapNodesGroup(comfyPage.page)
|
|
.getByRole('button', { name: 'Replace All', exact: true })
|
|
.click()
|
|
|
|
const replacedNodeOutputLinkCount = await comfyPage.page.evaluate(
|
|
() =>
|
|
window.app!.graph!.getNodeById(2)?.outputs[0]?.links?.length ?? 0
|
|
)
|
|
expect(
|
|
replacedNodeOutputLinkCount,
|
|
'Replaced upscaler should still drive its downstream consumer'
|
|
).toBeGreaterThan(0)
|
|
})
|
|
})
|
|
})
|
|
}
|
|
})
|