Skip to content

Conversation

@daniel-lxs
Copy link
Member

@daniel-lxs daniel-lxs commented Sep 22, 2025

Problem

Webview URIs in frontend caused memory efficiency, but broke image visibility for OpenRouter and other providers.

Solution

Store base64 data URLs directly in backend messages when first received from frontend:

  • Frontend: webview URIs (memory efficient) ✅
  • Backend: base64 in messages (instant API calls) ✅
  • Conversion: One-time only when storing ✅

Key Changes

  • Add normalizeImageRefsToDataUrls() for webview URI → base64 conversion
  • Update Task.ts to store base64 directly in backend messages
  • Security: Fix ReDoS and host injection vulnerabilities
  • Tests: Complete coverage with 5/5 unit tests

Performance

  • Before: Convert webview URI → base64 on every API call
  • After: Use pre-stored base64 (zero conversion overhead)

Fixes image visibility while maintaining webview memory efficiency goals.


Important

This PR optimizes image handling by storing base64 data URLs in backend messages, improving performance and security, and includes comprehensive tests.

  • Behavior:
    • Store base64 data URLs in backend messages for images, improving API call efficiency.
    • Convert webview URIs to base64 once during storage using normalizeImageRefsToDataUrls().
    • Handle pasted and dropped images by saving them as temporary files and converting to webview URIs.
  • Security:
    • Fix ReDoS and host injection vulnerabilities.
    • Filter out base64 data URIs from rendering in Thumbnails.
  • Performance:
    • Avoid repeated conversions by caching base64 data URLs in image-cache.ts.
    • Reduce memory usage by not storing base64 in frontend state.
  • Components:
    • Update ChatTextArea.tsx and ChatView.tsx to handle image uploads and conversions.
    • Modify Thumbnails.tsx to exclude base64 images from rendering.
  • Tests:
    • Add tests for image handling in image-handler.spec.ts and imageDataUrl.spec.ts.
    • Ensure Thumbnails component only renders safe image URIs in Thumbnails.spec.tsx.

This description was created by Ellipsis for 3ff9b21. You can customize this summary. It will automatically update as commits are pushed.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. UI/UX UI/UX related or focused labels Sep 22, 2025
@daniel-lxs daniel-lxs changed the title Webview images: never render base64; use globalStorage URIs; fix 401 Change the way that uploaded images are shown; remove base64 strings Sep 22, 2025
roomote[bot]

This comment was marked as outdated.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Sep 22, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Review] in Roo Code Roadmap Sep 22, 2025
@hannesrudolph hannesrudolph added PR - Needs Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Sep 22, 2025
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 23, 2025
daniel-lxs and others added 3 commits September 23, 2025 14:57
- Fixed CodeQL security issue by properly validating vscode-cdn.net domain instead of substring check
- Added missing copy action check for HTTPS/vscode-cdn URLs before opening image
- Updated tests to match the more secure URL validation logic
@roomote

This comment was marked as outdated.

@hannesrudolph hannesrudolph changed the title Change the way that uploaded images are shown; remove base64 strings Change the way that uploaded images are shown; remove base64 strings (Gray Screen Fix) Oct 31, 2025
@hannesrudolph hannesrudolph moved this from PR [Needs Review] to PR [Changes Requested] in Roo Code Roadmap Oct 31, 2025
@hannesrudolph
Copy link
Collaborator

image image

Images included with a message sent to the chatview are not showing up in the chatview. There was an image attached to this message that is not showing.

@daniel-lxs daniel-lxs closed this Oct 31, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Oct 31, 2025
@github-project-automation github-project-automation bot moved this from PR [Changes Requested] to Done in Roo Code Roadmap Oct 31, 2025
@daniel-lxs daniel-lxs reopened this Nov 3, 2025
@github-project-automation github-project-automation bot moved this from Done to New in Roo Code Roadmap Nov 3, 2025
@github-project-automation github-project-automation bot moved this from Done to Triage in Roo Code Roadmap Nov 3, 2025
@roomote
Copy link

roomote bot commented Nov 3, 2025

See this task on Roo Code Cloud

Critical Issue Found

This PR introduces a critical bug that breaks image display in the chat view:

Issue: Images stored in backend messages are converted to base64 data URLs (via normalizeImageRefsToDataUrls() in Task.ts), but the frontend's Thumbnails component filters out all base64 data URIs. This causes images attached to messages to never render.

Root cause: The PR's approach stores base64 in ClineMessage.images[] for API calls, but doesn't convert them back to webview URIs when the frontend needs to display them. The Thumbnails component (line 40) explicitly filters !i.startsWith("data:"), so all base64 images are rejected.

Impact: This matches the exact bug reported by @hannesrudolph - images included with messages are not showing up in the chatview.

TODO

  • Fix image display: Backend needs to store BOTH base64 (for API) AND webview URI (for frontend display) in messages, OR provide a conversion mechanism to regenerate webview URIs from stored file paths when displaying messages

Mention @roomote in a comment to trigger your PR Fixer agent and make changes to this pull request.

Comment on lines 1239 to 1243
// Convert webview URIs to base64 data URLs for backend storage (one-time conversion)
const { normalizeImageRefsToDataUrls } = await import("../../integrations/misc/imageDataUrl")
const base64Images = images ? await normalizeImageRefsToDataUrls(images) : undefined

await this.say("text", task, base64Images) // Store base64 in backend messages
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical bug: Images stored in ClineMessage.images[] are now base64 data URLs (after normalizeImageRefsToDataUrls conversion), but the frontend's Thumbnails component filters out all base64 data URIs (line 40 of Thumbnails.tsx). This means images attached to messages will never render in the chat view. The PR converts webview URIs to base64 for backend storage but doesn't provide a mechanism to convert them back to webview URIs when displaying messages in the frontend. This breaks the core functionality described in the user's report: "Images included with a message sent to the chatview are not showing up."

@hannesrudolph hannesrudolph added Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. and removed PR - Changes Requested labels Nov 3, 2025
- Add imagesBase64 field to ClineMessage schema for efficient dual storage
- Store both webview URIs (for display) and base64 (for API) when creating messages
- Remove repeated base64→file→URI conversions on every render
- Update all tools to use base64 from stored messages for API calls
- Eliminates file I/O overhead during rendering and message updates
- Improves performance by storing each format once instead of converting repeatedly
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Nov 3, 2025
- Function is now primarily used for testing
- Production code uses dual-storage (images + imagesBase64)
- Removed redundant cache since same hash always produces same file path
- Simplified implementation and removed obsolete test
Comment on lines 51 to 53
* NOTE: This function is primarily used for testing. In production, the dual-storage
* approach (images + imagesBase64 in ClineMessage) eliminates the need for this conversion.
* No caching is implemented since this is a test utility.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states "No caching is implemented" but line 98 still calls setImageBase64ForPath(filePath, dataUrl) which caches the reverse mapping. The comment should be updated to clarify that while forward caching (base64ToFilePathCache) was removed, reverse caching for image-cache lookups is still performed.

@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Review] in Roo Code Roadmap Nov 3, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Review] to PR [Needs Prelim Review] in Roo Code Roadmap Nov 3, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Nov 3, 2025
- Function was only used in tests, not in production code
- Dual-storage approach (images + imagesBase64) eliminated need for base64→file conversion
- Removed ~60 lines of function code + ~90 lines of tests
- Simplified imageDataUrl module to focus on production use case
- Kept normalizeImageRefsToDataUrls which is used in production (Task.ts)
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR - Needs Preliminary Review size:XL This PR changes 500-999 lines, ignoring generated files. UI/UX UI/UX related or focused

Projects

Status: PR [Needs Prelim Review]

Development

Successfully merging this pull request may close these issues.

3 participants