fix: improve screenshot functionality with better error handling and … #2955
Conversation
|
@naaa760 is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughRefactors screenshot capture and DOM rendering in apps/web/preload/script/api/screenshot.ts: adjusts getDisplayMedia constraints, strengthens promise/error handling for video playback, converts async functions to sync where appropriate, adds defensive guards and per-element try/catch, improves sorting and parsing fallbacks, and tightens rendering conditions and bounds checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as App (screenshot.ts)
participant M as mediaDevices.getDisplayMedia
participant V as HTMLVideoElement
participant C as Canvas
U->>A: captureScreenshot()
A->>M: request display media {video: {mediaSource: 'screen', ...}}
alt success
M-->>A: MediaStream
A->>V: attach stream, play()
alt video plays
V-->>A: playing
A->>C: draw frame, renderDomToCanvas()
note over A,C: Per-element try/catch, bounds/z-index/opacity fallbacks
A-->>U: return screenshot image
else play error
V-->>A: onerror/catch
A-->>U: reject with error
end
A->>M: stop tracks (cleanup)
else failure
M-->>A: error
A->>A: log and fallback path
A-->>U: reject or proceed with fallback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/preload/script/api/screenshot.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/preload/script/api/screenshot.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/preload/script/api/screenshot.ts
| 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; | ||
| }); |
There was a problem hiding this comment.
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.
| 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.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| } | ||
| }; | ||
| }; | ||
| video.onerror = reject; |
There was a problem hiding this comment.
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.
| video.onerror = reject; | |
| video.onerror = (e) => { stream.getTracks().forEach(track => track.stop()); reject(e); }; |
Description
Key improvements:
Related Issues
fixes: #2907
Fixes screenshot button not working in chat interface
Type of Change
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
Important
Improves screenshot functionality in
screenshot.tswith enhanced error handling and fallback mechanisms for reliable operation.captureScreenshot()inscreenshot.tswith better error handling and fallback to DOM rendering ifgetDisplayMediafails.captureScreenshot()to prevent rendering failures.renderDomToCanvas().renderDomToCanvas().renderDomToCanvas()to skip problematic elements.This description was created by
for 4c28f87. You can customize this summary. It will automatically update as commits are pushed.