Adfa 3802 make documentation bookshelf dynamic#1353
Open
jimturner-adfa wants to merge 5 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough
WalkthroughAdds a high-priority /pr/bs endpoint that queries bookshelf data, lazily caches a Pebble template ID, renders the response via instantiatePebbleTemplate with extra debug logs, and writes output with error responses gated on whether output started. ChangesBookshelf Endpoint Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
⚔️ Resolve merge conflicts
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 696-708: The code in WebServer.kt overwrites the existing cursor
when running the template-ID query (affecting bookshelfTemplateId) and never
closes the original cursor; fix by ensuring each Cursor is closed — either use a
second local variable for the template query (e.g., templateCursor) or wrap each
rawQuery result in a Kotlin use { } block so cursor.close() is called
automatically before assigning or returning; update the block around the
rawQuery/cursor.moveToFirst()/getInt(0) logic that sets bookshelfTemplateId and
any sendError branches to close the appropriate cursor(s).
- Around line 67-69: When the DB handle is reopened/rotated (the block that
reassigns the database variable), clear the template caches so old templates
from the previous DB aren’t reused: call templateCache.clear() and reset
bookshelfTemplateId = -1 immediately after the database swap (the same place
where the code reopens/assigns database). This ensures PebbleTemplate entries in
templateCache and the cached bookshelfTemplateId are invalidated when the
database changes.
- Around line 567-578: The catch block can append a second 500 because
outputStarted is only set true after realHandleBsEndpoint returns; modify the
flow so outputStarted is set to true as soon as any headers/body are written
(i.e., inside the write path) rather than only on return. Concretely, update
realHandleBsEndpoint (or the helper that calls writeNormalToClient) to accept a
mutable flag (e.g., AtomicBoolean or a setter lambda) or return a result object
so it flips outputStarted to true immediately when the first write/flush
happens, then keep the existing try/catch in handleBsEndpoint to rely on that
flag. Ensure references: handleBsEndpoint, realHandleBsEndpoint, and the write
method (writeNormalToClient) are changed accordingly so the catch sees the
correct outputStarted state.
- Around line 713-721: The catch block that logs the fetch error and calls
sendError must stop further processing so instantiatePebbleTemplate(...) is not
called with uninitialized jsonText or bookshelfTemplateId; modify the catch
(Exception e) handling around the fetch to, after logging and calling
sendError(writer, output, 500, ...), immediately return (or otherwise
short-circuit) from the surrounding method so cursor.close() remains in finally
and instantiatePebbleTemplate(...) is never reached when an exception occurred.
🪄 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: 7f80d995-fb24-456b-a5b1-4809b7f61d43
📒 Files selected for processing (1)
app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt
Comment on lines
+67
to
+69
| private val pebbleEngine = PebbleEngine.Builder().loader(StringLoader()).build() | ||
| private val templateCache = ConcurrentHashMap<Int, PebbleTemplate>() | ||
| private var bookshelfTemplateId : Int = -1; |
Contributor
There was a problem hiding this comment.
Invalidate these caches when the database handle changes.
Lines 308-313 can reopen database, but templateCache and bookshelfTemplateId survive that swap. After a reload, /pr/bs can keep using a template ID or compiled template from the previous database.
🤖 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 67 - 69, When the DB handle is reopened/rotated (the block that reassigns
the database variable), clear the template caches so old templates from the
previous DB aren’t reused: call templateCache.clear() and reset
bookshelfTemplateId = -1 immediately after the database swap (the same place
where the code reopens/assigns database). This ensures PebbleTemplate entries in
templateCache and the cached bookshelfTemplateId are invalidated when the
database changes.
Comment on lines
+567
to
+578
| private fun handleBsEndpoint(writer: PrintWriter, output: java.io.OutputStream) { | ||
| if (debugEnabled) log.debug("Entering handleBsEndpoint().") | ||
|
|
||
| var outputStarted = false | ||
|
|
||
| try { | ||
|
|
||
| outputStarted = realHandleBsEndpoint(writer, output) | ||
|
|
||
| } catch (e: Exception) { | ||
| log.error("Error handling /pr/bs endpoint: {}", e.message) | ||
| sendError(writer, output, 500, "Internal Server Error 6", "Error generating bookshelf HTML.", outputStarted) |
Contributor
There was a problem hiding this comment.
Set outputStarted before the first write.
outputStarted only becomes true after realHandleBsEndpoint() returns. If writeNormalToClient() throws after sending headers or part of the body, this catch will still append a second 500 because the flag is still false.
🤖 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 567 - 578, The catch block can append a second 500 because outputStarted
is only set true after realHandleBsEndpoint returns; modify the flow so
outputStarted is set to true as soon as any headers/body are written (i.e.,
inside the write path) rather than only on return. Concretely, update
realHandleBsEndpoint (or the helper that calls writeNormalToClient) to accept a
mutable flag (e.g., AtomicBoolean or a setter lambda) or return a result object
so it flips outputStarted to true immediately when the first write/flush
happens, then keep the existing try/catch in handleBsEndpoint to rely on that
flag. Ensure references: handleBsEndpoint, realHandleBsEndpoint, and the write
method (writeNormalToClient) are changed accordingly so the catch sees the
correct outputStarted state.
Comment on lines
+696
to
+708
| 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 | ||
| } | ||
|
|
||
| cursor.moveToFirst() | ||
| bookshelfTemplateId = cursor.getInt(0); |
Contributor
There was a problem hiding this comment.
Don't overwrite the first cursor before closing it.
The cursor from rawQuery(sql_query, ...) is replaced by the template-ID query, so the bookshelf-result cursor never gets closed on the initial /pr/bs request. Use a second local cursor or nested use {} blocks here.
🤖 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 696 - 708, The code in WebServer.kt overwrites the existing cursor when
running the template-ID query (affecting bookshelfTemplateId) and never closes
the original cursor; fix by ensuring each Cursor is closed — either use a second
local variable for the template query (e.g., templateCursor) or wrap each
rawQuery result in a Kotlin use { } block so cursor.close() is called
automatically before assigning or returning; update the block around the
rawQuery/cursor.moveToFirst()/getInt(0) logic that sets bookshelfTemplateId and
any sendError branches to close the appropriate cursor(s).
Comment on lines
+713
to
+721
| } catch (e: Exception) { | ||
| log.error("Error processing request: {}", e.message) | ||
| sendError(writer, output, 500, "Internal Server Error", e.message ?: "") | ||
|
|
||
| } finally { | ||
| cursor.close() | ||
| } | ||
|
|
||
| val result = instantiatePebbleTemplate(bookshelfTemplateId, jsonText, "/bookshelf", "application/json", "none") |
Contributor
There was a problem hiding this comment.
Return immediately after sending the fetch-time error.
If anything in the fetch block fails before jsonText or bookshelfTemplateId is initialized, this catch writes an error and then continues into instantiatePebbleTemplate(...). That turns one failure into a second exception and can trigger another response write.
Suggested 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.
Suggested change
| } catch (e: Exception) { | |
| log.error("Error processing request: {}", e.message) | |
| sendError(writer, output, 500, "Internal Server Error", e.message ?: "") | |
| } finally { | |
| cursor.close() | |
| } | |
| 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 713 - 721, The catch block that logs the fetch error and calls sendError
must stop further processing so instantiatePebbleTemplate(...) is not called
with uninitialized jsonText or bookshelfTemplateId; modify the catch (Exception
e) handling around the fetch to, after logging and calling sendError(writer,
output, 500, ...), immediately return (or otherwise short-circuit) from the
surrounding method so cursor.close() remains in finally and
instantiatePebbleTemplate(...) is never reached when an exception occurred.
jatezzz
requested changes
Jun 1, 2026
| if (debugEnabled) log.debug("before fetch bookshelf template ID = '${bookshelfTemplateId}'") | ||
|
|
||
| //Have we already fetched the template | ||
| if (bookshelfTemplateId == -1) { |
Collaborator
There was a problem hiding this comment.
Could you avoid the nested structure?
if () {
if () {
if () {
}
}
}|
|
||
| } finally { | ||
| projectDatabase?.close() | ||
| sendError(writer, output, 500, "Internal Server Error 6", "Error generating bookshelf HTML.", outputStarted) |
Collaborator
There was a problem hiding this comment.
You could create a constant to centralize the 500 value.
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @davidschachterADFA. The following files were modified: * `app/src/main/java/com/itsaky/androidide/localWebServer/WebServer.kt`
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.
First working version of dynamic bookshelf from curl
Need to add HTML template