fix(terminal): use pendingCommand in onNewIntent and respect terminat… #1015
Conversation
fix(terminal): auto-run on Play button
There was a problem hiding this comment.
Pull request overview
This PR fixes terminal session handling by properly utilizing the pendingCommand in the onNewIntent method and respecting the terminatePreviousSession flag. The changes ensure that when a new terminal command is launched, it can properly reuse or terminate existing sessions based on the command configuration.
Key Changes:
- Added session termination logic that respects the
terminatePreviousSessionflag before creating/reusing sessions - Implemented
pendingCommandhandling in theonNewIntentmethod to support new terminal launches in an existing activity - Extracted
sessionIdto a local variable to reduce redundant field accesses
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| core/main/src/main/java/com/rk/terminal/TerminalScreen.kt | Added logic to terminate previous sessions when terminatePreviousSession is true and extract sessionId for cleaner code |
| core/main/src/main/java/com/rk/activities/terminal/Terminal.kt | Implemented pendingCommand handling in onNewIntent with session termination support and fallback to pwd-based session creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (pendingCommand != null) { | ||
| val command = pendingCommand!! |
There was a problem hiding this comment.
Race condition: Reading the global mutable pendingCommand variable without synchronization. Since pendingCommand is a global var that gets set to null in MkSession.createSession() (line 142), there's a risk that it could be modified by another thread between checking it on line 273 and using it on line 274. Consider using a local copy or proper synchronization mechanism.
| if (command.terminatePreviousSession && | ||
| terminalActivity.sessionBinder?.get()!!.getSession(sessionId) != null) { | ||
| terminalActivity.sessionBinder?.get()!!.terminateSession(sessionId) | ||
| } |
There was a problem hiding this comment.
Unsafe null-assertion usage with multiple calls to terminalActivity.sessionBinder?.get()!!. The code uses safe navigation (?.) followed by non-null assertion (!!), which can throw NullPointerException if sessionBinder is null or if get() returns null. Consider extracting terminalActivity.sessionBinder?.get() to a local variable at the beginning of the pendingCommand block and using early return or null-safe calls.
| terminalActivity.sessionBinder?.get()!!.getService().currentSession.value = | ||
| pendingCommand!!.id | ||
| terminalActivity.sessionBinder?.get()!!.getSession(pendingCommand!!.id) | ||
| sessionId | ||
| terminalActivity.sessionBinder?.get()!!.getSession(sessionId) | ||
| ?: terminalActivity.sessionBinder | ||
| ?.get()!! | ||
| .createSession(pendingCommand!!.id, client, terminalActivity) | ||
| .createSession(sessionId, client, terminalActivity) |
There was a problem hiding this comment.
Repeated calls to terminalActivity.sessionBinder?.get()!! create code duplication and inefficiency. The same expression is called multiple times (lines 283, 285, 286-287). Extract this to a local variable to improve readability and avoid redundant getter calls.
|
|
||
| val session = | ||
| if (pendingCommand != null) { | ||
| val command = pendingCommand!! |
There was a problem hiding this comment.
Redundant null check and assertion. Line 273 already checks pendingCommand != null, so using pendingCommand!! on line 274 is unnecessary. Use smart casting instead by assigning directly to val command = pendingCommand without the non-null assertion.
| val command = pendingCommand!! | |
| val command = pendingCommand |
| this@Terminal.changeSession(info.id) | ||
|
|
||
| // Check if there's a pending command from launchInternalTerminal | ||
| val command = com.rk.exec.pendingCommand |
There was a problem hiding this comment.
Race condition: Reading the global mutable pendingCommand variable without synchronization. Since pendingCommand is a global var that gets set to null in MkSession.createSession() (line 142), there's a risk that it could be modified by another thread between reading and using it. Consider using a local copy or proper synchronization mechanism.
| val command = com.rk.exec.pendingCommand | |
| val command = synchronized(com.rk.exec.pendingCommandLock) { | |
| com.rk.exec.pendingCommand | |
| } |
No description provided.