refactor: extract filename via lastIndexOf instead of split/pop

src/stores/modelStore.ts

@@ -69,7 +69,9 @@ export class ComfyModelDef {
     this.path_index = pathIndex
     this.file_name = name
     this.normalized_file_name = name.replaceAll('\\', '/')
-    this.simplified_file_name = this.normalized_file_name.split('/').pop() ?? ''
+    this.simplified_file_name = this.normalized_file_name.slice(
+      this.normalized_file_name.lastIndexOf('/') + 1
+    )
     if (this.simplified_file_name.endsWith('.safetensors')) {
       this.simplified_file_name = this.simplified_file_name.slice(
         0,

src/stores/nodeBookmarkStore.test.ts

@@ -0,0 +1,256 @@
+import { createPinia, setActivePinia } from 'pinia'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+import { useNodeBookmarkStore } from '@/stores/nodeBookmarkStore'
+import type { ComfyNodeDefImpl } from '@/stores/nodeDefStore'
+
+const BOOKMARK_ID = 'Comfy.NodeLibrary.Bookmarks.V2'
+const CUSTOMIZATION_ID = 'Comfy.NodeLibrary.BookmarksCustomization'
+
+const { settings, setSpy, nodeDefs } = vi.hoisted(() => ({
+  settings: {} as Record<string, unknown>,
+  setSpy: vi.fn(),
+  nodeDefs: {} as Record<string, unknown>
+}))
+
+vi.mock('@/platform/settings/settingStore', async () => {
+  const { reactive } = await import('vue')
+  const reactiveSettings = reactive(settings)
+  setSpy.mockImplementation(async (id: string, value: unknown) => {
+    reactiveSettings[id] = value
+  })
+  return {
+    useSettingStore: () => ({
+      get: (id: string) => reactiveSettings[id],
+      set: setSpy
+    })
+  }
+})
+
+vi.mock('@/stores/nodeDefStore', () => ({
+  useNodeDefStore: () => ({ allNodeDefsByName: nodeDefs }),
+  buildNodeDefTree: (defs: unknown[]) => ({ key: 'root', children: defs }),
+  createDummyFolderNodeDef: (path: string) => ({
+    isDummyFolder: true,
+    nodePath: path,
+    name: path
+  })
+}))
+
+type BookmarkNodeFixture = Pick<
+  ComfyNodeDefImpl,
+  'isDummyFolder' | 'nodePath' | 'category' | 'name'
+>
+
+function folderNode(nodePath: string) {
+  const node = {
+    isDummyFolder: true,
+    nodePath,
+    category: nodePath.replace(/\/$/, ''),
+    name: nodePath
+  } satisfies BookmarkNodeFixture
+  return node as ComfyNodeDefImpl
+}
+
+function leafNode(name: string, nodePath = name) {
+  const node = {
+    isDummyFolder: false,
+    name,
+    nodePath,
+    category: ''
+  } satisfies BookmarkNodeFixture
+  return node as ComfyNodeDefImpl
+}
+
+beforeEach(() => {
+  setActivePinia(createPinia())
+  for (const key of Object.keys(settings)) delete settings[key]
+  for (const key of Object.keys(nodeDefs)) delete nodeDefs[key]
+  settings[BOOKMARK_ID] = []
+  settings[CUSTOMIZATION_ID] = {}
+  setSpy.mockClear()
+})
+
+describe('nodeBookmarkStore', () => {
+  it('reports isBookmarked by either nodePath or top-level name', () => {
+    settings[BOOKMARK_ID] = ['sampling/KSampler', 'LoadImage']
+    const store = useNodeBookmarkStore()
+
+    expect(store.isBookmarked(leafNode('KSampler', 'sampling/KSampler'))).toBe(
+      true
+    )
+    expect(store.isBookmarked(leafNode('LoadImage'))).toBe(true)
+    expect(store.isBookmarked(leafNode('VAEDecode'))).toBe(false)
+  })
+
+  it('adds a bookmark by appending to the current list', async () => {
+    settings[BOOKMARK_ID] = ['A']
+    const store = useNodeBookmarkStore()
+
+    await store.addBookmark('B')
+
+    expect(setSpy).toHaveBeenCalledWith(BOOKMARK_ID, ['A', 'B'])
+  })
+
+  it('toggles an un-bookmarked node by adding its name', async () => {
+    const store = useNodeBookmarkStore()
+
+    await store.toggleBookmark(leafNode('KSampler'))
+
+    expect(setSpy).toHaveBeenCalledWith(BOOKMARK_ID, ['KSampler'])
+  })
+
+  it('toggles a bookmarked node by deleting both nodePath and name', async () => {
+    settings[BOOKMARK_ID] = ['sampling/KSampler', 'KSampler']
+    const store = useNodeBookmarkStore()
+
+    await store.toggleBookmark(leafNode('KSampler', 'sampling/KSampler'))
+
+    expect(setSpy).toHaveBeenCalledWith(BOOKMARK_ID, ['KSampler'])
+    expect(setSpy).toHaveBeenLastCalledWith(BOOKMARK_ID, [])
+    expect(store.bookmarks).toEqual([])
+  })
+
+  it('creates a folder under a parent and at the root', async () => {
+    const store = useNodeBookmarkStore()
+
+    const rootPath = await store.addNewBookmarkFolder(undefined, 'Favorites')
+    expect(rootPath).toBe('Favorites/')
+
+    const childPath = await store.addNewBookmarkFolder(
+      folderNode('Favorites/'),
+      'Nested'
+    )
+    expect(childPath).toBe('Favorites/Nested/')
+  })
+
+  it('parses each bookmark into its parent category, dropping unknown node defs', () => {
+    nodeDefs['LoadImage'] = leafNode('LoadImage')
+    nodeDefs['KSampler'] = leafNode('KSampler')
+    nodeDefs['Canny'] = leafNode('Canny')
+    settings[BOOKMARK_ID] = [
+      'LoadImage',
+      'sampling/KSampler',
+      'image/preprocessors/Canny',
+      'sampling/Unknown',
+      'Folder/'
+    ]
+    const store = useNodeBookmarkStore()
+
+    const children = (
+      store.bookmarkedRoot as unknown as { children: BookmarkNodeFixture[] }
+    ).children
+
+    expect(
+      children.map((node) =>
+        node.isDummyFolder ? node.nodePath : [node.name, node.category]
+      )
+    ).toEqual([
+      ['LoadImage', ''],
+      ['KSampler', 'sampling'],
+      ['Canny', 'image/preprocessors'],
+      'Folder/'
+    ])
+  })
+
+  describe('renameBookmarkFolder', () => {
+    it('rejects renaming a non-folder node', async () => {
+      const store = useNodeBookmarkStore()
+      await expect(
+        store.renameBookmarkFolder(leafNode('KSampler'), 'New')
+      ).rejects.toThrow('Cannot rename non-folder node')
+    })
+
+    it('rejects a name containing a slash', async () => {
+      const store = useNodeBookmarkStore()
+      await expect(
+        store.renameBookmarkFolder(folderNode('Old/'), 'a/b')
+      ).rejects.toThrow('cannot contain')
+    })
+
+    it('rejects a rename that collides with an existing folder', async () => {
+      settings[BOOKMARK_ID] = ['Taken/']
+      const store = useNodeBookmarkStore()
+      await expect(
+        store.renameBookmarkFolder(folderNode('Old/'), 'Taken')
+      ).rejects.toThrow('already exists')
+    })
+
+    it('rewrites matching bookmark paths on a valid rename', async () => {
+      settings[BOOKMARK_ID] = ['Old/', 'Old/KSampler', 'Other/Node']
+      const store = useNodeBookmarkStore()
+
+      await store.renameBookmarkFolder(folderNode('Old/'), 'New')
+
+      expect(setSpy).toHaveBeenCalledWith(BOOKMARK_ID, [
+        'New/',
+        'New/KSampler',
+        'Other/Node'
+      ])
+    })
+
+    it('does nothing when the folder keeps the same path', async () => {
+      const store = useNodeBookmarkStore()
+
+      await store.renameBookmarkFolder(folderNode('Old/'), 'Old')
+
+      expect(setSpy).not.toHaveBeenCalled()
+    })
+  })
+
+  it('deletes a folder and all its descendants', async () => {
+    settings[BOOKMARK_ID] = ['Old/', 'Old/KSampler', 'Keep/Node']
+    const store = useNodeBookmarkStore()
+
+    await store.deleteBookmarkFolder(folderNode('Old/'))
+
+    expect(setSpy).toHaveBeenCalledWith(BOOKMARK_ID, ['Keep/Node'])
+  })
+
+  it('rejects deleting a non-folder node', async () => {
+    const store = useNodeBookmarkStore()
+
+    await expect(
+      store.deleteBookmarkFolder(leafNode('KSampler'))
+    ).rejects.toThrow('Cannot delete non-folder node')
+  })
+
+  describe('updateBookmarkCustomization', () => {
+    it('persists a non-default customization', async () => {
+      const store = useNodeBookmarkStore()
+
+      await store.updateBookmarkCustomization('Folder/', {
+        color: '#ff0000',
+        icon: 'pi-star'
+      })
+
+      expect(setSpy).toHaveBeenCalledWith(CUSTOMIZATION_ID, {
+        'Folder/': { color: '#ff0000', icon: 'pi-star' }
+      })
+    })
+
+    it('drops attributes set to their default values', async () => {
+      const store = useNodeBookmarkStore()
+
+      await store.updateBookmarkCustomization('Folder/', {
+        color: store.defaultBookmarkColor,
+        icon: store.defaultBookmarkIcon
+      })
+
+      expect(setSpy).toHaveBeenCalledWith(CUSTOMIZATION_ID, {
+        'Folder/': undefined
+      })
+    })
+  })
+
+  it('renames a customization entry, moving the old key to the new one', async () => {
+    settings[CUSTOMIZATION_ID] = { 'Old/': { color: '#abc' } }
+    const store = useNodeBookmarkStore()
+
+    await store.renameBookmarkCustomization('Old/', 'New/')
+
+    expect(setSpy).toHaveBeenCalledWith(CUSTOMIZATION_ID, {
+      'New/': { color: '#abc' }
+    })
+  })
+})

src/stores/nodeBookmarkStore.ts

@@ -50,9 +50,9 @@ export const useNodeBookmarkStore = defineStore('nodeBookmark', () => {
       .map((bookmark: string) => {
         if (bookmark.endsWith('/')) return createDummyFolderNodeDef(bookmark)
 
-        const parts = bookmark.split('/')
-        const name = parts.pop() ?? ''
-        const category = parts.join('/')
+        const slashIndex = bookmark.lastIndexOf('/')
+        const name = bookmark.slice(slashIndex + 1)
+        const category = bookmark.slice(0, Math.max(0, slashIndex))
         const srcNodeDef = nodeDefStore.allNodeDefsByName[name]
         if (!srcNodeDef) {
           return null

src/stores/subgraphStore.test.ts

@@ -285,26 +285,6 @@ describe('useSubgraphStore', () => {
     consoleSpy.mockRestore()
   })
 
-  it('should reject blueprints without a root node', async () => {
-    const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
-    await mockFetch({
-      'empty-node.json': {
-        ...mockGraph,
-        nodes: []
-      }
-    })
-
-    const error = consoleSpy.mock.calls.find(
-      ([message]) => message === 'Failed to load subgraph blueprint'
-    )?.[1]
-    expect(error).toBeInstanceOf(TypeError)
-    expect((error as Error).message).toBe(
-      "Subgraph blueprint 'empty-node' must contain a root node"
-    )
-    expect(store.subgraphBlueprints).toHaveLength(0)
-    consoleSpy.mockRestore()
-  })
-
   it('should handle global blueprint with rejected data promise gracefully', async () => {
     const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
     await mockFetch(

src/stores/subgraphStore.ts

@@ -39,10 +39,6 @@ async function confirmOverwrite(name: string): Promise<boolean | null> {
   })
 }
 
-type ValidSubgraphWorkflowJSON = ComfyWorkflowJSON & {
-  definitions: NonNullable<ComfyWorkflowJSON['definitions']>
-}
-
 export const useSubgraphStore = defineStore('subgraph', () => {
   class SubgraphBlueprint extends ComfyWorkflow {
     static override readonly basePath = 'subgraphs/'
@@ -58,20 +54,18 @@ export const useSubgraphStore = defineStore('subgraph', () => {
       this.hasPromptedSave = !confirmFirstSave
     }
 
-    validateSubgraph(): ValidSubgraphWorkflowJSON {
-      const activeState = this.activeState
-      if (!activeState?.definitions)
+    validateSubgraph() {
+      if (!this.activeState?.definitions)
         throw new Error(
           'The root graph of a subgraph blueprint must consist of only a single subgraph node'
         )
-      const validState = activeState as ValidSubgraphWorkflowJSON
-      const { subgraphs } = validState.definitions
-      const { nodes } = validState
+      const { subgraphs } = this.activeState.definitions
+      const { nodes } = this.activeState
       //Instanceof doesn't function as nodes are serialized
       function isSubgraphNode(node: ComfyNode) {
         return node && subgraphs.some((s) => s.id === node.type)
       }
-      if (nodes.length == 1 && isSubgraphNode(nodes[0])) return validState
+      if (nodes.length == 1 && isSubgraphNode(nodes[0])) return
       const errors: Record<SerializedNodeId, NodeError> = {}
       //mark errors for all but first subgraph node
       let firstSubgraphFound = false
@@ -94,7 +88,7 @@ export const useSubgraphStore = defineStore('subgraph', () => {
     }
 
     override async save(): Promise<UserFile> {
-      const activeState = this.validateSubgraph()
+      this.validateSubgraph()
       if (
         !this.hasPromptedSave &&
         useSettingStore().get('Comfy.Workflow.WarnBlueprintOverwrite')
@@ -103,7 +97,7 @@ export const useSubgraphStore = defineStore('subgraph', () => {
         this.hasPromptedSave = true
       }
       // Extract metadata from subgraph.extra to workflow.extra before saving
-      this.extractMetadataToWorkflowExtra(activeState)
+      this.extractMetadataToWorkflowExtra()
       const ret = await super.save()
       // Force reload to update initialState with saved metadata
       registerNodeDef(await this.load({ force: true }), {
@@ -116,14 +110,13 @@ export const useSubgraphStore = defineStore('subgraph', () => {
      * Moves all properties (except workflowRendererVersion) from subgraph.extra
      * to workflow.extra, then removes from subgraph.extra to avoid duplication.
      */
-    private extractMetadataToWorkflowExtra(
-      activeState: ValidSubgraphWorkflowJSON
-    ): void {
-      const subgraph = activeState.definitions.subgraphs?.[0]
+    private extractMetadataToWorkflowExtra(): void {
+      if (!this.activeState) return
+      const subgraph = this.activeState.definitions?.subgraphs?.[0]
       if (!subgraph?.extra) return
 
       const sgExtra = subgraph.extra as Record<string, unknown>
-      const workflowExtra = (activeState.extra ??= {}) as Record<
+      const workflowExtra = (this.activeState.extra ??= {}) as Record<
         string,
         unknown
       >
@@ -136,10 +129,10 @@ export const useSubgraphStore = defineStore('subgraph', () => {
     }
 
     override async saveAs(path: string) {
-      const activeState = this.validateSubgraph()
+      this.validateSubgraph()
       this.hasPromptedSave = true
       // Extract metadata from subgraph.extra to workflow.extra before saving
-      this.extractMetadataToWorkflowExtra(activeState)
+      this.extractMetadataToWorkflowExtra()
       const ret = await super.saveAs(path)
       // Force reload to update initialState with saved metadata
       registerNodeDef(await this.load({ force: true }), {
@@ -153,20 +146,14 @@ export const useSubgraphStore = defineStore('subgraph', () => {
       if (!force && this.isLoaded) return await super.load({ force })
       const loaded = await super.load({ force })
       const st = loaded.activeState
-      const rootNode = st.nodes[0]
-      if (!rootNode) {
-        throw new TypeError(
-          `Subgraph blueprint '${this.filename}' must contain a root node`
-        )
-      }
       const sg = (st.definitions?.subgraphs ?? []).find(
-        (sg) => sg.id == rootNode.type
+        (sg) => sg.id == st.nodes[0].type
       )
       if (!sg)
         throw new Error(
           'Loaded subgraph blueprint does not contain valid subgraph'
         )
-      sg.name = rootNode.title = this.filename
+      sg.name = st.nodes[0].title = this.filename
 
       // Copy blueprint metadata from workflow extra to subgraph extra
       // so it's available when editing via canvas.subgraph.extra
@@ -290,11 +277,7 @@ export const useSubgraphStore = defineStore('subgraph', () => {
     name: string = workflow.filename
   ) {
     const subgraphNode = workflow.changeTracker.initialState.nodes[0]
-    if (!subgraphNode) {
-      throw new TypeError(
-        `Subgraph blueprint '${name}' must contain a root node`
-      )
-    }
+    if (!subgraphNode) throw new Error('Invalid Subgraph Blueprint')
     subgraphNode.inputs ??= []
     subgraphNode.outputs ??= []
     //NOTE: Types are cast to string. This is only used for input coloring on previews