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-03-13 17:10:06 +00:00 · 2026-02-03 18:01:21 -08:00
parent 226fc7e840
commit 11b84be06b
3 changed files with 75 additions and 121 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
@@ -43,7 +43,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
@@ -52,19 +60,16 @@ interface Props {
  uploadSubfolder?: string
  isAssetMode?: boolean
  defaultLayoutMode?: LayoutMode
-}
-
-const props = defineProps<Props>()
+}>()

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

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

@@ -76,15 +81,14 @@ const outputMediaAssets = useMediaAssets('output')
 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: string | undefined =
-    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
 }
@@ -92,7 +96,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' }]
  }
@@ -104,28 +108,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 {
@@ -137,7 +136,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 []
@@ -155,10 +154,10 @@ function assetKindToMediaType(kind: AssetKind): string {
 }

 const outputItems = computed<FormDropdownItem[]>(() => {
-  if (!['image', 'video', 'audio', 'mesh'].includes(props.assetKind ?? ''))
+  if (!['image', 'video', 'audio', 'mesh'].includes(assetKind ?? ''))
    return []

-  const targetMediaType = assetKindToMediaType(props.assetKind!)
+  const targetMediaType = assetKindToMediaType(assetKind!)
  const outputFiles = outputMediaAssets.media.value.filter(
    (asset) => getMediaTypeFromFilename(asset.name) === targetMediaType
  )
@@ -174,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
    )
@@ -199,7 +191,6 @@ const missingValueItem = computed<FormDropdownItem | undefined>(() => {
    }
  }

-  // Check in local mode inputs/outputs
  const existsInInputs = inputItems.value.some(
    (item) => item.name === currentValue
  )
@@ -222,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),
@@ -250,9 +237,10 @@ const baseModelFilteredAssetItems = computed<FormDropdownItem[]>(() =>
 )

 const allItems = computed<FormDropdownItem[]>(() => {
-  if (props.isAssetMode && assetData) {
-    // Cloud assets not in user's library shouldn't appear as search results (COM-14333).
-    // Unlike local mode, cloud users can't access files they don't own.
+  if (isAssetMode && assetData) {
+    if (missingValueItem.value) {
+      return [missingValueItem.value, ...baseModelFilteredAssetItems.value]
+    }
    return baseModelFilteredAssetItems.value
  }
  return [
@@ -263,7 +251,7 @@ const allItems = computed<FormDropdownItem[]>(() => {
 })

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

@@ -290,13 +278,13 @@ const displayItems = 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':
@@ -315,14 +303,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':
@@ -336,7 +322,7 @@ const acceptTypes = computed(() => {
  }
 })

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

 watch(
  [modelValue, displayItems],
@@ -377,7 +363,7 @@ function updateSelectedItems(selectedItems: Set<string>) {

 // Handle multiple file uploads using shared uploadMediaBatch service
 const uploadFiles = async (files: File[]): Promise<string[]> => {
-  const folder = props.uploadFolder ?? 'input'
+  const folder = uploadFolder ?? 'input'
  const assetsStore = useAssetsStore()

  const results = await uploadMediaBatch(
@@ -405,17 +391,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)) {
@@ -424,12 +406,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])
    }

    // 5. Snapshot undo state so the image change gets its own undo entry
@@ -444,7 +424,7 @@ function getMediaUrl(
  filename: string,
  type: 'input' | 'output' = 'input'
 ): string {
-  if (!['image', 'video', 'audio', 'mesh'].includes(props.assetKind ?? ''))
+  if (!['image', 'video', 'audio', 'mesh'].includes(assetKind ?? ''))
    return ''
  const params = new URLSearchParams({ filename, type })
  appendCloudResParam(params, filename)