Adfa facility to disable webserver's pebble cache#1357
Conversation
Docstrings generation was requested by @davidschachterADFA. The following files were modified: * `app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`
jimturner-adfa
requested review from
alexmmiller,
davidschachterADFA and
hal-eisen-adfa
📝 WalkthroughRelease Notes: Pebble Cache Disable FacilityFeatures
Implementation Details
Risks & Best Practices Violations
Migration/Testing Considerations
WalkthroughWebServer.kt is refactored to implement template-based bookshelf rendering. A new ChangesBookshelf Template Rendering Pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt (1)
72-72: 💤 Low valueNon-atomic read-modify-write on
bookshelfTemplateIdin concurrent context.
bookshelfTemplateIdis a mutablevaraccessed fromrealHandleBsEndpointwhich can be called by multiple request-handling threads. The check-then-set pattern at lines 759-771 is racy. While functionally benign (same value is always written), consider usingAtomicIntegeror synchronization for clarity and correctness.♻️ Suggested fix using AtomicInteger
- private var bookshelfTemplateId : Int = -1; + private val bookshelfTemplateId = java.util.concurrent.atomic.AtomicInteger(-1)Then update the usage in
realHandleBsEndpoint:// Check if template ID is cached if (bookshelfTemplateId.get() == -1) { // ... query database ... bookshelfTemplateId.compareAndSet(-1, cursor.getInt(0)) } val templateId = bookshelfTemplateId.get()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` at line 72, The field bookshelfTemplateId is mutated non-atomically and read from realHandleBsEndpoint by concurrent request threads; replace the Int var with an AtomicInteger (or guard accesses with a synchronized block) and update all reads/writes in realHandleBsEndpoint to use AtomicInteger.get(), compareAndSet(-1, newValue) when caching the DB result, and AtomicInteger.get() when reading the templateId to use—this eliminates the racy check-then-set on bookshelfTemplateId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Around line 776-784: The catch in WebServer.kt currently logs and sends an
error response but does not stop further processing, allowing code after the
try/catch (which uses jsonText) to run with jsonText uninitialized; modify the
catch block in the request-handling method so that after calling
sendError(writer, output, 500, "Internal Server Error", e.message ?: "") it
immediately returns false (or otherwise exits the handler) to prevent reaching
instantiatePebbleTemplate(...) and using jsonText, ensuring cursor.close() still
runs in finally.
- Around line 490-492: The unconditional debug call "log.debug(\"context =
${context}\")" in WebServer.kt should be gated like other debug logs to avoid
interpolation overhead; wrap it with the existing debug check (e.g., use
log.isDebugEnabled or the file's debugEnabled pattern) or use a lazy/logging API
that defers message construction, referencing the "log.debug" call and the
"context" value so the message is only constructed and emitted when debugging is
enabled.
- Around line 759-771: The code reassigns the variable cursor when fetching the
'bookshelf' template ID, leaking the earlier cursor resource; before calling
database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'") reclose
the existing cursor (or use a new variable like templateCursor) so the original
cursor is closed, then proceed to check cursor.count, moveToFirst(), and set
bookshelfTemplateId; ensure the finally block still closes whichever cursor
variable(s) are used and that sendError and the rest of the logic remain
unchanged.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`:
- Line 72: The field bookshelfTemplateId is mutated non-atomically and read from
realHandleBsEndpoint by concurrent request threads; replace the Int var with an
AtomicInteger (or guard accesses with a synchronized block) and update all
reads/writes in realHandleBsEndpoint to use AtomicInteger.get(),
compareAndSet(-1, newValue) when caching the DB result, and AtomicInteger.get()
when reading the templateId to use—this eliminates the racy check-then-set on
bookshelfTemplateId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9380d000-c893-4a8f-a865-2aa7f2e0c81a
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
| /*******************DEBUGGING ONLY*******************/ | ||
| log.debug("context = ${context}") | ||
| /*******************DEBUGGING ONLY*******************/ |
There was a problem hiding this comment.
Unconditional debug logging should be gated.
This log.debug call is not guarded by debugEnabled, unlike other debug logging throughout the file. This will execute the string interpolation and logging call on every template instantiation even in production, causing unnecessary overhead.
🔧 Proposed fix
/*******************DEBUGGING ONLY*******************/
- log.debug("context = ${context}")
+ if (debugEnabled) log.debug("context = {}", context)
/*******************DEBUGGING ONLY*******************/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around
lines 490 - 492, The unconditional debug call "log.debug(\"context =
${context}\")" in WebServer.kt should be gated like other debug logs to avoid
interpolation overhead; wrap it with the existing debug check (e.g., use
log.isDebugEnabled or the file's debugEnabled pattern) or use a lazy/logging API
that defers message construction, referencing the "log.debug" call and the
"context" value so the message is only constructed and emitted when debugging is
enabled.
| if (bookshelfTemplateId == -1) { | ||
| cursor = database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'", arrayOf()) | ||
|
|
||
| if (cursor.count != 1) { | ||
| if (cursor.count == 0) | ||
| sendError(writer, output, 404, "Not Found") | ||
| else | ||
| sendError(writer, output, 500, "Corrupt database - ${cursor.count} Bookshelf templates found when 1 was expected.") | ||
| return false | ||
| } | ||
|
|
||
| try { | ||
| if (debugEnabled) log.debug("Retrieved {} rows.", cursor.count) | ||
| cursor.moveToFirst() | ||
| bookshelfTemplateId = cursor.getInt(0); |
There was a problem hiding this comment.
Cursor resource leak when template ID is fetched.
When bookshelfTemplateId == -1, the cursor variable is reassigned at line 760, losing the reference to the first cursor (from line 739). The finally block at line 781 only closes the second cursor, leaking the first.
🐛 Proposed fix - close first cursor before reassigning
//Have we already fetched the template
if (bookshelfTemplateId == -1) {
+ cursor.close() // Close the bookshelf JSON cursor before reusing variable
cursor = database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'", arrayOf())
if (cursor.count != 1) {Alternatively, use a separate variable for the template cursor to make the resource management clearer.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (bookshelfTemplateId == -1) { | |
| cursor = database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'", arrayOf()) | |
| if (cursor.count != 1) { | |
| if (cursor.count == 0) | |
| sendError(writer, output, 404, "Not Found") | |
| else | |
| sendError(writer, output, 500, "Corrupt database - ${cursor.count} Bookshelf templates found when 1 was expected.") | |
| return false | |
| } | |
| try { | |
| if (debugEnabled) log.debug("Retrieved {} rows.", cursor.count) | |
| cursor.moveToFirst() | |
| bookshelfTemplateId = cursor.getInt(0); | |
| if (bookshelfTemplateId == -1) { | |
| cursor.close() // Close the bookshelf JSON cursor before reusing variable | |
| cursor = database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'", arrayOf()) | |
| if (cursor.count != 1) { | |
| if (cursor.count == 0) | |
| sendError(writer, output, 404, "Not Found") | |
| else | |
| sendError(writer, output, 500, "Corrupt database - ${cursor.count} Bookshelf templates found when 1 was expected.") | |
| return false | |
| } | |
| cursor.moveToFirst() | |
| bookshelfTemplateId = cursor.getInt(0); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around
lines 759 - 771, The code reassigns the variable cursor when fetching the
'bookshelf' template ID, leaking the earlier cursor resource; before calling
database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'") reclose
the existing cursor (or use a new variable like templateCursor) so the original
cursor is closed, then proceed to check cursor.count, moveToFirst(), and set
bookshelfTemplateId; ensure the finally block still closes whichever cursor
variable(s) are used and that sendError and the rest of the logic remain
unchanged.
| } catch (e: Exception) { | ||
| log.error("Error processing request: {}", e.message) | ||
| sendError(writer, output, 500, "Internal Server Error", e.message ?: "") | ||
|
|
||
| } finally { | ||
| cursor.close() | ||
| } | ||
|
|
||
| if (debugEnabled) log.debug("html is '{}'.", html) // May output a lot of stuff but better too much than too little. --DS, 23-Feb-2026 | ||
| val result = instantiatePebbleTemplate(bookshelfTemplateId, jsonText, "/bookshelf", "application/json", "none") |
There was a problem hiding this comment.
Missing return false after error in catch block causes use of uninitialized variable.
The catch block at lines 776-778 sends an error response but doesn't return. Execution continues to line 784 where jsonText may be uninitialized (if the exception occurred before line 754), causing UninitializedPropertyAccessException.
🐛 Proposed fix
} catch (e: Exception) {
log.error("Error processing request: {}", e.message)
sendError(writer, output, 500, "Internal Server Error", e.message ?: "")
+ return false
} finally {
cursor.close()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e: Exception) { | |
| log.error("Error processing request: {}", e.message) | |
| sendError(writer, output, 500, "Internal Server Error", e.message ?: "") | |
| } finally { | |
| cursor.close() | |
| } | |
| if (debugEnabled) log.debug("html is '{}'.", html) // May output a lot of stuff but better too much than too little. --DS, 23-Feb-2026 | |
| val result = instantiatePebbleTemplate(bookshelfTemplateId, jsonText, "/bookshelf", "application/json", "none") | |
| } catch (e: Exception) { | |
| log.error("Error processing request: {}", e.message) | |
| sendError(writer, output, 500, "Internal Server Error", e.message ?: "") | |
| return false | |
| } finally { | |
| cursor.close() | |
| } | |
| val result = instantiatePebbleTemplate(bookshelfTemplateId, jsonText, "/bookshelf", "application/json", "none") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt` around
lines 776 - 784, The catch in WebServer.kt currently logs and sends an error
response but does not stop further processing, allowing code after the try/catch
(which uses jsonText) to run with jsonText uninitialized; modify the catch block
in the request-handling method so that after calling sendError(writer, output,
500, "Internal Server Error", e.message ?: "") it immediately returns false (or
otherwise exits the handler) to prevent reaching instantiatePebbleTemplate(...)
and using jsonText, ensuring cursor.close() still runs in finally.
| return String(bytes, 0, len, Charsets.ISO_8859_1) | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This comment/documentation and some other new ones in this file seem to have been added despite the functions not being new.
I think they can be removed or at the very least be more concise.
| val mapper = ObjectMapper() | ||
| val context: Map<String, Any> = mapper.readValue(dbContent.toString(Charsets.UTF_8), object : TypeReference<Map<String, Any>>() {}) | ||
|
|
||
| /*******************DEBUGGING ONLY*******************/ |
There was a problem hiding this comment.
This log should be safe to remove
|
|
||
| //Have we already fetched the template | ||
| if (bookshelfTemplateId == -1) { | ||
| cursor = database.rawQuery("SELECT id FROM Templates WHERE name = 'bookshelf'", arrayOf()) |
There was a problem hiding this comment.
The cursor wasn't previously closed before its reassignment here. Let's ensure safe closure.
| WHERE C.id = B.contentID | ||
| AND B.bookCategoryID = BC.id | ||
| GROUP BY BC.category | ||
| ORDER BY BC.category, |
There was a problem hiding this comment.
Adding ORDER BY after GROUP BY will sort the resulting category group, and not the JSON_GROUP_ARRAY itself. Is that the intention?
If that's not the intention, then this subquery will sort the items in JSON_GROUP_ARRAY:
FROM (
SELECT BC.category, BC.description as category_desc, IFNULL(B.title, C.path) as title, B.description as book_desc, C.path
FROM Content C
JOIN Bookshelf B ON C.id = B.contentID
JOIN BookCategories BC ON B.bookCategoryID = BC.id
ORDER BY BC.category, title
)
GROUP BY category
);
| val debugDatabaseTimestamp = getDatabaseTimestamp(config.debugDatabasePath, true) | ||
| if (debugDatabaseTimestamp > databaseTimestamp) { | ||
| database.close() | ||
| database = SQLiteDatabase.openDatabase(config.debugDatabasePath, null, SQLiteDatabase.OPEN_READONLY) |
There was a problem hiding this comment.
You might want to reset bookshelfTemplateId to -1 here because of the database swap happening, as well as when clearCacheEnabled is triggered.
3 participants
Clear the Pebble Cache (Code On the Go's cache) if the file
CodeOnTheGo.webserver.cs0
is in /sdcard/Download