feat: Implement text file upload functionality. 增加上传文本文件的按钮和功能 #5585
Conversation
|
@Dakai is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces significant updates to the handling of multimodal content and file uploads within the chat application. Key changes include the enhancement of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Dakai
changed the title
|
Your build has completed! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (4)
app/components/chat.module.scss (2)
12-14: Remove unnecessary commented-out codeThe lines 12-14 contain commented-out position styles. If these styles are no longer needed, it's better to remove them to keep the stylesheet clean and maintainable.
54-56: Clean up commented-out code in.attach-filesclassThe lines 54-56 and 59 have commented-out code. Removing these unused lines will improve readability and maintain the cleanliness of the stylesheet.
Also applies to: 59-59
app/components/chat.tsx (2)
1492-1502: RenameimagesDatatouploadedFilesfor clarityThe variable
imagesDatais used to store uploaded text files, which might be confusing. Rename it touploadedFilesto better reflect its purpose and improve code readability.Apply this diff:
- const imagesData: UploadFile[] = []; + const uploadedFiles: UploadFile[] = []; ... - imagesData.push(fileData); + uploadedFiles.push(fileData); ... - if ( - imagesData.length === 3 || - imagesData.length === files.length - ) { + if ( + uploadedFiles.length === 3 || + uploadedFiles.length === selectedFiles.length + ) {
591-591: Use localized strings for button textThe button text is hardcoded as
"Upload Plain Text File". To support localization and maintain consistency, please use theLocaleobject to fetch the string.Apply this diff:
- text={"Upload Plain Text File"} + text={Locale.Chat.InputActions.UploadFile}Also, add the corresponding entry in the localization files if it doesn't exist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
app/icons/upload-doc.svgis excluded by!**/*.svgyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
- app/client/api.ts (1 hunks)
- app/components/chat.module.scss (4 hunks)
- app/components/chat.tsx (12 hunks)
- app/store/chat.ts (5 hunks)
- app/utils.ts (3 hunks)
- package.json (3 hunks)
🔇 Additional comments (10)
package.json (4)
25-25: LGTM: Addition of "@types/react-file-icon" type definitions.Adding TypeScript type definitions for the "react-file-icon" library is a good practice. It will improve type checking and developer experience when using the library.
Line range hint
25-43: Summary of package.json changesThe changes to
package.jsonmostly align with the PR objectives of adding file upload functionality:
- Added "@types/react-file-icon" for improved TypeScript support.
- Added "react-file-icon" for displaying file icons.
- Retained "mermaid" library (clarification needed).
These additions support the implementation of the new file upload feature. However, please provide clarification on the retention of the "mermaid" library as requested in the previous comment.
36-36: Please clarify the retention of the "mermaid" dependency.The "mermaid" library (version ^10.6.1) has been retained in this PR. However, its retention doesn't seem directly related to the PR objectives of adding file upload functionality. Could you please clarify why this dependency was removed and then re-added without any version change? Is it necessary for the current feature implementation or future plans?
Let's check the usage of mermaid in the codebase:
43-43: LGTM: Addition of "react-file-icon" library.The addition of the "react-file-icon" library aligns with the PR objectives of implementing file upload functionality. This library will likely be used to display file icons for uploaded text files.
Let's verify the usage of this library in the codebase:
✅ Verification successful
Verified: "react-file-icon" library correctly added and used.
The "react-file-icon" library is successfully imported and utilized within
app/components/chat.tsxfor displaying file icons in the chat component.
- File:
app/components/chat.tsx
- Import:
import { FileIcon, defaultStyles } from "react-file-icon";- Usage:
<FileIcon {...style} glyphColor="#303030" />🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of react-file-icon in the codebase # Test: Search for import statements of react-file-icon echo "Searching for import statements of react-file-icon:" rg --type typescript --type tsx 'from [\'"]react-file-icon[\'"]' # Test: Search for FileIcon component usage echo "Searching for FileIcon component usage:" rg --type typescript --type tsx '<FileIcon'Length of output: 207
Script:
#!/bin/bash # Description: Re-verify the usage of react-file-icon in the codebase with corrected syntax # Test: Search for import statements of react-file-icon echo "Searching for import statements of react-file-icon:" rg --type typescript --type tsx "from ['\"]react-file-icon['\"]" # Test: Search for FileIcon component usage echo "Searching for FileIcon component usage:" rg --type typescript --type tsx "<FileIcon"Length of output: 388
Script:
#!/bin/bash # Description: Re-verify the usage of react-file-icon in the codebase using file extensions # Test: Search for import statements of react-file-icon echo "Searching for import statements of react-file-icon in .ts and .tsx files:" fd --extension ts --extension tsx --type f | xargs rg 'from [\'"]react-file-icon[\'"]' # Test: Search for FileIcon component usage echo "Searching for FileIcon component usage in .ts and .tsx files:" fd --extension ts --extension tsx --type f | xargs rg '<FileIcon'Length of output: 251
Script:
#!/bin/bash # Description: Re-verify the usage of react-file-icon in the codebase using corrected syntax # Test: Search for import statements of react-file-icon echo "Searching for import statements of react-file-icon in .ts and .tsx files:" fd --extension ts --extension tsx --type f | xargs rg "from ['\"]react-file-icon['\"]" # Test: Search for FileIcon component usage echo "Searching for FileIcon component usage in .ts and .tsx files:" fd --extension ts --extension tsx --type f | xargs rg "<FileIcon"Length of output: 812
app/components/chat.module.scss (2)
110-112: Verify the hover effect on.attach-file-iconSetting
opacity: 0on hover for.attach-file-iconwill make the icon invisible when hovered over. Is this the intended behavior? If not, consider adjusting the hover effect to enhance user experience.
567-574: New styles for file attachments look goodThe styles for
.chat-message-item-filesand its child elements are well-structured and should enhance the display of file attachments within chat messages.app/store/chat.ts (3)
9-9: Import statement added correctlyThe
UploadFiletype is correctly imported from the client API module.
22-22: Import includesreadFileContentThe
readFileContentfunction is properly imported from the utils module.
448-449: Verifycontentproperty type compatibilityYou're assigning
displayContenttosavedUserMessage.content, butdisplayContentis of typestring | MultimodalContent[]. Ensure that thecontentproperty ofChatMessagecan accept this type without causing type errors or runtime issues. If necessary, update theChatMessagetype definition to accommodate this change.app/components/chat.tsx (1)
104-105: Verifyreact-file-icondependency is includedThe code imports
react-file-icon, but please ensure thatreact-file-iconis added topackage.jsonunderdependenciesto prevent runtime errors.Run the following script to verify:
✅ Verification successful
react-file-icondependency is correctly included inpackage.json.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'react-file-icon' is listed in dependencies in package.json grep '"react-file-icon"' package.jsonLength of output: 72
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
app/client/api.ts (1)
File upload functionality is not implemented as per the PR objectives
The expected implementations for the file upload button and displaying uploaded files were not found in the codebase. While the interfaces
MultimodalContentandUploadFileare defined and used, the actual UI components and functionality for uploading and displaying files are missing.
- File upload button is not present.
- Displaying of uploaded files is not implemented.
- Token count calculation exists but is unrelated to the upload feature.
🔗 Analysis chain
Line range hint
1-47: Verify implementation of file upload functionalityWhile the changes to the
MultimodalContentinterface and the addition of theUploadFileinterface align with the PR objectives, the actual implementation of the file upload functionality (such as the upload button and displaying uploaded files) is not visible in this file.Could you please verify if the implementation exists in other files? It's important to ensure that:
- The file upload button is implemented as described in the PR objectives.
- The uploaded files are correctly displayed to the right of any uploaded images.
- The token count is calculated and displayed for uploaded files.
- The new interfaces (
MultimodalContentandUploadFile) are correctly used in the implementation.To help verify the implementation, you can run the following script:
This script will help locate the relevant implementations across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for file upload related implementations echo "Searching for file upload button implementation:" rg --type typescript --type tsx 'file.*upload.*button' echo "\nSearching for file display implementation:" rg --type typescript --type tsx 'display.*uploaded.*file' echo "\nSearching for token count calculation:" rg --type typescript --type tsx 'token.*count.*file' echo "\nSearching for usage of MultimodalContent and UploadFile interfaces:" rg --type typescript --type tsx 'MultimodalContent|UploadFile'Length of output: 822
Script:
#!/bin/bash # Search for file upload related implementations echo "Searching for file upload button implementation:" rg -g "*.ts" -g "*.tsx" 'file.*upload.*button' echo "\nSearching for file display implementation:" rg -g "*.ts" -g "*.tsx" 'display.*uploaded.*file' echo "\nSearching for token count calculation:" rg -g "*.ts" -g "*.tsx" 'token.*count.*file' echo "\nSearching for usage of MultimodalContent and UploadFile interfaces:" rg -g "*.ts" -g "*.tsx" 'MultimodalContent|UploadFile'Length of output: 3120
Script:
#!/bin/bash # Search for file upload button related implementations with alternative patterns echo "Searching for file upload button implementation with alternative patterns:" rg -g "*.ts" -g "*.tsx" 'file.*upload.*button|attach.*button|attachment.*button|handleUpload|uploadButton' echo "\nSearching for file display implementation with alternative patterns:" rg -g "*.ts" -g "*.tsx" 'display.*uploaded.*file|render.*uploaded.*file|UploadedFiles|FileList|showUploadedFiles' echo "\nSearching for token count calculation:" rg -g "*.ts" -g "*.tsx" 'token.*count.*file' echo "\nSearching for usage of MultimodalContent and UploadFile interfaces:" rg -g "*.ts" -g "*.tsx" 'MultimodalContent|UploadFile'Length of output: 3348
app/store/chat.ts (1)
Line range hint
398-427: Suggestion: Refactor image handling to reduce duplicationThe implementation correctly handles image attachments when there are no file attachments. However, there's duplication in the code for processing images.
Consider refactoring the image handling logic into a separate function to reduce duplication. Here's a suggested implementation:
function processImageAttachments(images: string[], content: string | MultimodalContent[]) { const imageContent = images.map((url) => ({ type: "image_url", image_url: { url }, })); const textContent = Array.isArray(content) ? content : [{ type: "text", text: content }]; return textContent.concat(imageContent); } // Then in the onUserInput function: if (attachImages && attachImages.length > 0) { mContent = processImageAttachments(attachImages, mContent); displayContent = processImageAttachments(attachImages, displayContent); } else { mContent = userContent; displayContent = userContent; }This refactoring will make the code more maintainable and reduce the risk of inconsistencies when handling images in different scenarios.
app/components/chat.tsx (1)
Line range hint
1948-2126: Consider refactoring the attachment rendering codeThe implementation for displaying attached files is consistent with the existing code for attached images and provides a good user experience. However, there are a couple of areas that could be improved:
The code for rendering file attachments is quite similar to the code for rendering image attachments. Consider refactoring this to reduce code duplication.
The conditional rendering based on the number of attached images (lines 2091-2110) seems overly complex. This could potentially be simplified using a more dynamic approach.
Here's a suggested refactoring to address these issues:
function renderAttachment(attachment: Image | UploadFile, index: number, onDelete: () => void) { const isImage = 'url' in attachment && !('tokenCount' in attachment); return ( <div key={index} className={isImage ? styles["attach-image"] : styles["attach-file"]}> {isImage ? ( <div style={{ backgroundImage: `url("${attachment.url}")` }} /> ) : ( <> <div className={styles["attach-file-icon"] + " no-dark"}> <FileIcon {...defaultStyles[(attachment as UploadFile).name.split('.').pop()?.toLowerCase() as DefaultExtensionType]} glyphColor="#303030" /> </div> <div className={styles[`attach-file-name-${getNameClass(attachImages.length)}`]}> {(attachment as UploadFile).name} {(attachment as UploadFile).tokenCount}K </div> </> )} <div className={styles["attach-image-mask"]}> <DeleteImageButton deleteImage={onDelete} /> </div> </div> ); } function getNameClass(imageCount: number) { if (imageCount === 0) return 'full'; if (imageCount === 1) return 'half'; if (imageCount === 2) return 'less'; return 'min'; } // In the render method: <div className={styles["attachments"]}> <div className={styles["attach-images"]}> {attachImages.map((image, index) => renderAttachment(image, index, () => setAttachImages(attachImages.filter((_, i) => i !== index))))} </div> <div className={styles["attach-files"]}> {attachFiles.map((file, index) => renderAttachment(file, index, () => setAttachFiles(attachFiles.filter((_, i) => i !== index))))} </div> </div>This refactoring creates a single
renderAttachmentfunction that handles both images and files, reducing code duplication. It also simplifies the conditional rendering of file names based on the number of attached images.app/utils.ts (1)
32-34: Remove Commented-Out CodeThe commented-out code can be removed to keep the codebase clean and maintainable.
- //const content = await response.text(); - //const result = file.name + "\n" + content; - //return result;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/client/api.ts (1 hunks)
- app/components/chat.module.scss (4 hunks)
- app/components/chat.tsx (13 hunks)
- app/store/chat.ts (5 hunks)
- app/utils.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (14)
app/client/api.ts (2)
32-32: LGTM: Changes align with PR objectivesThe modifications to the
MultimodalContentinterface successfully implement the text file upload functionality as described in the PR objectives. The addition of the "file_url" type and the correspondingfile_urlproperty withurl,name, andtokenCountprovides the necessary structure to handle uploaded text files.It's worth noting that the
tokenCountis correctly typed asnumber, addressing a previous review comment.Also applies to: 37-41
44-47: LGTM: New UploadFile interface addedThe new
UploadFileinterface is well-structured and aligns with the PR objectives for handling uploaded text files. It includes the necessary properties (name,url, and optionaltokenCount) to represent an uploaded file.The
tokenCountis correctly typed asnumber, maintaining consistency with theMultimodalContentinterface.app/store/chat.ts (3)
9-9: LGTM: Necessary imports added for file upload functionalityThe new imports
UploadFileandreadFileContentare correctly added to support the text file upload feature.Also applies to: 22-22
449-450: LGTM: Correct content saved for displayThe change to use
displayContentinstead ofmContentwhen saving the user message is correct. This ensures that the session stores the content as it should be displayed, including file and image attachments, which aligns with the PR objectives.
Line range hint
1-1180: Overall assessment: Implementation aligns with PR objectives, with minor improvements suggestedThe changes successfully implement the text file upload functionality as described in the PR objectives. The code handles both file and image attachments, structures the content correctly for API requests and display, and updates the session with the appropriate content.
Key points:
- The function signature and imports have been correctly updated.
- File and image attachment handling is implemented, though there's a minor issue with the condition for handling images.
- There's some code duplication in image handling that could be refactored.
- The session update correctly uses the display content, ensuring proper representation of attachments.
Please address the suggested improvements, particularly the image handling condition issue, to enhance the overall quality and reliability of the implementation.
app/components/chat.tsx (3)
49-49: LGTM: New imports for file upload functionalityThe new imports are consistent with the added functionality for uploading and displaying text files. They include necessary components and functions for file icons, file uploads, and token counting.
Also applies to: 80-81, 83-83, 77-77
453-456: LGTM: ChatActions component updated for file uploadThe ChatActions component has been appropriately updated with new props (uploadDocument and setAttachFiles) to support the new file upload functionality.
Line range hint
1-2159: Overall, good implementation of text file upload functionalityThe changes successfully implement the new feature for uploading and displaying text files. The code is well-integrated with the existing chat component and maintains consistency with the current codebase structure.
Main points for improvement:
- The
uploadDocumentfunction could be refactored into smaller, more manageable functions.- Error handling could be improved in the file upload process.
- The rendering of attachments (both images and files) could be simplified to reduce code duplication.
These improvements would enhance the maintainability and robustness of the code. Great job on implementing this new feature!
app/utils.ts (3)
4-4: Import Statement Updated CorrectlyThe addition of
UploadFileto the import statement is appropriate for the new functionality.
42-70: Use a Standard Tokenization Library for Accurate Token CountingThe previous suggestion to use a standard tokenizer is still applicable. Utilizing a standard library like
tiktokenorgpt-3-encoderwill provide more accurate token counts and simplify maintenance.
305-316: Issue Resolved: Correctly Handling Undefinedfile_urlThe function now properly checks if
c.file_urlis defined before adding it to thefilesarray, addressing the previous concern about potential issues with undefined values.app/components/chat.module.scss (3)
74-99: Duplicated styles in.attach-file-name-*classes have been consolidatedGreat job on using the
%attach-file-name-commonplaceholder selector and extending it in the.attach-file-name-*classes. This effectively reduces code duplication and enhances maintainability.
121-128: Refactor.delete-imagestyles for consistencyThe
.delete-imageclass inside.attach-filehas similar styles to the one within.attach-image, with differences in dimensions and float direction. To minimize code duplication and ensure consistency, it's advisable to use a mixin or extend a common class, allowing you to parameterize the differing properties.
562-569: Styles for.chat-message-item-filesare well-structuredThe implementation and styling for the
.chat-message-item-filesclass effectively organize file attachments within chat messages. The use of flex layout and appropriate spacing enhances readability and maintains a clean UI.
lloydzhou
added
help wanted
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- app/utils.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (2)
app/utils.ts (2)
42-70: 'countTokens' Function Implementation is AcceptableThe
countTokensfunction provides a rough estimation of the token count based on character weights. Given the constraints and the unavailability of standard tokenization libraries due to dependency issues, the implementation is acceptable for its intended purpose.
305-316: 'getMessageFiles' Function Correctly Extracts File URLsThe function accurately retrieves files from the message content by checking
c.type === "file_url"and ensuringc.file_urlis defined. This prevents potential issues with undefined file URLs.
| if (!file.url.includes(host_url.host)) { | ||
| throw new Error(`The URL ${file.url} is not allowed to access.`); | ||
| } |
There was a problem hiding this comment.
Enhance URL Validation by Comparing Hostnames Directly
The current check using file.url.includes(host_url.host) might not securely verify that the file URL belongs to the same host. Partial matches could lead to false positives or security vulnerabilities. To ensure robust URL validation, consider parsing the URLs and comparing the hostnames directly.
Apply this diff to improve URL validation:
export const readFileContent = async (file: UploadFile): Promise<string> => {
const host_url = new URL(window.location.href);
+ const file_url = new URL(file.url);
- if (!file.url.includes(host_url.host)) {
+ if (file_url.host !== host_url.host) {
throw new Error(`The URL ${file.url} is not allowed to access.`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!file.url.includes(host_url.host)) { | |
| throw new Error(`The URL ${file.url} is not allowed to access.`); | |
| } | |
| const host_url = new URL(window.location.href); | |
| const file_url = new URL(file.url); | |
| if (file_url.host !== host_url.host) { | |
| throw new Error(`The URL ${file.url} is not allowed to access.`); | |
| } |
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
在工具栏处增加了一个上传文本文件的按钮，文件内容会复制在用户输入的内容之后，以这样的格式发出：
📝 补充信息 | Additional Information
文件在上传后会显示一个粗略的 token 计数，以方便用户使用。
上传的文件会排在上传的图片右边，风格和大小和图片上传一致，过长的文件名部分会被隐藏。
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Dependencies