Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 5.2k
feat(composer): drop non-image files as filesystem paths#8549
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
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
b1ea4d66fbc8592d705b90308c5cf52c963File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -169,6 +169,9 @@ import { | ||
| submitComposerDraft, | ||
| } from "./composerSubmission"; | ||
| import { ComposerPromptLengthValidation } from "./ComposerPromptLengthValidation"; | ||
| import { getPrimaryKnownEnvironment } from "~/environments/primary"; | ||
| import { droppedPathsResolveInEnvironment, formatDroppedFilePaths } from "./droppedFilePaths"; | ||
| function ComposerVideoThumbnail({ file }: { file: File }) { | ||
| const setVideo = useCallback( | ||
| @@ -3148,6 +3151,65 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) | ||
| void addComposerAttachments(files); | ||
| }; | ||
| /** | ||
| * Insert dropped non-image files as filesystem paths, reporting to a toast | ||
| * rather than the thread error banner: a mixed drop's images own that banner, | ||
| * and a path failure there would read as though the whole drop failed. Only | ||
| * the desktop app can resolve a path from a dropped File, so in a browser tab | ||
| * this says so instead of silently swallowing the drop. Returns whether text | ||
| * was actually inserted. | ||
| */ | ||
| const insertDroppedFilePaths = (files: File[], hadImages: boolean): boolean => { | ||
| const reportFailure = (description: string) => { | ||
| toastManager.add({ type: "error", title: "Unable to add to chat", description }); | ||
| }; | ||
| const resolvePath = window.desktopBridge?.getPathForFile; | ||
| if (!resolvePath) { | ||
| reportFailure( | ||
| "Attaching files by path needs the desktop app. Paste an image, or type the path.", | ||
| ); | ||
| return false; | ||
| } | ||
| // The bridge reads a path on THIS machine. Inserting it into a thread whose | ||
| // agent runs elsewhere hands over a path that host cannot open — so gate on | ||
| // the thread's environment being the one this desktop app supervises, | ||
| // rather than on the bridge merely existing. | ||
| const primaryEnvironment = getPrimaryKnownEnvironment(); | ||
| if ( | ||
| !droppedPathsResolveInEnvironment({ | ||
| primarySource: primaryEnvironment?.source, | ||
| primaryEnvironmentId: primaryEnvironment?.environmentId, | ||
| threadEnvironmentId: environmentId, | ||
| }) | ||
| ) { | ||
| reportFailure( | ||
| "This thread runs on another machine, which cannot open a path from this one. Attach the file, or type a path that exists there.", | ||
| ); | ||
| return false; | ||
| } | ||
| const paths = files | ||
| .map((file) => resolvePath(file)) | ||
| .filter((path): path is string => path !== null); | ||
| const text = formatDroppedFilePaths(paths); | ||
| if (text.length === 0) { | ||
| reportFailure("Could not read the location of the dropped file(s)."); | ||
| return false; | ||
| } | ||
| if (!insertComposerTextAtEnd(text, { ensureLeadingBoundary: true })) { | ||
| // Pending plan questions is the ONLY refusal `addComposerImages` shares | ||
| // with the insert, so that is the only case a mixed drop has already been | ||
| // told about. Staying silent for the others — connecting, approval, | ||
| // project selection — would attach the images and drop the paths without | ||
| // a word, which is the failure this whole branch exists to avoid. | ||
| const alreadyReported = hadImages && pendingUserInputs.length > 0; | ||
| if (!alreadyReported) { | ||
| reportFailure("The composer is busy; try again once it is ready."); | ||
| } | ||
| return false; | ||
| } | ||
| return true; | ||
| }; | ||
| const insertComposerTextAtEnd = ( | ||
| text: string, | ||
| options?: { ensureLeadingBoundary?: boolean }, | ||
| @@ -3271,8 +3333,30 @@ export const ChatComposer = memo(function ChatComposer(props: ChatComposerProps) | ||
| composerEditorRef.current?.focusAt(cursor); | ||
| }, | ||
| addDroppedFiles: (files: File[]) => { | ||
| void addComposerAttachments(files); | ||
| focusComposer(); | ||
| // The composer attaches what it can render inline — images and video. | ||
| // Everything else (audio, PDF, archives, …) is handed over as a path so | ||
| // the agent opens it from disk itself, instead of being rejected as "not | ||
| // a supported image type" or pushing a 40MB recording across the wire. | ||
| const attachable = files.filter( | ||
| (file) => | ||
| file.type.startsWith("image/") || | ||
| videoMimeType({ name: file.name, mimeType: file.type }) !== undefined, | ||
| ); | ||
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. Path insert never runsHigh Severity The attachable filter treats every dropped file as attachable because Reviewed by Cursor Bugbot for commit f52c963. Configure here. | ||
| const byPath = files.filter((file) => !attachable.includes(file)); | ||
| if (attachable.length > 0) { | ||
| void addComposerAttachments(attachable); | ||
| } | ||
| const insertedPath = | ||
| byPath.length > 0 && insertDroppedFilePaths(byPath, attachable.length > 0); | ||
| // Focus unless a path just landed. `applyPromptReplacement` focuses on the | ||
| // next frame, once Lexical has reconciled; focusing synchronously right | ||
| // after the insert makes the not-yet-reconciled editor sync its stale empty | ||
| // state back over the text, so the drop looks like it silently did nothing | ||
| // — the footgun makeComposerMentionDragHandlers documents for the mention | ||
| // drop. Nothing inserted means nothing to lose, so focus as before. | ||
| if (!insertedPath) { | ||
| focusComposer(); | ||
| } | ||
| }, | ||
| insertTextAtEnd: insertComposerTextAtEnd, | ||
| openModelPicker: () => { | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| import { describe, expect, it } from "vite-plus/test"; | ||
| import { | ||
| droppedPathsResolveInEnvironment, | ||
| 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("escapes a double quote inside a quoted path", () => { | ||
| expect(quoteDroppedFilePath('/tmp/a " b.pdf')).toBe('"/tmp/a \\" b.pdf"'); | ||
| }); | ||
| it("leaves a double quote alone when there is no whitespace to quote for", () => { | ||
| expect(quoteDroppedFilePath('/tmp/a"b.pdf')).toBe('/tmp/a"b.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("preserves a trailing space in a filename instead of trimming it", () => { | ||
| expect(formatDroppedFilePaths(["/tmp/report "])).toBe('"/tmp/report "'); | ||
| }); | ||
| 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(""); | ||
| }); | ||
| }); | ||
| describe("droppedPathsResolveInEnvironment", () => { | ||
| const LOCAL = "env-local"; | ||
| const OTHER = "env-other"; | ||
| it("accepts a thread running on the desktop-managed environment", () => { | ||
| expect( | ||
| droppedPathsResolveInEnvironment({ | ||
| primarySource: "desktop-managed", | ||
| primaryEnvironmentId: LOCAL, | ||
| threadEnvironmentId: LOCAL, | ||
| }), | ||
| ).toBe(true); | ||
| }); | ||
| it("refuses a thread on a different environment even when the bridge is present", () => { | ||
| // The desktop app supervises env-local, but this thread runs on another | ||
| // machine: its agent cannot open a path read from this filesystem. | ||
| expect( | ||
| droppedPathsResolveInEnvironment({ | ||
| primarySource: "desktop-managed", | ||
| primaryEnvironmentId: LOCAL, | ||
| threadEnvironmentId: OTHER, | ||
| }), | ||
| ).toBe(false); | ||
| }); | ||
| it.each(["configured", "manual", "window-origin"])( | ||
| "refuses a %s connection, which always names a server elsewhere", | ||
| (source) => { | ||
| expect( | ||
| droppedPathsResolveInEnvironment({ | ||
| primarySource: source, | ||
| primaryEnvironmentId: LOCAL, | ||
| threadEnvironmentId: LOCAL, | ||
| }), | ||
| ).toBe(false); | ||
| }, | ||
| ); | ||
| it("refuses while either id is still unknown", () => { | ||
| // Bootstrapping is not evidence the thread runs here; guessing wrong | ||
| // inserts a path the agent silently cannot read. | ||
| expect( | ||
| droppedPathsResolveInEnvironment({ | ||
| primarySource: "desktop-managed", | ||
| primaryEnvironmentId: undefined, | ||
| threadEnvironmentId: LOCAL, | ||
| }), | ||
| ).toBe(false); | ||
| expect( | ||
| droppedPathsResolveInEnvironment({ | ||
| primarySource: "desktop-managed", | ||
| primaryEnvironmentId: LOCAL, | ||
| threadEnvironmentId: undefined, | ||
| }), | ||
| ).toBe(false); | ||
| }); | ||
| it("refuses when there is no primary environment at all", () => { | ||
| expect( | ||
| droppedPathsResolveInEnvironment({ | ||
| primarySource: undefined, | ||
| primaryEnvironmentId: undefined, | ||
| threadEnvironmentId: LOCAL, | ||
| }), | ||
| ).toBe(false); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| /** | ||
| * 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. | ||
| * | ||
| * A path is also only *meaningful* where the agent can open it. The desktop | ||
| * bridge resolves a path on the machine the client runs on, so the path is | ||
| * valid only when the thread's environment is that same machine — see | ||
| * `droppedPathsResolveInEnvironment`. | ||
| */ | ||
| /** | ||
| * Whether a filesystem path read from the local desktop bridge means anything | ||
| * to the environment a thread runs in. | ||
| * | ||
| * The bridge resolves the path on *this* machine. `desktop-managed` is the one | ||
| * environment the desktop app supervises here, so only a thread bound to that | ||
| * environment shares the filesystem the path names. Every other connection — | ||
| * a LAN or Tailscale host, a relay, a tunnel, another desktop acting as server | ||
| * — runs the agent on a different machine, where the path either does not | ||
| * exist or, worse, names a different file. | ||
| * | ||
| * Passing the ids explicitly keeps this pure: the caller reads the primary | ||
| * environment, this decides. | ||
| */ | ||
| export function droppedPathsResolveInEnvironment(input: { | ||
| readonly primarySource: string | undefined; | ||
| readonly primaryEnvironmentId: string | undefined; | ||
| readonly threadEnvironmentId: string | undefined; | ||
| }): boolean { | ||
| if (input.primarySource !== "desktop-managed") { | ||
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. 🟠 High In WSL-only mode, this predicate returns 🤖 Copy this AI Prompt to have your agent fix this: | ||
| return false; | ||
| } | ||
| // Both ids must be known. A missing id is not a match: bootstrapping is not | ||
| // evidence that the thread runs here, and guessing wrong inserts a path the | ||
| // agent silently cannot read. | ||
| return ( | ||
| input.primaryEnvironmentId !== undefined && | ||
| input.threadEnvironmentId !== undefined && | ||
| input.primaryEnvironmentId === input.threadEnvironmentId | ||
| ); | ||
| } | ||
| /** | ||
| * 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 { | ||
| if (!/\s/.test(path)) { | ||
| return path; | ||
| } | ||
| // A double quote is legal in a POSIX filename, so escape any before wrapping — | ||
| // otherwise the first inner quote reads as the closing delimiter. | ||
| return `"${path.replace(/"/g, '\\"')}"`; | ||
| } | ||
| /** | ||
| * The text inserted into the composer for a set of dropped paths. Entries that | ||
| * are empty or all whitespace are skipped (a bridge that can't resolve a file | ||
| * returns null, which the caller filters, but be defensive about "" too). | ||
| * A path that survives is never altered: leading and trailing spaces are legal | ||
| * in a filename, so trimming one would point the agent at a file that does not | ||
| * exist. Quoting keeps such a path readable instead. | ||
| */ | ||
| export function formatDroppedFilePaths(paths: ReadonlyArray<string>): string { | ||
| return paths | ||
| .filter((path) => path.trim().length > 0) | ||
| .map(quoteDroppedFilePath) | ||
| .join(" "); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.


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.
videoMimeTypereturnsstring | null(apps/web/src/types.ts), neverundefined, sovideoMimeType(...) !== undefinedis always true. Every dropped file therefore lands inattachable,byPathis always empty, andinsertDroppedFilePathsnever runs — the feature this PR adds is dead at the call site (and the no-overlap comparison should also fail typecheck).Switching to
!== nullre-exposes the classification gap flagged earlier: Finder omits the MIME type forIMG_1234.HEIC, sofile.type.startsWith("image/")routes it (and a typelessphoto.jpg) to the path branch, where a browser tab or a remote-environment thread discards it with an "Unable to add to chat" toast instead of attaching it, unlikeonComposerPaste. Reusing the already-importedclassifyComposerAttachmentFilekeeps drop and paste on the same rules and fixes both:Posted via Macroscope — UI Consistency