-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: support adding images with @ mentions #6804
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Updated parseMentions to detect and process image file mentions - Added image processing logic with size validation and memory tracking - Modified processUserContentMentions to handle image data URLs - Updated Task.ts to pass image-related parameters to mention processing - Added comprehensive tests for image mention functionality - Fixed existing tests to work with new parseMentions return type Closes #6802
| } | ||
| } | ||
| } catch (error) { | ||
| parsedText += `\n\n<image_content path="${mentionPath}">\nError fetching image: ${error.message}\n</image_content>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good integration of image validation and processing with use of ImageMemoryTracker. In the catch block, ensure error objects always have a meaningful message (e.g. by using String(error)) to prevent crashes if a non‐Error is thrown.
| parsedText += `\n\n<image_content path="${mentionPath}">\nError fetching image: ${error.message}\n</image_content>` | |
| parsedText += `\n\n<image_content path="${mentionPath}">\nError fetching image: ${error instanceof Error ? error.message : String(error)}\n</image_content>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed my own code and realized I should have tested it before submitting.
Review Findings
Critical Issues
Breaking API Change - The parseMentions() function in src/core/mentions/index.ts now returns { text: string; images: string[] } instead of just a string. This is a breaking change that will affect any code calling this function directly. Consider maintaining backward compatibility.
Important Suggestions
-
Error Handling -
processImageFile()insrc/core/tools/helpers/imageHelpers.tslacks error handling for corrupted images. Iffs.readFilesucceeds but the content is corrupted, it could cause issues. -
Memory Management - The
ImageMemoryTrackerinstances are created per call but never explicitly cleaned up, which could accumulate in long-running processes. -
i18n Consistency - Some error messages in
validateImageForProcessing()are hardcoded instead of using the i18n system (line 136 in imageHelpers.ts).
Minor Improvements
-
Test Clarity - Tests use
@/pathsyntax in inputs but expect'path'in outputs, which might be confusing. -
Code Documentation - The array flattening logic in
processUserContentMentions.ts(line 166) could use a comment explaining why it's necessary. -
Test Coverage - Consider adding tests for SVG and other supported image formats beyond PNG/JPG.
Overall, the implementation looks solid but needs some refinement around backward compatibility and error handling.
|
@roomote-agent address the self-review, push to this PR, do not create a new one |
|
I'm on it! I'll address the self-review comments you identified: Critical Issues:
Important Suggestions:
Minor Improvements:
I'll implement these fixes and push to this PR branch. |
- Maintain backward compatibility for parseMentions() return type - Add error handling for corrupted images in processImageFile() - Use i18n for memory limit error message - Improve test clarity with @/path syntax - Add documentation for array flattening logic - Add test coverage for SVG, WebP, and AVIF formats
|
I've successfully addressed all the self-review comments! Here's what was fixed: ✅ Completed FixesCritical Issues
Important Suggestions
Minor Improvements
📋 Changes Summary
Note: The |
This PR implements support for referencing images using the @ mention functionality, as requested in issue #6802.
Summary
Users can now reference image files using the
@mention syntax (e.g.,@/path/to/image.png), similar to how they reference regular files. The images are automatically processed and included in the message context when the model supports images.Changes
parseMentions()to detect image file extensions and process them separately from regular filesprocessUserContentMentions()to handle the new return type that includes both text and image data URLsFeatures
@/path/to/image.pngsyntax.rooignorerules for image filesTesting
src/core/mentions/__tests__/imageMentions.spec.tsScreenshots
The feature integrates seamlessly with the existing mention system - users can now reference images just like they reference files.
Closes #6802
Important
This PR adds support for referencing and processing images using
@mentions, integrating them into message contexts when supported by the model, with comprehensive testing and updates to related components.@/path/to/image.pngsyntax inparseMentions().processUserContentMentions()to handle text and image data URLs.imageMentions.spec.tsfor image mention functionality.processUserContentMentions.spec.tsto test new API behavior.imageHelpers.ts.Task.tsto check model support for images and process accordingly.tools.json.This description was created by
for 758f6d7. You can customize this summary. It will automatically update as commits are pushed.