mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-21 15:24:09 +00:00
fix: "Add Subgraph to Library" context menu not saving subgraph (#9056)
## Summary Fix "Add Subgraph to Library" context menu option which was bookmarking (UI favorite) instead of actually saving the subgraph as a reusable blueprint. ## Changes - **What**: Replace `nodeBookmarkStore.addBookmark()` with `subgraphStore.publishSubgraph()` in `addSubgraphToLibrary`, matching the working toolbar button behavior. Hide the menu option when multiple items are selected since `publishSubgraph` requires exactly one SubgraphNode. ## Review Focus The original implementation (PR #5218) used `addBookmark` which only adds a star/favorite — it never called the `publishSubgraph` function that serializes, prompts for a name, and saves the subgraph as a blueprint file. The toolbar button (`SaveToSubgraphLibrary.vue`) worked correctly because it calls the `Comfy.PublishSubgraph` command which uses `publishSubgraph`. The multi-select visibility guard (`!hasMultipleSelection`) matches `SaveToSubgraphLibrary.vue`'s `v-show` guard. "Unpack Subgraph" remains visible for multi-select since it handles multiple SubgraphNodes correctly. Fixes COM-15200 ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9056-fix-Add-Subgraph-to-Library-context-menu-not-saving-subgraph-30e6d73d36508177ba7ef97a2fe9b893) by [Unito](https://www.unito.io)
This commit is contained in:
@@ -100,7 +100,7 @@ describe('useSelectionMenuOptions - subgraph options', () => {
|
||||
expect(options[0]?.action).toBe(mocks.convertToSubgraph)
|
||||
})
|
||||
|
||||
it('includes convert, add to library, and unpack when subgraphs are selected', () => {
|
||||
it('includes convert and unpack but hides add to library when multiple items with subgraphs are selected', () => {
|
||||
const { getSubgraphOptions } = useSelectionMenuOptions()
|
||||
const options = getSubgraphOptions({
|
||||
hasSubgraphs: true,
|
||||
@@ -109,16 +109,11 @@ describe('useSelectionMenuOptions - subgraph options', () => {
|
||||
const labels = options.map((option) => option.label)
|
||||
|
||||
expect(labels).toContain('contextMenu.Convert to Subgraph')
|
||||
expect(labels).toContain('contextMenu.Add Subgraph to Library')
|
||||
expect(labels).not.toContain('contextMenu.Add Subgraph to Library')
|
||||
expect(labels).toContain('contextMenu.Unpack Subgraph')
|
||||
|
||||
const convertOption = options.find(
|
||||
(option) => option.label === 'contextMenu.Convert to Subgraph'
|
||||
)
|
||||
expect(convertOption?.action).toBe(mocks.convertToSubgraph)
|
||||
})
|
||||
|
||||
it('hides convert option when only a single subgraph is selected', () => {
|
||||
it('shows add to library and unpack when a single subgraph is selected', () => {
|
||||
const { getSubgraphOptions } = useSelectionMenuOptions()
|
||||
const options = getSubgraphOptions({
|
||||
hasSubgraphs: true,
|
||||
@@ -131,5 +126,10 @@ describe('useSelectionMenuOptions - subgraph options', () => {
|
||||
'contextMenu.Add Subgraph to Library',
|
||||
'contextMenu.Unpack Subgraph'
|
||||
])
|
||||
|
||||
const addToLibraryOption = options.find(
|
||||
(option) => option.label === 'contextMenu.Add Subgraph to Library'
|
||||
)
|
||||
expect(addToLibraryOption?.action).toBe(mocks.addSubgraphToLibrary)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -85,18 +85,18 @@ export function useSelectionMenuOptions() {
|
||||
}
|
||||
|
||||
if (hasSubgraphs) {
|
||||
options.push(
|
||||
{
|
||||
if (!hasMultipleSelection) {
|
||||
options.push({
|
||||
label: t('contextMenu.Add Subgraph to Library'),
|
||||
icon: 'icon-[lucide--folder-plus]',
|
||||
action: addSubgraphToLibrary
|
||||
},
|
||||
{
|
||||
label: t('contextMenu.Unpack Subgraph'),
|
||||
icon: 'icon-[lucide--expand]',
|
||||
action: unpackSubgraph
|
||||
}
|
||||
)
|
||||
})
|
||||
}
|
||||
options.push({
|
||||
label: t('contextMenu.Unpack Subgraph'),
|
||||
icon: 'icon-[lucide--expand]',
|
||||
action: unpackSubgraph
|
||||
})
|
||||
}
|
||||
|
||||
return options
|
||||
|
||||
90
src/composables/graph/useSubgraphOperations.test.ts
Normal file
90
src/composables/graph/useSubgraphOperations.test.ts
Normal file
@@ -0,0 +1,90 @@
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import { SubgraphNode } from '@/lib/litegraph/src/litegraph'
|
||||
|
||||
const mocks = vi.hoisted(() => ({
|
||||
publishSubgraph: vi.fn(),
|
||||
selectedItems: [] as unknown[]
|
||||
}))
|
||||
|
||||
vi.mock('@/composables/canvas/useSelectedLiteGraphItems', () => ({
|
||||
useSelectedLiteGraphItems: () => ({
|
||||
getSelectedNodes: vi.fn(() => [])
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('@/renderer/core/canvas/canvasStore', () => ({
|
||||
useCanvasStore: () => ({
|
||||
getCanvas: vi.fn(),
|
||||
get selectedItems() {
|
||||
return mocks.selectedItems
|
||||
},
|
||||
updateSelectedItems: vi.fn()
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('@/platform/workflow/management/stores/workflowStore', () => ({
|
||||
useWorkflowStore: () => ({
|
||||
activeWorkflow: null
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('@/stores/imagePreviewStore', () => ({
|
||||
useNodeOutputStore: () => ({
|
||||
revokeSubgraphPreviews: vi.fn()
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('@/stores/subgraphStore', () => ({
|
||||
useSubgraphStore: () => ({
|
||||
publishSubgraph: mocks.publishSubgraph
|
||||
})
|
||||
}))
|
||||
|
||||
function createSubgraphNode(): SubgraphNode {
|
||||
const node = Object.create(SubgraphNode.prototype)
|
||||
return node
|
||||
}
|
||||
|
||||
describe('useSubgraphOperations', () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
mocks.selectedItems = []
|
||||
})
|
||||
|
||||
it('addSubgraphToLibrary calls publishSubgraph when single SubgraphNode selected', async () => {
|
||||
mocks.selectedItems = [createSubgraphNode()]
|
||||
|
||||
const { useSubgraphOperations } =
|
||||
await import('@/composables/graph/useSubgraphOperations')
|
||||
const { addSubgraphToLibrary } = useSubgraphOperations()
|
||||
|
||||
await addSubgraphToLibrary()
|
||||
|
||||
expect(mocks.publishSubgraph).toHaveBeenCalledOnce()
|
||||
})
|
||||
|
||||
it('addSubgraphToLibrary does not call publishSubgraph when no items selected', async () => {
|
||||
mocks.selectedItems = []
|
||||
|
||||
const { useSubgraphOperations } =
|
||||
await import('@/composables/graph/useSubgraphOperations')
|
||||
const { addSubgraphToLibrary } = useSubgraphOperations()
|
||||
|
||||
await addSubgraphToLibrary()
|
||||
|
||||
expect(mocks.publishSubgraph).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('addSubgraphToLibrary does not call publishSubgraph when multiple items selected', async () => {
|
||||
mocks.selectedItems = [createSubgraphNode(), createSubgraphNode()]
|
||||
|
||||
const { useSubgraphOperations } =
|
||||
await import('@/composables/graph/useSubgraphOperations')
|
||||
const { addSubgraphToLibrary } = useSubgraphOperations()
|
||||
|
||||
await addSubgraphToLibrary()
|
||||
|
||||
expect(mocks.publishSubgraph).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
@@ -3,9 +3,7 @@ import { SubgraphNode } from '@/lib/litegraph/src/litegraph'
|
||||
import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore'
|
||||
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
|
||||
import { useNodeOutputStore } from '@/stores/imagePreviewStore'
|
||||
import { useNodeBookmarkStore } from '@/stores/nodeBookmarkStore'
|
||||
import { useNodeDefStore } from '@/stores/nodeDefStore'
|
||||
import { isLGraphNode } from '@/utils/litegraphUtil'
|
||||
import { useSubgraphStore } from '@/stores/subgraphStore'
|
||||
|
||||
/**
|
||||
* Composable for handling subgraph-related operations
|
||||
@@ -15,8 +13,7 @@ export function useSubgraphOperations() {
|
||||
const canvasStore = useCanvasStore()
|
||||
const workflowStore = useWorkflowStore()
|
||||
const nodeOutputStore = useNodeOutputStore()
|
||||
const nodeDefStore = useNodeDefStore()
|
||||
const nodeBookmarkStore = useNodeBookmarkStore()
|
||||
const subgraphStore = useSubgraphStore()
|
||||
|
||||
const convertToSubgraph = () => {
|
||||
const canvas = canvasStore.getCanvas()
|
||||
@@ -73,48 +70,13 @@ export function useSubgraphOperations() {
|
||||
|
||||
const addSubgraphToLibrary = async () => {
|
||||
const selectedItems = Array.from(canvasStore.selectedItems)
|
||||
|
||||
// Handle single node selection like BookmarkButton.vue
|
||||
if (selectedItems.length === 1) {
|
||||
const item = selectedItems[0]
|
||||
if (isLGraphNode(item)) {
|
||||
const nodeDef = nodeDefStore.fromLGraphNode(item)
|
||||
if (nodeDef) {
|
||||
await nodeBookmarkStore.addBookmark(nodeDef.nodePath)
|
||||
return
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Handle multiple nodes - convert to subgraph first then bookmark
|
||||
const selectedNodes = getSelectedNodes()
|
||||
|
||||
if (selectedNodes.length === 0) {
|
||||
return
|
||||
}
|
||||
|
||||
// Check if selection contains subgraph nodes
|
||||
const hasSubgraphs = selectedNodes.some(
|
||||
(node) => node instanceof SubgraphNode
|
||||
const subgraphNodes = selectedItems.filter(
|
||||
(item): item is SubgraphNode => item instanceof SubgraphNode
|
||||
)
|
||||
|
||||
if (!hasSubgraphs) {
|
||||
// Convert regular nodes to subgraph first
|
||||
convertToSubgraph()
|
||||
if (subgraphNodes.length !== 1) {
|
||||
return
|
||||
}
|
||||
|
||||
// For subgraph nodes, bookmark them
|
||||
let bookmarkedCount = 0
|
||||
for (const node of selectedNodes) {
|
||||
if (node instanceof SubgraphNode) {
|
||||
const nodeDef = nodeDefStore.fromLGraphNode(node)
|
||||
if (nodeDef) {
|
||||
await nodeBookmarkStore.addBookmark(nodeDef.nodePath)
|
||||
bookmarkedCount++
|
||||
}
|
||||
}
|
||||
}
|
||||
await subgraphStore.publishSubgraph()
|
||||
}
|
||||
|
||||
const isSubgraphSelected = (): boolean => {
|
||||
|
||||
Reference in New Issue
Block a user