fix: polish uploadService based on code review feedback

- M2: Blob MIME type now prefers source.type over default parameter - M3: Removed double toast on failed uploads - m1: Converted WidgetSelectDropdown.vue to Vue 3.5 reactive props - m3: Created createMockResponse() helper in tests - m4: Added test for empty batch edge case - m5: Removed verbose JSDoc comments Amp-Thread-ID: https://ampcode.com/threads/T-019c265d-f2f5-7028-95b4-5e031e447bd3
2026-02-23 08:14:06 +00:00 · 2026-02-03 18:01:21 -08:00
parent 09279a0baf
commit 103ef32ae4
3 changed files with 73 additions and 118 deletions
--- a/src/platform/assets/services/uploadService.test.ts
+++ b/src/platform/assets/services/uploadService.test.ts
@@ -10,6 +10,17 @@ vi.mock('@/scripts/api', () => ({
  }
 }))

+function createMockResponse(
+  status: number,
+  data?: { name: string; subfolder?: string }
+) {
+  return {
+    status,
+    statusText: status === 200 ? 'OK' : 'Error',
+    json: vi.fn().mockResolvedValue(data ?? {})
+  } as unknown as Response
+}
+
 describe('uploadService', () => {
  beforeEach(() => {
    vi.clearAllMocks()
@@ -18,16 +29,8 @@ describe('uploadService', () => {
  describe('uploadMedia', () => {
    it('uploads File successfully', async () => {
      const mockFile = new File(['content'], 'test.png', { type: 'image/png' })
-      const mockResponse = {
-        status: 200,
-        json: vi.fn().mockResolvedValue({
-          name: 'test.png',
-          subfolder: 'uploads'
-        })
-      }
-
      vi.mocked(api.fetchApi).mockResolvedValue(
-        mockResponse as Partial<Response> as Response
+        createMockResponse(200, { name: 'test.png', subfolder: 'uploads' })
      )

      const result = await uploadMedia({ source: mockFile })
@@ -40,16 +43,8 @@ describe('uploadService', () => {

    it('uploads Blob successfully', async () => {
      const mockBlob = new Blob(['content'], { type: 'image/png' })
-      const mockResponse = {
-        status: 200,
-        json: vi.fn().mockResolvedValue({
-          name: 'upload-123.png',
-          subfolder: ''
-        })
-      }
-
      vi.mocked(api.fetchApi).mockResolvedValue(
-        mockResponse as Partial<Response> as Response
+        createMockResponse(200, { name: 'upload-123.png', subfolder: '' })
      )

      const result = await uploadMedia({ source: mockBlob })
@@ -64,16 +59,8 @@ describe('uploadService', () => {
        blob: () => Promise.resolve(new Blob(['content']))
      } as Response)

-      const mockResponse = {
-        status: 200,
-        json: vi.fn().mockResolvedValue({
-          name: 'upload-456.png',
-          subfolder: ''
-        })
-      }
-
      vi.mocked(api.fetchApi).mockResolvedValue(
-        mockResponse as Partial<Response> as Response
+        createMockResponse(200, { name: 'upload-456.png', subfolder: '' })
      )

      try {
@@ -95,13 +82,8 @@ describe('uploadService', () => {

    it('includes subfolder in FormData', async () => {
      const mockFile = new File(['content'], 'test.png')
-      const mockResponse = {
-        status: 200,
-        json: vi.fn().mockResolvedValue({ name: 'test.png' })
-      }
-
      vi.mocked(api.fetchApi).mockResolvedValue(
-        mockResponse as Partial<Response> as Response
+        createMockResponse(200, { name: 'test.png' })
      )

      await uploadMedia(
@@ -134,14 +116,10 @@ describe('uploadService', () => {

    it('handles upload errors', async () => {
      const mockFile = new File(['content'], 'test.png')
-      const mockResponse = {
+      vi.mocked(api.fetchApi).mockResolvedValue({
        status: 500,
        statusText: 'Internal Server Error'
-      }
-
-      vi.mocked(api.fetchApi).mockResolvedValue(
-        mockResponse as Partial<Response> as Response
-      )
+      } as unknown as Response)

      const result = await uploadMedia({ source: mockFile })

@@ -162,13 +140,8 @@ describe('uploadService', () => {

    it('includes originalRef for mask uploads', async () => {
      const mockFile = new File(['content'], 'mask.png')
-      const mockResponse = {
-        status: 200,
-        json: vi.fn().mockResolvedValue({ name: 'mask.png' })
-      }
-
      vi.mocked(api.fetchApi).mockResolvedValue(
-        mockResponse as Partial<Response> as Response
+        createMockResponse(200, { name: 'mask.png' })
      )

      const originalRef = {
@@ -189,25 +162,26 @@ describe('uploadService', () => {
  })

  describe('uploadMediaBatch', () => {
+    it('returns empty array for empty input', async () => {
+      const results = await uploadMediaBatch([])
+
+      expect(results).toHaveLength(0)
+      expect(api.fetchApi).not.toHaveBeenCalled()
+    })
+
    it('uploads multiple files', async () => {
      const mockFiles = [
        new File(['1'], 'file1.png'),
        new File(['2'], 'file2.png')
      ]

-      const mockResponse1 = {
-        status: 200,
-        json: vi.fn().mockResolvedValue({ name: 'file1.png', subfolder: '' })
-      }
-
-      const mockResponse2 = {
-        status: 200,
-        json: vi.fn().mockResolvedValue({ name: 'file2.png', subfolder: '' })
-      }
-
      vi.mocked(api.fetchApi)
-        .mockResolvedValueOnce(mockResponse1 as Partial<Response> as Response)
-        .mockResolvedValueOnce(mockResponse2 as Partial<Response> as Response)
+        .mockResolvedValueOnce(
+          createMockResponse(200, { name: 'file1.png', subfolder: '' })
+        )
+        .mockResolvedValueOnce(
+          createMockResponse(200, { name: 'file2.png', subfolder: '' })
+        )

      const results = await uploadMediaBatch(
        mockFiles.map((source) => ({ source }))
--- a/src/platform/assets/services/uploadService.ts
+++ b/src/platform/assets/services/uploadService.ts
@@ -46,7 +46,7 @@ async function convertToFile(

  if (source instanceof Blob) {
    const name = filename || `upload-${Date.now()}.png`
-    return new File([source], name, { type: mimeType })
+    return new File([source], name, { type: source.type || mimeType })
  }

  // dataURL string
--- a/src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue
+++ b/src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue
@@ -38,7 +38,15 @@ import {
  filterWidgetProps
 } from '@/utils/widgetPropFilter'

-interface Props {
+const {
+  widget,
+  nodeType,
+  assetKind,
+  allowUpload = false,
+  uploadFolder,
+  isAssetMode = false,
+  defaultLayoutMode = 'grid'
+} = defineProps<{
  widget: SimplifiedWidget<string | undefined>
  nodeType?: string
  assetKind?: AssetKind
@@ -46,18 +54,16 @@ interface Props {
  uploadFolder?: ResultItemType
  isAssetMode?: boolean
  defaultLayoutMode?: LayoutMode
-}
-
-const props = defineProps<Props>()
+}>()

 provide(
  AssetKindKey,
-  computed(() => props.assetKind)
+  computed(() => assetKind)
 )

 const modelValue = defineModel<string | undefined>({
-  default(props: Props) {
-    return props.widget.options?.values?.[0] || ''
+  default() {
+    return widget.options?.values?.[0] || ''
  }
 })

@@ -68,14 +74,14 @@ const queueStore = useQueueStore()
 const transformCompatProps = useTransformCompatOverlayProps()

 const combinedProps = computed(() => ({
-  ...filterWidgetProps(props.widget.options, PANEL_EXCLUDED_PROPS),
+  ...filterWidgetProps(widget.options, PANEL_EXCLUDED_PROPS),
  ...transformCompatProps.value
 }))

 const getAssetData = () => {
-  const nodeType = props.widget.options?.nodeType ?? props.nodeType
-  if (props.isAssetMode && nodeType) {
-    return useAssetWidgetData(toRef(nodeType))
+  const resolvedNodeType = widget.options?.nodeType ?? nodeType
+  if (isAssetMode && resolvedNodeType) {
+    return useAssetWidgetData(toRef(resolvedNodeType))
  }
  return null
 }
@@ -83,7 +89,7 @@ const assetData = getAssetData()

 const filterSelected = ref('all')
 const filterOptions = computed<FilterOption[]>(() => {
-  if (props.isAssetMode) {
+  if (isAssetMode) {
    const categoryName = assetData?.category.value ?? 'All'
    return [{ name: capitalize(categoryName), value: 'all' }]
  }
@@ -95,28 +101,23 @@ const filterOptions = computed<FilterOption[]>(() => {
 })

 const ownershipSelected = ref<OwnershipOption>('all')
-const showOwnershipFilter = computed(() => props.isAssetMode)
+const showOwnershipFilter = computed(() => isAssetMode)

 const { ownershipOptions, availableBaseModels } = useAssetFilterOptions(
  () => assetData?.assets.value ?? []
 )

 const baseModelSelected = ref<Set<string>>(new Set())
-const showBaseModelFilter = computed(() => props.isAssetMode)
+const showBaseModelFilter = computed(() => isAssetMode)
 const baseModelOptions = computed<FilterOption[]>(() => {
-  if (!props.isAssetMode || !assetData) return []
+  if (!isAssetMode || !assetData) return []
  return availableBaseModels.value
 })

 const selectedSet = ref<Set<string>>(new Set())

-/**
- * Transforms a value using getOptionLabel if available.
- * Falls back to the original value if getOptionLabel is not provided,
- * returns undefined/null, or throws an error.
- */
 function getDisplayLabel(value: string): string {
-  const getOptionLabel = props.widget.options?.getOptionLabel
+  const getOptionLabel = widget.options?.getOptionLabel
  if (!getOptionLabel) return value

  try {
@@ -128,7 +129,7 @@ function getDisplayLabel(value: string): string {
 }

 const inputItems = computed<FormDropdownItem[]>(() => {
-  const values = props.widget.options?.values || []
+  const values = widget.options?.values || []

  if (!Array.isArray(values)) {
    return []
@@ -142,7 +143,7 @@ const inputItems = computed<FormDropdownItem[]>(() => {
  }))
 })
 const outputItems = computed<FormDropdownItem[]>(() => {
-  if (!['image', 'video'].includes(props.assetKind ?? '')) return []
+  if (!['image', 'video'].includes(assetKind ?? '')) return []

  const outputs = new Set<string>()

@@ -150,8 +151,8 @@ const outputItems = computed<FormDropdownItem[]>(() => {
  queueStore.historyTasks.forEach((task) => {
    task.flatOutputs.forEach((output) => {
      const isTargetType =
-        (props.assetKind === 'image' && output.mediaType === 'images') ||
-        (props.assetKind === 'video' && output.mediaType === 'video')
+        (assetKind === 'image' && output.mediaType === 'images') ||
+        (assetKind === 'video' && output.mediaType === 'video')

      if (output.type === 'output' && isTargetType) {
        const path = output.subfolder
@@ -172,18 +173,11 @@ const outputItems = computed<FormDropdownItem[]>(() => {
  }))
 })

-/**
- * Creates a fallback item for the current modelValue when it doesn't exist
- * in the available items list. This handles cases like template-loaded nodes
- * where the saved value may not exist in the current server environment.
- * Works for both local mode (inputItems/outputItems) and cloud mode (assetData).
- */
 const missingValueItem = computed<FormDropdownItem | undefined>(() => {
  const currentValue = modelValue.value
  if (!currentValue) return undefined

-  // Check in cloud mode assets
-  if (props.isAssetMode && assetData) {
+  if (isAssetMode && assetData) {
    const existsInAssets = assetData.assets.value.some(
      (asset) => getAssetFilename(asset) === currentValue
    )
@@ -197,7 +191,6 @@ const missingValueItem = computed<FormDropdownItem | undefined>(() => {
    }
  }

-  // Check in local mode inputs/outputs
  const existsInInputs = inputItems.value.some(
    (item) => item.name === currentValue
  )
@@ -220,12 +213,8 @@ const missingValueItem = computed<FormDropdownItem | undefined>(() => {
  }
 })

-/**
- * Transforms AssetItem[] to FormDropdownItem[] for cloud mode.
- * Uses getAssetFilename for display name, asset.name for label.
- */
 const assetItems = computed<FormDropdownItem[]>(() => {
-  if (!props.isAssetMode || !assetData) return []
+  if (!isAssetMode || !assetData) return []
  return assetData.assets.value.map((asset) => ({
    id: asset.id,
    name: getAssetFilename(asset),
@@ -248,7 +237,7 @@ const baseModelFilteredAssetItems = computed<FormDropdownItem[]>(() =>
 )

 const allItems = computed<FormDropdownItem[]>(() => {
-  if (props.isAssetMode && assetData) {
+  if (isAssetMode && assetData) {
    if (missingValueItem.value) {
      return [missingValueItem.value, ...baseModelFilteredAssetItems.value]
    }
@@ -262,7 +251,7 @@ const allItems = computed<FormDropdownItem[]>(() => {
 })

 const dropdownItems = computed<FormDropdownItem[]>(() => {
-  if (props.isAssetMode) {
+  if (isAssetMode) {
    return allItems.value
  }

@@ -278,13 +267,13 @@ const dropdownItems = computed<FormDropdownItem[]>(() => {
 })

 const mediaPlaceholder = computed(() => {
-  const options = props.widget.options
+  const options = widget.options

  if (options?.placeholder) {
    return options.placeholder
  }

-  switch (props.assetKind) {
+  switch (assetKind) {
    case 'image':
      return t('widgets.uploadSelect.placeholderImage')
    case 'video':
@@ -301,14 +290,12 @@ const mediaPlaceholder = computed(() => {
 })

 const uploadable = computed(() => {
-  if (props.isAssetMode) return false
-  return props.allowUpload === true
+  if (isAssetMode) return false
+  return allowUpload
 })

 const acceptTypes = computed(() => {
-  // Be permissive with accept types because backend uses libraries
-  // that can handle a wide range of formats
-  switch (props.assetKind) {
+  switch (assetKind) {
    case 'image':
      return 'image/*'
    case 'video':
@@ -320,7 +307,7 @@ const acceptTypes = computed(() => {
  }
 })

-const layoutMode = ref<LayoutMode>(props.defaultLayoutMode ?? 'grid')
+const layoutMode = ref<LayoutMode>(defaultLayoutMode)

 watch(
  [modelValue, dropdownItems],
@@ -357,7 +344,7 @@ function updateSelectedItems(selectedItems: Set<string>) {
 }

 const uploadFiles = async (files: File[]): Promise<string[]> => {
-  const folder = props.uploadFolder ?? 'input'
+  const folder = uploadFolder ?? 'input'
  const assetsStore = useAssetsStore()

  const results = await uploadMediaBatch(
@@ -383,17 +370,13 @@ async function handleFilesUpdate(files: File[]) {
  if (!files || files.length === 0) return

  try {
-    // 1. Upload files to server
    const uploadedPaths = await uploadFiles(files)

    if (uploadedPaths.length === 0) {
-      toastStore.addAlert(t('toastMessages.uploadFailed'))
      return
    }

-    // 2. Update widget options to include new files
-    // This simulates what addToComboValues does but for SimplifiedWidget
-    const values = props.widget.options?.values
+    const values = widget.options?.values
    if (Array.isArray(values)) {
      uploadedPaths.forEach((path) => {
        if (!values.includes(path)) {
@@ -402,12 +385,10 @@ async function handleFilesUpdate(files: File[]) {
      })
    }

-    // 3. Update widget value to the first uploaded file
    modelValue.value = uploadedPaths[0]

-    // 4. Trigger callback to notify underlying LiteGraph widget
-    if (props.widget.callback) {
-      props.widget.callback(uploadedPaths[0])
+    if (widget.callback) {
+      widget.callback(uploadedPaths[0])
    }
  } catch (error) {
    console.error('Upload error:', error)
@@ -419,7 +400,7 @@ function getMediaUrl(
  filename: string,
  type: 'input' | 'output' = 'input'
 ): string {
-  if (!['image', 'video'].includes(props.assetKind ?? '')) return ''
+  if (!['image', 'video'].includes(assetKind ?? '')) return ''
  return `/api/view?filename=${encodeURIComponent(filename)}&type=${type}`
 }
 </script>