Merge pull request #56029 from NousResearch/fix/desktop-drop-folder-attach

apps/desktop/src/app/chat/hooks/use-composer-actions.test.ts

@@ -1,6 +1,6 @@
-import { describe, expect, it } from 'vitest'
+import { afterEach, describe, expect, it, vi } from 'vitest'
 
-import { type DroppedFile, partitionDroppedFiles } from './use-composer-actions'
+import { type DroppedFile, extractDroppedFiles, HERMES_PATHS_MIME, partitionDroppedFiles } from './use-composer-actions'
 
 // A Finder/Explorer drop carries a native File handle; an in-app drag (project
 // tree, gutter line ref) is path-only. The split decides whether a drop becomes
@@ -39,6 +39,18 @@ describe('partitionDroppedFiles', () => {
     expect(inAppRefs).toEqual([lineRef])
   })
 
+  it('routes an OS folder drop (path-only, isDirectory) to inAppRefs, not the upload pipeline', () => {
+    // extractDroppedFiles emits a dropped directory as a path-only entry so it
+    // stays a @folder: ref instead of hitting file.attach, which can't stage a
+    // directory ("file not found on gateway and no data_url provided").
+    const folder = inAppRef('/Users/jeff/projects/hermes', { isDirectory: true })
+
+    const { inAppRefs, osDrops } = partitionDroppedFiles([folder])
+
+    expect(osDrops).toEqual([])
+    expect(inAppRefs).toEqual([folder])
+  })
+
   it('splits a mixed drop and preserves order within each group', () => {
     const a = inAppRef('a.ts')
     const b = osDrop('/abs/b.pdf')
@@ -55,3 +67,114 @@ describe('partitionDroppedFiles', () => {
     expect(partitionDroppedFiles([])).toEqual({ inAppRefs: [], osDrops: [] })
   })
 })
+
+// Minimal DataTransfer stand-in. A real OS drop populates BOTH `items` (which
+// alone carries webkitGetAsEntry for folder detection) and `files`; the mock
+// mirrors that so the dedup path is exercised too.
+interface StubEntry {
+  path: string
+  isDirectory: boolean
+}
+
+function stubTransfer(entries: StubEntry[], internalRaw = ''): DataTransfer & { _pathByFile: Map<File, string> } {
+  const files = entries.map(entry => new File(['x'], entry.path.split('/').pop() || 'f'))
+  const pathByFile = new Map(files.map((file, i) => [file, entries[i].path]))
+
+  const items: Record<number | string, unknown> = { length: entries.length }
+  entries.forEach((entry, i) => {
+    items[i] = {
+      kind: 'file' as const,
+      getAsFile: () => files[i],
+      webkitGetAsEntry: () => ({ isDirectory: entry.isDirectory, isFile: !entry.isDirectory })
+    }
+  })
+
+  return {
+    getData: (mime: string) => (mime === HERMES_PATHS_MIME ? internalRaw : ''),
+    files: {
+      length: files.length,
+      item: (i: number) => files[i] ?? null
+    },
+    items,
+    _pathByFile: pathByFile
+  } as unknown as DataTransfer & { _pathByFile: Map<File, string> }
+}
+
+describe('extractDroppedFiles', () => {
+  afterEach(() => {
+    vi.unstubAllGlobals()
+  })
+
+  const stubBridge = (transfer: DataTransfer & { _pathByFile: Map<File, string> }) => {
+    vi.stubGlobal('window', {
+      hermesDesktop: {
+        getPathForFile: (file: File) => transfer._pathByFile.get(file) ?? ''
+      }
+    })
+  }
+
+  it('emits a dropped directory as a path-only entry with isDirectory (no File to upload)', () => {
+    const transfer = stubTransfer([
+      { path: '/Users/jeff/projects/hermes', isDirectory: true }
+    ]) as DataTransfer & { _pathByFile: Map<File, string> }
+
+    stubBridge(transfer)
+
+    const result = extractDroppedFiles(transfer)
+
+    expect(result).toHaveLength(1)
+    expect(result[0]?.isDirectory).toBe(true)
+    expect(result[0]?.path).toBe('/Users/jeff/projects/hermes')
+    // A directory carries no bytes — it must NOT ride the File/upload pipeline.
+    expect(result[0]?.file).toBeUndefined()
+    // And it partitions as an in-app ref (→ @folder:), never an OS upload drop.
+    expect(partitionDroppedFiles(result).osDrops).toEqual([])
+  })
+
+  it('still emits a dropped file with its native File handle for the upload pipeline', () => {
+    const transfer = stubTransfer([
+      { path: '/Users/jeff/Downloads/report.pdf', isDirectory: false }
+    ]) as DataTransfer & { _pathByFile: Map<File, string> }
+
+    stubBridge(transfer)
+
+    const result = extractDroppedFiles(transfer)
+
+    expect(result).toHaveLength(1)
+    expect(result[0]?.isDirectory).toBeFalsy()
+    expect(result[0]?.path).toBe('/Users/jeff/Downloads/report.pdf')
+    expect(result[0]?.file).toBeInstanceOf(File)
+    expect(partitionDroppedFiles(result).osDrops).toHaveLength(1)
+  })
+
+  it('classifies a mixed folder+file drop independently', () => {
+    const transfer = stubTransfer([
+      { path: '/abs/src', isDirectory: true },
+      { path: '/abs/notes.txt', isDirectory: false }
+    ]) as DataTransfer & { _pathByFile: Map<File, string> }
+
+    stubBridge(transfer)
+
+    const result = extractDroppedFiles(transfer)
+    const { inAppRefs, osDrops } = partitionDroppedFiles(result)
+
+    expect(inAppRefs.map(entry => entry.path)).toEqual(['/abs/src'])
+    expect(inAppRefs[0]?.isDirectory).toBe(true)
+    expect(osDrops.map(entry => entry.path)).toEqual(['/abs/notes.txt'])
+  })
+
+  it('does not duplicate a folder that appears in both items and files', () => {
+    // Chromium lists a dropped folder in transfer.files too (as a size-0 File);
+    // the items pass claims its path first so the files fallback skips it.
+    const transfer = stubTransfer([
+      { path: '/abs/project', isDirectory: true }
+    ]) as DataTransfer & { _pathByFile: Map<File, string> }
+
+    stubBridge(transfer)
+
+    const result = extractDroppedFiles(transfer)
+
+    expect(result).toHaveLength(1)
+    expect(result[0]?.isDirectory).toBe(true)
+  })
+})

apps/desktop/src/app/chat/hooks/use-composer-actions.ts

@@ -44,7 +44,8 @@ export interface DroppedFile {
   file?: File
   /** Absolute filesystem path. Empty when an OS drop didn't carry one. */
   path: string
-  /** True if the entry is a directory. Currently only set by in-app drags. */
+  /** True if the entry is a directory. Set by in-app drags, and by OS drops via
+   * DataTransferItem.webkitGetAsEntry(). */
   isDirectory?: boolean
   /** First line number for in-app line-ref drags (source view gutter). */
   line?: number
@@ -108,39 +109,50 @@ export function extractDroppedFiles(transfer: DataTransfer): DroppedFile[] {
     // Malformed payload — fall through to native files.
   }
 
-  const fileList = transfer.files
-
-  if (fileList) {
-    for (let i = 0; i < fileList.length; i += 1) {
-      const file = fileList.item(i)
-
-      if (!file || seenFiles.has(file)) {
-        continue
-      }
-
-      seenFiles.add(file)
-      let path = ''
-
-      if (getPath) {
-        try {
-          path = getPath(file) || ''
-        } catch {
-          path = ''
-        }
-      }
-
-      if (path && seenPaths.has(path)) {
-        continue
-      }
-
-      if (path) {
-        seenPaths.add(path)
-      }
-
-      result.push({ file, path })
+  // Add a native OS-drop entry. A dropped directory has no byte content to
+  // upload, so it's emitted as a path-only entry with `isDirectory: true` —
+  // that routes it to a `@folder:` ref / folder attachment (like the folder
+  // picker) instead of the file-upload pipeline, which can't stage a directory
+  // (the gateway can't read its bytes and there's no data_url to send).
+  const pushNativeEntry = (file: File, isDirectory: boolean) => {
+    if (seenFiles.has(file)) {
+      return
     }
+
+    seenFiles.add(file)
+    let path = ''
+
+    if (getPath) {
+      try {
+        path = getPath(file) || ''
+      } catch {
+        path = ''
+      }
+    }
+
+    if (path && seenPaths.has(path)) {
+      return
+    }
+
+    if (path) {
+      seenPaths.add(path)
+    }
+
+    if (isDirectory) {
+      if (path) {
+        result.push({ isDirectory: true, path })
+      }
+
+      return
+    }
+
+    result.push({ file, path })
   }
 
+  // Process items first: DataTransferItem.webkitGetAsEntry() is the only
+  // synchronous way to tell a dropped folder from a file, and it lives only on
+  // items (not transfer.files). Must be read here, inside the drop handler,
+  // before the DataTransfer detaches.
   const items = transfer.items
 
   if (items) {
@@ -151,32 +163,39 @@ export function extractDroppedFiles(transfer: DataTransfer): DroppedFile[] {
         continue
       }
 
+      let isDirectory = false
+
+      try {
+        const entry = typeof item.webkitGetAsEntry === 'function' ? item.webkitGetAsEntry() : null
+        isDirectory = entry?.isDirectory === true
+      } catch {
+        isDirectory = false
+      }
+
       const file = item.getAsFile()
 
-      if (!file || seenFiles.has(file)) {
+      if (!file) {
         continue
       }
 
-      seenFiles.add(file)
-      let path = ''
+      pushNativeEntry(file, isDirectory)
+    }
+  }
 
-      if (getPath) {
-        try {
-          path = getPath(file) || ''
-        } catch {
-          path = ''
-        }
-      }
+  // Fallback for environments that populate transfer.files but not items.
+  // webkitGetAsEntry isn't available on this path, so directory detection
+  // relies on the items pass above; anything reaching here is treated as a file.
+  const fileList = transfer.files
 
-      if (path && seenPaths.has(path)) {
+  if (fileList) {
+    for (let i = 0; i < fileList.length; i += 1) {
+      const file = fileList.item(i)
+
+      if (!file) {
         continue
       }
 
-      if (path) {
-        seenPaths.add(path)
-      }
-
-      result.push({ file, path })
+      pushNativeEntry(file, false)
     }
   }