ADFA-2807 Webserver endpoint pr/ex returns 200 if experimental mode is on, 404 if not#976
Merged
hal-eisen-adfa merged 10 commits intostagefrom Feb 13, 2026
Merged
ADFA-2807 Webserver endpoint pr/ex returns 200 if experimental mode is on, 404 if not#976hal-eisen-adfa merged 10 commits intostagefrom
hal-eisen-adfa merged 10 commits intostagefrom
Conversation
…s on, 404 if not. Moved the db and pr endpoints to pr/db and pr/pr. Added more debug messages. Made a few cleanups to get rid of warnings and use idiomatic Kotlin.
Contributor
📝 WalkthroughRelease Notes: WebServer Endpoint Updates (PR
|
| Cohort / File(s) | Summary |
|---|---|
WebServer core & API app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt |
Exposed ServerConfig.experimentsEnablePath; replaced socket setReuseAddress(true) call with reuseAddress property assignment; added debug traces around start/stop flow. |
Routing & endpoints app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt |
Consolidated /pr routing to startsWith("pr/"); introduced handleExEndpoint; unified dispatch to pr/db, pr/pr, pr/ex, or 404/500. |
DB access, fragments & resource safety app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt |
Replaced deprecated cursor patterns with count checks; added try/finally to ensure cursor.close() for main and per-fragment queries; guarded single-row expectations and moved data extraction into safe blocks. |
Compression & response handling app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt |
Updated Brotli handling (set "br" when supported; robust local decompression fallback); consolidated header construction and Content-Length calculation; centralized sendError with composite messages and consistent multi-line responses. |
HTML generation & logging app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt |
Refactored HTML assembly for /db and /pr, added debug entry/exit logs, explicit closing tags, and safer header building for tables. |
Parsing & misc fixes app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt |
Switched header parsing to isNotEmpty() checks; minor formatting and log-level adjustments for accept/close failures and exception paths. |
Sequence Diagram
sequenceDiagram
participant Client
participant WebServer
participant DB
participant FileSystem
Client->>WebServer: HTTP GET /pr/...
WebServer->>WebServer: route via startsWith("pr/")
alt pr/db or pr/pr
WebServer->>DB: query table (use cursor.count)
DB-->>WebServer: rows/result
WebServer->>WebServer: build HTML table, close cursors (finally)
WebServer-->>Client: HTTP 200 + HTML
else pr/ex
WebServer->>FileSystem: check experimentsEnablePath exists
FileSystem-->>WebServer: exists / not exists
alt exists
WebServer-->>Client: HTTP 200 + experiments HTML
else
WebServer-->>Client: HTTP 404 (experiments disabled)
end
else unknown pr/* variant
WebServer-->>Client: HTTP 500 (internal error code)
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
- Fix/adfa 1894 #841 — Overlapping edits to
WebServer.ktaround/prrouting,ServerConfig, and cursor/resource-cleanup changes. - Update WebServer.kt -fix old bug found by CodeRabbit #843 — Related modifications to
logDatabaseLastChangedand LastChange handling used inWebServer.kt.
Suggested reviewers
- hal-eisen-adfa
Poem
🐰 I hopped through handlers, tidy and spry,
Routed pr paths beneath a starlit sky,
A tiny flag lets experiments play,
Cursors closed safe at the end of day,
Hop, log, respond — then nibble on hay.
🚥 Pre-merge checks | ✅ 2 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. | |
| Merge Conflict Detection | ❌ Merge conflicts detected (1 file): ⚔️ app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (content)These conflicts must be resolved before merging into stage. |
Resolve conflicts locally and push changes to this branch. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and specifically describes the main change: adding a new pr/ex endpoint with conditional status codes based on experimental mode. |
| Description check | ✅ Passed | The description is directly related to the changeset, mentioning the pr/ex endpoint addition, endpoint path migrations, debug messages, and code cleanups that align with the changes. |
✏️ 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-2807
⚔️ Resolve merge conflicts (beta)
- Auto-commit resolved conflicts to branch
fix/ADFA-2807 - Create stacked PR with resolved conflicts
- Post resolved changes as copyable diffs in a comment
No actionable comments were generated in the recent review. 🎉
🧹 Recent nitpick comments
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (1)
441-443: Dead code:openDatabasenever returnsnull— it throws on failure.
SQLiteDatabase.openDatabase(...)throws on error, soprojectDatabaseis nevernullafter line 437. Thecatchat line 449 already handles that path. This branch is unreachable.Proposed simplification
try { projectDatabase = SQLiteDatabase.openDatabase(config.projectDatabasePath, null, SQLiteDatabase.OPEN_READONLY) - - if (projectDatabase == null) { - log.error("Error handling /pr/pr endpoint 2. Could not open ${config.projectDatabasePath}.") - sendError(writer, 500, "Internal Server Error 5", "Error accessing database 2") - - } else { - realHandlePrEndpoint(writer, output, projectDatabase) - } + realHandlePrEndpoint(writer, output, projectDatabase) } catch (e: Exception) {
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.
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
Copyable Unit Test EditsCopyable Editsapp/src/test/java/org/appdevforall/localwebserver/WebServerTest.ktThis is a new file. |
![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: 5
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 228-236: The cursor returned by database.rawQuery(...) is not
closed on the early-return paths after checking rowCount, causing a resource
leak; update the logic in the method containing the cursor (the block using
variables cursor and rowCount and calling sendError) to ensure cursor.close() is
always executed before any return on error (or better, wrap usage in cursor.use
{ ... } or a try/finally) so that the normal-path close at the end is not the
only cleanup; ensure both the 0 and >1 branches call cursor.close() (or are
inside a cursor.use) before invoking sendError.
- Line 473: The debug exit log contains a copy-paste typo: change the message in
the debug call at the end of realHandlePrEndpoint so it reads "Leaving
realHandlePrEndpoint()." instead of "Leaving realHandleDbEndpoint()." — locate
the debugEnabled check and its log.debug(...) invocation inside the
realHandlePrEndpoint function and update the string accordingly.
- Around line 531-542: sendError currently computes Content-Length using
String.length and relies on PrintWriter.println which can add platform-dependent
line endings; change sendError(signature) to accept the client's OutputStream in
addition to the PrintWriter, build the response body as a UTF-8 byte array (e.g.
val bodyBytes = ("$code $message" + if (details.isEmpty()) "" else
"\n$details").toByteArray(Charsets.UTF_8)), set Content-Length = bodyBytes.size,
write the HTTP response headers using the writer but terminate header section
with a single CRLF (explicit "\r\n\r\n" or writer.println() then
writer.flush()), flush the writer, and then write bodyBytes directly to the
OutputStream (and flush) instead of using PrintWriter.println so the byte count
and line endings are exact; mirror the pattern used in writeNormalToClient.
- Around line 263-269: cursor2.moveToFirst() may return false for missing
fragment paths causing cursor2.getBlob(0) to throw; update the block that
queries with query2 so you check for row existence (e.g., test
cursor2.moveToFirst() or cursor2.count > 0) before calling getBlob(0) and set
dbContent2 to a safe default (null or empty ByteArray) or handle the missing-row
case appropriately; ensure you keep the existing try/finally that closes cursor2
and reference the same symbols (cursor2, query2, moveToFirst, getBlob,
dbContent2) when making the change.
- Line 218: The else branch handling unknown "pr/" sub-paths in WebServer.kt
currently calls sendError(writer, 500, ...), which should be a 404; update the
handler (the branch in the request dispatch logic that matches "pr/" paths—the
else case) to call sendError(writer, 404, "Not Found") (or similar 404 message)
instead of sendError(..., 500, ...), ensuring sendError usage remains consistent
with other handlers and keeping the response content unchanged aside from the
status code and message.
🧹 Nitpick comments (3)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (3)
192-198:parts.isEmpty()is always false — dead code branch.
String.split(",")in Kotlin always returns at least one element (even for an empty string it returns[""]). Thebreakon line 194 is unreachable. This shadowedpartsvariable will never be empty.Consider removing the dead branch or replacing it with a more meaningful guard (e.g., checking if the single element is blank).
372-373: Exception details exposed to client in error response.
e.messageis forwarded verbatim to the HTTP response, which could leak internal paths, SQL errors, or other sensitive details to the client. Even on localhost, consider logging the details server-side only and returning a generic message to the client.Proposed fix
- sendError(writer, 500, "Internal Server Error 4", "Error generating database table: ${e.message}") + sendError(writer, 500, "Internal Server Error 4")The same pattern applies to lines 407, 415 (
handlePrEndpoint), and line 234 in the main handler.
521-521: Debug logging inescapeHtmlis extremely noisy.This fires for every table cell (140+ times for a 20-row × 7-column table) plus titles. Consider removing this log or gating it behind a more granular verbosity flag.
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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/localWebServer/WebServer.kt (1)
512-520:⚠️ Potential issue | 🟠 Major
Content-Lengthmismatch due to extra newline fromprintln.The triple-quoted string already ends with a blank line (the
\nbefore the closing"""), which forms the required header-body separator. Thenprintlnappends an additional platform line separator. This extra byte is written to the output stream beforehtmlBytes, butContent-Lengthonly accounts forhtmlBytes.size. Clients may truncate or misparse the response.Use
println, or usewriter.print("...\r\n\r\n")explicitly for the header terminator:Proposed fix
- writer.println("""HTTP/1.1 200 OK -Content-Type: text/html; charset=utf-8 -Content-Length: ${htmlBytes.size} -Connection: close -""") + writer.print("HTTP/1.1 200 OK\r\nContent-Type: text/html; charset=utf-8\r\nContent-Length: ${htmlBytes.size}\r\nConnection: close\r\n\r\n") writer.flush()
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 264-272: In WebServer.kt remove the redundant unconditional call
to cursor2.moveToFirst() inside the try block so only the conditional if
(cursor2.moveToFirst()) is used to check for rows; update the block around
cursor2, dbContent2 and path2 (the fragment-fetch logic) to rely on the single
conditional check and keep the existing debugEnabled/log.error and break
behavior unchanged.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (1)
186-198:parts.isEmpty()check on line 193 is unreachable.
String.split(",")in Kotlin always returns a list with at least one element. Thisifbranch is dead code. Not harmful, but worth noting for clarity.
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 (4)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (4)
328-330:⚠️ Potential issue | 🟡 MinorRedundant
cursor.close()— already closed in thefinallyblock at line 254.The
try/finallyat lines 233–255 guarantees the cursor is closed. This second call at line 330 is dead code and should be removed to avoid confusion.Proposed fix
output.write(dbContent) output.flush() - cursor.close() }
336-392:⚠️ Potential issue | 🟠 MajorCursor resource leak in
handleDbEndpoint—schemaCursoranddataCursorlacktry/finallycleanup.If an exception is thrown after either cursor is opened but before its
.close()call (e.g., during column iteration or HTML building), the cursor leaks. The outercatchdoesn't close them. This was fixed elsewhere in this PR (e.g.,cursor2at line 276); apply the same pattern here.Proposed fix (sketch)
val schemaCursor = database.rawQuery(schemaQuery, arrayOf()) - val columnCount = schemaCursor.count - val columnNames = mutableListOf<String>() - - while (schemaCursor.moveToNext()) { - if (debugEnabled) log.debug("columnNames are {}.", columnNames) - - columnNames.add(schemaCursor.getString(1)) // Column name is at index 1 - } - schemaCursor.close() + val columnNames: List<String> + val columnCount: Int + try { + columnCount = schemaCursor.count + val names = mutableListOf<String>() + while (schemaCursor.moveToNext()) { + if (debugEnabled) log.debug("columnNames are {}.", names) + names.add(schemaCursor.getString(1)) + } + columnNames = names + } finally { + schemaCursor.close() + }Apply similarly around
dataCursor.
518-531:⚠️ Potential issue | 🟠 Major
Content-Lengthmismatch —printlnadds an extra line ending not accounted for in the body size.The raw string template already ends with a blank line (the empty line before
"""), which produces the header-body separator (\n\n). Thenwriter.println(...)appends an additional\n. This extra byte is sent between the header terminator andoutput.write(htmlBytes), butContent-Lengthonly countshtmlBytes.size. Clients will consider the extra newline as part of the body and truncate the last byte ofhtmlBytes.Use
writer.print(...)instead ofwriter.println(...), or remove the trailing blank line from the raw string.Proposed fix
- writer.println("""HTTP/1.1 200 OK + writer.print("""HTTP/1.1 200 OK Content-Type: text/html; charset=utf-8 Content-Length: ${htmlBytes.size} Connection: close -""") + +""")
456-485:⚠️ Potential issue | 🟠 Major
cursorinrealHandlePrEndpointis not wrapped intry/finally.Same resource leak pattern as
handleDbEndpoint— if an exception occurs during row iteration (lines 471–481), the cursor at line 456 is never closed.Proposed fix (sketch)
val cursor = projectDatabase.rawQuery(query, arrayOf()) + try { if (debugEnabled) log.debug("Retrieved {} rows.", cursor.count) ... // existing row iteration ... - cursor.close() + } finally { + cursor.close() + }
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (2)
186-197: Header parsing breaks early after findingAccept-Encoding.This means if
Accept-Encodingappears before other headers, the rest are unread. Currently fine since only GET is supported and the connection is closed after each request, but worth noting if you later need to parse additional headers.
343-347: Debug log inside the loop logs incomplete column list each iteration.Line 344 logs
columnNamesbefore the current name is added, and repeats on every iteration. The final state is already logged at line 350. Consider removing the in-loop log or moving it after theadd.
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 (2)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (2)
478-507:⚠️ Potential issue | 🟠 MajorResource leak:
cursorinrealHandlePrEndpointis not closed in afinallyblock.Every other cursor in this PR was moved into
try/finally(lines 233–254, 276–287, 345–373, 380–398), but this one still uses a barecursor.close()at line 507. Any exception thrown during iteration (lines 493–503) or HTML building will skip the close.Proposed fix — wrap in try/finally
val cursor = projectDatabase.rawQuery(query, arrayOf()) - if (debugEnabled) log.debug("Retrieved {} rows.", cursor.count) - - var html = getTableHtml("Projects", "Projects") + """ + try { + if (debugEnabled) log.debug("Retrieved {} rows.", cursor.count) + + var html = getTableHtml("Projects", "Projects") + """ <tr> <th>Id</th> ... </tr>""" - while (cursor.moveToNext()) { - html += """<tr> + while (cursor.moveToNext()) { + html += """<tr> ... </tr>""" - } + } - html += "</table></body></html>" + html += "</table></body></html>" - cursor.close() + } finally { + cursor.close() + }
545-553:⚠️ Potential issue | 🟠 Major
writer.printlnadds an extra line separator after the header block, causing a Content-Length mismatch.The raw string template already ends with a blank line (the required HTTP header/body separator).
printlnthen appends an additional platform-dependent line separator. This extra byte(s) sits between headers and the actual body, so an HTTP client countingContent-Lengthbytes from the start of the body will mis-frame the response.Use
writethe headers as bytes, matching the pattern you'd use insendError:Proposed fix
- writer.println("""HTTP/1.1 200 OK + writer.print("""HTTP/1.1 200 OK Content-Type: text/html; charset=utf-8 Content-Length: ${htmlBytes.size} Connection: close """)
🧹 Nitpick comments (2)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (2)
280-283: Missing-fragment error is suppressed when debug is off.
log.erroron line 281 is guarded bydebugEnabled, so a missing fragment (likely a data-integrity issue) is silently swallowed in production. Consider logging this unconditionally since it signals corrupt or incomplete data, unlike the surrounding debug traces.- } else { - if (debugEnabled) log.error("No fragment found for path '{}'.", path2) + } else { + log.error("No fragment found for path '{}'.", path2)
540-541: Debug logs dump entire HTML response bodies.Lines 400, 509, and 541 log the full HTML string, which can be very large (e.g., all rows of a DB table). Even guarded by
debugEnabled, this can flood logs and cause GC pressure. Consider logging only the length, or truncating.
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/localWebServer/WebServer.kt (1)
63-78:⚠️ Potential issue | 🟡 MinorCursor leak in
logDatabaseLastChangedifgetStringthrows.The
cursorat line 69 is closed only on the happy path (line 74). IfmoveToFirst()succeeds butgetString(0)orgetString(1)throws, the catch block at line 75 is entered and the cursor is never closed. The rest of the file consistently usestry/finallyfor cursor cleanup — this function should follow suit.Proposed fix
fun logDatabaseLastChanged() { try { val query = """ SELECT changeTime, who FROM LastChange """ val cursor = database.rawQuery(query, arrayOf()) - if (cursor.count > 0) { - cursor.moveToFirst() - log.debug("Database last change: {} {}.", cursor.getString(0), cursor.getString(1)) + try { + if (cursor.count > 0) { + cursor.moveToFirst() + log.debug("Database last change: {} {}.", cursor.getString(0), cursor.getString(1)) + } + } finally { + cursor.close() } - cursor.close() } catch (e: Exception) { log.debug("Could not retrieve database last change info: {}", e.message) } }
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (1)
345-371:selectColumnsis interpolated directly into SQL — safe here, but fragile.Column names from
PRAGMA table_infoare not user-supplied, so this isn't exploitable. However, if this pattern were copied for user-controlled input it would be a SQL injection vector. A brief comment noting why interpolation is safe (values come from schema introspection) would prevent future confusion.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (2)
63-75:⚠️ Potential issue | 🟡 MinorEnsure the cursor closes on exceptions in
logDatabaseLastChanged.
cursor.close()is skipped ifmoveToFirst()/getString()throws. Wrap it inuse {}for guaranteed cleanup.🔧 Suggested fix
- val cursor = database.rawQuery(query, arrayOf()) - if (cursor.count > 0) { - cursor.moveToFirst() - log.debug("Database last change: {} {}.", cursor.getString(0), cursor.getString(1)) - } - cursor.close() + database.rawQuery(query, emptyArray<String>()).use { cursor -> + if (cursor.moveToFirst()) { + log.debug("Database last change: {} {}.", cursor.getString(0), cursor.getString(1)) + } + }
268-293:⚠️ Potential issue | 🟡 MinorAvoid serving partial content when a fragment is missing.
When a fragment is missing, the loop breaks and the server still returns200with truncated content. Consider returning an error instead.💡 Suggested fix
- var fragmentNumber = 1 - var dbContent2 = dbContent + var fragmentNumber = 1 + var dbContent2 = dbContent + var missingFragmentPath: String? = null @@ - } else { - log.error("No fragment found for path '{}'.", path2) - break - } + } else { + log.error("No fragment found for path '{}'.", path2) + missingFragmentPath = path2 + break + } @@ - } + } + if (missingFragmentPath != null) { + return sendError( + writer, + 500, + "Internal Server Error", + "Missing fragment for path: $missingFragmentPath" + ) + }
🤖 Fix all issues with AI agents
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 542-550: The response headers are being sent with writer.println
on a multi-line string that already ends with a blank line, which causes an
extra CRLF before the body and makes Content-Length incorrect; in
writeNormalToClient() replace the writer.println call (or otherwise stop using
println for the header block) so the headers string ends with exactly one
CRLFCRLF and use writer.print (or trim the trailing newline in the triple-quoted
string) to ensure the body starts immediately and Content-Length
(htmlBytes.size) matches the actual body bytes.
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
2 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.
Added endpoint, pr/ex, to return 200 if experimental mode is on, 404 if not. Moved the db and pr endpoints to pr/db and pr/pr. Added more debug messages. Made a few cleanups to get rid of warnings and use idiomatic Kotlin.