-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
manager: fixed webui memory leaks #2737
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
Co-Authored-By: 梦璃酱 <[email protected]> Co-Authored-By: 白彩恋 <[email protected]>
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.
Pull Request Overview
This PR fixes memory leaks in the WebUI component by properly cleaning up WebView resources and improving resource management. The changes focus on preventing memory leaks that can occur when WebView instances are not properly destroyed.
- Replaced
loadUrl()withevaluateJavascript()for better performance and security - Improved WebView cleanup in
onDestroy()with proper resource disposal - Enhanced string building using
buildStringfor better memory efficiency
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| WebViewInterface.kt | Improved string building and replaced deprecated loadUrl() calls with evaluateJavascript() |
| WebUIActivity.kt | Added proper WebView cleanup in onDestroy() and improved resource management |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| completableFuture.thenAccept { result -> | ||
| val emitExitCode = | ||
| "javascript: (function() { try { ${callbackFunc}.emit('exit', ${result.code}); } catch(e) { console.error(`emitExit error: \${e}`); } })();" | ||
| $$"(function() { try { $${callbackFunc}.emit('exit', $${result.code}); } catch(e) { console.error(`emitExit error: ${e}`); } })();" |
Copilot
AI
Sep 13, 2025
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.
Invalid string template syntax. The $$ prefix is not valid Kotlin syntax. This should use regular string interpolation with $ or raw strings.
| $$"(function() { try { $${callbackFunc}.emit('exit', $${result.code}); } catch(e) { console.error(`emitExit error: ${e}`); } })();" | |
| """(function() { try { ${callbackFunc}.emit('exit', ${result.code}); } catch(e) { console.error(`emitExit error: ${'$'}{e}`); } })();""" |
| val moduleId = intent.getStringExtra("id") ?: finishAndRemoveTask().let { return } | ||
| val name = intent.getStringExtra("name") ?: finishAndRemoveTask().let { return } |
Copilot
AI
Sep 13, 2025
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.
[nitpick] The .let { return } pattern is unclear and could be confusing. Consider using explicit null checks with early returns for better readability: if (moduleId == null) { finishAndRemoveTask(); return }
| val moduleId = intent.getStringExtra("id") ?: finishAndRemoveTask().let { return } | |
| val name = intent.getStringExtra("name") ?: finishAndRemoveTask().let { return } | |
| val moduleId = intent.getStringExtra("id") | |
| if (moduleId == null) { | |
| finishAndRemoveTask() | |
| return | |
| } | |
| val name = intent.getStringExtra("name") | |
| if (name == null) { | |
| finishAndRemoveTask() | |
| return | |
| } |
|
|
||
| private var rootShell: Shell? = null | ||
| private val rootShell by lazy { createRootShell(true) } | ||
| private var webView = null as WebView? |
Copilot
AI
Sep 13, 2025
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.
Using null as WebView? is redundant. Simply use private var webView: WebView? = null for cleaner code.
| private var webView = null as WebView? | |
| private var webView: WebView? = null |
#2731