Skip to content

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: prevent browser screenshots from displaying as base64 text after multiple actions #7796

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 101 additions & 12 deletions webview-ui/src/components/chat/BrowserSessionRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,27 @@ const BrowserSessionRow = memo((props: BrowserSessionRowProps) => {
currentStateMessages.push(message)
const resultData = JSON.parse(message.text || "{}") as BrowserActionResult

// Validate screenshot data to ensure it's a proper data URL
let validatedScreenshot = resultData.screenshot
if (validatedScreenshot && typeof validatedScreenshot === "string") {
// 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+/]+=*$/)) {
Copy link
Contributor Author

@roomote roomote bot Sep 8, 2025

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")
Copy link
Contributor Author

@roomote roomote bot Sep 8, 2025

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:

Suggested change
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.

validatedScreenshot = undefined
}
}
}

// Add page with current state and previous next actions
result.push({
currentState: {
url: resultData.currentUrl,
screenshot: resultData.screenshot,
screenshot: validatedScreenshot,
mousePosition: resultData.currentMousePosition,
consoleLogs: resultData.logs,
messages: [...currentStateMessages],
Expand Down Expand Up @@ -296,18 +312,9 @@ const BrowserSessionRow = memo((props: BrowserSessionRowProps) => {
backgroundColor: "var(--vscode-input-background)",
}}>
{displayState.screenshot ? (
<img
src={displayState.screenshot}
<ScreenshotImage
screenshot={displayState.screenshot}
alt={t("chat:browser.screenshot")}
style={{
position: "absolute",
top: 0,
left: 0,
width: "100%",
height: "100%",
objectFit: "contain",
cursor: "pointer",
}}
onClick={() =>
vscode.postMessage({
type: "openImage",
Expand Down Expand Up @@ -576,4 +583,86 @@ const BrowserCursor: React.FC<{ style?: React.CSSProperties }> = ({ style }) =>
)
}

// Separate component to handle screenshot rendering with error boundaries
const ScreenshotImage: React.FC<{
screenshot: string
alt: string
onClick: () => void
}> = React.memo(({ screenshot, alt, onClick }) => {
const [imageError, setImageError] = useState(false)
const [validatedSrc, setValidatedSrc] = useState<string>("")

useEffect(() => {
// Validate and sanitize the screenshot data
if (screenshot && typeof screenshot === "string") {
// Check if it's a valid data URL
if (screenshot.startsWith("data:image/")) {
// Extract the base64 part and validate it
const base64Match = screenshot.match(/^data:image\/[^;]+;base64,(.+)$/)
if (base64Match && base64Match[1]) {
try {
// Validate base64 by attempting to decode a small portion
// This will throw if the base64 is invalid
atob(base64Match[1].substring(0, 100))
Copy link
Contributor Author

@roomote roomote bot Sep 8, 2025

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:

Suggested change
atob(base64Match[1].substring(0, 100))
const validatedSrc = useMemo(() => {
// validation logic here
return validated ? screenshot : null;
}, [screenshot]);

// If successful, use the original screenshot
setValidatedSrc(screenshot)
setImageError(false)
} catch (_e) {
console.error("Invalid base64 in screenshot data")
setImageError(true)
}
} else {
setImageError(true)
}
} else {
setImageError(true)
}
} else {
setImageError(true)
}
}, [screenshot])

if (imageError || !validatedSrc) {
// Fallback UI when screenshot is invalid
return (
<div
Copy link
Contributor Author

@roomote roomote bot Sep 8, 2025

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:

Suggested change
<div
<div
role="img"
aria-label="Screenshot unavailable"
style={{

style={{
position: "absolute",
top: 0,
left: 0,
width: "100%",
height: "100%",
display: "flex",
alignItems: "center",
justifyContent: "center",
backgroundColor: "var(--vscode-editor-background)",
color: "var(--vscode-descriptionForeground)",
fontSize: "12px",
textAlign: "center",
padding: "20px",
}}>
Screenshot unavailable
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Sep 8, 2025

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.

Suggested change
Screenshot unavailable
{t('chat:browser.screenshotUnavailable')}

This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.

</div>
)
}

return (
<img
src={validatedSrc}
alt={alt}
style={{
position: "absolute",
top: 0,
left: 0,
width: "100%",
height: "100%",
objectFit: "contain",
cursor: "pointer",
}}
onClick={onClick}
onError={() => setImageError(true)}
/>
)
})

export default BrowserSessionRow
Loading