-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: prevent browser screenshots from displaying as base64 text after multiple actions #7796
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
… multiple actions - Added ScreenshotImage component with proper error handling and validation - Validates screenshot data URLs before rendering to ensure they are proper image data - Provides fallback UI when screenshot data is invalid or corrupted - Fixes issue where screenshots would display as raw base64 text after ~5 browser actions Fixes #7795
| textAlign: "center", | ||
| padding: "20px", | ||
| }}> | ||
| Screenshot unavailable |
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.
The fallback UI displays the hardcoded text 'Screenshot unavailable'. Please localize this string using the translation function (e.g. t('chat:browser.screenshotUnavailable')) to support internationalization.
| Screenshot unavailable | |
| {t('chat:browser.screenshotUnavailable')} |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| // Ensure the screenshot is a valid data URL | ||
| if (!validatedScreenshot.startsWith("data:image/")) { | ||
| // If it's just base64 without the data URL prefix, add it | ||
| if (validatedScreenshot.match(/^[A-Za-z0-9+/]+=*$/)) { |
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.
Is this regex pattern intentionally strict? Valid base64 can sometimes contain newlines and whitespace in certain implementations. Could we consider trimming whitespace first or using a more comprehensive pattern like /^[A-Za-z0-9+/\s]+=*$/ to handle edge cases?
| validatedScreenshot = `data:image/webp;base64,${validatedScreenshot}` | ||
| } else { | ||
| // Invalid screenshot data, set to undefined | ||
| console.warn("Invalid screenshot data detected, skipping screenshot") |
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.
The console warning could be more specific about what validation failed. Consider:
| console.warn("Invalid screenshot data detected, skipping screenshot") | |
| console.warn("Invalid screenshot data detected: expected base64 string without data URL prefix") |
This would help with debugging when issues occur.
| try { | ||
| // Validate base64 by attempting to decode a small portion | ||
| // This will throw if the base64 is invalid | ||
| atob(base64Match[1].substring(0, 100)) |
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.
Performance consideration: This validation runs on every render when the screenshot prop changes. Since the validation result won't change for the same input, could we consider memoizing this validation or moving it outside the effect?
Example approach:
| atob(base64Match[1].substring(0, 100)) | |
| const validatedSrc = useMemo(() => { | |
| // validation logic here | |
| return validated ? screenshot : null; | |
| }, [screenshot]); |
| if (imageError || !validatedSrc) { | ||
| // Fallback UI when screenshot is invalid | ||
| return ( | ||
| <div |
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.
For better accessibility, could we add an aria-label or role attribute to this fallback UI? Screen reader users would benefit from more context:
| <div | |
| <div | |
| role="img" | |
| aria-label="Screenshot unavailable" | |
| style={{ |
|
This doesn't seem like the right approach |
Description
This PR fixes an issue where browser action screenshots would display as raw base64 text instead of rendered images after approximately 5 browser actions.
Problem
After ~5 browser actions, the
browser_actiontool stops displaying screenshots as images and instead shows massive walls of base64-encoded text in the chat interface, making browser automation workflows extremely difficult to follow and debug.Solution
ScreenshotImagecomponent with robust error handling and validationChanges
webview-ui/src/components/chat/BrowserSessionRow.tsx:ScreenshotImagecomponent (lines 587-666)Testing
Impact
This fix ensures that screenshots consistently display as rendered images throughout the entire browser session, providing visual feedback for each action and preventing the UI from being flooded with base64 text.
Fixes #7795
Important
Fixes browser action screenshots displaying as base64 text by adding
ScreenshotImagecomponent with validation inBrowserSessionRow.tsx.ScreenshotImagecomponent inBrowserSessionRow.tsxfor rendering screenshots with validation and error handling.ScreenshotImagecomponent with error boundaries and base64 validation.imgtag withScreenshotImageinBrowserSessionRow.tsx.This description was created by
for a41d774. You can customize this summary. It will automatically update as commits are pushed.