-
Notifications
You must be signed in to change notification settings - Fork 5.3k
feat(composer): drop non-image files as filesystem paths #7749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
602ce81
d16df52
82f2be0
4186b1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,6 +122,7 @@ import { | |
| submitComposerDraft, | ||
| } from "./composerSubmission"; | ||
| import { ComposerPromptLengthValidation } from "./ComposerPromptLengthValidation"; | ||
| import { formatDroppedFilePaths } from "./droppedFilePaths"; | ||
|
|
||
| type ComposerCommandMenuPosition = { | ||
| bottom: number; | ||
|
|
@@ -2563,6 +2564,37 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) | |
| void addComposerImages(imageFiles); | ||
| }; | ||
|
|
||
| /** | ||
| * Insert dropped non-image files as filesystem paths. Only the desktop app | ||
| * can resolve a path from a dropped File, so in a browser tab this reports | ||
| * why nothing was inserted instead of silently swallowing the drop. | ||
| */ | ||
| const insertDroppedFilePaths = (files: File[]) => { | ||
| const resolvePath = window.desktopBridge?.getPathForFile; | ||
| if (!resolvePath) { | ||
| setThreadError( | ||
| activeThreadId, | ||
| "Attaching files by path needs the desktop app. Paste an image, or type the path.", | ||
| ); | ||
| return; | ||
| } | ||
| const paths = files | ||
| .map((file) => resolvePath(file)) | ||
| .filter((path): path is string => path !== null); | ||
| const text = formatDroppedFilePaths(paths); | ||
| if (text.length === 0) { | ||
| setThreadError(activeThreadId, "Could not read the location of the dropped file(s)."); | ||
| return; | ||
| } | ||
| if (!insertComposerTextAtEnd(text, { ensureLeadingBoundary: true })) { | ||
| toastManager.add({ | ||
| type: "error", | ||
| title: "Unable to add to chat", | ||
| description: "The composer is busy; try again once it is ready.", | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| const insertComposerTextAtEnd = ( | ||
| text: string, | ||
| options?: { ensureLeadingBoundary?: boolean }, | ||
|
|
@@ -2686,7 +2718,23 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) | |
| composerEditorRef.current?.focusAt(cursor); | ||
| }, | ||
| addDroppedFiles: (files: File[]) => { | ||
| void addComposerImages(files); | ||
| // Images become attachments; everything else (audio, PDF, video, …) is | ||
| // handed over as a path so the agent opens it from disk itself. | ||
| const images = files.filter((file) => file.type.startsWith("image/")); | ||
| const nonImages = files.filter((file) => !file.type.startsWith("image/")); | ||
| if (images.length > 0) { | ||
| void addComposerImages(images); | ||
| } | ||
| if (nonImages.length > 0) { | ||
| // Deliberately no focusComposer() on this path. `applyPromptReplacement` | ||
| // focuses on the next frame, once Lexical has reconciled; focusing | ||
| // synchronously here makes the not-yet-reconciled editor sync its stale | ||
| // empty state back over the text we just inserted, so the drop looks | ||
| // like it silently did nothing. Same footgun the file-tree mention drop | ||
| // documents in makeComposerMentionDragHandlers. | ||
| insertDroppedFilePaths(nonImages); | ||
| return; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mixed drops fire duplicate toastsLow Severity A mixed image and non-image drop while plan questions are pending triggers two separate error toasts: Additional Locations (2)Reviewed by Cursor Bugbot for commit 602ce81. Configure here.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All three are fair — fixed in 82f2be0. Path failures now go to a toast instead of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Failed path insert skips focusLow Severity The non-image branch always Reviewed by Cursor Bugbot for commit 602ce81. Configure here.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All three are fair — fixed in 82f2be0. Path failures now go to a toast instead of |
||
| focusComposer(); | ||
| }, | ||
| insertTextAtEnd: insertComposerTextAtEnd, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import { describe, expect, it } from "vite-plus/test"; | ||
|
|
||
| import { formatDroppedFilePaths, quoteDroppedFilePath } from "./droppedFilePaths"; | ||
|
|
||
| describe("quoteDroppedFilePath", () => { | ||
| it("leaves a path without whitespace alone", () => { | ||
| expect(quoteDroppedFilePath("/Users/me/notes.pdf")).toBe("/Users/me/notes.pdf"); | ||
| }); | ||
|
|
||
| it("quotes a path containing spaces", () => { | ||
| expect(quoteDroppedFilePath("/Users/me/Voice Memos/note 1.m4a")).toBe( | ||
| '"/Users/me/Voice Memos/note 1.m4a"', | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe("formatDroppedFilePaths", () => { | ||
| it("joins several paths with a space", () => { | ||
| expect(formatDroppedFilePaths(["/a/one.opus", "/b/two.pdf"])).toBe("/a/one.opus /b/two.pdf"); | ||
| }); | ||
|
|
||
| it("skips empty and whitespace-only entries", () => { | ||
| expect(formatDroppedFilePaths(["", " ", "/a/one.opus"])).toBe("/a/one.opus"); | ||
| }); | ||
|
|
||
| it("returns an empty string when nothing resolved", () => { | ||
| expect(formatDroppedFilePaths([])).toBe(""); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /** | ||
| * Files dropped from the OS that aren't images are handed to the agent by | ||
| * *path*, not by content: it can open the file itself, so a 40MB recording or | ||
| * a PDF never has to cross the wire as an attachment. This mirrors dropping a | ||
| * file into a terminal, where the shell receives the path. | ||
| * | ||
| * Paths are only available in the desktop app (Electron's `webUtils`); in a | ||
| * browser tab the File object carries no filesystem path at all. | ||
| */ | ||
|
|
||
| /** | ||
| * Quote a path for the prompt when whitespace would make where it ends | ||
| * ambiguous. This is prompt text, not a shell command — the goal is a clear | ||
| * boundary for the reader, not shell-injection safety. | ||
| */ | ||
| export function quoteDroppedFilePath(path: string): string { | ||
| return /\s/.test(path) ? `"${path}"` : path; | ||
|
macroscopeapp[bot] marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| /** | ||
| * The text inserted into the composer for a set of dropped paths. Empty and | ||
| * whitespace-only paths are dropped (a bridge that can't resolve a file | ||
| * returns null, which the caller filters, but be defensive about "" too). | ||
| */ | ||
| export function formatDroppedFilePaths(paths: ReadonlyArray<string>): string { | ||
| return paths | ||
| .map((path) => path.trim()) | ||
| .filter((path) => path.length > 0) | ||
| .map(quoteDroppedFilePath) | ||
|
macroscopeapp[bot] marked this conversation as resolved.
|
||
| .join(" "); | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed drops overwrite image errors
Medium Severity
When a drop mixes images and non-images,
insertDroppedFilePathsalways callssetThreadErroron path failure and can replace a message just set byaddComposerImages. In the browser, a successful image attach still shows the desktop-only path error, which reads like the whole drop failed and can lead to retrying with duplicate attachments.Additional Locations (1)
apps/web/src/components/chat/ChatComposer.tsx#L2719-L2737Reviewed by Cursor Bugbot for commit 602ce81. Configure here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All three are fair — fixed in 82f2be0. Path failures now go to a toast instead of
setThreadError, so they can't overwrite the banner the image attach owns or make a successful image attach read as a failed drop. The 'composer is busy' refusal is suppressed when the same drop also had images, since that refusal comes from the state that already rejected them. AndinsertDroppedFilePathsnow returns whether it inserted, sofocusComposer()is skipped only when text actually landed — the stale-state rationale only applies to a successful insert. Re-verified the drop end to end after the refactor.