Open
Conversation
…erver responses to submitted playground are now neatly organized and stylized web pages.
|
To enable Rovo Dev code reviews, link your GitHub account to your Atlassian account. This is a one-time task that takes less than a minute. Once your account is linked, resubmit the pull request to trigger a code review. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
📝 WalkthroughRelease Notes for PR
|
| Cohort / File(s) | Summary |
|---|---|
ServerConfig API change app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt |
Added public fileDirPath: String to ServerConfig constructor/public API. |
MainActivity wiring app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt |
startWebServer now constructs ServerConfig(..., fileDirPath = this@MainActivity.filesDir.absolutePath). |
Playground execution flow & helpers app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt |
Introduced POST /playground handling: reads Content-Length and body, decodes POST field, writes Playground.java under fileDirPath, compiles (javac) and runs (java) with timeout enforcement, truncates outputs, builds and sends HTML responses; added WebServer.JavaExecutionResult, helper methods (createFileFromPost, compileAndRunJava, sendHtmlResponse), constants, enhanced logging and error responses. |
Sequence Diagram
sequenceDiagram
participant Client
participant WebServer
participant FS as "FileSystem (filesDir)"
participant Javac as "javac (Compiler)"
participant Java as "java (Runtime)"
Client->>WebServer: POST /playground (Java source)
WebServer->>WebServer: parse headers & body
WebServer->>FS: write Playground.java to fileDirPath
WebServer->>Javac: run javac Playground.java
alt compile success
Javac-->>WebServer: compile success
WebServer->>Java: run java Playground (with timeout)
alt run finished
Java-->>WebServer: program output
else timed out
Java-->>WebServer: timeout signal
end
else compile failure
Javac-->>WebServer: compile errors
end
WebServer->>WebServer: truncate/format outputs, build HTML
WebServer-->>Client: 200 HTML response (compile & run outputs)
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- ADFA-2807 Webserver endpoint pr/ex returns 200 if experimental mode is on, 404 if not #976 — also modifies
ServerConfigby adding a constructor field (closely related API change).
Suggested reviewers
- hal-eisen-adfa
Poem
🐰
I dug a file beneath the moon,
Pasted Java, tapped a tune,
Javac hummed and runtime ran,
Truncated lines — a bunny's plan,
I hopped away with a happy spoon.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the main change as a proof-of-concept playgrounds feature aligned with the changeset's focus on adding Java code execution capabilities. |
| Description check | ✅ Passed | The description accurately relates to the changeset, explaining that the PR enables the web server to receive, compile, and execute Java code for interactive playgrounds. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
fix/ADFA-1284
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (1)
615-615: Source file not cleaned up after execution.The
.classfile is deleted but the sourcePlayground.javafile containing user code remains on disk. Consider cleaning up both files.Files.deleteIfExists(Paths.get(classFile.path)) + Files.deleteIfExists(Paths.get(sourceFile.path))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt(1 hunks)app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt(13 hunks)
🧰 Additional context used
🪛 detekt (1.23.8)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
[warning] 518-518: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (2)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (2)
530-531: Verify that Java binaries exist at expected paths.The hardcoded paths to
javacandjavabinaries assume a specific installation structure. If these don't exist, the process will fail with a confusing error.Consider adding a check or logging when binaries are not found:
val javacPath = "$directoryPath/usr/bin/javac" val javaPath = "$directoryPath/usr/bin/java" + + if (!File(javacPath).exists()) { + log.error("javac not found at: {}", javacPath) + return JavaExecutionResult( + compileOutput = "Error: Java compiler not found at $javacPath", + runOutput = "", + timedOut = false + ) + }
631-653: LGTM!The
JavaExecutionResultdata class andsendHtmlResponsehelper are well-structured and follow good practices.
app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
jatezzz
requested changes
Dec 17, 2025
app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt
Outdated
Show resolved
Hide resolved
…onstructor call in MainActivity.kt to avoid passing a whole Activity reference to the web server constructor call, creating the possibility for leaks if MainActivity is ever destroyed and recreated.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt (1)
340-340: Remove unused variable.The variable
applicationContextis defined but never used. This appears to be leftover code.🔎 Proposed fix
- val applicationContext = this lifecycleScope.launch(Dispatchers.IO) {Note: This relates to the previously flagged memory leak concern. Since the variable isn't used, simply removing it resolves the issue entirely.
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (2)
541-545: Consider logging exceptions in stream gobbler threads.The empty catch blocks in the output-reading threads silently swallow
IOExceptions. While this may be intentional for stream gobblers that terminate when processes end, it can hide issues like encoding problems or unexpected stream closures.🔎 Proposed improvement
val javacGobbler = Thread { try { javac.inputStream.bufferedReader().forEachLine { compileOutputBuilder.appendLine(it) } - } catch (_: IOException) {} + } catch (e: IOException) { + if (debugEnabled) log.debug("IOException reading javac output: {}", e.message) + } }.apply { start() }Apply similar changes to the java output gobbler thread.
Also applies to: 591-595
615-615: Consider handling file deletion failures.If
Files.deleteIfExistsfails (e.g., due to permissions or file locks), the.classfile will remain in the filesDir, potentially accumulating over time.🔎 Proposed improvement
- Files.deleteIfExists(Paths.get(classFile.path)) + try { + Files.deleteIfExists(Paths.get(classFile.path)) + } catch (e: Exception) { + if (debugEnabled) log.debug("Failed to delete class file: {}", e.message) + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt(1 hunks)app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt(13 hunks)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Universal APK
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt (1)
339-350: Server binds to localhost by default, limiting exposure—verify authentication needs for local code execution.The WebServer uses
bindName: String = "localhost"as the default binding address (ServerConfig line 28), which means the HTTP server only accepts connections from the local device. Since MainActivity.kt does not override this parameter, the server is not exposed to network access or remote attackers.However, the fundamental security concern remains valid: this server accepts arbitrary Java code via HTTP POST and executes it with
ProcessBuilder. While localhost binding restricts access to local processes, there is no authentication mechanism. Any local app or process with network access to the device (including on rooted devices or through physical access) can execute code.For a development-focused IDE tool, localhost-only code execution may be acceptable, but confirm this is intentional. Consider documenting:
- That the server is localhost-only by design
- Whether authentication is needed for production or shared-device scenarios
- The implications of code execution in the app's internal files directory
The 5-second execution timeout and process isolation via Android's sandbox provide some mitigation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt(1 hunks)
🔇 Additional comments (1)
app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt (1)
344-344: No memory leak issue exists in the current implementation. The ServerConfig data class does not accept anapplicationContextor Activity reference parameter—only String and Int values (databasePath,fileDirPath,port,bindName,debugDatabasePath,debugEnablePath). Line 344 passes only String paths viaabsolutePath, which are safely stored and used by WebServer without retaining any Activity references.Likely an incorrect or invalid review comment.
jatezzz
requested changes
Dec 22, 2025
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
…nted-out check on contents of the Accept-Encoding header.
jatezzz
reviewed
Dec 23, 2025
Collaborator
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
Take a look to the AI comments, may be you can improve the code 👍
…date field names on incoming POST data
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
🧰 Additional context used
🪛 detekt (1.23.8)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
[warning] 537-537: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (3)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (3)
203-206: Good security improvement!The POST endpoint restriction properly addresses the previous security concern. Only requests to the designated
playground/executeendpoint will be processed.
208-285: LGTM - Robust POST handling and valid HTML!The POST data reading loop properly handles incomplete reads, and the HTML structure is now valid with the button correctly placed in the body. Previous concerns about logging sensitive data and invalid HTML have been addressed.
649-671: LGTM - Clean helper structures.The
JavaExecutionResultdata class andsendHtmlResponsehelper are well-structured and follow good practices for HTTP response handling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (3)
45-45: Critical: Concurrency issue remains unresolved.The shared
Playground.javafile path means concurrent POST requests will race and overwrite each other's source code, leading to incorrect compilation/execution results or failures.🔎 Suggested fix using unique temporary files
- private val playgroundFilePath = "${config.fileDirPath}/Playground.java" + // Generate unique file per request to avoid concurrency issuesThen in
createFileFromPost, generate a unique path per request:private fun createFileFromPost(input: CharArray): File { val decoded = URLDecoder.decode(String(input), StandardCharsets.UTF_8.name()) val prefix = "$postDataFieldName=" val inputAsString = if (decoded.startsWith(prefix)) { decoded.substring(prefix.length) } else { log.warn("Expecting a data field named $postDataFieldName") "" } // Use unique file per request val uniqueFilePath = "${config.fileDirPath}/Playground_${System.nanoTime()}_${Thread.currentThread().id}.java" val file = File(uniqueFilePath) try { file.writeText(inputAsString) } catch (e: Exception) { log.error("Error creating playground file: {}", e.message) throw IOException("Failed to write playground source file", e) } return file }Also ensure cleanup of both
.javaand.classfiles after execution incompileAndRunJava.
517-537: Two unresolved issues in POST data parsing and error handling.
Line 521:
startsWith(postDataFieldName)accepts malformed input like "data123" or "dataXYZ" instead of requiring exactly "data=".Lines 533-535: The caught exception is logged but not propagated, silently hiding file-write failures. The caller cannot detect this error and proceeds with an empty or stale file.
🔎 Proposed fix
private fun createFileFromPost(input: CharArray): File { val decoded = URLDecoder.decode(String(input), StandardCharsets.UTF_8.name()) - val inputAsString = if (decoded.startsWith(postDataFieldName)) { + val prefix = "$postDataFieldName=" + val inputAsString = if (decoded.startsWith(prefix)) { // Add 1 to substring start index to handle the "=" between the POST data // field name and the POST data field data. - decoded.substring(postDataFieldName.length + 1) + decoded.substring(prefix.length) } else{ log.warn("Expecting a data field named " + postDataFieldName) "" } val file = File(playgroundFilePath) try { file.writeText(inputAsString) } catch (e: Exception) { - log.debug("Error creating file") + log.error("Error creating playground file: {}", e.message) + throw IOException("Failed to write playground source file", e) } return file }Based on static analysis hints.
544-545: Missing validation for Java executable paths.The hardcoded
javacPathandjavaPathare used without verifying the executables exist. If either is missing,ProcessBuilder.start()throws anIOExceptionthat propagates unhandled to line 228, resulting in a generic 500 error without indicating the root cause.🔎 Add validation before ProcessBuilder
val javacPath = "${config.fileDirPath}/usr/bin/javac" val javaPath = "${config.fileDirPath}/usr/bin/java" + + // Validate executables exist + val javacFile = File(javacPath) + val javaFile = File(javaPath) + + if (!javacFile.exists() || !javacFile.canExecute()) { + log.error("Java compiler not found or not executable: {}", javacPath) + return JavaExecutionResult( + compileOutput = "Error: Java compiler (javac) not found at $javacPath", + runOutput = "", + timedOut = false + ) + } + + if (!javaFile.exists() || !javaFile.canExecute()) { + log.error("Java runtime not found or not executable: {}", javaPath) + return JavaExecutionResult( + compileOutput = "Error: Java runtime (java) not found at $javaPath", + runOutput = "", + timedOut = false + ) + } val timeoutSeconds = 5L
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (1)
629-629: Consider cleaning up source files to prevent accumulation.Only the
.classfile is deleted. The source.javafile is never removed. While currently mitigated by using a shared filename (overwrites on next request), this will become a resource leak if the concurrency issue is fixed by using unique per-request filenames.🔎 Clean up both source and class files
log.debug("Run output length: {}. Truncated to: {}.", originalRunLength, runOutput.length) Files.deleteIfExists(Paths.get(classFile.path)) + Files.deleteIfExists(Paths.get(sourceFile.path)) // Timeout for executing programAdditionally, consider cleanup in the failure path (lines 586-590) before returning:
if (!classFile.exists()) { // Compilation failed or timed out during compile. No run attempt. val failureMessage = if (!didJavacFinish) { "\n*** Compilation timed out after $timeoutSeconds seconds. ***" } else { "" } + // Clean up source file even on failure + Files.deleteIfExists(Paths.get(sourceFile.path)) return JavaExecutionResult( compileOutput = compileOutput + failureMessage, runOutput = "Program execution skipped due to compilation failure.", timedOut = !didJavacFinish ) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
🧰 Additional context used
🪛 detekt (1.23.8)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
[warning] 533-533: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (4)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (4)
199-202: Good security improvement: POST endpoint validation.Restricting POST requests to the specific
playground/executeendpoint prevents unauthorized code execution via arbitrary paths.
205-216: Solid defensive handling of POST data reads.The loop correctly handles incomplete socket reads by accumulating data until
contentLengthbytes are received, with proper early-termination detection.
231-276: HTML response structure is correct and secure.The button is now properly placed within
<body>(line 241), and user output is correctly escaped viaescapeHtml()to prevent XSS attacks.
645-667: Clean utility implementations.The
JavaExecutionResultdata class provides clear structure for compilation/execution results, andsendHtmlResponseproperly encapsulates HTTP response logic with correct headers and encoding.
Comment on lines
189
to
191
| if (requestLine.contains("Content-Length")) { | ||
| contentLength = requestLine.split(" ")[1].toInt() | ||
| } |
Contributor
There was a problem hiding this comment.
Fragile header parsing could fail with malformed input.
The split(" ")[1] approach assumes exactly one space between the header name and value. Extra whitespace will cause ArrayIndexOutOfBoundsException.
🔎 More robust parsing approach
if (requestLine.contains("Content-Length")) {
- contentLength = requestLine.split(" ")[1].toInt()
+ // Split on ":" and trim whitespace for robust parsing
+ val parts = requestLine.split(":", limit = 2)
+ if (parts.size == 2) {
+ contentLength = parts[1].trim().toIntOrNull() ?: 0
+ }
}🤖 Prompt for AI Agents
In app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt around
lines 189 to 191, the code uses requestLine.split(" ")[1].toInt() which is
fragile and will throw ArrayIndexOutOfBoundsException for extra/missing
whitespace; instead, detect the header case-insensitively, find the ':'
separator (or use split(":", 2)), verify the result has two parts, trim the
value, then parseInt inside a try/catch (or use toIntOrNull) and only set
contentLength when parsing succeeds; ensure you handle missing/invalid values
gracefully (log or ignore) to avoid crashes.
…orders for failures, 1px green border for success, compilation wall-clock time added to output.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 573-574: The code spawns an unsandboxed Java process via
ProcessBuilder(javaPath, "-cp", dir.absolutePath, fileName) assigned to the
variable java which runs untrusted code with app privileges; update the
ProcessBuilder invocation to launch the JVM with a restrictive security policy
and JVM flags (e.g. add "-Djava.security.manager" and
"-Djava.security.policy=/path/to/policy") or else run the process inside a
restricted environment (container, separate low-privilege user, or chroot) and
pass the policy path and minimal permissions; ensure the created process drops
privileges and redirects io safely before start(), and validate
javaPath/dir/fileName inputs to avoid injection.
- Around line 204-206: The POST handler that checks (method == "POST" && path ==
playgroundEndpoint) currently allocates CharArray(contentLength) directly from
the Content-Length header which can cause OOM/DoS; add a MAX_CONTENT_LENGTH
constant (e.g. a few MB) and validate contentLength before allocation: if
contentLength is negative or greater than MAX_CONTENT_LENGTH, reject the request
with an appropriate error/HTTP 413 response; otherwise proceed to allocate
CharArray(contentLength) and read the body. Update the block that uses
contentLength and CharArray(...) to perform this validation and early-return on
oversized payloads.
- Around line 568-570: The current logic checks classFile.exists() after
compilation and may run a stale .class if javac failed or timed out; before
invoking the compiler (the javac run invoked where didJavacFinish is set and
compileTime/timeoutSeconds are measured), delete any existing classFile (the
File represented by classFile) so a previous .class cannot persist; ensure the
deletion happens only prior to compilation and handle any deletion failure by
logging/erroring appropriately so downstream code (which returns
JavaExecutionResult with compileOutput, "Execution skipped.", !didJavacFinish,
compileTime, timeoutSeconds) won't execute a stale artifact.
- Around line 585-590: The code currently deletes only the compiled .class file
(Files.deleteIfExists(Paths.get(classFile.path))) but does not remove the
temporary .java source created by createFileFromPost, leaking user code; update
the cleanup in the method that returns JavaExecutionResult (the block around
runOutput, truncationLimit and Files.deleteIfExists) to also delete the source
file (the file object produced by createFileFromPost or its variable name, e.g.
sourceFile or javaFile) using
Files.deleteIfExists(Paths.get(<sourceFile>.path)), ensuring this runs
regardless of success/failure (add it in the finally/cleanup section or
immediately alongside deleting classFile) so both .class and .java are removed.
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
…th provided by playground client
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/itsaky/androidide/activities/MainActivity.kt (1)
326-333:⚠️ Potential issue | 🟠 MajorGuard against multiple WebServer starts across Activity recreation.
startWebServer()runs on everyonCreatebut no longer tracks/stops the instance. On rotation or task recreation this can spawn another server, fail to bind, and still open the database. Consider holding a singleton (Application/service) or restoring a guarded instance with explicit stop.🧭 Possible fix (singleton + lifecycle)
+ private var webServer: WebServer? = null + private fun startWebServer() { lifecycleScope.launch(Dispatchers.IO) { try { + if (webServer != null) return@launch val dbFile = Environment.DOC_DB log.info("Starting WebServer - using database file from: {}", dbFile.absolutePath) - val webServer = WebServer(ServerConfig(databasePath = dbFile.absolutePath, fileDirPath = this@MainActivity.filesDir.absolutePath)) - webServer.start() + webServer = WebServer(ServerConfig(databasePath = dbFile.absolutePath, fileDirPath = this@MainActivity.filesDir.absolutePath)) + webServer?.start() } catch (e: Exception) { log.error("Failed to start WebServer", e) } } } + + override fun onDestroy() { + webServer?.stop() + webServer = null + super.onDestroy() + _binding = null + }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 351-362: The guard that exempts the playground path ("path !=
playgroundEndpoint") lets GET requests to playground/execute skip the
rowCount==0 check and later cause a CursorIndexOutOfBoundsException when
cursor.getBlob(0) is called; remove that exemption so the rowCount check applies
to all paths: update the conditional around rowCount (and its when branch that
calls sendError) to not special-case playgroundEndpoint, ensuring rowCount == 0
returns a 404 via sendError before any cursor.getBlob(0) access.
- Around line 787-804: The free-standing private function sendHtmlResponse is
declared outside the WebServer class; move it into the WebServer class as a
private member method (alongside writeNormalToClient and sendError) so it can
access instance properties like config and debugEnabled if needed, adjust its
visibility to private within the class and update indentation/placement
accordingly so it compiles as part of WebServer.
- Around line 199-310: The handler currently accepts POST but only processes
POST to playgroundEndpoint; add an early rejection for any POST where path !=
playgroundEndpoint by calling sendError(writer, 405, "Method Not Allowed") (or
similar) and returning, so non-playground POSTs don't fall through to the DB/GET
logic; insert this check after you parse method/path (use the existing method,
path and playgroundEndpoint symbols) and before the code that serves GET/DB
content (i.e., before continuing past the playground POST handling and before
the DB-serving logic).
- Line 50: The shared playgroundFilePath ("playgroundFilePath") is a single
shared file and currently safe only because the accept loop in start() handles
one client at a time; add a clear comment above the playgroundFilePath
declaration stating this single-threaded assumption and warning that if start()
is changed to handle connections concurrently (e.g., thread-per-connection or
coroutine-per-connection), this must be replaced with per-request unique temp
files (or synchronized access) to avoid concurrent overwrites; mention both the
playgroundFilePath symbol and the start() accept loop so future maintainers know
where to look and what to change.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`: - Line 50: The shared playgroundFilePath ("playgroundFilePath") is a single shared file and currently safe only because the accept loop in start() handles one client at a time; add a clear comment above the playgroundFilePath declaration stating this single-threaded assumption and warning that if start() is changed to handle connections concurrently (e.g., thread-per-connection or coroutine-per-connection), this must be replaced with per-request unique temp files (or synchronized access) to avoid concurrent overwrites; mention both the playgroundFilePath symbol and the start() accept loop so future maintainers know where to look and what to change.app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (1)
50-50: SharedPlayground.javafile path — safe only because the server is single-threaded.The accept loop in
start()processes one client at a time, so concurrent overwrites can't happen today. However, if the server is ever made multi-threaded (e.g., thread-per-connection), this will silently break. A comment noting this assumption would help future maintainers.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` at line 50, The shared playgroundFilePath ("playgroundFilePath") is a single shared file and currently safe only because the accept loop in start() handles one client at a time; add a clear comment above the playgroundFilePath declaration stating this single-threaded assumption and warning that if start() is changed to handle connections concurrently (e.g., thread-per-connection or coroutine-per-connection), this must be replaced with per-request unique temp files (or synchronized access) to avoid concurrent overwrites; mention both the playgroundFilePath symbol and the start() accept loop so future maintainers know where to look and what to change.
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 227-231: The POST payload validation incorrectly maps
contentLength <= 0 to 413; update the check in the POST handler (where
playgroundEndpoint, contentLength and maxContentLength are used) to distinguish
missing/invalid length from oversized payload: if contentLength <= 0 return
sendError(..., 411, "Length Required") (or 400 if you prefer) and only return
sendError(..., 413, "Payload Too Large") when contentLength > maxContentLength;
adjust the condition and error codes in the WebServer.kt POST branch that calls
sendError accordingly.
---
Duplicate comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 222-224: The current parsing in WebServer.kt that sets
contentLength by doing requestLine.split(" ")[1].toInt() is fragile and can
throw ArrayIndexOutOfBoundsException or NumberFormatException; update the logic
in the request-parsing block (where contentLength is assigned) to split the
header on ":" (e.g., split(":", 2)), check that the header name equals
"Content-Length" case-insensitively, trim the value, and use a safe parse
(toIntOrNull) to assign contentLength only when the parsed value is non-null
(otherwise ignore or log a warning) so malformed or oddly spaced headers won’t
crash the server.
- Around line 738-739: The code uses hardcoded javacPath and javaPath in
WebServer.kt and calls ProcessBuilder.start() without verifying the binaries
exist or are executable; update the handler that constructs these paths
(variables javacPath and javaPath) to check Files.exists/Files.isExecutable (or
equivalent) on each path before spawning processes, and if
missing/non-executable log a clear error and return an informative HTTP error
response (or throw a descriptive IOException) rather than letting
ProcessBuilder.start() propagate an unhandled exception; ensure the error
message includes which executable path is missing so callers and logs show the
root cause.
- Around line 705-724: In createFileFromPost, tighten the POST-field check and
stop swallowing write errors: change the prefix check from
decoded.startsWith(postDataFieldName) to require the equals sign (e.g.,
decoded.startsWith(postDataFieldName + "=")) so only "data=" matches, and when
calling file.writeText(inputAsString) catch exceptions only to log the full
error (use log.error with the exception) and propagate a failure (rethrow the
exception or throw a checked IOException) instead of returning the zero-length
File; ensure the method does not return the file on write failure so callers can
handle the error.
- Around line 199-310: The POST handling currently only services
playgroundEndpoint but lets other POSTs fall through into GET/DB logic; add an
early guard after the method/path are parsed (using the existing method and path
variables) to reject any POST where path != playgroundEndpoint by calling
sendError(writer, 405, "Method Not Allowed") (or 404 if you prefer) and return,
so non-playground POST requests do not proceed to the DB/GET handlers; update
references around playgroundEndpoint, method, path, and sendError to implement
this early-return check.
- Around line 351-362: The conditional that skips the "row not found" guard for
playgroundEndpoint causes GET requests to playground/execute to proceed with an
empty cursor and crash at cursor.getBlob(0); remove the special-case exemption
so the rowCount check always runs (i.e., change the if condition that references
rowCount and playgroundEndpoint to only check rowCount != 1), or alternatively
add an explicit empty-cursor/sendError(404, ...) before any cursor.getBlob
usage; target the block around rowCount, playgroundEndpoint, sendError, and the
subsequent cursor.getBlob(0) in WebServer.kt to ensure GETs to
playgroundEndpoint receive a 404 when rowCount == 0.
- Around line 734-764: The compileAndRunJava function can accidentally run a
stale .class from a previous successful build if javac fails or times out; to
fix this, remove any existing class file before invoking javac (i.e., check
classFile.exists() and delete it prior to starting ProcessBuilder) so a leftover
.class cannot be executed on compile failure, and ensure any deletion failures
are handled/logged reliably; reference compileAndRunJava, classFile, javac, and
the ProcessBuilder invocation when making the change.
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Outdated
Show resolved
Hide resolved
added 3 commits
February 17, 2026 09:57
…rocessing POST requests in the playground handler. Add a more meaningful message to exceptions thrown when the playground handler cannot create a new file.
… not /playground/execute
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (1)
710-714: Stale comment: the=is already included inprefix.Lines 712-713 say "Add 1 to substring start index to handle the '='" but the code now uses
prefix.lengthwhereprefixis"$postDataFieldName="— the=is already accounted for. The comment is misleading and should be removed or updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around lines 710 - 714, The inline comment next to the substring call is stale/misleading: since prefix is defined as "$postDataFieldName=" the "=" is already included and there is no extra "+1" adjustment; update or remove the comment near the decoded.substring(prefix.length) usage (related symbols: prefix, postDataFieldName, decoded.substring) so it accurately reflects that prefix.length already accounts for the "=" rather than suggesting an additional +1 is applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 745-749: The error message for the missing javac executable uses
javaPath instead of javacPath; update the throw in the block that checks
File(javacPath).exists() (in WebServer.kt, where javaPath and javacPath are
validated) to throw IOException("javac executable not found at " + javacPath) so
the exception reports the correct path.
---
Duplicate comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 352-363: The current guard exempts the playground endpoint from
the "no rows" check causing a GET to playground/execute to proceed with an empty
cursor and crash; update the conditional in the block that checks rowCount (the
if using rowCount != 1 && path != playgroundEndpoint) to remove the
playgroundEndpoint exemption so that the sendError(...) 404 path is applied for
rowCount == 0 (this ensures cursor.getBlob(...) is only reached when a row
exists); keep existing sendError(...) branches intact and do not change POST
handling earlier in the method.
- Around line 222-224: The Content-Length parsing in WebServer.kt is fragile:
replace the current requestLine.split(" ")[1].toInt() logic with a robust parser
that splits the header line on ":" with a limit of 2 (e.g. headerLine.split(":",
2)), trims the resulting value, and uses toIntOrNull() to safely parse into
contentLength; if parsing returns null, treat as missing/invalid and leave
contentLength unchanged or handle the error case (log/ignore) to avoid
ArrayIndexOutOfBoundsException or NumberFormatException when processing the
request headers in the method that reads request lines.
- Around line 737-773: The compileAndRunJava method can execute a stale .class
if a prior compilation left it behind; before starting the compilation (i.e.,
before creating the ProcessBuilder/javac and timing code), delete the existing
classFile (File dir, val classFile = File(dir, "$fileName.class")) by calling
classFile.delete() (and optionally logging if delete failed) so that a leftover
.class cannot be executed after a failed/timeout compile; keep the rest of the
javac/javacGobbler/didJavacFinish logic unchanged and ensure classFile existence
is only checked after this pre-compilation deletion and the new compilation
completes.
- Around line 227-231: The current check treats missing/zero Content-Length as
413; change the logic in WebServer.kt where you inspect contentLength (used with
method/path/playgroundEndpoint and sendError) so missing/invalid lengths return
the correct status: if contentLength == 0 return sendError(writer, 411, "Length
Required"); if contentLength < 0 (invalid parse) return sendError(writer, 400,
"Bad Request"); otherwise if contentLength > maxContentLength return
sendError(writer, 413, "Payload Too Large"). Update the conditional around
contentLength/maxContentLength accordingly.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 710-714: The inline comment next to the substring call is
stale/misleading: since prefix is defined as "$postDataFieldName=" the "=" is
already included and there is no extra "+1" adjustment; update or remove the
comment near the decoded.substring(prefix.length) usage (related symbols:
prefix, postDataFieldName, decoded.substring) so it accurately reflects that
prefix.length already accounts for the "=" rather than suggesting an additional
+1 is applied.
Daniel-ADFA
reviewed
Feb 17, 2026
Comment on lines
272
to
303
|
|
||
| val html = buildString { | ||
| appendLine("<!DOCTYPE html><html><head><meta charset='UTF-8'>") | ||
| appendLine("<style>") | ||
| appendLine("body { font-family: sans-serif; padding: 20px; background-color: #f8f9fa; font-size: 24px; }") | ||
| appendLine("h2 { font-size: 28px; margin-top: 0; }") | ||
| appendLine("pre, p, button { font-size: 24px; }") | ||
| appendLine(".output-box { background-color: white; padding: 15px; border-radius: 5px; margin-bottom: 20px; overflow-x: auto; }") | ||
| appendLine("#compiler-output-container { border: $compBorderWidth solid $compBorderColor; }") | ||
| appendLine("#program-output-container { border: $runBorderWidth solid $runBorderColor; }") | ||
| appendLine("pre { white-space: pre-wrap; word-wrap: break-word; margin: 0; }") | ||
| appendLine("</style></head><body>") | ||
| appendLine("<button onclick='window.history.back()'>Back to code editor</button>") | ||
|
|
||
| // Program Output - Logic updated for timeout message | ||
| appendLine("<div id='program-output-container' class='output-box'><h2>Program Output</h2>") | ||
|
|
||
| val runOutputDisplay = when { | ||
| result.timedOut -> "Program timed out after ${result.timeoutLimit} seconds." | ||
| result.runOutput.isBlank() -> "No output." | ||
| else -> result.runOutput | ||
| } | ||
|
|
||
| appendLine("<pre id='program-output'>${escapeHtml(runOutputDisplay)}</pre></div>") | ||
|
|
||
| // Compiler Output | ||
| appendLine("<div id='compiler-output-container' class='output-box'><h2>Compiler Output</h2>") | ||
| appendLine("<p style='margin: 0 0 10px 0; font-weight: bold;'>Time to compile: ${result.compileTimeMs}ms</p>") | ||
| appendLine("<pre id='compiler-output'>${escapeHtml(result.compileOutput.ifBlank { "Compilation successful." })}</pre></div>") | ||
|
|
||
| appendLine("</body></html>") | ||
| } |
Contributor
There was a problem hiding this comment.
Can this be moved into separate functions? To aid readability:
companion object {
private const val ERROR_COLOR = "#ff0000"
private const val WARNING_COLOR = "#ffff00"
private const val SUCCESS_COLOR = "#00ff00"
private const val ERROR_WIDTH = "3px"
private const val SUCCESS_WIDTH = "1px"
private const val CSS_TEMPLATE = """
body { font-family: sans-serif; padding: 20px; background-color: #f8f9fa; font-size: 24px; }
h2 { font-size: 28px; margin-top: 0; }
pre, p, button { font-size: 24px; }
.output-box {
background-color: white;
padding: 15px;
border-radius: 5px;
margin-bottom: 20px;
overflow-x: auto;
}
.error-border { border: $ERROR_WIDTH solid $ERROR_COLOR; }
.warning-border { border: $ERROR_WIDTH solid $WARNING_COLOR; }
.success-border { border: $SUCCESS_WIDTH solid $SUCCESS_COLOR; }
pre { white-space: pre-wrap; word-wrap: break-word; margin: 0; }
""".trimIndent()
}
private fun generatePlaygroundHtml(result: JavaExecutionResult): String {
val compilerStatus = when {
result.compileOutput.contains("e rror:", ignoreCase = true) -> "error"
result.compileOutput.contains("w arning:", ignoreCase = true) ||
result.compileOutput.contains("d eprecation", ignoreCase = true) -> "warning"
else -> "success"
}
val runtimeStatus = if (result.timedOut) "error" else "success"
return buildString {
appendLine("<!DOCTYPE html><html><head><meta charset='UTF-8'>")
appendLine("<style>$CSS_TEMPLATE</style>")
appendLine("</head><body>")
appendLine("<button onclick='window.history.back()' class='back-button'>← Back to Editor</button>")
// Program Output
appendLine("""
<div class='output-box ${runtimeStatus}-border'>
<h2> Program Output</h2>
<pre>${escapeHtml(getRunOutputDisplay(result))}</pre>
</div>
""".trimIndent())
// Compiler Output
appendLine("""
<div class='output-box ${compilerStatus}-border'>
<h2>🔨 Compiler Output</h2>
<p><strong>Compile time:</strong> ${result.compileTimeMs}ms</p>
<pre>${escapeHtml(result.compile Output.ifBlank { " Compilation successful!" })}</pre>
</div>
""".trimIndent())
appendLine("</body></html>")
}
}
private fun getRunOutputDisplay(result: JavaExecutionResult): String = when {
result.timedOut -> " Program timed out after ${result.timeoutLimit} seconds."
result.runOutput.isBlank() -> "No output produced."
else -> result.runOutput
}
Collaborator
Author
There was a problem hiding this comment.
Yes, this is possible, I just committed changes that mostly follow the example code that you gave here.
…play bug so that if java/javac are missing the correct error message gets tied to the IOException
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (3)
262-311: Remove dead commented-out code block.Lines 262–309 are the old inline HTML-generation approach wrapped in
/* ... */. Now thatgeneratePlaygroundHtmlis the canonical path, this stale block should be deleted rather than left as a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around lines 262 - 311, Remove the large dead commented-out HTML-generation block (the /* ... */ section that builds html and sets comp/run styles) so the file no longer contains the stale inline HTML; keep the active call path that uses generatePlaygroundHtml (and the existing sendHtmlResponse(writer, output, html) usage) so the code uses the canonical generatePlaygroundHtml helper and no commented code remains referencing local variables like result, runOutputDisplay, or escapeHtml.
17-22: Non-idiomatic Java-style semicolons on imports.Kotlin does not require semicolons. These trailing
;on lines 17, 20, 21, and 22 are Java idioms that add noise in a Kotlin file.♻️ Proposed cleanup
-import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeUnit import java.net.URLDecoder import java.nio.charset.StandardCharsets -import java.nio.file.Files; -import java.nio.file.Paths; -import java.io.IOException; +import java.nio.file.Files +import java.nio.file.Paths +import java.io.IOException🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around lines 17 - 22, The import statements in WebServer.kt use Java-style trailing semicolons (e.g., imports for TimeUnit, Files, Paths, IOException); remove the semicolons from those import lines so they follow Kotlin idioms (e.g., change "import java.util.concurrent.TimeUnit;" to "import java.util.concurrent.TimeUnit") and ensure no other imports in the file end with a semicolon.
245-245: Use SLF4J parameterized logging for consistency.Every other
log.debugcall in this file uses{}placeholders; this one uses Kotlin string interpolation, which always evaluates the argument even when the debug level is disabled.♻️ Proposed change
- log.debug("Received data, length='${data.size}'") + log.debug("Received data, length='{}'", data.size)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` at line 245, The debug log uses Kotlin string interpolation which eagerly evaluates data.size; change the log.debug call in WebServer.kt to use SLF4J parameterized logging (e.g., log.debug("Received data, length='{}'", data.size)) so it matches other calls and avoids evaluating the argument when debug is disabled—locate the log.debug invocation around the data handling code and replace the interpolated string with a "{}" placeholder and pass data.size as the subsequent parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 845-886: The code conflates compilation vs execution timeouts via
JavaExecutionResult.timedOut; add two explicit flags (e.g.,
JavaExecutionResult.compileTimedOut and JavaExecutionResult.runTimedOut), set
them in the compile and run flows where timedOut is currently assigned, and
update generatePlaygroundHtml and getRunOutputDisplay to use runTimedOut for the
Program Output box and compileTimedOut to force compilerStatus="error" (and
display a clear compile timeout message instead of "Compilation successful!");
ensure any uses of result.timeoutLimit are disambiguated or kept appropriate for
run messages.
- Around line 882-886: The timeout message in getRunOutputDisplay has a leading
space causing an extra space in the rendered output; update the when branch for
result.timedOut in getRunOutputDisplay to remove the leading space so the
message begins with "Program timed out..." (reference: function
getRunOutputDisplay and type JavaExecutionResult) and ensure other branches
remain unchanged.
- Around line 779-796: The cleanup of generated files is not guaranteed because
Files.deleteIfExists for classFile and sourceFile only runs on the happy path;
wrap the process execution and output-gobbler logic (the ProcessBuilder start
assigned to java, the javaGobbler Thread and runOutputBuilder usage,
waitFor/destroyForcibly, and runOutput truncation) in a try-finally so that in
finally you always: ensure java is terminated (destroy/destroyForcibly if
needed), join the javaGobbler, and call Files.deleteIfExists for classFile and
sourceFile; adjust ordering to avoid NPEs if start() failed and null-check
java/javaGobbler before acting.
---
Duplicate comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 222-224: The current fragile parsing in WebServer.kt that does
contentLength = requestLine.split(" ")[1].toInt() can throw
ArrayIndexOutOfBoundsException or NumberFormatException; update the parsing
logic in the request-processing code that reads requestLine (referencing the
requestLine variable and contentLength assignment) to: detect the header with
startsWith("Content-Length", ignoreCase = true) or split on ":" with limit=2,
trim() the right-hand side, use toIntOrNull() to safely parse, and only assign
contentLength if the parsed value is non-null (and >= 0); otherwise ignore the
header or log a warning instead of letting an exception propagate.
- Around line 356-367: The current guard lets requests to playgroundEndpoint
bypass the rowCount==0 check, causing cursor.getBlob(0) to throw when no DB row
exists; update the logic in the request handling (the block using path,
playgroundEndpoint, rowCount, cursor.moveToFirst(), and cursor.getBlob(0)) to
always handle rowCount==0 (or check cursor.moveToFirst() result) before
accessing cursor.getBlob(0), and return/sendError(writer, 404, ...) when no
record is found so playground requests do not attempt to read from an empty
cursor.
- Around line 741-777: The compileAndRunJava function can run an old .class if a
prior build produced it; before starting javac delete any existing classFile
(the File referenced by classFile) so a stale artifact cannot be executed after
a compilation timeout/failure, and adjust the returned JavaExecutionResult logic
(used after the javac wait/timeout and classFile.exists() check) to rely only on
the newly produced class; specifically: in compileAndRunJava remove or unlink
classFile (if exists) immediately before launching ProcessBuilder(javacPath,
sourceFile.absolutePath) and keep the existing timeout/cleanup behavior for
javac/javacGobbler so that classFile.exists() only reflects the current
compilation result.
- Around line 229-231: The current check in WebServer.kt treating contentLength
== 0 the same as too-large payload returns 413; update the logic in the request
body handling (where variable contentLength is compared to maxContentLength and
sendError is called) so that if contentLength is 0 (or the Content-Length header
is missing) you call sendError(writer, 411, "Length Required") instead of 413,
and only return 413 via sendError(writer, 413, "Payload Too Large") when
contentLength > maxContentLength; keep the existing sendError usage and names to
locate and change the conditional.
- Line 50: The class-level fixed playgroundFilePath (playgroundFilePath) in
WebServer causes races for concurrent playground/execute requests; remove that
shared property and instead generate a per-request unique file path inside the
playground/execute handler (eg. use File.createTempFile or append a UUID to
"Playground.java"), write the incoming code to that per-request file, invoke
existing compile/run logic against that per-request file, and ensure the temp
file is cleaned up after execution (or use try/finally) so concurrent requests
do not overwrite each other.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 262-311: Remove the large dead commented-out HTML-generation block
(the /* ... */ section that builds html and sets comp/run styles) so the file no
longer contains the stale inline HTML; keep the active call path that uses
generatePlaygroundHtml (and the existing sendHtmlResponse(writer, output, html)
usage) so the code uses the canonical generatePlaygroundHtml helper and no
commented code remains referencing local variables like result,
runOutputDisplay, or escapeHtml.
- Around line 17-22: The import statements in WebServer.kt use Java-style
trailing semicolons (e.g., imports for TimeUnit, Files, Paths, IOException);
remove the semicolons from those import lines so they follow Kotlin idioms
(e.g., change "import java.util.concurrent.TimeUnit;" to "import
java.util.concurrent.TimeUnit") and ensure no other imports in the file end with
a semicolon.
- Line 245: The debug log uses Kotlin string interpolation which eagerly
evaluates data.size; change the log.debug call in WebServer.kt to use SLF4J
parameterized logging (e.g., log.debug("Received data, length='{}'", data.size))
so it matches other calls and avoids evaluating the argument when debug is
disabled—locate the log.debug invocation around the data handling code and
replace the interpolated string with a "{}" placeholder and pass data.size as
the subsequent parameter.
Comment on lines
+779
to
+796
| // Execution | ||
| val java = ProcessBuilder(javaPath, "-cp", dir.absolutePath, fileName) | ||
| .directory(dir).redirectErrorStream(true).start() | ||
|
|
||
| val runOutputBuilder = StringBuilder() | ||
| val javaGobbler = Thread { | ||
| try { java.inputStream.bufferedReader().forEachLine { runOutputBuilder.appendLine(it) } } catch (_: IOException) {} | ||
| }.apply { start() } | ||
|
|
||
| val didJavaFinish = java.waitFor(timeoutSeconds, TimeUnit.SECONDS) | ||
| if (!didJavaFinish) java.destroyForcibly() | ||
| javaGobbler.join(1000) | ||
|
|
||
| var runOutput = runOutputBuilder.toString() | ||
| if (runOutput.length > truncationLimit) runOutput = runOutput.substring(0, truncationLimit) + " [Truncated]" | ||
|
|
||
| Files.deleteIfExists(Paths.get(classFile.path)) | ||
| Files.deleteIfExists(Paths.get(sourceFile.path)) |
Contributor
There was a problem hiding this comment.
File cleanup not guarded by finally — source/class files leak on exception.
Files.deleteIfExists at lines 795–796 is only reached on the happy path. If the java process's ProcessBuilder.start(), the gobbler thread join, or output truncation throws, both the .java source and .class files are left on disk indefinitely. Wrap cleanup in a try-finally block.
🛡️ Proposed fix
+ try {
val java = ProcessBuilder(javaPath, "-cp", dir.absolutePath, fileName)
.directory(dir).redirectErrorStream(true).start()
// ... gobbler, waitFor, output processing ...
- Files.deleteIfExists(Paths.get(classFile.path))
- Files.deleteIfExists(Paths.get(sourceFile.path))
-
- return JavaExecutionResult(compileOutput, runOutput, !didJavaFinish, compileTime, timeoutSeconds)
+ return JavaExecutionResult(compileOutput, runOutput, !didJavaFinish, compileTime, timeoutSeconds)
+ } finally {
+ Files.deleteIfExists(Paths.get(classFile.path))
+ Files.deleteIfExists(Paths.get(sourceFile.path))
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around
lines 779 - 796, The cleanup of generated files is not guaranteed because
Files.deleteIfExists for classFile and sourceFile only runs on the happy path;
wrap the process execution and output-gobbler logic (the ProcessBuilder start
assigned to java, the javaGobbler Thread and runOutputBuilder usage,
waitFor/destroyForcibly, and runOutput truncation) in a try-finally so that in
finally you always: ensure java is terminated (destroy/destroyForcibly if
needed), join the javaGobbler, and call Files.deleteIfExists for classFile and
sourceFile; adjust ordering to avoid NPEs if start() failed and null-check
java/javaGobbler before acting.
Comment on lines
+845
to
+886
| private fun generatePlaygroundHtml(result: JavaExecutionResult): String { | ||
| val compilerStatus = when { | ||
| result.compileOutput.contains("error:", ignoreCase = true) -> "error" | ||
| result.compileOutput.contains("warning:", ignoreCase = true) || | ||
| result.compileOutput.contains("deprecation", ignoreCase = true) -> "warning" | ||
| else -> "success" | ||
| } | ||
|
|
||
| val runtimeStatus = if (result.timedOut) "error" else "success" | ||
|
|
||
| return buildString { | ||
| appendLine("<!DOCTYPE html><html><head><meta charset='UTF-8'>") | ||
| appendLine("<style>$CSS_TEMPLATE</style>") | ||
| appendLine("</head><body>") | ||
| appendLine("<button onclick='window.history.back()' class='back-button'>← Back to Editor</button>") | ||
|
|
||
| // Program Output | ||
| appendLine(""" | ||
| <div class='output-box ${runtimeStatus}-border'> | ||
| <h2>Program Output</h2> | ||
| <pre>${escapeHtml(getRunOutputDisplay(result))}</pre> | ||
| </div> | ||
| """.trimIndent()) | ||
|
|
||
| // Compiler Output | ||
| appendLine(""" | ||
| <div class='output-box ${compilerStatus}-border'> | ||
| <h2>Compiler Output</h2> | ||
| <p><strong>Compile time:</strong> ${result.compileTimeMs}ms</p> | ||
| <pre>${escapeHtml(result.compileOutput.ifBlank { "Compilation successful!" })}</pre> | ||
| </div> | ||
| """.trimIndent()) | ||
|
|
||
| appendLine("</body></html>") | ||
| } | ||
| } | ||
|
|
||
| private fun getRunOutputDisplay(result: JavaExecutionResult): String = when { | ||
| result.timedOut -> " Program timed out after ${result.timeoutLimit} seconds." | ||
| result.runOutput.isBlank() -> "No output produced." | ||
| else -> result.runOutput | ||
| } |
Contributor
There was a problem hiding this comment.
timedOut conflates compilation timeout with execution timeout, producing misleading output.
JavaExecutionResult.timedOut is set to true in two distinct cases:
- javac timed out (line 776):
runOutput = "Execution skipped.", buttimedOut = true. - java timed out (line 798): expected.
In generatePlaygroundHtml, runtimeStatus and getRunOutputDisplay both key off timedOut, so when the compiler times out, the Program Output box turns red and displays "Program timed out after X seconds." — which is factually wrong. Additionally, compilerStatus will resolve to "success" (green border) because compileOutput is empty/partial, even though compilation failed.
Consider splitting the field into compileTimedOut and runTimedOut, or using an enum, so each output section can display accurate status information.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around
lines 845 - 886, The code conflates compilation vs execution timeouts via
JavaExecutionResult.timedOut; add two explicit flags (e.g.,
JavaExecutionResult.compileTimedOut and JavaExecutionResult.runTimedOut), set
them in the compile and run flows where timedOut is currently assigned, and
update generatePlaygroundHtml and getRunOutputDisplay to use runTimedOut for the
Program Output box and compileTimedOut to force compilerStatus="error" (and
display a clear compile timeout message instead of "Compilation successful!");
ensure any uses of result.timeoutLimit are disambiguated or kept appropriate for
run messages.
Comment on lines
+882
to
+886
| private fun getRunOutputDisplay(result: JavaExecutionResult): String = when { | ||
| result.timedOut -> " Program timed out after ${result.timeoutLimit} seconds." | ||
| result.runOutput.isBlank() -> "No output produced." | ||
| else -> result.runOutput | ||
| } |
Contributor
There was a problem hiding this comment.
Leading space in the timeout message.
" Program timed out..." has a leading space that will appear visually in the <pre> output.
🔎 Proposed fix
- result.timedOut -> " Program timed out after ${result.timeoutLimit} seconds."
+ result.timedOut -> "Program timed out after ${result.timeoutLimit} seconds."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around
lines 882 - 886, The timeout message in getRunOutputDisplay has a leading space
causing an extra space in the rendered output; update the when branch for
result.timedOut in getRunOutputDisplay to remove the leading space so the
message begins with "Program timed out..." (reference: function
getRunOutputDisplay and type JavaExecutionResult) and ensure other branches
remain unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This allows CoGo's local web server to receive, compile, and execute Java code. The main use case for now are code "playgrounds", or interactive coding fragments in web pages that are served to the user.
Ticket
https://appdevforall.atlassian.net/browse/ADFA-1284-- "Create proof of concept for interactive in-browser code environment (like Kotlin's "playground" feature)"
Details
This makes the server accept users' Java code via the POST HTTP method and adds logic for compiling and executing the Java code with javac/java.