Prompt the user for attachement type after dragging the file#2563
Prompt the user for attachement type after dragging the file#2563wwwillchen merged 5 commits intodyad-sh:mainfrom
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 enhances the user experience for file attachments by introducing an intermediate step when files are dragged and dropped. Instead of automatically attaching files as chat context, users are now presented with a dialog to explicitly choose how they want the files to be handled: either as context for the AI or as files to be uploaded to the codebase. This provides greater control and clarity for the user regarding the purpose of their attached files. Highlights
Changelog
Activity
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
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Greptile OverviewGreptile SummaryThis PR adds a dialog to prompt users for attachment type (chat context vs. upload to codebase) after dragging or pasting files. The implementation introduces a
Issues Found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ChatInput
participant useAttachments
participant FileAttachmentTypeDialog
participant attachmentsAtom
User->>ChatInput: Drag/Paste files
ChatInput->>useAttachments: handleDrop/handlePaste
useAttachments->>useAttachments: setPendingFiles(files)
useAttachments-->>ChatInput: pendingFiles updated
ChatInput->>FileAttachmentTypeDialog: Show dialog (pendingFiles)
FileAttachmentTypeDialog-->>User: Display options
User->>FileAttachmentTypeDialog: Select attachment type
FileAttachmentTypeDialog->>useAttachments: confirmPendingFiles(type)
useAttachments->>useAttachments: addAttachments(pendingFiles, type)
useAttachments->>attachmentsAtom: setAttachments([...attachments, ...new])
useAttachments->>useAttachments: setPendingFiles(null)
useAttachments-->>FileAttachmentTypeDialog: pendingFiles = null
FileAttachmentTypeDialog-->>User: Dialog closes
User->>ChatInput: Submit message
ChatInput->>useAttachments: Check pendingFiles
alt pendingFiles exists
ChatInput-->>User: Block submission
else pendingFiles is null
ChatInput-->>User: Allow submission
end
|
![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
This pull request introduces a dialog that prompts the user to select an attachment type ('chat context' or 'upload to codebase') after dragging and dropping files into the chat input. The changes are well-implemented across the useAttachments hook, a new FileAttachmentTypeDialog component, and updates to ChatInput and HomeChatInput. The e2e tests have also been updated to cover this new user flow. My feedback includes a suggestion to improve the implementation of useCallback in the useAttachments hook for better stability and adherence to React best practices.
🔍 Multi-Agent Code ReviewFound 3 new issue(s) flagged by 3 independent reviewers. Summary
Issues to Address
🟢 Low Priority Issues (2 items)
See inline comments for details. Generated by multi-agent consensus review (3 independent agents) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
wwwillchen
left a comment
wwwillchen
left a comment
There was a problem hiding this comment.
feedback from mtg: should do this same UX for pasting via keyboard shortcut
|
@BugBot run |
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 specialized agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (4 items)
Generated by Dyadbot swarm code review (3 agents, consensus-based) |
🔍 Dyadbot Code Review Summary (Updated)Verdict: ⛔ NO - Do NOT merge — Has significant data-integrity and UX regression issues that need to be fixed. Reviewed by 3 specialized agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
Note: Issue #1 was upgraded to HIGH after cross-discussion — all three reviewers agreed this is a real data-loss scenario where files are silently dropped. 🟢 Low Priority Notes (4 items)
Generated by Dyadbot swarm code review (3 agents, consensus-based) |
|
@BugBot run |
🔍 Dyadbot Swarm Code Review SummaryVerdict: ⛔ NO - Do NOT merge — Has a HIGH severity data-loss issue plus multiple MEDIUM issues that should be addressed. Reviewed by 3 specialized agents (Correctness Expert, Code Health Expert, UX Wizard) using consensus-based discussion. Issues Summary
🆕 = newly found by this swarm review (not in prior reviews) RecommendationIssue #1 (HIGH) is a data-loss scenario that must be addressed. Issue #2 (MEDIUM) is a significant UX regression for the common screenshot paste workflow. Issues #3-#6 are new findings from this swarm review that should also be addressed. 🟢 Low Priority Notes (6 items)
Generated by Dyadbot swarm code review (3 agents, consensus-based) |
azizmejri1
force-pushed
the
file-attachement-type
branch
from
e53fa88 to
a74a16f
Compare
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 specialized agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (4 items)
ℹ️ Pre-existing Notes (not blocking)
Reviewer Assessments
Bottom line: No HIGH issues. The MEDIUM UX suggestions (showing filenames, accessibility improvements) are nice-to-haves that could be addressed in a follow-up. The core implementation is solid and the feature meaningfully improves the attachment flow. Generated by Dyadbot code review |
@wwwillchen done |
| </div> | ||
| </div> | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🟡 MEDIUM | interaction | Found by: UX
No visual feedback showing which file(s) the user is deciding about
When the dialog appears after a drag/drop or paste, the user sees "How would you like to attach this file?" but has no indication of WHICH file(s) they just dropped. For a single file, showing the filename would help confirm they dropped the right file. For multiple files, a brief list would give the user confidence about what they're attaching.
💡 Suggestion: Display the filename(s) in the dialog body below the description. For multiple files, show a compact list (max 3-4 visible, rest behind a "+ N more" indicator).
| <div className="text-xs text-muted-foreground mt-0.5"> | ||
| {t("attachFileContextExample")} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🟡 MEDIUM | accessibility | Found by: UX
Card-style buttons could benefit from aria-label for screen readers
The two option buttons are styled as card-like elements with title + description text. While they are semantic <button> elements (good), screen reader users will hear the full inner text concatenated which may be confusing (e.g. "Attach file as chat context Example use case: screenshot of the app to point out a UI issue").
💡 Suggestion: Add aria-label attributes to each button with concise labels (e.g., "Attach as chat context" and "Upload to codebase"), or use aria-describedby to separate the title from the description for screen readers.
github-actions
bot
added
the
needs-human:review-issue
|
@BugBot run |
- Add type="button" and focus-visible ring styles to dialog buttons - Prevent submit while attachment type dialog is open - Clear pendingFiles in clearAttachments to avoid orphaned state - Deduplicate confirmPendingFiles by reusing addAttachments - Add i18n translations for FileAttachmentTypeDialog (en, pt-BR, zh-CN) Co-Authored-By: Claude Opus 4.6 <[email protected]>
azizmejri1
force-pushed
the
file-attachement-type
branch
from
ec27eef to
b625847
Compare
|
@BugBot run |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const confirmPendingFiles = useCallback( | ||
| (type: "chat-context" | "upload-to-codebase") => { | ||
| if (pendingFiles) { | ||
| addAttachments(pendingFiles, type); | ||
| setPendingFiles(null); | ||
| } | ||
| }, | ||
| [pendingFiles, addAttachments], | ||
| ); |
There was a problem hiding this comment.
addAttachments missing from dependency array causes stale closure
The useCallback depends on addAttachments but doesn't include it in the dependency array. This will capture a stale version of addAttachments and could cause bugs if addAttachments behavior changes.
| const confirmPendingFiles = useCallback( | |
| (type: "chat-context" | "upload-to-codebase") => { | |
| if (pendingFiles) { | |
| addAttachments(pendingFiles, type); | |
| setPendingFiles(null); | |
| } | |
| }, | |
| [pendingFiles, addAttachments], | |
| ); | |
| const confirmPendingFiles = useCallback( | |
| (type: "chat-context" | "upload-to-codebase") => { | |
| if (pendingFiles) { | |
| addAttachments(pendingFiles, type); | |
| setPendingFiles(null); | |
| } | |
| }, | |
| [pendingFiles, addAttachments], | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/useAttachments.ts
Line: 82:90
Comment:
`addAttachments` missing from dependency array causes stale closure
The `useCallback` depends on `addAttachments` but doesn't include it in the dependency array. This will capture a stale version of `addAttachments` and could cause bugs if `addAttachments` behavior changes.
```suggestion
const confirmPendingFiles = useCallback(
(type: "chat-context" | "upload-to-codebase") => {
if (pendingFiles) {
addAttachments(pendingFiles, type);
setPendingFiles(null);
}
},
[pendingFiles, addAttachments],
);
```
How can I resolve this? If you propose a fix, please make it concise.
| setPendingFiles(null); | ||
| } | ||
| }, | ||
| [pendingFiles, addAttachments], |
There was a problem hiding this comment.
🟡 MEDIUM | code-health | Found by: Code Health, Endorsed by: Correctness, UX
addAttachments is not memoized, making confirmPendingFiles useCallback ineffective
addAttachments is listed in the dependency array of confirmPendingFiles's useCallback, but addAttachments itself is not wrapped in useCallback. Since it's a plain function declared in the hook body, it's recreated on every render — which means confirmPendingFiles also gets a new reference on every render, defeating the purpose of the useCallback wrapper.
💡 Suggestion: Either wrap addAttachments in useCallback as well, or inline the attachment-creation logic directly within confirmPendingFiles to avoid the stale dependency.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 specialized agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (2 items)
🚫 Dropped Issues (0 items)No issues were dropped during discussion. Generated by Dyadbot swarm code review |
🔍 Dyadbot Code Review — Additional Finding from DiscussionAfter the initial review was posted, the cross-reviewer discussion surfaced one additional consensus issue: 🟡 MEDIUM — Send button
|
🎭 Playwright Test Results❌ Some tests failed
Summary: 694 passed, 3 failed, 8 flaky, 220 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/security_review.spec.ts \
e2e-tests/select_component.spec.ts
|
…#2563) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/dyad-sh/dyad/pull/2563" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> <!-- This is an auto-generated description by cubic. --> --- Show a dialog after dragging or pasting files into chat to choose how to attach them—as chat context or upload to codebase. This makes intent explicit and blocks submit, drop, and paste until you choose. - **New Features** - Added FileAttachmentTypeDialog; integrated in ChatInput and HomeChatInput. i18n (en, pt-BR, zh-CN) with singular/plural titles and descriptions. - Updated useAttachments with pendingFiles and confirm/cancel. Drag/paste set pendingFiles; prevent attaching while pending; clearAttachments also clears pendingFiles; submit blocked when dialog is open. - Fixed e2e to select “Attach file as chat context”; dialog buttons use type="button" with focus-visible ring. - **Refactors** - confirmPendingFiles reuses addAttachments to deduplicate logic. <sup>Written for commit b625847. Summary will update on new commits.</sup> <!-- End of auto-generated description by cubic. --> --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
2 participants
Summary by cubic
Show a dialog after dragging or pasting files into chat to choose how to attach them—as chat context or upload to codebase. This makes intent explicit and blocks submit, drop, and paste until you choose.
New Features
Refactors
Written for commit b625847. Summary will update on new commits.