[fix] code review feedback

Author: arjansingh

src/components/dialog/GlobalDialog.vue

@@ -3,10 +3,19 @@
   <Dialog
     v-for="item in dialogStore.dialogStack"
     :key="item.key"
+    :ref="(el) => setDialogRef(item.key, el)"
     v-model:visible="item.visible"
     class="global-dialog"
     v-bind="item.dialogComponentProps"
-    :pt="item.dialogComponentProps.pt"
+    :pt="{
+      ...item.dialogComponentProps.pt,
+      transition: {
+        enterFromClass: 'opacity-0 scale-75',
+        enterActiveClass: 'transition-all duration-200 ease-out',
+        leaveActiveClass: 'transition-all duration-200 ease-in',
+        leaveToClass: 'opacity-0 scale-75'
+      }
+    }"
     :aria-labelledby="item.key"
   >
     <template #header>
@@ -36,10 +45,40 @@
 
 <script setup lang="ts">
 import Dialog from 'primevue/dialog'
+import { onMounted, ref } from 'vue'
 
 import { useDialogStore } from '@/stores/dialogStore'
 
 const dialogStore = useDialogStore()
+
+// Store refs to Dialog components so we can call their close() method
+const dialogRefs = ref<Record<string, any>>({})
+
+function setDialogRef(dialogKey: string, el: any) {
+  if (el) {
+    dialogRefs.value[dialogKey] = el
+  } else {
+    delete dialogRefs.value[dialogKey]
+  }
+}
+
+// Expose method for dialogStore to trigger proper close
+function triggerDialogClose(dialogKey: string) {
+  const dialogRef = dialogRefs.value[dialogKey]
+  if (!dialogRef || typeof dialogRef.close !== 'function') return
+
+  dialogRef.close()
+}
+
+// Register with dialogStore so it can call Dialog.close() directly
+onMounted(() => {
+  dialogStore.registerGlobalDialogCloseFn(triggerDialogClose)
+})
+
+// Make the function available to the dialog store
+defineExpose({
+  triggerDialogClose
+})
 </script>
 
 <style>

src/lib/litegraph/src/litegraph.ts

@@ -140,7 +140,7 @@ export { BaseWidget } from './widgets/BaseWidget'
 
 export { LegacyWidget } from './widgets/LegacyWidget'
 
-export { isComboWidget } from './widgets/widgetMap'
+export { isComboWidget, isAssetWidget } from './widgets/widgetMap'
 // Additional test-specific exports
 export { LGraphButton } from './LGraphButton'
 export { MovingOutputLink } from './canvas/MovingOutputLink'

src/lib/litegraph/src/widgets/AssetWidget.ts

@@ -13,6 +13,22 @@ export class AssetWidget
     this.value = widget.value?.toString() ?? ''
   }
 
+  override set value(value: IAssetWidget['value']) {
+    const oldValue = this.value
+    super.value = value
+
+    // Force canvas redraw when value changes to show update immediately
+    if (oldValue !== value && this.node.graph?.list_of_graphcanvas) {
+      for (const canvas of this.node.graph.list_of_graphcanvas) {
+        canvas.setDirty(true)
+      }
+    }
+  }
+
+  override get value(): IAssetWidget['value'] {
+    return super.value
+  }
+
   override get _displayValue(): string {
     return String(this.value) //FIXME: Resolve asset name
   }

src/lib/litegraph/src/widgets/widgetMap.ts

@@ -1,5 +1,6 @@
 import type { LGraphNode } from '@/lib/litegraph/src/LGraphNode'
 import type {
+  IAssetWidget,
   IBaseWidget,
   IComboWidget,
   IWidget,
@@ -132,4 +133,9 @@ export function isComboWidget(widget: IBaseWidget): widget is IComboWidget {
   return widget.type === 'combo'
 }
 
+/** Type guard: Narrow **from {@link IBaseWidget}** to {@link IAssetWidget}. */
+export function isAssetWidget(widget: IBaseWidget): widget is IAssetWidget {
+  return widget.type === 'asset'
+}
+
 // #endregion Type Guards

src/platform/assets/components/AssetBrowserModal.vue

@@ -12,7 +12,7 @@
         :nav-items="availableCategories"
       >
         <template #header-icon>
-          <div :class="cn('icon-[lucide--folder]', 'size-4')" />
+          <div class="icon-[lucide--folder] size-4" />
         </template>
         <template #header-title>{{ $t('assetBrowser.browseAssets') }}</template>
       </LeftSidePanel>
@@ -47,7 +47,6 @@ import type { AssetDisplayItem } from '@/platform/assets/composables/useAssetBro
 import { useAssetBrowser } from '@/platform/assets/composables/useAssetBrowser'
 import type { AssetItem } from '@/platform/assets/schemas/assetSchema'
 import { OnCloseKey } from '@/types/widgetTypes'
-import { cn } from '@/utils/tailwindUtil'
 
 const props = defineProps<{
   nodeType?: string
@@ -63,10 +62,8 @@ const emit = defineEmits<{
   close: []
 }>()
 
-// Provide the close function for BaseModalLayout to inject
-provide(OnCloseKey, props.onClose || (() => {}))
+provide(OnCloseKey, props.onClose ?? (() => {}))
 
-// Use AssetBrowser composable for all business logic
 const {
   searchQuery,
   selectedCategory,
@@ -76,22 +73,17 @@ const {
   selectAssetWithCallback
 } = useAssetBrowser(props.assets)
 
-// Dialog controls panel visibility via prop
 const shouldShowLeftPanel = computed(() => {
   return props.showLeftPanel ?? true
 })
 
-// Handle close button - call both the prop callback and emit the event
 const handleClose = () => {
   props.onClose?.()
   emit('close')
 }
 
-// Handle asset selection and emit to parent
 const handleAssetSelectAndEmit = async (asset: AssetDisplayItem) => {
-  emit('asset-select', asset) // Emit the full asset object
-
-  // Use composable for detail fetching and callback execution
+  emit('asset-select', asset)
   await selectAssetWithCallback(asset.id, props.onSelect)
 }
 </script>

src/platform/assets/composables/useAssetBrowser.ts

@@ -172,24 +172,17 @@ export function useAssetBrowser(assets: AssetItem[] = []) {
     assetId: string,
     onSelect?: (filename: string) => void
   ): Promise<void> {
-    // Always log selection for debugging
     if (import.meta.env.DEV) {
-      console.log('Asset selected:', assetId)
+      console.debug('Asset selected:', assetId)
     }
 
-    // If no callback provided, just return (no need to fetch details)
     if (!onSelect) {
       return
     }
 
     try {
-      // Fetch complete asset details to get user_metadata
       const detailAsset = await assetService.getAssetDetails(assetId)
-
-      // Extract filename from user_metadata
       const filename = detailAsset.user_metadata?.filename
-
-      // Validate filename using Zod schema
       const validatedFilename = assetFilenameSchema.safeParse(filename)
       if (!validatedFilename.success) {
         console.error(
@@ -201,7 +194,6 @@ export function useAssetBrowser(assets: AssetItem[] = []) {
         return
       }
 
-      // Execute callback with validated filename
       onSelect(validatedFilename.data)
     } catch (error) {
       console.error(`Failed to fetch asset details for ${assetId}:`, error)

src/platform/assets/composables/useAssetBrowserDialog.ts

@@ -1,7 +1,10 @@
+import { useRafFn } from '@vueuse/core'
+import { nextTick } from 'vue'
+
 import AssetBrowserModal from '@/platform/assets/components/AssetBrowserModal.vue'
 import type { AssetItem } from '@/platform/assets/schemas/assetSchema'
 import { assetService } from '@/platform/assets/services/assetService'
-import { useDialogStore } from '@/stores/dialogStore'
+import { type DialogComponentProps, useDialogStore } from '@/stores/dialogStore'
 
 interface AssetBrowserDialogProps {
   /** ComfyUI node type for context (e.g., 'CheckpointLoaderSimple') */
@@ -22,6 +25,42 @@ export const useAssetBrowserDialog = () => {
   const dialogKey = 'global-asset-browser'
   let onHideComplete: (() => void) | null = null
 
+  /**
+   * Why we need Promise coordination with PrimeVue's onAfterHide:
+   *
+   * PrimeVue Dialog uses Vue's <transition> with specific animation lifecycle hooks.
+   * Source: https://github.com/primefaces/primevue/blob/72590856123052fa798b185dc2c88928a325258f/packages/primevue/src/dialog/Dialog.vue#L3-L4
+   *
+   * The animation flow is:
+   * 1. visible=false triggers @leave event
+   *    https://github.com/primefaces/primevue/blob/72590856123052fa798b185dc2c88928a325258f/packages/primevue/src/dialog/Dialog.vue#L162-L170
+   * 2. CSS animation runs (~300ms)
+   * 3. @after-leave fires, emitting 'after-hide'
+   *    https://github.com/primefaces/primevue/blob/72590856123052fa798b185dc2c88928a325258f/packages/primevue/src/dialog/Dialog.vue#L171-L180
+   *
+   * Why Vue/VueUse lifecycle hooks cannot manage this:
+   *
+   * - Vue's onBeforeUnmount/onUnmounted track COMPONENT lifecycle (mounting/unmounting)
+   *   Source: https://github.com/vuejs/core/blob/928af5fe2f5f366b5c28b8549c3728735c8d8318/packages/runtime-core/src/apiLifecycle.ts#L91-94
+   *   These only fire when a component is being destroyed, not during visibility changes
+   *
+   * - VueUse's tryOnUnmounted only fires when component unmounts
+   *   Source: https://github.com/vueuse/vueuse/blob/cf905ccfb5dd7a6a3e65aa087a034b5157c9f9fb/packages/shared/tryOnUnmounted/index.ts
+   *   It checks getLifeCycleTarget() and registers onUnmounted, but component remains mounted during animation
+   *
+   * - The Dialog component remains mounted during animation - only visibility changes
+   * - CSS transitions are managed by the browser, not Vue's reactivity system
+   * - Only PrimeVue's onAfterHide callback knows when the CSS animation completes
+   *
+   * Without Promise coordination, calling hide() synchronously after widget update
+   * causes both operations in the same event tick, preventing proper animation.
+   *
+   * Why nextTick() doesn't solve this:
+   * - nextTick() only defers to the next microtask, not the next animation frame
+   * - PrimeVue's animation timing depends on CSS transition duration (~300ms)
+   * - We need to wait for the actual animation completion, not just DOM updates
+   * - Only onAfterHide callback provides the correct timing signal
+   */
   function hide(): Promise<void> {
     return new Promise((resolve) => {
       onHideComplete = resolve
@@ -30,15 +69,25 @@ export const useAssetBrowserDialog = () => {
   }
 
   async function show(props: AssetBrowserDialogProps) {
-    const handleAssetSelected = async (assetPath: string) => {
-      // Update the widget value immediately - don't wait for animation
-      props.onAssetSelected?.(assetPath)
-      // Then trigger the hide animation
-      await hide()
-    }
+    const handleAssetSelected = async (filename: string) => {
+      // This function is called by selectAssetWithCallback with the validated filename
+      props.onAssetSelected?.(filename)
 
-    // Default dialog configuration for AssetBrowserModal
-    const dialogComponentProps = {
+      // Wait for Vue to flush all synchronous updates (widget value, DOM updates)
+      await nextTick()
+
+      // Wait for next animation frame when PrimeVue sets up animation state
+      return new Promise<void>((resolve) => {
+        const { pause } = useRafFn(
+          () => {
+            pause()
+            void hide().then(resolve)
+          },
+          { immediate: true }
+        )
+      })
+    }
+    const dialogComponentProps: DialogComponentProps = {
       headless: true,
       modal: true,
       closable: true,
@@ -51,7 +100,7 @@ export const useAssetBrowserDialog = () => {
       },
       pt: {
         root: {
-          class: 'rounded-2xl overflow-hidden'
+          class: 'rounded-2xl overflow-hidden asset-browser-dialog'
         },
         header: {
           class: 'p-0 hidden'
@@ -62,17 +111,16 @@ export const useAssetBrowserDialog = () => {
       }
     }
 
-    // Fetch assets for the specific node type, fallback to empty array on error
-    let assets: AssetItem[] = []
-    try {
-      assets = await assetService.getAssetsForNodeType(props.nodeType)
-    } catch (error) {
-      console.error(
-        'Failed to fetch assets for node type:',
-        props.nodeType,
-        error
-      )
-    }
+    const assets: AssetItem[] = await assetService
+      .getAssetsForNodeType(props.nodeType)
+      .catch((error) => {
+        console.error(
+          'Failed to fetch assets for node type:',
+          props.nodeType,
+          error
+        )
+        return []
+      })
 
     dialogStore.showDialog({
       key: dialogKey,

src/platform/assets/services/assetService.ts

@@ -140,15 +140,9 @@ function createAssetService() {
       return []
     }
 
-    // Find the category for this node type by reverse lookup in modelToNodeMap
+    // Find the category for this node type using efficient O(1) lookup
     const modelToNodeStore = useModelToNodeStore()
-    const modelToNodeMap = modelToNodeStore.modelToNodeMap
-
-    const category = Object.keys(modelToNodeMap).find((categoryKey) =>
-      modelToNodeMap[categoryKey].some(
-        (provider) => provider.nodeDef.name === nodeType
-      )
-    )
+    const category = modelToNodeStore.getCategoryForNodeType(nodeType)
 
     if (!category) {
       return []

src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts

@@ -3,11 +3,8 @@ import { ref } from 'vue'
 import MultiSelectWidget from '@/components/graph/widgets/MultiSelectWidget.vue'
 import { t } from '@/i18n'
 import type { LGraphNode } from '@/lib/litegraph/src/litegraph'
-import type {
-  IAssetWidget,
-  IBaseWidget,
-  IComboWidget
-} from '@/lib/litegraph/src/types/widgets'
+import { isAssetWidget, isComboWidget } from '@/lib/litegraph/src/litegraph'
+import type { IBaseWidget } from '@/lib/litegraph/src/types/widgets'
 import { useAssetBrowserDialog } from '@/platform/assets/composables/useAssetBrowserDialog'
 import { assetService } from '@/platform/assets/services/assetService'
 import { useSettingStore } from '@/platform/settings/settingStore'
@@ -29,15 +26,6 @@ import {
 
 import { useRemoteWidget } from './useRemoteWidget'
 
-// Type guards for widget safety
-function isAssetWidget(widget: IBaseWidget): widget is IAssetWidget {
-  return widget.type === 'asset'
-}
-
-function isComboWidget(widget: IBaseWidget): widget is IComboWidget {
-  return widget.type === 'combo'
-}
-
 const getDefaultValue = (inputSpec: ComboInputSpec) => {
   if (inputSpec.default) return inputSpec.default
   if (inputSpec.options?.length) return inputSpec.options[0]
@@ -94,16 +82,15 @@ const addComboWidget = (
         if (!isAssetWidget(widget)) {
           throw new Error(`Expected asset widget but received ${widget.type}`)
         }
-        const assetWidget = widget
         await assetBrowserDialog.show({
           nodeType: node.comfyClass || '',
           inputName: inputSpec.name,
-          currentValue: assetWidget.value,
+          currentValue: widget.value,
           onAssetSelected: (filename: string) => {
-            assetWidget.value = filename
+            widget.value = filename
             // Must call widget.callback to notify litegraph of value changes
             // This ensures proper serialization and triggers any downstream effects
-            assetWidget.callback?.(assetWidget.value)
+            widget.callback?.(widget.value)
           }
         })
       }