fix: review round 2 — custom schemes, error UX, file chooser, docs#2
Merged
fix: review round 2 — custom schemes, error UX, file chooser, docs#2
Conversation
- Handle mailto:, tel:, geo: and other non-HTTP schemes in shouldOverrideUrlLoading by dispatching to system handler instead of returning false (which let WebView try to load them) - Differentiate error types (offline/SSL/page) with distinct error screens instead of funneling everything through "No Connection" - Use FileChooserParams.createIntent() for proper multi-MIME support instead of manually taking only the first acceptType - Return null (not emptyArray) on file chooser cancellation per WebView contract - Fix README signing instructions to match actual PUMPERLY_* property names and document VERSION_NAME/VERSION_CODE overrides - Narrow .gitguardian.yml to only app/build.gradle.kts
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdjusted CI/docs ignore and release signing docs; introduced WebViewError enum and generalized error UI/flows; updated file-chooser to use Intents; tightened in-app URL handling; changed backup policy in AndroidManifest; moved keystore path in release workflow. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/pumperly/app/MainActivity.kt`:
- Around line 338-358: The SSL error button is labeled "Go Back" but currently
invokes retryLoading(), which will reattempt the same URL and hit the same SSL
error; update the error handling so when WebViewError.SSL is shown (in
showErrorPage) the errorButton click invokes a new SSL-specific action instead
of retryLoading(): modify either showErrorPage to attach a different onClick
handler for errorButton when type == WebViewError.SSL (e.g., call a new method
like handleSslError() or call webView.goBack()/loadUrl(BASE_URL)), and update
retryLoading() only for PAGE/OFFLINE cases so SSL uses the back-or-base
behavior; reference showErrorPage, WebViewError.SSL, errorButton, retryLoading,
and consider adding handleSslError (or reuse webView.goBack/loadUrl(BASE_URL)).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ddcde53-f325-4aa5-9aaf-373311041f05
📒 Files selected for processing (4)
.gitguardian.ymlREADME.mdapp/src/main/java/com/pumperly/app/MainActivity.ktapp/src/main/java/com/pumperly/app/PumperlyWebView.kt
- Hide error overlay in onPageStarted so back-navigation to a working page dismisses it (was only clearing the flag, not the UI) - SSL error button now goes back in history instead of retrying the same failing URL (matches "Go Back" label) - README: use -P flags / env vars instead of local.properties (which findProperty doesn't auto-load for arbitrary keys) - Remove broken banner.svg reference from README
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/src/main/java/com/pumperly/app/MainActivity.kt (1)
383-389: Consider feedback when retry fails due to no network.If
isNetworkAvailable()returns false, the function silently does nothing. The user taps "Retry" and the screen doesn't change. A toast or keeping the same error visible (current behavior) is acceptable, but explicit feedback ("Still offline") would be clearer.Optional: add user feedback on retry failure
private fun retryLoading() { if (isNetworkAvailable()) { hideErrorPage() val currentUrl = webView.url ?: BASE_URL webView.loadUrl(currentUrl) + } else { + // Optionally show a toast or re-display offline error + showErrorPage(WebViewError.OFFLINE) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/pumperly/app/MainActivity.kt` around lines 383 - 389, The retryLoading function currently does nothing when isNetworkAvailable() is false; update retryLoading (in MainActivity::retryLoading) to provide explicit user feedback when offline—e.g., keep the error page visible and show a Toast or Snackbar with a short message like "Still offline" (or similar) instead of silently returning; keep the existing hideErrorPage(), BASE_URL fallback and webView.loadUrl(currentUrl) behavior for the online path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/pumperly/app/MainActivity.kt`:
- Around line 383-389: The retryLoading function currently does nothing when
isNetworkAvailable() is false; update retryLoading (in
MainActivity::retryLoading) to provide explicit user feedback when offline—e.g.,
keep the error page visible and show a Toast or Snackbar with a short message
like "Still offline" (or similar) instead of silently returning; keep the
existing hideErrorPage(), BASE_URL fallback and webView.loadUrl(currentUrl)
behavior for the online path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10adca72-2b18-474f-86f3-81cec492648b
📒 Files selected for processing (2)
README.mdapp/src/main/java/com/pumperly/app/MainActivity.kt
✅ Files skipped from review due to trivial changes (1)
- README.md
- Store pending URL when deep link arrives while offline; retryLoading and swipe-refresh now load the intended URL instead of BASE_URL - Disable android:allowBackup to prevent WebView cookies/storage from leaking into device backup/restore - Wrap file chooser createIntent() in try/catch so devices without a picker return null to WebView instead of crashing
Gradle file() in app module resolves relative to app/, so PUMPERLY_KEYSTORE_PATH must be 'release.keystore' not 'app/release.keystore' (which resolved to app/app/release.keystore).
The ActivityNotFoundException can also happen at launch() time, not just createIntent(). Wrap the launcher call so devices without a file picker return null to WebView cleanly.
There was a problem hiding this comment.
Actionable comments posted: 4
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/pumperly/app/PumperlyWebView.kt (1)
69-80:⚠️ Potential issue | 🟠 MajorAdd
onReceivedHttpError()override for 4xx/5xx main-frame responses.The current
onReceivedError()only catches network/connection failures, not HTTP error responses (404, 500, etc.). These require a separateonReceivedHttpError()override, so server errors won't trigger the native error UX.Proposed implementation
+import android.webkit.WebResourceResponse + override fun onReceivedError( view: WebView, request: WebResourceRequest, error: WebResourceError ) { @@ onError(type, request.url?.toString()) } } + + override fun onReceivedHttpError( + view: WebView, + request: WebResourceRequest, + errorResponse: WebResourceResponse + ) { + super.onReceivedHttpError(view, request, errorResponse) + if (request.isForMainFrame && errorResponse.statusCode >= 400) { + onError(WebViewError.PAGE, request.url?.toString()) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/pumperly/app/PumperlyWebView.kt` around lines 69 - 80, Add an override for onReceivedHttpError in PumperlyWebView (in addition to onReceivedError) to catch 4xx/5xx main-frame HTTP responses: implement onReceivedHttpError(view: WebView, request: WebResourceRequest, errorResponse: WebResourceResponse), call super.onReceivedHttpError(...), check request.isForMainFrame and if errorResponse.statusCode is in 400..599 then call onError(WebViewError.PAGE, request.url?.toString()) so server error responses trigger the native error UX.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Line 44: The PUMPERLY_KEYSTORE_PATH environment variable currently points to
release.keystore while the workflow decodes the keystore to
app/release.keystore, causing a path mismatch; fix by making them consistent:
either change the env var PUMPERLY_KEYSTORE_PATH to app/release.keystore where
it’s set, or change the decode step so the artifact is written to
release.keystore at the repository root (aligning with Gradle's file(...)
resolution). Also update the same env var occurrence referenced later (line 52
in the workflow) to match whichever option you choose.
In `@app/src/main/java/com/pumperly/app/MainActivity.kt`:
- Around line 346-370: showErrorPage() currently updates UI but doesn't update
the currentError state, causing mismatch with handleErrorAction() and the
errorButton label; update showErrorPage(type: WebViewError) to set currentError
= type at the start (or before returning) and update hideErrorPage() to clear
currentError (e.g., set to null) so state and visible UI (errorView,
errorButton) remain in sync; ensure any callers that directly call
showErrorPage() rely on this state change and that handleErrorAction() reads
currentError to decide behavior.
- Around line 340-343: Wrap the call to
fileChooserLauncher.launch(chooserIntent) in a try/catch for
ActivityNotFoundException so that if launch throws you immediately clear
fileUploadCallback (call fileUploadCallback?.onReceiveValue(null) and set
fileUploadCallback = null) and optionally notify the WebView via
filePathCallback?.onReceiveValue(null) before rethrowing or logging; update the
block around fileUploadCallback, filePathCallback and fileChooserLauncher.launch
to ensure no stale callback remains set on failure.
- Around line 253-255: The onError callback currently ignores the failing URL;
update the handler to save that URL into pendingUrl before setting currentError
and calling showErrorPage so retry logic uses the exact failed URL (from
PumperlyWebViewClient) instead of falling back to webView.url. Locate the
onError lambda (where currentError and showErrorPage are set) and assign
pendingUrl = failedUrl (use the URL parameter passed into onError), ensuring
retry code that reads pendingUrl will retry the correct address.
---
Outside diff comments:
In `@app/src/main/java/com/pumperly/app/PumperlyWebView.kt`:
- Around line 69-80: Add an override for onReceivedHttpError in PumperlyWebView
(in addition to onReceivedError) to catch 4xx/5xx main-frame HTTP responses:
implement onReceivedHttpError(view: WebView, request: WebResourceRequest,
errorResponse: WebResourceResponse), call super.onReceivedHttpError(...), check
request.isForMainFrame and if errorResponse.statusCode is in 400..599 then call
onError(WebViewError.PAGE, request.url?.toString()) so server error responses
trigger the native error UX.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f660df8c-7730-4728-b9bf-f1704d0b60e0
📒 Files selected for processing (4)
.github/workflows/release.ymlapp/src/main/AndroidManifest.xmlapp/src/main/java/com/pumperly/app/MainActivity.ktapp/src/main/java/com/pumperly/app/PumperlyWebView.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/AndroidManifest.xml
| run: ./gradlew bundleRelease -PVERSION_NAME=${{ steps.version.outputs.name }} -PVERSION_CODE=${{ steps.version.outputs.code }} | ||
| env: | ||
| PUMPERLY_KEYSTORE_PATH: app/release.keystore | ||
| PUMPERLY_KEYSTORE_PATH: release.keystore |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how file() is called in the signing config and whether any project-specific configuration affects path resolution
rg -n "storeFile" app/build.gradle.kts
rg -n "file\(" app/build.gradle.kts
# Check if there's any rootProject reference that might affect resolution
rg -n "rootProject" app/build.gradle.kts settings.gradle.ktsRepository: GeiserX/Pumperly-android
Length of output: 275
🏁 Script executed:
# Check the keystorePath definition and surrounding context in app/build.gradle.kts
cat -n app/build.gradle.kts | sed -n '1,50p'Repository: GeiserX/Pumperly-android
Length of output: 2109
🏁 Script executed:
# Also check if keystorePath is reading from the environment variable
rg -n "keystorePath|PUMPERLY_KEYSTORE_PATH" app/build.gradle.ktsRepository: GeiserX/Pumperly-android
Length of output: 374
Keystore path mismatch: file is decoded to app/release.keystore but env var is release.keystore.
Gradle's file() function (line 33) resolves paths relative to the project root. Setting PUMPERLY_KEYSTORE_PATH: release.keystore means Gradle will look for {root}/release.keystore, but the workflow decodes the keystore to app/release.keystore. This mismatch will fail the build.
Fix either:
- Change env var to
app/release.keystore, OR - Decode keystore to repository root instead of
app/directory
Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml at line 44, The PUMPERLY_KEYSTORE_PATH
environment variable currently points to release.keystore while the workflow
decodes the keystore to app/release.keystore, causing a path mismatch; fix by
making them consistent: either change the env var PUMPERLY_KEYSTORE_PATH to
app/release.keystore where it’s set, or change the decode step so the artifact
is written to release.keystore at the repository root (aligning with Gradle's
file(...) resolution). Also update the same env var occurrence referenced later
(line 52 in the workflow) to match whichever option you choose.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/src/main/java/com/pumperly/app/MainActivity.kt (2)
253-256:⚠️ Potential issue | 🟡 MinorStore the failing URL for correct retry behavior.
The
failingUrlparameter is discarded. When retry is triggered,retryLoading()falls back towebView.url ?: BASE_URL, which may not be the URL that actually failed.Proposed fix
- onError = { type, _ -> + onError = { type, failingUrl -> + if (!failingUrl.isNullOrBlank()) { + pendingUrl = failingUrl + } currentError = type showErrorPage(type) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/pumperly/app/MainActivity.kt` around lines 253 - 256, The onError handler is ignoring the failingUrl parameter so retryLoading() can't retry the actual failed page; update the onError lambda (the onError = { type, _ -> ... } block) to capture and store the failingUrl into a field (e.g., failedUrl or lastFailingUrl), set currentError as before and call showErrorPage(type), and then change retryLoading() to prefer that stored failingUrl (falling back to webView.url or BASE_URL only if the stored failingUrl is null/empty) so retries target the real failing URL.
132-134:⚠️ Potential issue | 🟡 MinorSet
currentErrorbefore callingshowErrorPage.
showErrorPage(WebViewError.OFFLINE)is called without updatingcurrentError. IfcurrentErrorholds a stale value from a previous error,handleErrorAction()will execute the wrong branch.Same issue at lines 289 and 438.
Proposed fix: Set currentError inside showErrorPage
private fun showErrorPage(type: WebViewError) { + currentError = type when (type) {And clear it in
hideErrorPage:private fun hideErrorPage() { + currentError = null errorView.visibility = View.GONE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/pumperly/app/MainActivity.kt` around lines 132 - 134, The bug is that showErrorPage(WebViewError.OFFLINE) is invoked without updating currentError, so handleErrorAction() may act on a stale value; update currentError to the intended WebViewError before calling showErrorPage (or better: modify showErrorPage(error: WebViewError) to assign currentError = error at the start), and ensure hideErrorPage clears currentError (set currentError = null) so subsequent logic uses the correct error state; apply this change to all occurrences (the blocks that set pendingUrl/isSplashReady then call showErrorPage).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/com/pumperly/app/MainActivity.kt`:
- Around line 253-256: The onError handler is ignoring the failingUrl parameter
so retryLoading() can't retry the actual failed page; update the onError lambda
(the onError = { type, _ -> ... } block) to capture and store the failingUrl
into a field (e.g., failedUrl or lastFailingUrl), set currentError as before and
call showErrorPage(type), and then change retryLoading() to prefer that stored
failingUrl (falling back to webView.url or BASE_URL only if the stored
failingUrl is null/empty) so retries target the real failing URL.
- Around line 132-134: The bug is that showErrorPage(WebViewError.OFFLINE) is
invoked without updating currentError, so handleErrorAction() may act on a stale
value; update currentError to the intended WebViewError before calling
showErrorPage (or better: modify showErrorPage(error: WebViewError) to assign
currentError = error at the start), and ensure hideErrorPage clears currentError
(set currentError = null) so subsequent logic uses the correct error state;
apply this change to all occurrences (the blocks that set
pendingUrl/isSplashReady then call showErrorPage).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c79d8ba-9fbe-484b-8090-360e311c3998
📒 Files selected for processing (1)
app/src/main/java/com/pumperly/app/MainActivity.kt
- Add onReceivedHttpError to show PAGE error on server 4xx/5xx - Move currentError state into showErrorPage/hideErrorPage so all callers stay in sync automatically - Store failing URL from WebViewClient callback in pendingUrl so retry reloads the exact page that failed
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
1 participant
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.
Summary
Fixes all findings from security/UX review round 2:
mailto:,tel:,geo:and other non-HTTP schemes now dispatch to the system handler instead of being swallowed by WebView (null host →return falsewas the bug)PUMPERLY_*Gradle property names, plus documentsVERSION_NAME/VERSION_CODEoverridesFileChooserParams.createIntent()for proper multi-MIME support; returnsnullon cancel per WebView contract.gitguardian.ymlnarrowed from**/*.gradle.ktsto onlyapp/build.gradle.ktsTest plan
mailto:link on pumperly.com → should open email client, not show WebView errorSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores