Files
ComfyUI_frontend/browser_tests/AGENTS.md
Alexander Brown ab6c44aabf feat: remove deprecated group nodes, auto-convert to subgraphs on load (#12931)
## Summary

Removes the deprecated Group Nodes feature and replaces it with a
load-time migration that auto-converts any group nodes in a loaded
workflow into Subgraphs (with accepted lossiness).

## Changes

- **What**:
- `groupNode.ts` is now a migration-only extension.
`beforeConfigureGraph` registers temporary node types from
`extra.groupNodes` so instances are created during `configure`; a new
`afterConfigureGraph` hook converts every group node in the root graph
to a subgraph (via `LGraph.convertToSubgraph`), re-scanning until none
remain, then deletes `extra.groupNodes`. A failed conversion removes the
offending node so loading never hangs or breaks.
- Kept the minimum needed: `GroupNodeConfig` (builds the
input/output/widget maps), a slimmed `GroupNodeHandler` exposing a
rewritten `convertToNodes()` that no longer depends on the execution
DTOs, the `globalDefs`/`addCustomNodeDefs` path, and the `nodeDefStore`
`Object.assign` shim the migration relies on to detect group nodes.
- Deleted: the Manage Group Nodes dialog (`groupNodeManage.ts`/`.css`),
execution DTOs (`executableGroupNodeDto.ts`,
`executableGroupNodeChildDTO.ts`), the create/builder flow, recreate,
commands, keybindings, menus, the `isGroupNode` branches in the
right-side panel / error grouping / focus composable, the group-node
branches in node templates, dead i18n keys, and the now-unused
`serialise` clipboard helper.
- Rewrote `browser_tests/tests/groupNode.spec.ts` to assert
auto-conversion; deleted the `ManageGroupNode` page object and
`manageGroupNode()` helper.
  - Net: ~2,700 lines removed across 23 files (7 files deleted).
- **Breaking**: Group nodes can no longer be created, managed, or
executed. Existing workflows still load — their group nodes are
converted to subgraphs on open.

## Review Focus

- The load-time migration in `afterConfigureGraph` and the rewritten
`GroupNodeHandler.convertToNodes()` (no longer uses the execution
`getInnerNodes()` / DTOs; derives inner node type/index from
`groupData.nodeData.nodes` and relies on `deserialiseAndCreate` +
selection ordering).
- Kept `nodeDefStore`'s `Object.assign(this, obj)` shim: the migration
depends on it to propagate the group-node marker symbol onto the
registered node definition.

### Accepted lossiness
- Group nodes nested inside subgraphs (or inside other group nodes)
convert into the root graph rather than their original container —
essentially nonexistent in real legacy workflows since group nodes
predate subgraphs.
- Temporary `workflow>name` node types stay registered for the session;
instantiating one auto-converts it to a subgraph.

## Verification

`pnpm typecheck`, `typecheck:browser`, `knip`, `oxlint`, `eslint`, and
`oxfmt` are green (also enforced by pre-commit hooks). Unit tests for
the touched files could not be run locally due to a pre-existing
environment error (`file:///assets/images/*.svg` passed to a Node
filename API at import time, which also fails on unmodified test files);
the browser spec requires a live server.

---------

Co-authored-by: Amp <amp@ampcode.com>
Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: AustinMroz <austin@comfy.org>
Co-authored-by: GitHub Action <action@github.com>
2026-06-18 21:16:24 +00:00

6.4 KiB

E2E Testing Guidelines

See @docs/guidance/playwright.md for Playwright best practices (auto-loaded for *.spec.ts). See @browser_tests/FLAKE_PREVENTION_RULES.md when triaging or editing flaky browser tests.

Directory Structure

browser_tests/
├── assets/           - Test data (JSON workflows, images)
├── fixtures/
│   ├── ComfyPage.ts      - Main fixture (delegates to helpers)
│   ├── ComfyMouse.ts     - Mouse interaction helper
│   ├── VueNodeHelpers.ts - Vue Nodes 2.0 helpers
│   ├── selectors.ts      - Centralized TestIds
│   ├── data/             - Static test data (mock API responses, workflow JSONs, node definitions)
│   ├── components/       - Page object classes (locators, user interactions)
│   │   ├── Actionbar.ts
│   │   ├── ContextMenu.ts
│   │   ├── SettingDialog.ts
│   │   ├── SidebarTab.ts
│   │   ├── Templates.ts
│   │   ├── Topbar.ts
│   │   └── ...
│   ├── helpers/          - Focused helper classes (domain-specific actions)
│   │   ├── CanvasHelper.ts
│   │   ├── CommandHelper.ts
│   │   ├── KeyboardHelper.ts
│   │   ├── NodeOperationsHelper.ts
│   │   ├── SettingsHelper.ts
│   │   ├── WorkflowHelper.ts
│   │   └── ...
│   └── utils/            - Standalone utility functions (used by tests or fixtures)
│       ├── builderTestUtils.ts
│       ├── clipboardSpy.ts
│       ├── fitToView.ts
│       ├── perfReporter.ts
│       └── ...
└── tests/            - Test files (*.spec.ts)

Architectural Separation

  • fixtures/data/ — Static test data only. Mock API responses, workflow JSONs, node definitions. No code, no imports from Playwright.
  • fixtures/components/ — Page object components. Classes that own locators for a specific UI region (e.g. Actionbar, ContextMenu, SettingDialog).
  • fixtures/helpers/ — Helper classes that coordinate actions across multiple regions without owning a locator surface of their own (e.g. CanvasHelper, WorkflowHelper, NodeOperationsHelper).
  • fixtures/utils/ — Standalone utility functions. Exported functions (not classes) used by tests or fixtures (e.g. fitToView, clipboardSpy, builderTestUtils).

Placement Rule

When adding a new file, use this decision tree:

flowchart TD
    A[New file in browser_tests/fixtures/] --> B{Has any code?}
    B -- No, JSON/data only --> D[fixtures/data/]
    B -- Yes --> C{Is it a class?}
    C -- No, exported functions --> U[fixtures/utils/]
    C -- Yes --> E{Owns locators for a<br/>specific UI region?}
    E -- Yes --> P[fixtures/components/]
    E -- No, coordinates actions<br/>across the app --> H[fixtures/helpers/]

Page Object Locator Style

Define UI element locators as public readonly properties assigned in the constructor — not as getter methods. Getters that simply return a locator add unnecessary indirection and hide the object shape from IDE auto-complete.

// ✅ Correct — public readonly, assigned in constructor
export class MyDialog extends BaseDialog {
  public readonly submitButton: Locator
  public readonly cancelButton: Locator

  constructor(page: Page) {
    super(page)
    this.submitButton = this.root.getByRole('button', { name: 'Submit' })
    this.cancelButton = this.root.getByRole('button', { name: 'Cancel' })
  }
}

// ❌ Avoid — getter-based locators
export class MyDialog extends BaseDialog {
  get submitButton() {
    return this.root.getByRole('button', { name: 'Submit' })
  }
}

Keep as getters only when:

  • Lazy initialization is needed (this._tab ??= new Tab(this.page))
  • The value is computed from runtime state (e.g. get id() { return this.userIds[index] })
  • It's a private convenience accessor (e.g. private get page() { return this.comfyPage.page })

When a class has cached locator properties, prefer reusing them in methods rather than rebuilding locators from scratch.

Polling Assertions

Prefer expect.poll() over expect(async () => { ... }).toPass() when the block contains a single async call with a single assertion. expect.poll() is more readable and gives better error messages (shows actual vs expected on failure).

// ✅ Correct — single async call + single assertion
await expect
  .poll(() => comfyPage.nodeOps.getGraphNodesCount(), { timeout: 250 })
  .toBe(0)

// ❌ Avoid — nested expect inside toPass
await expect(async () => {
  expect(await comfyPage.nodeOps.getGraphNodesCount()).toBe(0)
}).toPass({ timeout: 250 })

Reserve toPass() for blocks with multiple assertions or complex async logic that can't be expressed as a single polled value.

Gotchas

Symptom Cause Fix
subtree intercepts pointer events on DOM widgets Canvas z-999 overlay intercepts click() Use Playwright's locator.dispatchEvent('contextmenu', { bubbles: true, cancelable: true, button: 2 })
Context menu empty or wrong items Node not selected Select node first: vueNodes.selectNode() or nodeRef.click('title')
navigateIntoSubgraph timeout Node too small in test asset JSON Use node size [400, 200] minimum

After Making Changes

  • Run pnpm typecheck:browser after modifying TypeScript files in this directory
  • Run pnpm exec eslint browser_tests/path/to/file.ts to lint specific files
  • Run pnpm exec oxlint browser_tests/path/to/file.ts to check with oxlint

Skill Documentation

A Playwright test-writing skill exists at .claude/skills/writing-playwright-tests/SKILL.md.

The skill documents meta-level guidance only (gotchas, anti-patterns, decision guides). It does not duplicate fixture APIs - agents should read the fixture code directly in browser_tests/fixtures/.