fix(multimodal): detect data: URIs as DataUrl in tracker - #493
Conversation
Summary of ChangesHello @CatherineSue, 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 addresses a critical bug where base64-encoded images provided as 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
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAsyncMultiModalTracker::push_part now distinguishes data URLs from regular URLs for image parts by parsing the URL and mapping to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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 addresses an issue where data: URIs were incorrectly handled as remote URLs. The fix introduces a check to identify data: URIs and classify them as MediaSource::DataUrl, which is the correct approach. I have one suggestion to make this detection more robust by using the url crate for parsing instead of a simple prefix check. This will improve correctness by properly handling various URL formats and edge cases.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@multimodal/src/tracker.rs`:
- Around line 74-78: The current check uses url.starts_with("data:") which is
case-sensitive and can misclassify valid RFC 2397 data URLs; update the branch
that builds the MediaSource (the code that sets source for the url variable) to
perform a case-insensitive prefix check (e.g., verify url has at least 5 chars
and use an ASCII case-insensitive comparison like eq_ignore_ascii_case("data:")
on the first 5 chars) so that MediaSource::DataUrl(...) is chosen for any case
variant of "data:" and otherwise fall back to MediaSource::Url(...).
Problem: When an image_url content part contains a data: URI (e.g. "data:image/jpeg;base64,..."), the tracker treated it as a regular URL and attempted an HTTP fetch, which fails. Fix: Check if the URL starts with "data:" and route to MediaSource::DataUrl instead of MediaSource::Url. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
edf8a68 to
72237d2
Compare
Replace manual prefix check with url::Url::parse() scheme check. This correctly handles edge cases like relative URLs starting with "data:" and is more idiomatic since the url crate is already a dependency. Signed-off-by: Chang Su <chang.s.su@oracle.com>
2 participants
Description
Problem
When a user sends an image via a
data:URI (base64 inline image) through theimage_urlfield in a chat message, the multimodal tracker incorrectly classifies it asMediaSource::Urland attempts to fetch it as a remote URL. This causes the request to fail sincedata:URIs are not valid HTTP endpoints.Solution
Add a check in
AsyncMultiModalTrackerto detectdata:URI prefixes in theimage_urlpath and route them toMediaSource::DataUrlinstead ofMediaSource::Url.Changes
tracker.rs, addeddata:prefix detection in theImageUrlhandler to correctly dispatch toMediaSource::DataUrlTest Plan
Tested with a chat completion request containing a base64-encoded image in
image_url.url:{ "type": "image_url", "image_url": { "url": "data:image/png;base64,iVBOR..." } }Verified the image is decoded inline rather than triggering an HTTP fetch.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit