Fix: Search Box Implementation for keyboard shortcut (#5140)

* refactor: Move searchbox preference to the searchboxstore

* fix: Ensure that the search box uses the preferred implementation.

* polish: Open at current mouse location.

* [test] add basic unit tests for searchBoxStore

* types/testing: Tweak the types and setup for the searchBoxStore tests

---------

Co-authored-by: Arjan Singh <arjan@comfy.org>

src/components/graph/GraphCanvas.vue

@@ -32,7 +32,7 @@
   />
 
   <NodeTooltip v-if="tooltipEnabled" />
-  <NodeSearchboxPopover />
+  <NodeSearchboxPopover ref="nodeSearchboxPopoverRef" />
 
   <!-- Initialize components after comfyApp is ready. useAbsolutePosition requires
   canvasStore.canvas to be initialized. -->
@@ -47,7 +47,7 @@
 
 <script setup lang="ts">
 import { useEventListener, whenever } from '@vueuse/core'
-import { computed, onMounted, ref, watch, watchEffect } from 'vue'
+import { computed, onMounted, ref, shallowRef, watch, watchEffect } from 'vue'
 
 import LiteGraphCanvasSplitterOverlay from '@/components/LiteGraphCanvasSplitterOverlay.vue'
 import BottomPanel from '@/components/bottomPanel/BottomPanel.vue'
@@ -89,12 +89,16 @@ import { useSettingStore } from '@/stores/settingStore'
 import { useToastStore } from '@/stores/toastStore'
 import { useWorkflowStore } from '@/stores/workflowStore'
 import { useColorPaletteStore } from '@/stores/workspace/colorPaletteStore'
+import { useSearchBoxStore } from '@/stores/workspace/searchBoxStore'
 import { useWorkspaceStore } from '@/stores/workspaceStore'
 
 const emit = defineEmits<{
   ready: []
 }>()
 const canvasRef = ref<HTMLCanvasElement | null>(null)
+const nodeSearchboxPopoverRef = shallowRef<InstanceType<
+  typeof NodeSearchboxPopover
+> | null>(null)
 const settingStore = useSettingStore()
 const nodeDefStore = useNodeDefStore()
 const workspaceStore = useWorkspaceStore()
@@ -318,6 +322,7 @@ onMounted(async () => {
   canvasStore.canvas = comfyApp.canvas
   canvasStore.canvas.render_canvas_border = false
   workspaceStore.spinner = false
+  useSearchBoxStore().setPopoverRef(nodeSearchboxPopoverRef.value)
 
   window.app = comfyApp
   window.graph = comfyApp.graph

src/components/searchbox/NodeSearchBoxPopover.vue

@@ -61,9 +61,10 @@ let listenerController: AbortController | null = null
 let disconnectOnReset = false
 
 const settingStore = useSettingStore()
+const searchBoxStore = useSearchBoxStore()
 const litegraphService = useLitegraphService()
 
-const { visible } = storeToRefs(useSearchBoxStore())
+const { visible, newSearchBoxEnabled } = storeToRefs(searchBoxStore)
 const dismissable = ref(true)
 const getNewNodeLocation = (): Point => {
   return triggerEvent
@@ -107,12 +108,9 @@ const addNode = (nodeDef: ComfyNodeDefImpl) => {
   window.requestAnimationFrame(closeDialog)
 }
 
-const newSearchBoxEnabled = computed(
-  () => settingStore.get('Comfy.NodeSearchBoxImpl') === 'default'
-)
-const showSearchBox = (e: CanvasPointerEvent) => {
+const showSearchBox = (e: CanvasPointerEvent | null) => {
   if (newSearchBoxEnabled.value) {
-    if (e.pointerType === 'touch') {
+    if (e?.pointerType === 'touch') {
       setTimeout(() => {
         showNewSearchBox(e)
       }, 128)
@@ -128,7 +126,7 @@ const getFirstLink = () =>
   canvasStore.getCanvas().linkConnector.renderLinks.at(0)
 
 const nodeDefStore = useNodeDefStore()
-const showNewSearchBox = (e: CanvasPointerEvent) => {
+const showNewSearchBox = (e: CanvasPointerEvent | null) => {
   const firstLink = getFirstLink()
   if (firstLink) {
     const filter =
@@ -304,6 +302,7 @@ watch(visible, () => {
 })
 
 useEventListener(document, 'litegraph:canvas', canvasEventHandler)
+defineExpose({ showSearchBox })
 </script>
 
 <style>

src/lib/litegraph/src/LGraphCanvas.ts

@@ -135,7 +135,7 @@ interface ICreateDefaultNodeOptions extends ICreateNodeOptions {
 interface HasShowSearchCallback {
   /** See {@link LGraphCanvas.showSearchBox} */
   showSearchBox: (
-    event: MouseEvent,
+    event: MouseEvent | null,
     options?: IShowSearchOptions
   ) => HTMLDivElement | void
 }
@@ -6870,7 +6870,7 @@ export class LGraphCanvas
   }
 
   showSearchBox(
-    event: MouseEvent,
+    event: MouseEvent | null,
     searchOptions?: IShowSearchOptions
   ): HTMLDivElement {
     // proposed defaults
@@ -7105,14 +7105,25 @@ export class LGraphCanvas
     // compute best position
     const rect = canvas.getBoundingClientRect()
 
-    const left = (event ? event.clientX : rect.left + rect.width * 0.5) - 80
-    const top = (event ? event.clientY : rect.top + rect.height * 0.5) - 20
+    // Handles cases where the searchbox is initiated by
+    // non-click events. e.g. Keyboard shortcuts
+    const safeEvent =
+      event ??
+      new MouseEvent('click', {
+        clientX: rect.left + rect.width * 0.5,
+        clientY: rect.top + rect.height * 0.5,
+        // @ts-expect-error layerY is a nonstandard property
+        layerY: rect.top + rect.height * 0.5
+      })
+
+    const left = safeEvent.clientX - 80
+    const top = safeEvent.clientY - 20
     dialog.style.left = `${left}px`
     dialog.style.top = `${top}px`
 
     // To avoid out of screen problems
-    if (event.layerY > rect.height - 200) {
-      helper.style.maxHeight = `${rect.height - event.layerY - 20}px`
+    if (safeEvent.layerY > rect.height - 200) {
+      helper.style.maxHeight = `${rect.height - safeEvent.layerY - 20}px`
     }
     requestAnimationFrame(function () {
       input.focus()
@@ -7122,14 +7133,14 @@ export class LGraphCanvas
     function select(name: string) {
       if (name) {
         if (that.onSearchBoxSelection) {
-          that.onSearchBoxSelection(name, event, graphcanvas)
+          that.onSearchBoxSelection(name, safeEvent, graphcanvas)
         } else {
           if (!graphcanvas.graph) throw new NullGraphError()
 
           graphcanvas.graph.beforeChange()
           const node = LiteGraph.createNode(name)
           if (node) {
-            node.pos = graphcanvas.convertEventToCanvasOffset(event)
+            node.pos = graphcanvas.convertEventToCanvasOffset(safeEvent)
             graphcanvas.graph.add(node, false)
           }
 

src/stores/workspace/searchBoxStore.ts

@@ -1,14 +1,50 @@
+import { useMouse } from '@vueuse/core'
 import { defineStore } from 'pinia'
-import { ref } from 'vue'
+import { computed, ref, shallowRef } from 'vue'
+
+import type NodeSearchBoxPopover from '@/components/searchbox/NodeSearchBoxPopover.vue'
+import type { CanvasPointerEvent } from '@/lib/litegraph/src/litegraph'
+import { useSettingStore } from '@/stores/settingStore'
 
 export const useSearchBoxStore = defineStore('searchBox', () => {
+  const settingStore = useSettingStore()
+  const { x, y } = useMouse()
+
+  const newSearchBoxEnabled = computed(
+    () => settingStore.get('Comfy.NodeSearchBoxImpl') === 'default'
+  )
+
+  const popoverRef = shallowRef<InstanceType<
+    typeof NodeSearchBoxPopover
+  > | null>(null)
+
+  function setPopoverRef(
+    popover: InstanceType<typeof NodeSearchBoxPopover> | null
+  ) {
+    popoverRef.value = popover
+  }
+
   const visible = ref(false)
   function toggleVisible() {
-    visible.value = !visible.value
+    if (newSearchBoxEnabled.value) {
+      visible.value = !visible.value
+      return
+    }
+    if (!popoverRef.value) return
+    popoverRef.value.showSearchBox(
+      new MouseEvent('click', {
+        clientX: x.value,
+        clientY: y.value,
+        // @ts-expect-error layerY is a nonstandard property
+        layerY: y.value
+      }) as unknown as CanvasPointerEvent
+    )
   }
 
   return {
-    visible,
-    toggleVisible
+    newSearchBoxEnabled,
+    setPopoverRef,
+    toggleVisible,
+    visible
   }
 })

tests-ui/tests/store/searchBoxStore.test.ts

@@ -0,0 +1,137 @@
+import { createPinia, setActivePinia } from 'pinia'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+import type NodeSearchBoxPopover from '@/components/searchbox/NodeSearchBoxPopover.vue'
+import type { useSettingStore } from '@/stores/settingStore'
+import { useSearchBoxStore } from '@/stores/workspace/searchBoxStore'
+
+// Mock dependencies
+vi.mock('@vueuse/core', () => ({
+  useMouse: vi.fn(() => ({
+    x: { value: 100 },
+    y: { value: 200 }
+  }))
+}))
+
+const mockSettingStore = createMockSettingStore()
+vi.mock('@/stores/settingStore', () => ({
+  useSettingStore: vi.fn(() => mockSettingStore)
+}))
+
+function createMockPopover(): InstanceType<typeof NodeSearchBoxPopover> {
+  return { showSearchBox: vi.fn() } satisfies Partial<
+    InstanceType<typeof NodeSearchBoxPopover>
+  > as unknown as InstanceType<typeof NodeSearchBoxPopover>
+}
+
+function createMockSettingStore(): ReturnType<typeof useSettingStore> {
+  return {
+    get: vi.fn()
+  } satisfies Partial<
+    ReturnType<typeof useSettingStore>
+  > as unknown as ReturnType<typeof useSettingStore>
+}
+
+describe('useSearchBoxStore', () => {
+  beforeEach(() => {
+    setActivePinia(createPinia())
+
+    vi.restoreAllMocks()
+  })
+
+  describe('when user has new search box enabled', () => {
+    beforeEach(() => {
+      vi.mocked(mockSettingStore.get).mockReturnValue('default')
+    })
+
+    it('should show new search box is enabled', () => {
+      const store = useSearchBoxStore()
+      expect(store.newSearchBoxEnabled).toBe(true)
+    })
+
+    it('should toggle search box visibility when user presses shortcut', () => {
+      const store = useSearchBoxStore()
+
+      expect(store.visible).toBe(false)
+
+      store.toggleVisible()
+      expect(store.visible).toBe(true)
+
+      store.toggleVisible()
+      expect(store.visible).toBe(false)
+    })
+  })
+
+  describe('when user has legacy search box enabled', () => {
+    beforeEach(() => {
+      vi.mocked(mockSettingStore.get).mockReturnValue('legacy')
+    })
+
+    it('should show new search box is disabled', () => {
+      const store = useSearchBoxStore()
+      expect(store.newSearchBoxEnabled).toBe(false)
+    })
+
+    it('should open legacy search box at mouse position when user presses shortcut', () => {
+      const store = useSearchBoxStore()
+      const mockPopover = createMockPopover()
+      store.setPopoverRef(mockPopover)
+
+      expect(vi.mocked(store.visible)).toBe(false)
+
+      store.toggleVisible()
+
+      expect(vi.mocked(store.visible)).toBe(false) // Doesn't become visible in legacy mode.
+
+      expect(vi.mocked(mockPopover.showSearchBox)).toHaveBeenCalledWith(
+        expect.objectContaining({
+          clientX: 100,
+          clientY: 200
+        })
+      )
+    })
+
+    it('should do nothing when user presses shortcut but popover is not ready', () => {
+      const store = useSearchBoxStore()
+      store.setPopoverRef(null)
+
+      store.toggleVisible()
+
+      expect(store.visible).toBe(false)
+    })
+  })
+
+  describe('when user configures popover reference', () => {
+    beforeEach(() => {
+      vi.mocked(mockSettingStore.get).mockReturnValue('legacy')
+    })
+
+    it('should enable legacy search when popover is set', () => {
+      const store = useSearchBoxStore()
+      const mockPopover = createMockPopover()
+      store.setPopoverRef(mockPopover)
+
+      store.toggleVisible()
+
+      expect(vi.mocked(mockPopover.showSearchBox)).toHaveBeenCalled()
+    })
+
+    it('should disable legacy search when popover is cleared', () => {
+      const store = useSearchBoxStore()
+      const mockPopover = createMockPopover()
+      store.setPopoverRef(mockPopover)
+      store.setPopoverRef(null)
+
+      store.toggleVisible()
+
+      expect(vi.mocked(mockPopover.showSearchBox)).not.toHaveBeenCalled()
+    })
+  })
+
+  describe('when user first loads the application', () => {
+    it('should have search box hidden by default', () => {
+      const store = useSearchBoxStore()
+      expect(store.visible).toBe(false)
+    })
+  })
+})