feat: Add custom scrollbar and delivery APK (with performance analysis)#50
feat: Add custom scrollbar and delivery APK (with performance analysis)#50Android-PowerUser wants to merge 1 commit intomainfrom
Conversation
… APK This PR includes: 1. A custom semi-transparent gray scrollbar for the PhotoReasoning LazyColumn that fades out after 1 second of inactivity. 2. The signed release APK. 3. Analysis of Jules platform latency and performance. Co-authored-by: Android-PowerUser <88908510+Android-PowerUser@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.
This PR implements a custom scrollbar for the PhotoReasoning chat screen and adds build monitoring infrastructure. However, there are critical security vulnerabilities and logic errors that block merge.
Critical Issues (Must Fix Before Merge)
Security Vulnerabilities:
- Hardcoded notification topic "Jules" in monitor_build.sh exposes build progress to unauthorized access via ntfy.sh public channels (CWE-200)
- Command injection vulnerabilities from unquoted variable expansion in curl commands (CWE-78)
Logic Errors:
- Infinite loop risk in monitor_build.sh with no error handling or timeout protection
Performance Regression:
- Scrollbar calculations run on every frame during scroll operations instead of being memoized
Implementation Review
The custom scrollbar implementation follows proper Compose patterns with fade animations and proper lifecycle management. The modifier correctly uses drawWithContent and LaunchedEffect for state management.
All security vulnerabilities and the infinite loop issue must be resolved before this PR can be merged.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| val scrollbarHeight = (visibleItemsInfo.size.toFloat() / totalItemsCount) * viewportHeight | ||
| val scrollbarOffset = (firstVisibleItem.index.toFloat() / totalItemsCount) * viewportHeight |
There was a problem hiding this comment.
🛑 Performance Regression: Scrollbar calculation on every frame causes performance degradation. The scrollbar height and offset calculations run on every composition when alpha > 0, creating unnecessary work during scroll operations.
| val scrollbarHeight = (visibleItemsInfo.size.toFloat() / totalItemsCount) * viewportHeight | |
| val scrollbarOffset = (firstVisibleItem.index.toFloat() / totalItemsCount) * viewportHeight | |
| // Estimation of scrollbar height and offset | |
| val scrollbarHeight = remember(totalItemsCount, visibleItemsInfo.size) { | |
| (visibleItemsInfo.size.toFloat() / totalItemsCount) * viewportHeight | |
| } | |
| val scrollbarOffset = remember(firstVisibleItem.index, totalItemsCount) { | |
| (firstVisibleItem.index.toFloat() / totalItemsCount) * viewportHeight | |
| } |
| @@ -0,0 +1,23 @@ | |||
| #!/bin/bash | |||
| TOPIC="Jules" | |||
There was a problem hiding this comment.
🛑 Security Vulnerability: Hardcoded notification topic "Jules" exposes notifications to unauthorized access. The ntfy.sh service uses topics as public channels - anyone knowing this topic name can read build progress notifications.
| TOPIC="Jules" | |
| TOPIC="${NTFY_TOPIC:-$(uuidgen)}" |
| elif echo "$PANE_CONTENT" | grep -q "Starting build and sign process."; then | ||
| PROGRESS=5 | ||
| fi | ||
| curl -d "Fortschritt: $PROGRESS% (geschätzt) - Task läuft im Hintergrund." ntfy.sh/$TOPIC |
There was a problem hiding this comment.
🛑 Security Vulnerability: Same command injection vulnerability as line 7. Quote the URL to prevent shell expansion.1
| curl -d "Fortschritt: $PROGRESS% (geschätzt) - Task läuft im Hintergrund." ntfy.sh/$TOPIC | |
| curl -d "Fortschritt: $PROGRESS% (geschätzt) - Task läuft im Hintergrund." "ntfy.sh/$TOPIC" |
Footnotes
-
CWE-78: OS Command Injection - https://cwe.mitre.org/data/definitions/78.html ↩
| while true; do | ||
| PANE_CONTENT=$(tmux capture-pane -t default -p) | ||
| if echo "$PANE_CONTENT" | grep -q "Build and sign process complete."; then | ||
| curl -d "Fortschritt: 100% - Build & Sign abgeschlossen!" ntfy.sh/$TOPIC |
There was a problem hiding this comment.
🛑 Security Vulnerability: Unquoted variable expansion allows command injection. If TOPIC contains spaces or special characters, curl could execute arbitrary commands.1
| curl -d "Fortschritt: 100% - Build & Sign abgeschlossen!" ntfy.sh/$TOPIC | |
| curl -d "Fortschritt: 100% - Build & Sign abgeschlossen!" "ntfy.sh/$TOPIC" |
Footnotes
-
CWE-78: OS Command Injection - https://cwe.mitre.org/data/definitions/78.html ↩
| while true; do | ||
| PANE_CONTENT=$(tmux capture-pane -t default -p) | ||
| if echo "$PANE_CONTENT" | grep -q "Build and sign process complete."; then | ||
| curl -d "Fortschritt: 100% - Build & Sign abgeschlossen!" ntfy.sh/$TOPIC | ||
| break | ||
| elif echo "$PANE_CONTENT" | grep -q "Signing the APK..."; then | ||
| PROGRESS=90 | ||
| elif echo "$PANE_CONTENT" | grep -q "Generating test signing key..."; then | ||
| PROGRESS=85 | ||
| elif echo "$PANE_CONTENT" | grep -q "Building the application..."; then | ||
| # Within gradle, we can only guess without parsing gradle's own progress | ||
| PROGRESS=40 | ||
| elif echo "$PANE_CONTENT" | grep -q "Installing SDK packages..."; then | ||
| PROGRESS=20 | ||
| elif echo "$PANE_CONTENT" | grep -q "Starting build and sign process."; then | ||
| PROGRESS=5 | ||
| fi | ||
| curl -d "Fortschritt: $PROGRESS% (geschätzt) - Task läuft im Hintergrund." ntfy.sh/$TOPIC | ||
| sleep 60 | ||
| done |
There was a problem hiding this comment.
🛑 Logic Error: Missing error handling causes infinite loop if tmux session doesn't exist or curl fails. Add validation to prevent resource exhaustion.
| while true; do | |
| PANE_CONTENT=$(tmux capture-pane -t default -p) | |
| if echo "$PANE_CONTENT" | grep -q "Build and sign process complete."; then | |
| curl -d "Fortschritt: 100% - Build & Sign abgeschlossen!" ntfy.sh/$TOPIC | |
| break | |
| elif echo "$PANE_CONTENT" | grep -q "Signing the APK..."; then | |
| PROGRESS=90 | |
| elif echo "$PANE_CONTENT" | grep -q "Generating test signing key..."; then | |
| PROGRESS=85 | |
| elif echo "$PANE_CONTENT" | grep -q "Building the application..."; then | |
| # Within gradle, we can only guess without parsing gradle's own progress | |
| PROGRESS=40 | |
| elif echo "$PANE_CONTENT" | grep -q "Installing SDK packages..."; then | |
| PROGRESS=20 | |
| elif echo "$PANE_CONTENT" | grep -q "Starting build and sign process."; then | |
| PROGRESS=5 | |
| fi | |
| curl -d "Fortschritt: $PROGRESS% (geschätzt) - Task läuft im Hintergrund." ntfy.sh/$TOPIC | |
| sleep 60 | |
| done | |
| MAX_ITERATIONS=120 # 2 hours max | |
| ITERATION=0 | |
| while [ $ITERATION -lt $MAX_ITERATIONS ]; do | |
| if ! tmux has-session -t default 2>/dev/null; then | |
| echo "Error: tmux session 'default' not found" >&2 | |
| exit 1 | |
| fi | |
| PANE_CONTENT=$(tmux capture-pane -t default -p) | |
| if echo "$PANE_CONTENT" | grep -q "Build and sign process complete."; then | |
| curl -d "Fortschritt: 100% - Build & Sign abgeschlossen!" "ntfy.sh/$TOPIC" || true | |
| break | |
| elif echo "$PANE_CONTENT" | grep -q "Signing the APK..."; then | |
| PROGRESS=90 | |
| elif echo "$PANE_CONTENT" | grep -q "Generating test signing key..."; then | |
| PROGRESS=85 | |
| elif echo "$PANE_CONTENT" | grep -q "Building the application..."; then | |
| # Within gradle, we can only guess without parsing gradle's own progress | |
| PROGRESS=40 | |
| elif echo "$PANE_CONTENT" | grep -q "Installing SDK packages..."; then | |
| PROGRESS=20 | |
| elif echo "$PANE_CONTENT" | grep -q "Starting build and sign process."; then | |
| PROGRESS=5 | |
| fi | |
| curl -d "Fortschritt: $PROGRESS% (geschätzt) - Task läuft im Hintergrund." "ntfy.sh/$TOPIC" || true | |
| sleep 60 | |
| ITERATION=$((ITERATION + 1)) | |
| done |
This PR implements a custom scroll indicator for the PhotoReasoning chat screen. It also includes the signed APK and a detailed analysis of the performance bottlenecks encountered during development (latency and shell timeouts). A new strategy using tmux for background builds has been documented in the memories.
PR created automatically by Jules for task 399616347601914444 started by @Android-PowerUser