mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-06-07 00:05:11 +00:00
60a4dc3001
## 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>