Audit codebase and implement comprehensive improvements - #22
Conversation
…dules - Shared KMP: Fixed Float.MIN_VALUE issue in bounding box calculation, optimized DBSCAN cluster performance, added division-by-zero protection in Region, and added unit tests. - Android App: Unified region containment checks with shared KMP logic, fixed main thread handler clearing in ConfigFileObserver, and improved Xposed logger thread lifecycle. - Module Scripts: Removed bash-isms from CLI script, updated default profile directory path, and made APK integrity check fallback to unzip. - PC Tool: Fixed process argument handling for paths with spaces in ADBHelper, added process timeouts, and guarded Debian package target based on fakeroot availability. - Build & Docs: Updated Gradle DSL accessors, added default version properties, and updated documentation. Co-authored-by: Laviesss <88744635+Laviesss@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1d2614e61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val dx = x - x1 | ||
| val dy = y - y1 | ||
| (dx * dx + dy * dy) <= (x2 * x2) |
There was a problem hiding this comment.
Keep circle hit tests in screen space
When a user configures a type=1 circular block or exclude zone on a non-square display, the overlay still renders it with radius x2 * width, but this shared contains check compares normalized x/y deltas directly, which makes the effective vertical radius x2 * height. Because both the Xposed hook and overlay services now call region.contains, tall phones will block or whitelist touches well outside the drawn circle; keep the circle hit test in pixel/screen space or pass the display dimensions into this calculation.
Useful? React with 👍 / 👎.
Laviesss
left a comment
There was a problem hiding this comment.
Review complete — summary in the review body; two line comments with suggested fixes. Net positive: mergeable after addressing the two comments (or filing follow-up issues) and adding a CHANGELOG note for the circle-geometry change.
| val reader = process.inputStream.bufferedReader() | ||
| val output = reader.readText() | ||
| process.waitFor() | ||
| val finished = process.waitFor(10, TimeUnit.SECONDS) |
There was a problem hiding this comment.
The timeout here is effectively dead code: reader.readText() on the line above blocks until the process closes stdout (or EOF), so process.waitFor(10, SECONDS) is only reached after the process already exited. A wedged adb (device offline, server hung) still blocks the PC tool indefinitely — the exact case this timeout is meant to protect against.
Suggested fix — bound the read instead of the wait:
val output = try {
// read up to the timeout, then kill
val future = CompletableFuture.supplyAsync { reader.readText() }
future.get(10, TimeUnit.SECONDS)
} catch (_: TimeoutException) {
process.destroyForcibly()
""
} catch (e: Exception) {
process.destroyForcibly()
println("Error running command ${cmd.firstOrNull()}: ${e.message}")
""
}(executor must be a daemon thread so the pool doesn't outlive the JVM).
| [ -z "$x1" ] && continue | ||
| echo "$x1" | grep -qE '^[0-9]+$' || continue | ||
|
|
||
| get_regions | while read -r line; do |
There was a problem hiding this comment.
FOUND is set inside the get_regions | while ... pipeline, which runs in a subshell — the assignment never propagates to the parent shell. Combined with the removed [ "$FOUND" -eq 0 ] guard (old line ~1297), remove_region with a nonexistent ID now silently succeeds: the region isn't found, nothing is removed, and mv still rewrites the config file with no error reported.
Also note the echo "Removed region..." fires before the file is actually rewritten — if the region exists this is fine, but the pre-delete message plus the missing error path is the regression here.
Suggested fix:
FOUND=0
REGIONS=$(get_regions) # capture once, no pipe subshell
printf '%s\n' "$REGIONS" | while IFS= read -r line; do
[ -z "$line" ] && continue
COUNT=$((COUNT + 1))
if [ "$COUNT" -eq "$target_id" ]; then
FOUND=1
echo "Removed region [$target_id]: $line"
else
echo "$line" >> "$TEMP_FILE"
fi
done
if [ "$FOUND" -eq 0 ]; then
rm -f "$TEMP_FILE"
echo "Error: Region $target_id not found"
return 1
fi(The same subshell pattern also makes COUNT in show_status dead in the parent — harmless there since TOTAL is computed separately, but worth knowing.)
There was a problem hiding this comment.
Good catch! I have refactored remove_region in module/system/bin/inputblocker to avoid subshell pipeline execution when processing regions line-by-line. FOUND is now tracked in the main shell environment, and if FOUND is 0, an error message is printed and execution terminates with exit status 1 without modifying the config file.
…ogic - ADBHelper.kt: Bound stream read with CompletableFuture and TimeoutException to forcibly destroy wedged ADB processes and prevent blocking the application thread. - module/system/bin/inputblocker: Eliminate subshell pipeline in remove_region and show_status, ensuring FOUND variable propagates correctly and non-existent region IDs error cleanly. Co-authored-by: Laviesss <88744635+Laviesss@users.noreply.github.com>
This change conducts an overall audit across all components in the monorepo, fixing bugs in bounding box calculations, process execution, thread management, shell scripts, and build configurations while adding unit tests for the core logic.
PR created automatically by Jules for task 10188766728484556702 started by @Laviesss