File upload #3
File upload #3
Conversation
Summary of ChangesHello @Ericzhou-ez, 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 delivers a complete file upload solution for the Witely chat application, significantly enhancing user interaction by allowing them to attach diverse media and document types. The system is designed with a strong emphasis on security, ensuring all uploads are authenticated, sanitized, and stored securely. It also provides a seamless user experience through an intuitive drag-and-drop interface and intelligent model compatibility features, which guide users in selecting appropriate AI models for their attached content. Highlights
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
|
![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 is an impressive pull request that adds a comprehensive file upload system. The code is well-structured, with new logic for file handling, validation, and compatibility checking correctly separated into dedicated utility files. The UI changes, including the drag-and-drop functionality and model compatibility feedback, significantly improve the user experience.
I've identified a few areas for improvement, including a critical issue in components/message.tsx that prevents component updates, and some high-severity issues related to type safety and security. I've also left some medium-severity comments on maintainability and documentation consistency. Overall, great work on this complex feature.
components/message.tsx
Outdated
| } | ||
|
|
||
| return false; | ||
| return true; |
There was a problem hiding this comment.
The comparison function for React.memo always returns true, which will prevent the PreviewMessage component from ever re-rendering, even when its props change. This is a critical bug that will cause the UI to become stale and not reflect new data.
You should either remove the custom comparison function to default to a shallow comparison, or implement a proper deep comparison. Since fast-deep-equal is already imported in this file, you can use it for a correct deep comparison.
| return true; | |
| return equal(prevProps, nextProps); |
There was a problem hiding this comment.
no this fix would prevent rerenders during streaming
app/(chat)/api/chat/route.ts
Outdated
|
|
||
| // Check if content is too large (max 0.5MB for text files) | ||
| const maxSize = 0.5 * 1024 * 1024; | ||
| if (content.length > maxSize) { |
There was a problem hiding this comment.
Using content.length to check the file size is not reliable for strings containing multi-byte characters (e.g., emojis, non-ASCII characters). This could lead to incorrect size validation. To get the accurate byte length of a UTF-8 string, you should use new TextEncoder().encode(content).length.
| if (content.length > maxSize) { | |
| if (new TextEncoder().encode(content).length > maxSize) { |
app/(chat)/api/files/upload/route.ts
Outdated
| } | ||
|
|
||
| // Sanitize filename to prevent path traversal | ||
| const sanitizedFilename = filename.replace(/[^a-zA-Z0-9._-]/g, "_"); |
There was a problem hiding this comment.
The PR description mentions using the sanitize-filename library for sanitization, but the implementation uses a simple regex replacement. While this provides basic protection, a dedicated library is more robust as it handles many more edge cases, invalid characters across different operating systems, and other security considerations. I recommend using the sanitize-filename library as intended to improve security and robustness.
You would need to add import sanitize from "sanitize-filename"; at the top of the file.
| const sanitizedFilename = filename.replace(/[^a-zA-Z0-9._-]/g, "_"); | |
| const sanitizedFilename = sanitize(filename); |
components/message.tsx
Outdated
| key={ | ||
| fileAttachment.url ?? | ||
| `attachment-${Math.random()}` | ||
| } |
There was a problem hiding this comment.
Using Math.random() as a fallback for a React key is an anti-pattern. Keys must be stable across renders for React to correctly identify and update elements. A random key will cause the component to be re-mounted on every render, which can lead to performance degradation and state-related bugs.
Since you are mapping over attachmentsFromMessage, you can get the attachmentIndex and use it to create a stable key.
| key={ | |
| fileAttachment.url ?? | |
| `attachment-${Math.random()}` | |
| } | |
| key={ | |
| fileAttachment.url ?? | |
| `attachment-${index}-${attachmentIndex}` | |
| } |
| .filter( | ||
| (p: any): p is { type: "file"; url: string; name: string } => | ||
| p.type === "file" | ||
| ); |
There was a problem hiding this comment.
The type guard (p: any): p is ... is unsafe. The p parameter comes from a jsonb field and could be anything (e.g., null, a string, a number), not just an object. Accessing p.type directly could throw a runtime error if p is not an object. You should add checks to ensure p is a non-null object before accessing its properties.
| .filter( | |
| (p: any): p is { type: "file"; url: string; name: string } => | |
| p.type === "file" | |
| ); | |
| .filter( | |
| (p: any): p is { type: "file"; url: string; name: string } => | |
| typeof p === "object" && p !== null && p.type === "file" | |
| ); |
| const content = await response.text(); | ||
|
|
||
| // Check if content is too large (max 0.5MB for text files) | ||
| const maxSize = 0.5 * 1024 * 1024; |
There was a problem hiding this comment.
The max size for text files (0.5 * 1024 * 1024) is hardcoded here. It would be better to define this as a named constant, for example MAX_TEXT_FILE_SIZE_BYTES, at the top of the file or in a shared constants file. This improves maintainability and makes the code easier to understand.
| attachments={attachments.map((att: any) => ({ | ||
| name: att.name, | ||
| mediaType: att.mediaType, | ||
| }))} |
There was a problem hiding this comment.
The att parameter is typed as any, which bypasses TypeScript's type checking and reduces type safety. A more specific type should be used. Since the attachments array is filtered to contain only file parts, you can use a type assertion to safely access the name and mediaType properties.
| attachments={attachments.map((att: any) => ({ | |
| name: att.name, | |
| mediaType: att.mediaType, | |
| }))} | |
| attachments={attachments.map((att) => ({ | |
| name: (att as { name?: string }).name, | |
| mediaType: (att as { mediaType?: string }).mediaType, | |
| }))} |
| className="absolute top-0.5 right-0.5 size-3 rounded-full p-0 opacity-0 transition-opacity group-hover:opacity-100" | ||
| onClick={onRemove} | ||
| size="sm" | ||
| variant="destructive" | ||
| variant={"outline"} | ||
| > | ||
| <CrossSmallIcon size={8} /> | ||
| <CrossSmallIcon size={2} /> | ||
| </Button> |
There was a problem hiding this comment.
The remove button for attachments is quite small (size-3) and uses an outline variant, which might make it difficult to see and click, especially on smaller screens. Consider making the button slightly larger (e.g., size-4) and using a more prominent variant like destructive to improve visibility and usability.
| className="absolute top-0.5 right-0.5 size-3 rounded-full p-0 opacity-0 transition-opacity group-hover:opacity-100" | |
| onClick={onRemove} | |
| size="sm" | |
| variant="destructive" | |
| variant={"outline"} | |
| > | |
| <CrossSmallIcon size={8} /> | |
| <CrossSmallIcon size={2} /> | |
| </Button> | |
| className="absolute top-0.5 right-0.5 size-4 rounded-full p-0 opacity-0 transition-opacity group-hover:opacity-100" | |
| onClick={onRemove} | |
| size="sm" | |
| variant="destructive" | |
| > | |
| <CrossSmallIcon size={8} /> |
lib/ai/FILE_UPLOAD_GUIDE.md
Outdated
|
|
||
| 1. Text files fetched from URLs have a 30-second timeout | ||
| 2. Text file content is limited to 0.5MB to prevent context overflow | ||
| 3. Maximum 10 attachments per message |
There was a problem hiding this comment.
The documentation states a maximum of 10 attachments per message, but the implementation in lib/ai/file-upload.ts (MAX_ATTACHMENTS) and the PR description specify a limit of 8. Please update the documentation to be consistent with the code.
| 3. Maximum 10 attachments per message | |
| 3. Maximum 8 attachments per message |
| }, | ||
| { | ||
| protocol: "https", | ||
| hostname: "**.public.blob.vercel-storage.com", |
There was a problem hiding this comment.
The hostname **.public.blob.vercel-storage.com uses a multi-level wildcard (**). Vercel Blob storage URLs typically follow a pattern like <app-id>.public.blob.vercel-storage.com. A single-level wildcard (*) would be more specific and secure here. I recommend changing this to *.public.blob.vercel-storage.com to follow the principle of least privilege.
| hostname: "**.public.blob.vercel-storage.com", | |
| hostname: "*.public.blob.vercel-storage.com", |
Dashboard-User-Id: 4a95fa0f-db5a-4a0d-b46b-558f51207d70
|
|
What does that even mean bro... |
Dashboard-User-Id: 4a95fa0f-db5a-4a0d-b46b-558f51207d70
|
✅ Optimization complete for commit A new optimization PR is available: #4 targeting |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
File Upload Implementation
Overview
This PR implements a comprehensive file upload system for the Witely chat application, enabling users to attach and process multiple file types (images, PDFs, and text files) with intelligent model-based compatibility checking and robust security measures.
🎯 Key Features
File Type Support
Model-Based Compatibility
Drag & Drop Interface
Security & Validation
📊 Changes Summary
New Files
lib/ai/file-compatibility.ts- Model compatibility checking utilitieslib/ai/file-upload.ts- Shared file validation and upload logiclib/ai/FILE_UPLOAD_GUIDE.md- Comprehensive implementation documentationcomponents/attachment-loader.tsx- Loading state for file attachmentscomponents/drag-drop-wrapper.tsx- Drag-and-drop functionalitycomponents/file-drop-overlay.tsx- Visual feedback during drag operationspublic/icons/*.svg- File type icons (CSV, PDF, TXT, folder, uploaded-file)Modified Files
app/(chat)/api/chat/route.ts- Enhanced to process file attachmentsapp/(chat)/api/files/upload/route.ts- Improved security and validationcomponents/chat.tsx- Integrated file upload functionalitycomponents/multimodal-input.tsx- Added file selection UIcomponents/message.tsx- Enhanced attachment displaycomponents/messages.tsx- Better attachment handlingcomponents/model-selector.tsx- Model compatibility filteringcomponents/preview-attachment.tsx- Enhanced preview with type-specific icons🏗️ Architecture
File Upload Flow
Type Safety
All file operations use strict TypeScript types:
🔒 Security Measures
sanitize-filenamelibrary🎨 UI/UX Improvements
🧪 Testing Considerations
📝 Documentation
Comprehensive implementation guide included in
lib/ai/FILE_UPLOAD_GUIDE.mdcovering:🚀 Performance
Promise.all🐛 Bug Fixes
📋 Commits
🔮 Future Improvements
✅ Checklist
🔗 Related Issues
Ready for review ✨