Skip to content

fix: improve screenshot functionality with better error handling and … #2955

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: improve screenshot functionality with better error handling and … #2955

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
137 changes: 83 additions & 54 deletions apps/web/preload/script/api/screenshot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,35 +24,44 @@ export async function captureScreenshot(): Promise<{
const stream = await navigator.mediaDevices.getDisplayMedia({
video: {
width: viewportWidth,
height: viewportHeight
} as MediaTrackConstraints
height: viewportHeight,
mediaSource: 'screen',
} as any,
});

const video = document.createElement('video');
video.srcObject = stream;
video.autoplay = true;
video.muted = true;

await new Promise<void>((resolve) => {
await new Promise<void>((resolve, reject) => {
video.onloadedmetadata = () => {
video.play();
video.play().catch(reject);
video.oncanplay = () => {
context.drawImage(video, 0, 0, viewportWidth, viewportHeight);
stream.getTracks().forEach(track => track.stop());
resolve();
try {
context.drawImage(video, 0, 0, viewportWidth, viewportHeight);
stream.getTracks().forEach((track) => track.stop());
resolve();
} catch (drawError) {
reject(drawError);
}
};
};
video.onerror = reject;
Copy link
Contributor

Choose a reason for hiding this comment

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

If video.onerror fires, stream tracks might not be stopped, leading to potential resource leaks. Consider adding cleanup (e.g. stopping tracks) in the error path or a finally block.

Suggested change
video.onerror = reject;
video.onerror = (e) => { stream.getTracks().forEach(track => track.stop()); reject(e); };

});
Comment on lines +37 to 51
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Always stop the screen-capture stream on failures.

If drawImage, video.play(), or video.onerror paths reject, the promise exits without stopping stream tracks. The browser keeps capturing the user’s screen (and keeps the share indicator active) until GC, which is a privacy and resource leak. Move the stream.getTracks().forEach(track => track.stop()) call into a shared cleanup that runs on both success and error paths before resolving/rejecting.

-                await new Promise<void>((resolve, reject) => {
+                await new Promise<void>((resolve, reject) => {
+                    const cleanup = () => {
+                        stream.getTracks().forEach((track) => track.stop());
+                    };
+                    const handleError = (error: unknown) => {
+                        cleanup();
+                        reject(error);
+                    };
                     video.onloadedmetadata = () => {
-                        video.play().catch(reject);
+                        video.play().catch(handleError);
                         video.oncanplay = () => {
                             try {
                                 context.drawImage(video, 0, 0, viewportWidth, viewportHeight);
-                                stream.getTracks().forEach((track) => track.stop());
-                                resolve();
-                            } catch (drawError) {
-                                reject(drawError);
+                                cleanup();
+                                resolve();
+                            } catch (drawError) {
+                                handleError(drawError);
                             }
                         };
                     };
-                    video.onerror = reject;
+                    video.onerror = handleError;
                 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await new Promise<void>((resolve, reject) => {
video.onloadedmetadata = () => {
video.play();
video.play().catch(reject);
video.oncanplay = () => {
context.drawImage(video, 0, 0, viewportWidth, viewportHeight);
stream.getTracks().forEach(track => track.stop());
resolve();
try {
context.drawImage(video, 0, 0, viewportWidth, viewportHeight);
stream.getTracks().forEach((track) => track.stop());
resolve();
} catch (drawError) {
reject(drawError);
}
};
};
video.onerror = reject;
});
await new Promise<void>((resolve, reject) => {
const cleanup = () => {
stream.getTracks().forEach((track) => track.stop());
};
const handleError = (error: unknown) => {
cleanup();
reject(error);
};
video.onloadedmetadata = () => {
video.play().catch(handleError);
video.oncanplay = () => {
try {
context.drawImage(video, 0, 0, viewportWidth, viewportHeight);
- stream.getTracks().forEach((track) => track.stop());
cleanup();
resolve();
} catch (drawError) {
handleError(drawError);
}
};
};
video.onerror = handleError;
});
🤖 Prompt for AI Agents 
In apps/web/preload/script/api/screenshot.ts around lines 37 to 51, the promise
currently only stops stream tracks on the successful draw path, leaving the
screen-capture stream running on errors; extract a shared cleanup that calls
stream.getTracks().forEach(t => t.stop()) and invoke it before both resolve()
and reject(), e.g. attach centralized cleanup logic run in video.oncanplay
success and in all rejection paths (video.play().catch, video.onerror, and the
catch around drawImage), and also remove or null out event handlers after
cleanup to avoid double-calls.


// Convert canvas to base64 string with compression
const base64 = await compressImage(canvas);
console.log(`Screenshot captured - Size: ~${Math.round((base64.length * 0.75) / 1024)} KB`);
console.log(
`Screenshot captured - Size: ~${Math.round((base64.length * 0.75) / 1024)} KB`,
);
return {
mimeType: 'image/jpeg',
data: base64,
};
} catch (displayError) {
console.log('getDisplayMedia failed, falling back to DOM rendering:', displayError);
// Continue to fallback
}
}

Expand All @@ -61,7 +70,9 @@ export async function captureScreenshot(): Promise<{

// Convert canvas to base64 string with compression
const base64 = await compressImage(canvas);
console.log(`DOM screenshot captured - Size: ~${Math.round((base64.length * 0.75) / 1024)} KB`);
console.log(
`DOM screenshot captured - Size: ~${Math.round((base64.length * 0.75) / 1024)} KB`,
);
return {
mimeType: 'image/jpeg',
data: base64,
Expand Down Expand Up @@ -143,60 +154,74 @@ async function compressImage(canvas: HTMLCanvasElement): Promise<string> {
return canvas.toDataURL('image/jpeg', 0.1);
}

async function renderDomToCanvas(context: CanvasRenderingContext2D, width: number, height: number) {
// Set white background
context.fillStyle = '#ffffff';
context.fillRect(0, 0, width, height);

// Get all visible elements in the viewport
const elements = document.querySelectorAll('*');
const visibleElements: { element: HTMLElement; rect: DOMRect; styles: CSSStyleDeclaration }[] = [];

// Filter and collect visible elements with their computed styles
for (const element of elements) {
if (element instanceof HTMLElement) {
const rect = element.getBoundingClientRect();
const styles = window.getComputedStyle(element);

// Check if element is visible and within viewport
if (
rect.width > 0 &&
rect.height > 0 &&
rect.left < width &&
rect.top < height &&
rect.right > 0 &&
rect.bottom > 0 &&
styles.visibility !== 'hidden' &&
styles.display !== 'none' &&
parseFloat(styles.opacity) > 0
) {
visibleElements.push({ element, rect, styles });
function renderDomToCanvas(context: CanvasRenderingContext2D, width: number, height: number) {
try {
// Set white background
context.fillStyle = '#ffffff';
context.fillRect(0, 0, width, height);

// Get all visible elements in the viewport
const elements = document.querySelectorAll('*');
const visibleElements: {
element: HTMLElement;
rect: DOMRect;
styles: CSSStyleDeclaration;
}[] = [];

// Filter and collect visible elements with their computed styles
for (const element of elements) {
if (element instanceof HTMLElement) {
try {
const rect = element.getBoundingClientRect();
const styles = window.getComputedStyle(element);

// Check if element is visible and within viewport
if (
rect.width > 0 &&
rect.height > 0 &&
rect.left < width &&
rect.top < height &&
rect.right > 0 &&
rect.bottom > 0 &&
styles.visibility !== 'hidden' &&
styles.display !== 'none' &&
parseFloat(styles.opacity || '1') > 0
) {
visibleElements.push({ element, rect, styles });
}
} catch (elementError) {
// Skip elements that cause errors when accessing their properties
console.warn('Skipping element due to error:', element, elementError);
}
}
}
}

// Sort elements by z-index and document order
visibleElements.sort((a, b) => {
const aZIndex = parseInt(a.styles.zIndex) || 0;
const bZIndex = parseInt(b.styles.zIndex) || 0;
return aZIndex - bZIndex;
});
// Sort elements by z-index and document order
visibleElements.sort((a, b) => {
const aZIndex = parseInt(a.styles.zIndex || '0') || 0;
const bZIndex = parseInt(b.styles.zIndex || '0') || 0;
return aZIndex - bZIndex;
});

// Render each visible element
for (const { element, rect, styles } of visibleElements) {
try {
await renderElement(context, element, rect, styles);
} catch (error) {
console.warn('Failed to render element:', element, error);
// Render each visible element
for (const { element, rect, styles } of visibleElements) {
try {
renderElement(context, element, rect, styles);
} catch (error) {
console.warn('Failed to render element:', element, error);
}
}
} catch (error) {
console.error('Error in renderDomToCanvas:', error);
throw error;
}
}

async function renderElement(
function renderElement(
context: CanvasRenderingContext2D,
element: HTMLElement,
rect: DOMRect,
styles: CSSStyleDeclaration
styles: CSSStyleDeclaration,
) {
const { left, top, width, height } = rect;

Expand All @@ -207,7 +232,11 @@ async function renderElement(

// Render background
const backgroundColor = styles.backgroundColor;
if (backgroundColor && backgroundColor !== 'rgba(0, 0, 0, 0)' && backgroundColor !== 'transparent') {
if (
backgroundColor &&
backgroundColor !== 'rgba(0, 0, 0, 0)' &&
backgroundColor !== 'transparent'
) {
context.fillStyle = backgroundColor;
context.fillRect(left, top, width, height);
}
Expand Down Expand Up @@ -272,4 +301,4 @@ async function renderElement(
context.fillText('Image', left + width / 2, top + height / 2);
}
}
}
}