-
Notifications
You must be signed in to change notification settings - Fork 33
feat fix clipboard bug in browser #98
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
base: main
Are you sure you want to change the base?
Conversation
| if (!document.hidden) { | ||
| this.syncClipboard() | ||
| } | ||
| }) |
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.
Bug: Event listeners not removed causing memory leak
The window focus and document visibilitychange event listeners added in mounted() are never removed in beforeDestroy(). This creates a memory leak because the listeners hold references to the component instance, preventing garbage collection. Each time the component is mounted and destroyed, additional listeners accumulate on these global objects.
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.
@mateoCuervo i think bugbot has a valid point here
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.
Performed full review of 3dfcec2...f6acd4f
Analysis
-
Memory leak due to event listeners (window 'focus' and document 'visibilitychange') not being cleaned up in the beforeDestroy() lifecycle hook, which will cause issues when the video component is destroyed and recreated.
-
Potential race condition in the clipboard synchronization polling mechanism, where the syncClipboard() method could have overlapping executions if operations take longer than the polling interval.
-
Performance impact from aggressive clipboard polling every 500ms, which may be excessive and resource-intensive, especially on lower-end devices.
-
Missing concurrency guards to prevent multiple simultaneous clipboard operations when synchronization takes longer than expected.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
1 files reviewed | 0 comments | Edit Agent Settings • Read Docs
Mesa DescriptionChecklist
Note Adds Chromium-focused clipboard sync in
Written by Cursor Bugbot for commit f6acd4f. This will update automatically on new commits. Configure here. Description generated by Mesa. Update settings |
Checklist
-https://linear.app/onkernel/issue/KERNEL-459/fix-copy-and-paste-intoout-of-live-view
Note
Adds Chromium-focused clipboard sync in
video.vuevia focus/visibility listeners and a focused polling fallback, with proper cleanup.images/chromium-headful/client/src/components/video.vue:private _clipboardPollInterval?: numberfor polling control.beforeDestroy()to prevent leaks.Written by Cursor Bugbot for commit f6acd4f. This will update automatically on new commits. Configure here.
Tested
Was only able to test locally via
Loom screen recording with video recording tested in localhost:8080
https://www.loom.com/share/a04ce4073afd48e8a65e33b393c16bda