improving file attachement handling#2757
Conversation
|
@BugBot run |
Summary of ChangesHello @azizmejri1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the way file attachments are handled within the system. It transitions from an indirect, ID-based resolution system to a more direct and secure file copying mechanism. The change aims to improve clarity for the AI by providing direct temporary file paths and enhances security by validating source paths for copy operations. This also introduces a dedicated UI component for better user feedback on file copy actions. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
azizmejri1
force-pushed
the
improve-file-handling
branch
from
66f25fe to
4cd66bd
Compare
|
@BugBot run |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request refactors the file attachment handling mechanism. It replaces the write_file tool with a new copy_file tool for uploading files to the codebase. This change involves removing the FileUploadsState utility and directly providing temporary file paths to the AI for copying. A new DyadCopy component and associated parsing logic have been added to the frontend to display these copy operations. The changes improve the clarity and security of file handling by explicitly using a copy operation from a temporary, controlled directory. One comment's rule reference was removed as it incorrectly linked to a prompt injection rule for a path traversal issue.
I am having trouble creating individual review comments. Click here to see my feedback.
src/pro/main/ipc/handlers/local_agent/tools/copy_file.ts (44-60)
This block handles the resolution of the from path, allowing both absolute paths (with a critical security check) and relative paths. The security check ensures that absolute paths are confined to ALLOWED_TEMP_DIR, preventing path traversal attacks.
src/ipc/processors/response_processor.ts (423-435)
This security check is crucial. It ensures that if an absolute path is provided for the from argument in a dyad-copy tag, it must reside within the designated TEMP_DIR. This prevents malicious actors from using the copy_file tool to access or copy arbitrary files on the system, which could lead to information disclosure or system compromise. This directly addresses a potential path traversal vulnerability.
src/ipc/processors/response_processor.ts (417-465)
This new block of code processes dyad-copy tags. It includes important security checks to ensure that absolute from paths are restricted to the TEMP_DIR, preventing arbitrary file access. It also handles file existence checks, directory creation, file copying, and Git staging. This is a critical new piece of functionality.
src/pro/main/ipc/handlers/local_agent/tools/copy_file.ts (1-81)
This new file defines the copyFileTool, which is a core part of the new file attachment handling. It includes schema validation, consent preview, XML building, and the execution logic for copying files. The security check for absolute paths within ALLOWED_TEMP_DIR is a critical aspect of this tool.
src/ipc/handlers/chat_stream_handlers.ts (772-779)
The system prompt for local-agent mode is updated to guide the AI to use the copy_file tool with from and to paths. This replaces the old write_file and DYAD_ATTACHMENT_0 instructions, ensuring the AI uses the new, more secure method.
When files are attached for upload to the codebase, use the `copy_file` tool to copy them from their temporary path into the project.\n\nExample:\n```\ncopy_file(from="/tmp/dyad-attachments/abc123.png", to="src/assets/logo.png", description="Copy uploaded image into project")\n```\n\nThe temporary file paths are provided in the attachment information above.\n`;src/ipc/processors/response_processor.ts (113-115)
The removal of FileUploadsState instantiation and related methods (getFileUploadsForChat, clear) from processFullResponseActions confirms that the old file ID resolution logic is entirely removed from the backend processing. This simplifies the code and removes a layer of indirection.
src/ipc/handlers/chat_stream_handlers.ts (784-789)
Similarly, the system prompt for non-local-agent mode (when hasUploadedAttachments is true) is updated to use the dyad-copy tag with from and to attributes. This ensures consistency across different modes for file copying.
When files are attached for upload to the codebase, copy them into the project using this format:\n\n<dyad-copy from="/tmp/dyad-attachments/abc123.png" to="src/assets/logo.png" description="Copy uploaded file"></dyad-copy>\n\nThe temporary file paths are provided in the attachment information above.\n`;e2e-tests/fixtures/engine/local-agent/upload-to-codebase.ts (5-7)
The comment here is slightly misleading. The AI receives a temp file path and uses the copy_file tool to copy it. The original description mentioned write_file and resolving an attachment ID, which is no longer the case. The updated description is more accurate.
src/ipc/processors/response_processor.ts (422-452)
The removal of the fileUploadsMap and the logic to resolve DYAD_ATTACHMENT_0 from dyadWriteTags processing is a direct consequence of the new copy_file approach. This simplifies the write_file tool's responsibility to just writing content, while copy_file handles the transfer of uploaded files.
src/ipc/utils/file_uploads_state.ts (1-75)
The file_uploads_state.ts file and its FileUploadsState class have been completely removed. This is a significant change, as the previous mechanism of mapping file IDs to uploaded content is no longer used. The new approach directly passes temporary file paths, making this state management obsolete.
src/ipc/handlers/chat_stream_handlers.ts (314-315)
The prompt for upload-to-codebase attachments is updated to instruct the AI to use the copy_file tool and provides the temporary file path directly. This is a crucial change that aligns with the new file handling strategy.
attachmentInfo += `\n\nFile to upload to codebase: "${attachment.name}" (temp path: ${filePath})\nUse the copy_file tool (or <dyad-copy> tag) to copy this file into the codebase at the appropriate location.\n`;
e2e-tests/fixtures/engine/local-agent/upload-to-codebase.ts (16-20)
The write_file tool has been replaced with copy_file. The arguments have also changed from path and content to from and to, reflecting the new copy operation. This is a significant change in how file uploads are handled.
name: "copy_file",
args: {
from: "/tmp/dyad-attachments/abc123.png",
to: "assets/uploaded-file.png",
description: "Copy uploaded file to codebase",
},src/pro/main/ipc/handlers/local_agent/tools/file_upload_utils.ts (1-75)
The file_upload_utils.ts file has been removed, along with its resolveFileUploadContent function. This is consistent with the new approach where file IDs are no longer used, and temporary file paths are directly provided for copying.
src/pro/main/ipc/handlers/local_agent/tools/write_file.ts (53)
The logic to resolve file upload IDs has been removed from writeFileTool.execute. The write_file tool now directly writes the provided args.content, as the responsibility for handling uploaded files has shifted to the copy_file tool.
fs.writeFileSync(fullFilePath, args.content);
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (5 items)
🚫 Dropped False Positives (2 items)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
the
needs-human:review-issue
|
@BugBot run |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (3 items)
🚫 Dropped False Positives (8 items)
Generated by Dyadbot multi-agent code review |
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All code review issues addressed except one E2E fixture concern that requires a design decision about the test infrastructure. Unresolved Threads
Resolved Threads
Product Principle SuggestionsThe following suggestions could improve
🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (4 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@BugBot run |
Claude Code Review SummaryPR Confidence: 4/5All actionable review threads have been addressed; one E2E test fixture thread remains open for human decision on test infrastructure design. Unresolved Threads
Resolved Threads
Product Principle SuggestionsThe following suggestions could improve rules/product-principles.md to help resolve ambiguous cases in the future:
Generated by Claude Code |
I was thinking saving the attachments to dyad-media will allow the users to see their attached files in the file explorer ,unlike using a hidden folder like .dyad/media (this argument may become invalid after shipping the media library) |
|
@azizmejri1 got it. I think we should do |
wwwillchen
left a comment
wwwillchen
left a comment
There was a problem hiding this comment.
LGTM. before merging, let's do the .dyad/media change from the earlier comment.
after this PR, let's do a clean-up routine similar to what we do for aimessagesjson:
Line 93 in b48edb0
e.g. delete media files in .dyad/media more than 30 days old
|
@BugBot run |
![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: 098f1d7311
ℹ️ 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".
| attachmentInfo += `\n\nFile to upload to codebase: ${attachment.name} (file id: ${fileId})\n`; | ||
| if (attachment.attachmentType === "upload-to-codebase") { | ||
| // Provide the .dyad/media path so the AI can copy it into the codebase | ||
| attachmentInfo += `\n\nFile to upload to codebase: "${attachment.name}" (path: ${persistentPath})\nUse the copy_file tool (or <dyad-copy> tag) to copy this file into the codebase at the appropriate location.\n`; |
There was a problem hiding this comment.
For upload-to-codebase attachments, this inserts persistentPath (an absolute local filesystem path) into attachmentInfo, and that text is later appended to userPrompt and sent to the model. In hosted-model sessions this leaks user-specific path data (for example usernames/home directory structure) to third-party providers and unnecessarily expands the privacy surface compared to the previous opaque IDs; use an app-relative path like .dyad/media/<hash> for model-facing instructions instead.
Useful? React with 👍 / 👎.
| fs.mkdirSync(mediaDir, { recursive: true }); | ||
| } | ||
| await ensureDyadGitignored(appPath); |
There was a problem hiding this comment.
Attachment handling now unconditionally creates ${appPath}/.dyad/media and updates .gitignore before processing the request; if an app was imported from a non-writable location (for example skipCopy imports on read-only paths), mkdirSync/ensureDyadGitignored will throw and the whole chat request fails even for chat-context-only uploads. A fallback to temp storage (or deferring writes until a code-modifying action) is needed to avoid breaking attachment usage in those environments.
Useful? React with 👍 / 👎.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. This PR already has extensive review coverage (~50+ existing comments). After cross-referencing, only one new MEDIUM issue was found that hasn't been previously flagged. Issues Summary
🟢 Low Priority Notes (10 items)
🚫 Dropped False Positives (11 items)
Generated by Dyadbot multi-agent code review |
| <div | ||
| className={`relative ${SIZE_CLASSES[size]} rounded-lg overflow-hidden border border-border/60 cursor-pointer hover:brightness-90 transition-all`} | ||
| onClick={() => setIsExpanded(true)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| setIsExpanded(true); | ||
| } | ||
| }} | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-label={`Expand image: ${name}`} | ||
| title={name} | ||
| > | ||
| <img |
There was a problem hiding this comment.
🟡 MEDIUM | visual-feedback
Image thumbnails have no loading placeholder
When an image attachment renders, there's no placeholder or skeleton shown while the image loads from the dyad-media:// protocol. Users see an empty bordered box that suddenly pops in with the image, causing a perceived layout shift or flash of empty content. This is especially noticeable for larger images or slower disk reads.
💡 Suggestion: Add a lightweight loading skeleton or a subtle Image icon placeholder inside the thumbnail container that shows until the image's onLoad event fires. A simple approach:
const [imageLoaded, setImageLoaded] = useState(false);
// In the thumbnail:
{!imageLoaded && (
<div className="absolute inset-0 flex items-center justify-center bg-muted animate-pulse">
<Image size={20} className="text-muted-foreground" />
</div>
)}
<img ... onLoad={() => setImageLoaded(true)} />
🎭 Playwright Test Results❌ Some tests failed
Summary: 715 passed, 4 failed, 9 flaky, 224 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/capacitor.spec.ts \
e2e-tests/concurrent_chat.spec.ts \
e2e-tests/context_manage.spec.ts \
e2e-tests/setup_flow.spec.ts
|
🎭 Playwright Test Results❌ Some tests failed
Summary: 722 passed, 1 failed, 8 flaky, 224 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/setup_flow.spec.ts
|
github-actions
bot
added
needs-human:final-check
🎭 Playwright Test Results✅ All tests passed!
Total: 724 tests passed (8 flaky) (224 skipped)
|
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. This PR already has extensive review coverage (~60+ existing comments across many rounds). After cross-referencing, three new MEDIUM findings below. Issues Summary
🟢 Low Priority Notes (1 item)
🚫 Dropped False Positives (7 items)
Generated by Dyadbot multi-agent code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c666281a2e
ℹ️ 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".
| to: string; | ||
| description?: string; | ||
| }[] { | ||
| const dyadCopyRegex = /<dyad-copy([^>]*?)(?:>([\s\S]*?)<\/dyad-copy>|\/>)/gi; |
There was a problem hiding this comment.
getDyadCopyTags now executes self-closing copy tags (<dyad-copy ... />), but the renderer path (parseCustomTags in src/components/chat/DyadMarkdownParser.tsx) only recognizes paired tags (<dyad-copy>...</dyad-copy>). When a provider emits the self-closing form, the copy still runs in processFullResponseActions, but no DyadCopy card/state is rendered, so users lose visibility into a state-changing action.
Useful? React with 👍 / 👎.
| const mediaDir = path.join(appPath, DYAD_MEDIA_DIR_NAME); | ||
| if (!fs.existsSync(mediaDir)) { | ||
| fs.mkdirSync(mediaDir, { recursive: true }); | ||
| } | ||
| await ensureDyadGitignored(appPath); |
There was a problem hiding this comment.
Attachment processing now always writes into {appPath}/.dyad/media and updates .gitignore before streaming starts. In read-only or permission-restricted projects, this throws in chat:stream before any model call, so even chat-context attachments cannot be used. This is a regression from temp-dir-backed handling because it turns a normal user action into a hard request failure in those environments.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // Process all file copies | ||
| const dyadCopyTags = getDyadCopyTags(fullResponse); |
There was a problem hiding this comment.
🟡 MEDIUM | ordering
Copy operations execute before writes, breaking write-then-copy sequences
Copy tags are processed here (lines 419-450) before write tags (lines 452+). If an AI response contains a <dyad-write> to create a file followed by a <dyad-copy> that references that newly-written file as its source, the copy will fail with "Source file does not exist" because copies execute first.
While the primary use case is copying from .dyad/media/ (pre-existing files), the copy_file tool description says it can "copy files within the codebase," which enables this write-then-copy pattern.
💡 Suggestion: Either move copy processing to after write processing, or document that copies from files created in the same response are not supported.
| * ".dyad" + "media" path segments with at least one segment (filename) after, | ||
| * then confirms the resolved path doesn't escape via ".." traversal. | ||
| */ | ||
| export function isFileWithinAnyDyadMediaDir(absPath: string): boolean { |
There was a problem hiding this comment.
🟡 MEDIUM | missing-why-comment
Two similar validation functions lack guidance on when to use which
isWithinDyadMediaDir(absPath, appPath) (line 12) and isFileWithinAnyDyadMediaDir(absPath) (line 30) serve overlapping purposes with different trade-offs. The first is precise but requires a known app path; the second uses heuristic segment matching for contexts where the app path isn't available (e.g., shell_handler.ts). However, there's no comment explaining this distinction, and a future maintainer could easily use the wrong one or attempt to consolidate them.
💡 Suggestion: Add a brief doc comment above each: e.g., /** Use when app path is available (preferred - more precise). */ and /** Use when app path is unknown (e.g., generic IPC handlers). Less precise but safe. */
| @@ -0,0 +1,58 @@ | |||
| import type React from "react"; | |||
| import type { ReactNode } from "react"; | |||
| import { Copy } from "lucide-react"; | |||
There was a problem hiding this comment.
🟡 MEDIUM | icon-semantics
Clipboard 'Copy' icon is semantically misleading for a file copy operation
The Copy icon from lucide-react is a clipboard/document-copy icon (two overlapping rectangles), universally associated with "copy to clipboard." Combined with the "Copy" badge text, users may mistake this card for a clipboard action rather than a file duplication operation. Other file-operation cards use semantically appropriate icons: DyadWrite uses Pencil, DyadRename uses FileEdit, DyadDelete uses a trash icon.
💡 Suggestion: Use a file-oriented icon like Files (two file icons), CopyPlus, or FileOutput from lucide-react to clearly convey "file copy."
github-actions
bot
added
needs-human:review-issue
🎭 Playwright Test Results❌ Some tests failed
Summary: 710 passed, 5 failed, 10 flaky, 230 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/mcp.spec.ts \
e2e-tests/problems.spec.ts
|
github-actions
bot
added
needs-human:final-check
🎭 Playwright Test Results✅ All tests passed!
Total: 721 tests passed (9 flaky) (230 skipped)
|
🎭 Playwright Test Results❌ Some tests failed
Summary: 720 passed, 2 failed, 6 flaky, 230 skipped Failed Tests🪟 Windows
|
2 participants
closes #2576