mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-07-11 01:28:03 +00:00
## Summary Right-clicking a bypassed node showed two bypass-related items in the Vue "More Options" context menu (FE-720): - Plain `Bypass` from the legacy LiteGraph `getExtraMenuOptions` hook in `litegraphService.ts` - `Remove Bypass` (with `Ctrl+B` and an icon) from the Vue `getBypassOption` composable The Vue menu's exact-label deduplicator in `contextMenuConverter.ts` collapsed the unbypassed case (both emit `Bypass` → Vue source wins) but not the bypassed case (`Bypass` vs `Remove Bypass`), so the duplicate leaked through whenever the node was bypassed. ### before <img width="1920" height="958" alt="fe-720-before" src="https://github.com/user-attachments/assets/ef001aca-d70e-4798-ac61-01cc34c31e44" /> ### after <img width="1920" height="958" alt="fe-720-after" src="https://github.com/user-attachments/assets/d6d2bf4b-cb98-4b30-9dac-9bd4b68a7e36" /> #### single active node (KSampler) <img width="1920" height="958" alt="fe-720-1-unbypassed-node-menu" src="https://github.com/user-attachments/assets/bec9cd47-2f2d-4adb-b95b-266e7969a36c" /> #### single bypassed node (Load Checkpoint) <img width="1920" height="958" alt="fe-720-2-bypassed-node-menu" src="https://github.com/user-attachments/assets/91f80157-836d-4fce-adad-474f31baff04" /> #### KSampler + bypassed Load Checkpoint <img width="1920" height="958" alt="fe-720-3-mixed-selection-menu" src="https://github.com/user-attachments/assets/e4780b16-08e5-4f87-80e9-3ff65a5acdae" /> ## Root cause `src/services/litegraphService.ts` pushes a `Bypass` entry from its legacy `getExtraMenuOptions` hook in addition to the Vue `getBypassOption`. In Vue-menu mode both reach the menu; the exact-label dedup in `contextMenuConverter.ts` only collapses them when the labels match, which fails once the node is bypassed and the Vue side switches to `Remove Bypass`. ## Fix Add `Bypass` and `Remove Bypass` to the `HARD_BLACKLIST` in `contextMenuConverter.ts`. The blacklist filters the legacy emission out of the Vue conversion pipeline (`convertContextMenuToOptions`) before it is ever merged, so Vue's `getBypassOption` is the single source of the bypass item in every node state — no duplicate is created in the first place. This is the established convention for legacy items that the Vue menu replaces (`Properties`, `Colors`, `Shapes`, `Title`, `Mode`, `Properties Panel`, `Copy (Clipspace)`); Bypass is the same category. `litegraphService.ts` reverts to a plain `content: 'Bypass'` and no longer imports `areAllSelectedNodesInMode` or i18n keys for this entry. The Vue `getBypassOption` label is still derived from the same selection-aware predicate (`areAllSelectedNodesInMode`) that `toggleSelectedNodesMode` uses, so on mixed selections the label stays in sync with the action — it shows `Bypass` when clicking would bypass the rest, rather than `Remove Bypass`. **Trade-off:** the classic LiteGraph canvas menu (`Comfy.VueNodes.Enabled: false`) renders `litegraphService`'s options directly without going through `convertContextMenuToOptions`, so it shows a plain `Bypass` regardless of node state. This matches the pre-PR behavior (the legacy push was already a hardcoded `Bypass`), so it is not a regression. ## Considered and rejected - **`equivalents` map** (`bypass: ['bypass', 'remove bypass']`) — would collapse `Bypass` and `Remove Bypass` as synonyms, which is semantically wrong: they are distinct actions that must stay distinguishable, and the rule would also misfire on the unbypassed case. A converter test locks in that they are not treated as equivalents. - **State-aware label on the legacy push** (matching the Vue label so the exact-label dedup collapses them) — works, and additionally gives the classic canvas menu a state-aware label, but it couples `litegraphService` to the selection predicate and i18n keys solely to keep a downstream dedup load-bearing. `HARD_BLACKLIST` removes the duplicate at the source instead of creating, converting, then collapsing it. The only thing lost is the classic-menu state-aware label, which was never present pre-PR. - **Gating the legacy push on `Comfy.UseNewMenu === 'Disabled'`** — the setting that selects the legacy vs Vue context menu is `Comfy.VueNodes.Enabled`, not `Comfy.UseNewMenu` (an unrelated top-menu-bar toggle). Gating on `UseNewMenu` would drop the Bypass entry from the legacy canvas menu for the OSS default (`VueNodes.Enabled: false` + `UseNewMenu: 'Top'`). - **Suppressing the legacy callback via `SUPPRESSED_LITEGRAPH_CALLBACKS`** — matches by callback identity and adds cross-file coupling for what is a simple label-based filter that `HARD_BLACKLIST` already expresses. ## Cleanups (review feedback) - Removed the now-dead `NodeSelectionState.bypassed` field and its producer (no consumers after the label switch). - Replaced the `vue-i18n` mock in `useNodeMenuOptions.test.ts` with a real `createI18n` instance per `docs/testing/vitest-patterns.md`; removed a `ts-expect-error` via a typed hoisted `app` mock. - Simplified `getSelectedNodeArray` to `Object.values(app.canvas.selected_nodes ?? {})`. ## Tests - `useSelectedLiteGraphItems.test.ts` — `areAllSelectedNodesInMode`: all-bypassed → true, mixed → false, empty → false. - `useNodeMenuOptions.test.ts` — Vue label is `Bypass` (active / mixed) and `Remove Bypass` (all bypassed). - `contextMenuConverter.test.ts` — the legacy `Bypass` push is filtered by `HARD_BLACKLIST` so the Vue item is the only bypass entry (keeps shortcut/source); `Bypass` and `Remove Bypass` are not treated as label equivalents. - `browser_tests/tests/vueNodes/interactions/node/contextMenu.spec.ts` — e2e regression: exactly one bypass-family item per node state. Verified live on a bypassed Load Checkpoint: single `Remove Bypass` → toggle un-bypasses → single `Bypass`; no duplicate, rest of the menu intact. - Fixes FE-720 --------- Co-authored-by: Alexander Brown <drjkl@comfy.org>
84 lines
2.6 KiB
TypeScript
84 lines
2.6 KiB
TypeScript
import { expect } from '@playwright/test'
|
|
import type { Locator, Page } from '@playwright/test'
|
|
|
|
export class ContextMenu {
|
|
public readonly primeVueMenu: Locator
|
|
public readonly litegraphMenu: Locator
|
|
public readonly litegraphContextMenu: Locator
|
|
public readonly menuItems: Locator
|
|
protected readonly anyMenu: Locator
|
|
|
|
constructor(public readonly page: Page) {
|
|
this.primeVueMenu = page.locator('.p-contextmenu, .p-menu')
|
|
this.litegraphMenu = page.locator('.litemenu')
|
|
this.litegraphContextMenu = page.locator('.litecontextmenu')
|
|
this.menuItems = page.locator('.p-menuitem, .litemenu-entry')
|
|
this.anyMenu = this.primeVueMenu
|
|
.or(this.litegraphMenu)
|
|
.or(this.litegraphContextMenu)
|
|
}
|
|
|
|
async clickMenuItem(name: string): Promise<void> {
|
|
await this.page.getByRole('menuitem', { name }).click()
|
|
}
|
|
|
|
async clickMenuItemExact(name: string): Promise<void> {
|
|
await this.page.getByRole('menuitem', { name, exact: true }).click()
|
|
await this.waitForHidden()
|
|
}
|
|
|
|
menuItem(name: string): Locator {
|
|
return this.anyMenu.getByRole('menuitem', { name, exact: true })
|
|
}
|
|
|
|
/**
|
|
* Click a litegraph menu entry. Selects the most recently opened matching
|
|
* entry so nested submenu items can be reached without being shadowed by
|
|
* the parent menu still visible behind them.
|
|
*/
|
|
async clickLitegraphMenuItem(name: string): Promise<void> {
|
|
await this.page
|
|
.locator('.litemenu-entry:visible', { hasText: name })
|
|
.last()
|
|
.click()
|
|
}
|
|
|
|
async isVisible(): Promise<boolean> {
|
|
return await this.anyMenu.isVisible()
|
|
}
|
|
|
|
async assertHasItems(items: string[]): Promise<void> {
|
|
for (const item of items) {
|
|
await expect
|
|
.soft(this.page.getByRole('menuitem', { name: item }))
|
|
.toBeVisible()
|
|
}
|
|
}
|
|
|
|
async openFor(locator: Locator): Promise<this> {
|
|
await locator.click({ button: 'right' })
|
|
await expect(this.anyMenu).toBeVisible()
|
|
return this
|
|
}
|
|
|
|
/**
|
|
* Select a Vue node by clicking its header, then right-click to open
|
|
* the context menu. Vue nodes require a selection click before the
|
|
* right-click so the correct per-node menu items appear.
|
|
*/
|
|
async openForVueNode(header: Locator): Promise<this> {
|
|
await header.click()
|
|
await header.click({ button: 'right' })
|
|
await this.primeVueMenu.waitFor({ state: 'visible' })
|
|
return this
|
|
}
|
|
|
|
async waitForHidden(): Promise<void> {
|
|
await Promise.all([
|
|
this.primeVueMenu.waitFor({ state: 'hidden' }),
|
|
this.litegraphMenu.waitFor({ state: 'hidden' }),
|
|
this.litegraphContextMenu.waitFor({ state: 'hidden' })
|
|
])
|
|
}
|
|
}
|