t3code/support non-image chat attachments - #3927
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size:M
ApprovabilityVerdict: Needs human review This PR introduces new text attachment functionality with async file writing and prompt manipulation. Multiple unresolved review comments identify potential race conditions and state management bugs (including a high-severity parallel drops issue) that warrant human review. You can customize Macroscope's approvability policy. Learn more. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| setIsDragOverComposer(false); | ||
| const files = Array.from(event.dataTransfer.files); | ||
| addComposerImages(files); | ||
| void addComposerAttachments(files); |
There was a problem hiding this comment.
Send races text attachment
Medium Severity
When text files are dropped or pasted, their asynchronous processing to write the file and add its link to the prompt is not awaited. This allows the composer's prompt to be submitted prematurely, resulting in messages sent without the intended attachment link and potentially leaving orphaned attachment files.
Reviewed by Cursor Bugbot for commit f044e40. Configure here.
| currentPrompt.length, | ||
| currentPrompt.length, | ||
| `${separator}${serializeComposerFileLink(resolvePathLinkTarget(relativePath, gitCwd))} `, | ||
| ); |
There was a problem hiding this comment.
Parallel drops corrupt prompt
High Severity
Each text attachment appends using applyPromptReplacement at promptRef.current.length captured after its own async work, with no serialization across overlapping addComposerAttachments calls. Concurrent drops can insert links at the same stale offset and splice into the middle of the prompt instead of the end.
Reviewed by Cursor Bugbot for commit f044e40. Configure here.
| const imageFiles = files.filter((file) => file.type.startsWith("image/")); | ||
| if (imageFiles.length === 0) return; | ||
| event.preventDefault(); | ||
| addComposerImages(imageFiles); |
There was a problem hiding this comment.
Mixed batch errors overwritten
Medium Severity
In a mixed drop, text failures use error = (await addComposerTextAttachment(file)) ?? error, but image size and count checks assign error = ... directly. A later image error replaces an earlier text error, and a lingering image error can remain after a later text file attaches successfully.
Reviewed by Cursor Bugbot for commit f044e40. Configure here.
| const imageFiles = files.filter((file) => file.type.startsWith("image/")); | ||
| if (imageFiles.length === 0) return; | ||
| event.preventDefault(); | ||
| addComposerImages(imageFiles); |
There was a problem hiding this comment.
Image limit skips text files
Low Severity
When the per-message image cap is reached, addComposerAttachments breaks out of the file loop. Any non-image files later in the same DataTransfer list are never passed to addComposerTextAttachment, so they are dropped silently aside from the image-limit error.
Reviewed by Cursor Bugbot for commit f044e40. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f044e403be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| setIsDragOverComposer(false); | ||
| const files = Array.from(event.dataTransfer.files); | ||
| addComposerImages(files); | ||
| void addComposerAttachments(files); |
There was a problem hiding this comment.
For dropped markdown/text files this handler now fire-and-forgets an async path: addComposerAttachments waits for file.arrayBuffer() and projects.writeFile before inserting the markdown link. If the user presses Send before that RPC finishes, onSend snapshots the prompt without the file link and clears it; the async callback then appends the link afterward, so the turn is sent without the attachment and a stale attachment link is left in the composer.
Useful? React with 👍 / 👎.
| return `'${file.name}' exceeds the 1 MB text attachment limit.`; | ||
| } | ||
| const safeName = file.name.replace(/[^a-zA-Z0-9._-]+/g, "-") || "context.md"; | ||
| const relativePath = `.t3/attachments/${randomUUID()}/${safeName}`; |
There was a problem hiding this comment.
This writes every dropped text/markdown attachment under .t3/attachments inside the user's project, but the code does not ensure that .t3/ is ignored for arbitrary workspaces. In projects that have not already added that ignore rule, attaching a file creates untracked files that show up in git status and can be accidentally committed along with the user's changes.
Useful? React with 👍 / 👎.
| applyPromptReplacement( | ||
| currentPrompt.length, | ||
| currentPrompt.length, | ||
| `${separator}${serializeComposerFileLink(resolvePathLinkTarget(relativePath, gitCwd))} `, |
There was a problem hiding this comment.
When the first message is sent in New worktree mode, gitCwd is still the original project root because the worktree has not been created yet, so this inserts an absolute link to .t3/attachments in the original checkout. The bootstrap path then creates the worktree and starts the provider session there, leaving the model with a prompt that points outside its active workspace/sandbox instead of to a file in the worktree.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7e248cb18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const currentPrompt = promptRef.current; | ||
| const separator = currentPrompt.length > 0 && !/\s$/.test(currentPrompt) ? " " : ""; | ||
| applyPromptReplacement( |
There was a problem hiding this comment.
When a dropped text file is still awaiting arrayBuffer()/writeProjectFile and the user switches to another thread or draft, this continuation reads the shared promptRef.current after it has been updated for the new composer, while the closure still writes through the old composerDraftTarget. That can overwrite the old draft with the new thread's prompt plus the attachment link, or append a stale link after navigation; capture the original draft prompt/target or abort if the target changed before applying the replacement.
Useful? React with 👍 / 👎.
jakeleventhal
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a5cf3e282
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const file of files) { | ||
| if (!file.type.startsWith("image/")) { | ||
| error = `Unsupported file type for '${file.name}'. Please attach image files only.`; | ||
| error = (await addComposerTextAttachment(file)) ?? error; |
There was a problem hiding this comment.
When a drop contains many non-image files, this branch awaits addComposerTextAttachment for every file and writes each one (up to 1 MB) into the workspace before appending a link. The existing attachment count guard only applies in the image branch below, so a user can accidentally enqueue hundreds of workspace writes/refreshes and create a very large prompt by dropping many small text files; add a count or aggregate-size limit before writing text attachments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
There are 6 total unresolved issues (including 4 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 26a4c8e. Configure here.
| }); | ||
| }) | ||
| .catch(() => null); | ||
| if (result === null) return `'${file.name}' is not a supported text file.`; |
There was a problem hiding this comment.
Write failures show unsupported file
Medium Severity
In addComposerTextAttachment, workspace write failures and Thrown errors from writeProjectFile are folded into null and surfaced as “not a supported text file,” hiding real attach/write problems from valid UTF-8 inputs.
Reviewed by Cursor Bugbot for commit 26a4c8e. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26a4c8e14f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| applyPromptReplacement( | ||
| currentPrompt.length, | ||
| currentPrompt.length, | ||
| `${separator}${serializeComposerFileLink(resolvePathLinkTarget(relativePath, gitCwd))} `, |
There was a problem hiding this comment.
When the workspace root is an unrecognized absolute prefix such as /workspace and the dropped text file has no extension (Dockerfile, Makefile, LICENSE), this inserts a prompt link like [Dockerfile](/workspace/.../.t3/attachments/.../Dockerfile). ChatMarkdown only classifies absolute POSIX links as file links when the root is allow-listed or the basename has an extension (apps/web/src/markdown-links.ts lines 95-116), so the sent message renders as a normal broken web link instead of the file chip/preview link. Using the relative .t3/... target here, or broadening file-link detection, keeps these generated attachments usable in container/cloud workspaces.
Useful? React with 👍 / 👎.
- Store supported text files in the project workspace - Insert attachment links into the chat prompt
- Base user-message collapse thresholds on visible link text - Add regression coverage for markdown attachment links
26a4c8e to
2b01a69
Compare
There was a problem hiding this comment.
💡 Codex Review
t3code/apps/web/src/components/chat/ChatComposer.tsx
Lines 1926 to 1928 in 2b01a69
Continue processing text files after reaching the image cap
When the composer already has eight images, or the current drop reaches that limit, encountering one additional image executes this break and silently skips every later file in the same drop. This now incorrectly discards otherwise supported text attachments that happen to follow the excess image, even though the reported limit applies only to images; record the image error and continue scanning the remaining files instead.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const visibleText = text.replace(USER_MESSAGE_FILE_LINK_PATTERN, (_source, label: string) => | ||
| label.replace(/\\(.)/g, "$1"), | ||
| ); |
There was a problem hiding this comment.
When a long link-shaped string appears inside inline/fenced code or is escaped (for example, `[x](/very-long-literal-path)`), ChatMarkdown renders the entire destination literally, but this raw regex still replaces it with only x. A visibly long user message can therefore fall below the 600-character threshold and lose its collapse controls; restrict the replacement to links that Markdown actually parses as links rather than matching the source text indiscriminately.
Useful? React with 👍 / 👎.
| let error: string | null = null; | ||
| for (const file of files) { | ||
| if (!file.type.startsWith("image/")) { | ||
| error = `Unsupported file type for '${file.name}'. Please attach image files only.`; | ||
| error = (await addComposerTextAttachment(file)) ?? error; |
There was a problem hiding this comment.
When a drop includes a text file, this loop yields after nextImageCount has already snapshotted composerImagesRef.current; if another image drop completes while the text file is being written, the first continuation validates and appends its images using the stale count. Since composerDraftStore.addImages does not enforce the cap, the composer can end up with more than the shared eight-image limit used by ProviderSendTurnInput, so reserve slots atomically or re-read the current count after each await.
Useful? React with 👍 / 👎.
|
I need this, plenty of times I want to give context to the chat and I had to copy the path manually in order to work, in codex cli automatically I can paste and it gives the filepath so the model can fetch it. |
2 participants
Verification
Summary
.t3/attachmentsand insert standard file-link chips into the promptWhy
The composer previously rejected every dropped file that was not an image. This made it impossible to attach Markdown, source, configuration, or other text files as agent context.
Text attachments now reuse the existing project file-writing and Markdown file-link paths, so they render and behave like other file references. Files are limited to 1 MB and rejected when they contain NUL bytes or invalid UTF-8.
The message collapse heuristic also now measures displayed link labels instead of their backing absolute paths. Without this, prompts containing several attachments could appear hidden even though their visible content was short.
Validation
pnpm exec vp test apps/web/src/components/chat/MessagesTimeline.test.tsxpnpm exec vp checkpnpm exec vp run typecheckNote
Low Risk
Composer and timeline UI changes only; text files are validated and written through existing project file APIs with size and encoding limits.
Overview
The chat composer now accepts non-image files on drop (and still handles images through the same path). Dropped text files are validated as UTF-8 (no NUL bytes), capped at 1 MB, written under
.t3/attachments/<uuid>/, and the prompt gets a markdown file link via the existing project write + file-link flow.User message collapse in the timeline no longer treats long link URLs as message bulk—it measures visible link labels only, so short prompts with many attachment links stay expanded.
A regression test covers the collapse behavior for prompts with many file links.
Reviewed by Cursor Bugbot for commit 2b01a69. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add non-image file attachment support to the chat composer
.t3/attachments/<uuid>/<safeName>in the project, with a markdown file link inserted into the prompt.addComposerAttachmentshandler in ChatComposer.tsx routes files to the appropriate path based on MIME type.shouldCollapseUserMessagein MessagesTimeline.tsx now strips markdown link destinations before measuring message length, so messages with short labels but long file URLs are no longer incorrectly collapsed.Macroscope summarized 2b01a69.