fix(inappbrowser): handle redirect links and improve download/notification flow (Android) #406
Conversation
# Conflicts: # README.md
…d-related strings
|
| Status | Scanner |  Critical |
 High |
 Medium |
 Low |
Total (0) |
|---|---|---|---|---|---|---|
| Open Source Security | 0 | 0 | 0 | 0 | See details |
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.
# Conflicts: # package-lock.json
WalkthroughAdds comprehensive native download support and lifecycle management to WebViewDialog (DownloadManager integration with HTTP fallback, notifications, broadcast receiver lifecycle, TTL-based bridge download allowances), improves file chooser/postMessage handling and WebView stability, and adds three string resources plus a package.json script. Changes
Sequence DiagramsequenceDiagram
participant WebView
participant JSBridge as JS Bridge
participant WVD as WebViewDialog
participant DM as DownloadManager
participant HTTP as HttpURLConnection
participant MS as MediaStore
participant Notif as NotificationSystem
participant Receiver as DownloadReceiver
WebView->>JSBridge: postMessage({ allowDownload })
JSBridge->>WVD: postMessage payload
WVD->>WVD: register URL in allowDownloadExpiryMap (TTL)
WebView->>WVD: navigates / requests URL
WVD->>WVD: shouldOverrideUrlLoading -> check allowDownloadExpiryMap
alt URL allowed for download
WVD->>DM: startDownloadFromUrl (preserve cookies, UA, filename)
DM->>Receiver: enqueue + system download progress
Receiver->>WVD: on completion -> handleDownloadCompletion
WVD->>MS: resolve file / MIME
WVD->>Notif: notifyDownloadSaved (open/share intent)
WVD->>JSBridge: postMessage/downloadFinished
else DownloadManager stalled/failed
WVD->>HTTP: downloadWithHttpURLConnection (fallback)
HTTP->>MS: save file (MediaStore or external path)
WVD->>Notif: notifyDownloadSaved
WVD->>JSBridge: postMessage/downloadFinished
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (1)
3108-3210: Critical:super.dismiss()is called twice, causing potential crashes.The
dismiss()method has duplicate cleanup code that will cause issues:
super.dismiss()is called at line 3196 and again at line 3207unregisterDownloadCompleteReceiver()is called at line 3109, then the receiver is unregistered again at lines 3182-3186activeDownloadFileNames.clear()is called at line 3163 and again at line 3192Apply this diff to remove the duplicate code:
activeDownloadFileNames.clear(); allowDownloadExpiryMap.clear(); // Shutdown executor service safely if (executorService != null && !executorService.isShutdown()) { try { executorService.shutdown(); if (!executorService.awaitTermination(500, TimeUnit.MILLISECONDS)) { executorService.shutdownNow(); } } catch (InterruptedException e) { Thread.currentThread().interrupt(); executorService.shutdownNow(); } catch (Exception e) { Log.e("InAppBrowser", "Error shutting down executor: " + e.getMessage()); } } - try { - if (downloadCompleteReceiver != null) { - try { - getContext().unregisterReceiver(downloadCompleteReceiver); - } catch (Exception ignore) {} - downloadCompleteReceiver = null; - } - } catch (Exception e) { - Log.w("InAppBrowser", "Error unregistering download receiver: " + e.getMessage()); - } - synchronized (activeDownloadFileNames) { - activeDownloadFileNames.clear(); - } - - try { - super.dismiss(); - } catch (Exception e) { - Log.e("InAppBrowser", "Error dismissing dialog: " + e.getMessage()); - } - // Clear any remaining proxied requests synchronized (proxiedRequestsHashmap) { proxiedRequestsHashmap.clear(); } try { super.dismiss(); } catch (Exception e) { Log.e("InAppBrowser", "Error dismissing dialog: " + e.getMessage()); } }
🧹 Nitpick comments (2)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (2)
3671-3696: Redundant receiver registration logic.The receiver registration at lines 3671-3696 duplicates the registration already performed by
registerDownloadCompleteReceiver()called at line 312 inpresentWebView(). SincedownloadCompleteReceiver != nullcheck is performed, this block will never execute under normal circumstances.Consider removing this redundant registration block or consolidating the receiver management into a single location to reduce complexity and avoid confusion.
- // register receiver once (lazy) - if (downloadCompleteReceiver == null) { - downloadCompleteReceiver = new android.content.BroadcastReceiver() { - @Override - public void onReceive(Context ctx, Intent intent) { - try { - long completedId = intent.getLongExtra(android.app.DownloadManager.EXTRA_DOWNLOAD_ID, -1L); - if (completedId == -1L) return; - handleDownloadCompletion(completedId); - } catch (Throwable ex) { - Log.e("InAppBrowser", "Download complete receiver error: " + ex.getMessage(), ex); - } - } - }; - try { - IntentFilter filter = new IntentFilter(android.app.DownloadManager.ACTION_DOWNLOAD_COMPLETE); - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - // Android 12+ requires explicit exported flag for non-system receivers - getContext().registerReceiver(downloadCompleteReceiver, filter, Context.RECEIVER_NOT_EXPORTED); - } else { - getContext().registerReceiver(downloadCompleteReceiver, filter); - } - Log.d("InAppBrowser", "Download complete receiver registered"); - } catch (Exception e) { - Log.w("InAppBrowser", "Could not register download receiver: " + e.getMessage()); - } - } + // Receiver is already registered in presentWebView() via registerDownloadCompleteReceiver()
3541-3668: Consider cancellation when dialog is dismissed during download status polling.The 10-second polling loop (5s + 5s) continues executing even if the dialog is dismissed. While the code handles exceptions gracefully, it may perform unnecessary work and log confusing messages after the user has closed the browser.
Consider checking if the dialog is still active before proceeding with fallback downloads:
executorService.execute(() -> { try { + // Check if dialog was dismissed + if (!isShowing()) { + Log.d("InAppBrowser", "Dialog dismissed, skipping download status check"); + return; + } // Primera espera Thread.sleep(5000); + if (!isShowing()) return; // ... rest of status checks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java(34 hunks)android/src/main/res/values/strings.xml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_android
🔇 Additional comments (7)
android/src/main/res/values/strings.xml (1)
13-13: LGTM!The new
select_filestring resource properly supports i18n for file chooser prompts and aligns with thegetStringResourceOrDefaultusage inWebViewDialog.java.android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (6)
125-132: LGTM!The download tracking fields are well-designed with appropriate TTL for temporary authorization and proper separation of concerns between URL authorization and download tracking.
420-429: LGTM!Cookie manager initialization with third-party cookie support is properly guarded and wrapped in try-catch for robustness.
1694-1767: LGTM!The MIME type detection helper provides a robust fallback chain: ContentResolver → extension mapping → magic bytes inspection. Resource cleanup is properly handled in finally blocks.
2496-2515: LGTM!The download authorization bridge is well-implemented with proper TTL expiry handling, synchronization, and cleanup of expired entries.
4259-4274: LGTM!Clean utility method that enables graceful i18n with fallback to English defaults when string resources are unavailable.
1480-1519: LGTM!The enhanced MIME type parsing properly handles various
acceptattribute formats (comma-separated, extensions, wildcards) and leveragesEXTRA_MIME_TYPESfor improved file picker behavior.
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
Outdated
Show resolved
Hide resolved
| String channelName = "Descargas de la app"; | ||
| NotificationManager nm = (NotificationManager) ctx.getSystemService(Context.NOTIFICATION_SERVICE); | ||
|
|
||
| // Create channel if needed (Android O+) | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| NotificationChannel ch = nm.getNotificationChannel(channelId); | ||
| if (ch == null) { | ||
| ch = new NotificationChannel(channelId, channelName, NotificationManager.IMPORTANCE_DEFAULT); | ||
| ch.setDescription("Notificaciones de descargas guardadas"); | ||
| nm.createNotificationChannel(ch); |
There was a problem hiding this comment.
Hardcoded Spanish strings inconsistent with i18n approach.
The notification channel uses hardcoded Spanish text ("Descargas de la app", "Notificaciones de descargas guardadas") while the notification content uses English. This is inconsistent with the i18n approach used elsewhere (e.g., getStringResourceOrDefault for file chooser).
Consider extracting these to string resources for proper localization:
- String channelName = "Descargas de la app";
+ String channelName = getStringResourceOrDefault("download_channel_name", "App Downloads");
// ...
- ch.setDescription("Notificaciones de descargas guardadas");
+ ch.setDescription(getStringResourceOrDefault("download_channel_description", "Download completion notifications"));And add to strings.xml:
<string name="download_channel_name">App Downloads</string>
<string name="download_channel_description">Download completion notifications</string>🤖 Prompt for AI Agents
In android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
around lines 3948 to 3957, the notification channel name and description are
hardcoded in Spanish; replace these hardcoded literals by loading localized
string resources (use the existing pattern such as
getStringResourceOrDefault(ctx, "download_channel_name", "App Downloads") or
ctx.getString(R.string.download_channel_name) with a fallback) for both the
channel name and description, and add corresponding entries to strings.xml
(download_channel_name and download_channel_description) so the channel uses
proper i18n with sensible English defaults.
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
Show resolved
Hide resolved
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
Outdated
Show resolved
Hide resolved
…ialog.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (1)
3181-3210: Duplicatesuper.dismiss()calls and receiver unregistration.The
super.dismiss()is called twice (lines 3196 and 3207), and the download receiver is unregistered both viaunregisterDownloadCompleteReceiver()at line 3109 and again inline at lines 3181-3190. This is redundant and the secondsuper.dismiss()call is unnecessary.Apply this diff to remove the duplicate code:
- try { - if (downloadCompleteReceiver != null) { - try { - getContext().unregisterReceiver(downloadCompleteReceiver); - } catch (Exception ignore) {} - downloadCompleteReceiver = null; - } - } catch (Exception e) { - Log.w("InAppBrowser", "Error unregistering download receiver: " + e.getMessage()); - } - synchronized (activeDownloadFileNames) { - activeDownloadFileNames.clear(); - } - - try { - super.dismiss(); - } catch (Exception e) { - Log.e("InAppBrowser", "Error dismissing dialog: " + e.getMessage()); - } - // Clear any remaining proxied requests synchronized (proxiedRequestsHashmap) { proxiedRequestsHashmap.clear(); }
♻️ Duplicate comments (4)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (4)
3919-3921: Race condition on_webViewaccess remains.This issue was flagged in a previous review. The
_webViewreference is accessed without synchronization and could become null between check and use ifdismiss()is called concurrently.
3948-3957: Hardcoded Spanish strings remain inconsistent with i18n approach.This issue was flagged in a previous review. The notification channel uses hardcoded Spanish text while the notification content uses English. Consider using
getStringResourceOrDefault()for consistency.
4047-4066: Dead code: Unreachable Q+ check in pre-Q branch remains.This issue was flagged in a previous review. The
Build.VERSION.SDK_INT >= Build.VERSION_CODES.Qcheck at line 4051 is inside theelsebranch that only executes for pre-Q devices.
3684-3696: API compatibility issue:RECEIVER_NOT_EXPORTEDrequires API 33, not API 31.At line 3686,
Build.VERSION_CODES.S(API 31) is used to guard theContext.RECEIVER_NOT_EXPORTEDflag, but this flag was introduced in API 33 (TIRAMISU). This will crash on API 31-32 devices. TheregisterDownloadCompleteReceiver()method at line 4177 correctly usesTIRAMISU.Apply this diff to fix the version check:
try { IntentFilter filter = new IntentFilter(android.app.DownloadManager.ACTION_DOWNLOAD_COMPLETE); - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - // Android 12+ requires explicit exported flag for non-system receivers + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + // Android 13+ requires explicit exported flag for non-system receivers getContext().registerReceiver(downloadCompleteReceiver, filter, Context.RECEIVER_NOT_EXPORTED); } else { getContext().registerReceiver(downloadCompleteReceiver, filter); }
🧹 Nitpick comments (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (1)
3671-3696: Consolidate receiver registration to use the helper method.The receiver is already registered in
presentWebView()viaregisterDownloadCompleteReceiver()(line 312). This inline registration instartDownloadFromUrl()is redundant and uses the wrong API level check (Sinstead ofTIRAMISU). Consider removing this inline block and relying solely on the helper method.Apply this diff to remove the redundant inline registration:
Log.d("InAppBrowser", "DownloadManager enqueued id=" + id + " file=" + finalFileName + " url=" + url); // Post-enqueue check: wait 5s, then check status; if still PENDING, wait another 5s; if still not running/successful -> fallback // IMPORTANT: capture UA and cookies here so the background thread doesn't access WebView methods. final String capturedUA = ua; final String capturedCookies = cookies; executorService.execute(() -> { // ... existing status check code ... }); - - // register receiver once (lazy) - if (downloadCompleteReceiver == null) { - downloadCompleteReceiver = new android.content.BroadcastReceiver() { - @Override - public void onReceive(Context ctx, Intent intent) { - try { - long completedId = intent.getLongExtra(android.app.DownloadManager.EXTRA_DOWNLOAD_ID, -1L); - if (completedId == -1L) return; - handleDownloadCompletion(completedId); - } catch (Throwable ex) { - Log.e("InAppBrowser", "Download complete receiver error: " + ex.getMessage(), ex); - } - } - }; - try { - IntentFilter filter = new IntentFilter(android.app.DownloadManager.ACTION_DOWNLOAD_COMPLETE); - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - // Android 12+ requires explicit exported flag for non-system receivers - getContext().registerReceiver(downloadCompleteReceiver, filter, Context.RECEIVER_NOT_EXPORTED); - } else { - getContext().registerReceiver(downloadCompleteReceiver, filter); - } - Log.d("InAppBrowser", "Download complete receiver registered"); - } catch (Exception e) { - Log.w("InAppBrowser", "Could not register download receiver: " + e.getMessage()); - } - } } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java(34 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_android
🔇 Additional comments (13)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (13)
125-132: LGTM! The download tracking infrastructure with TTL-based expiry map and active downloads tracking is well-designed.
176-212: LGTM! The structured message parsing forallowDownloadbridge messages with proper synchronization and fallback handling is well-implemented.
420-429: LGTM! Good defensive setup for cookie handling with appropriate exception handling.
467-488: LGTM! The DownloadListener is properly attached to capture attachment downloads with appropriate error handling.
859-956: LGTM! The popup WebView handling for external link opening is robust with proper cleanup in finally blocks and defensive handling against double-execution.
1480-1529: LGTM! The MIME type parsing handles various input formats well (comma-separated, extensions, explicit MIME types) with appropriate fallbacks.
1539-1581: LGTM! The file chooser result handling with try-catch-finally ensures proper callback cleanup even on errors.
1661-1692: LGTM! The helper correctly queriesDISPLAY_NAMEfrom ContentResolver with proper cursor cleanup and path-based fallback.
1694-1767: LGTM! The three-tier MIME detection (ContentResolver → extension → magic bytes) provides robust fallback handling.
2496-2516: LGTM! The allowDownload bridge check properly validates TTL and prevents replay by removing consumed entries.
4158-4182: LGTM! This method correctly usesBuild.VERSION_CODES.TIRAMISU(API 33) for theRECEIVER_NOT_EXPORTEDflag check, unlike the inline registration instartDownloadFromUrl.
4184-4196: LGTM! Proper cleanup with defensive handling for the already-unregistered case.
4261-4275: LGTM! Good utility for localized string lookup with fallback. Consider using this consistently innotifyDownloadSaved()for the notification channel name and description.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (1)
3107-3210: Clean up duplicated receiver unregistration, map clearing, andsuper.dismiss()calls
dismiss()currently:
- Calls
unregisterDownloadCompleteReceiver()at the top.- Later tries to unregister
downloadCompleteReceiveragain and clearsactiveDownloadFileNamesa second time.- Calls
super.dismiss()inside atryblock (around Line 3196) and then callssuper.dismiss()again in anothertry/catchat the end.This duplication is confusing and risks double dismiss semantics (e.g., duplicate
OnDismissListenercallbacks) even if the framework is tolerant.Suggested cleanup:
- Keep all receiver unregistration and
activeDownloadFileNamesclearing in one place (preferably the helper).- Remove the second manual unregister + extra
activeDownloadFileNames.clear().- Call
super.dismiss()exactly once at the end of the method inside a singletry/catch.That will make the dialog lifecycle easier to reason about and reduce surprises.
♻️ Duplicate comments (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (1)
4048-4066: Remove unreachable Q+ branch inside pre‑Q fallback pathInside
downloadWithHttpURLConnection, theelse { // Legacy: pre-Q }block still contains an innerif (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q)branch that, by construction, is never taken and only adds noise.You can simplify to the pre‑Q logic only, per the earlier suggestion, e.g.:
- } else { - // Legacy: prefer public Downloads if app has permission, otherwise use app-specific external files dir - File downloadsDir = null; - try { - if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { - // Shouldn't get here (we handle Q+ earlier) but fallback to app files dir - downloadsDir = getContext().getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS); - } else { - if ( - activity != null && - ContextCompat.checkSelfPermission(activity, Manifest.permission.WRITE_EXTERNAL_STORAGE) == - PackageManager.PERMISSION_GRANTED - ) { - downloadsDir = Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS); - } else { - // use app-specific external files directory to avoid permission requirement - downloadsDir = getContext().getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS); - Log.d("InAppBrowser", "No WRITE_EXTERNAL_STORAGE permission - saving fallback file to app external files dir"); - } - } - } catch (Exception e) { + } else { + // Legacy: prefer public Downloads if app has permission, otherwise use app-specific external files dir + File downloadsDir = null; + try { + if ( + activity != null && + ContextCompat.checkSelfPermission(activity, Manifest.permission.WRITE_EXTERNAL_STORAGE) == + PackageManager.PERMISSION_GRANTED + ) { + downloadsDir = Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS); + } else { + downloadsDir = getContext().getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS); + Log.d("InAppBrowser", "No WRITE_EXTERNAL_STORAGE permission - saving fallback file to app external files dir"); + } + } catch (Exception e) { Log.w("InAppBrowser", "Could not determine downloads dir, falling back to app files dir: " + e.getMessage()); downloadsDir = getContext().getExternalFilesDir(Environment.DIRECTORY_DOWNLOADS); }This matches the previously raised suggestion and keeps the code easier to follow.
🧹 Nitpick comments (2)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (2)
3670-3695: Deduplicate BroadcastReceiver registration for download completion
presentWebView()already callsregisterDownloadCompleteReceiver(), and you now have dedicatedregisterDownloadCompleteReceiver/unregisterDownloadCompleteReceiverhelpers. The extraif (downloadCompleteReceiver == null) { ... registerReceiver(...) }block at the end ofstartDownloadFromUrlduplicates that logic (including API‑level gating) and is effectively redundant.To reduce maintenance and the chance of future divergence, consider removing the inline registration in
startDownloadFromUrland relying solely onregisterDownloadCompleteReceiver()during dialog setup.
3941-3997: Channel strings now localized; consider localizing notification text tooUsing
getStringResourceOrDefaultfor the download notification channel name/description is a nice i18n improvement, and the helper itself is defensive and reusable.You may optionally take this a step further by moving
"Download saved","File downloaded", and"Open file"into string resources and resolving them through the same helper, so the full download UX (channel + notification content + chooser title) is consistently localizable.Also applies to: 4261-4275
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java(34 hunks)android/src/main/res/values/strings.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- android/src/main/res/values/strings.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_android
🔇 Additional comments (4)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (4)
125-133: Allow-download handshake + TTL map looks solidThe JSON-based
allowDownloadhandling inJavaScriptInterface.postMessage, the short‑livedallowDownloadExpiryMap, and the early check inshouldOverrideUrlLoadingform a clear, explicit gate before starting native downloads, and the synchronized map access avoids races on that path. No changes needed here.Also applies to: 168-212, 2497-2515
1476-1658: Robust file picker MIME handling and URI helpersNormalizing
acceptType, buildingmimeTypes+EXTRA_MIME_TYPES, and logging resolved names/MIME types (viagetFileNameFromUrianddetectMimeTypeFromUri) significantly improves compatibility and debuggability. Cursor and stream lifecycles are handled correctly, and fallbacks (*/*, path-based name) are sensible.Also applies to: 1661-1767
3529-3684: Code referenced in review does not exist in current fileThe review comment references lines 3529-3684, 3710-3939, and 3999-4156 in WebViewDialog.java, but the file contains only 3059 lines. Additionally, searches for the key methods and code patterns mentioned—
handleDownloadCompletion,downloadWithHttpURLConnection, post-enqueue download logic, andactiveDownloadFileNames—return no results in the codebase.The code snippet shown in the review (with
DownloadManagerenqueue, post-enqueue status checks, and fallback logic) does not appear in the current state of the repository. Without the actual code to review, the specific concerns about double fallback downloads and null filename issues cannot be verified.If this review is for code in a pending changeset or different branch, ensure the correct file state and line numbers are provided for verification.
129-133: This review comment references code that has been removed from the codebase. The download feature implementation (includingactiveDownloadFileNames,handleDownloadCompletion,registerDownloadCompleteReceiver, anddownloadCompleteReceiver) that existed in commit 5957880 has been completely removed in subsequent commits. The current version of WebViewDialog.java (line 3059) does not contain any of the code mentioned in this review comment. The field declarations, BroadcastReceiver setup, synchronization patterns, and all related download handling code no longer exist in the file. Therefore, no action is required on this review comment.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java(34 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_android
🔇 Additional comments (21)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (21)
176-209: LGTM! Enhanced message bridge correctly handles allowDownload payloads.The enhanced postMessage implementation correctly:
- Parses JSON payloads defensively
- Registers download permissions with TTL in a thread-safe manner (synchronized block)
- Forwards both structured and raw messages to plugin callbacks
- Includes appropriate error handling and logging
420-429: LGTM! Cookie handling properly configured for downloads.Enabling cookie acceptance and third-party cookies is essential for the download flow to preserve authentication state. The implementation correctly:
- Wraps configuration in defensive try-catch
- Checks API level before calling
setAcceptThirdPartyCookies(API 21+)- Logs any configuration failures
467-488: LGTM! DownloadListener properly attached for centralized download handling.The DownloadListener correctly:
- Captures all download parameters (URL, User-Agent, Content-Disposition, MIME type)
- Delegates to
startDownloadFromUrlfor unified handling- Includes defensive error handling to prevent crashes
- Provides good logging for debugging
1480-1520: LGTM! Enhanced MIME type handling for file picker.The implementation correctly:
- Parses various MIME type formats (comma-separated, extensions, single types)
- Uses
EXTRA_MIME_TYPESfor better Android file picker behavior- Includes defensive error handling
- Falls back gracefully to "/" if parsing fails
1539-1584: LGTM! File picker result handling with proper cleanup.The result handler correctly:
- Handles both single and multiple file selections
- Logs detected MIME types and filenames for debugging
- Uses try-catch-finally to ensure callback cleanup
- Prevents memory leaks by setting
mFilePathCallback = nullin finally block
1662-1767: LGTM! Robust MIME type and filename detection with multiple fallbacks.The helper methods provide excellent robustness:
getFileNameFromUri: Queries ContentResolver first, falls back to path parsingdetectMimeTypeFromUri: Three-tier detection (ContentResolver → extension → magic bytes)- Proper resource cleanup (cursors, streams) in finally blocks
- Magic bytes detection covers common formats (PDF, PNG, JPG, ZIP, text)
The text detection heuristic (90% printable characters) is reasonable for identifying text files.
2497-2515: LGTM! AllowDownload bridge correctly integrated into navigation flow.The implementation correctly:
- Checks
allowDownloadExpiryMapwith synchronization to prevent races- Validates TTL to prevent stale entries from triggering downloads
- Removes entry after use to prevent duplicate handling
- Returns
trueto prevent default WebView navigation- Includes defensive error handling
3378-3391: LGTM! Comprehensive error code mapping for debugging.The helper method provides complete coverage of DownloadManager error codes with a fallback for unknown codes. This is valuable for debugging download failures.
3397-3484: LGTM! Comprehensive filename extraction and sanitization.The implementation provides robust filename handling:
- RFC 5987
filename*=UTF-8''...parsing with URL decoding- Fallback to simple
filename="..."pattern- URLUtil.guessFileName as final fallback
- Thorough sanitization (path traversal chars, control chars, whitespace)
- Extension handling from MIME type if missing
- Length limits (120 for name, 10 for extension)
3506-3523: LGTM! Proper permission-aware destination handling.The destination directory selection correctly:
- Uses public Downloads on Android 10+ (relies on scoped storage)
- Checks WRITE_EXTERNAL_STORAGE permission on pre-Q devices
- Falls back to app-specific external files dir when permission not granted
- Logs the fallback for debugging
This avoids permission issues while maximizing file accessibility.
3537-3668: Innovative post-enqueue status check with fallback.The two-stage status check (5s + 5s) is a creative solution to detect stalled DownloadManager tasks:
- Waits 5s, checks if still PENDING, waits another 5s
- If still PENDING or not running/successful after 10s, triggers HttpURLConnection fallback
- Correctly captures UA and cookies before background thread to avoid WebView access violations
- Cleans up DownloadManager task before fallback
This addresses real-world issues where DownloadManager can stall indefinitely.
However, verify that Spanish comments (lines 3543, 3598) are intentional or should be translated:
- // Primera espera + // First wait- // Si sigue PENDING después de 10s, considerarlo atascado y fallback + // If still PENDING after 10s, consider it stalled and fallback
3710-3771: LGTM! Robust download completion handling with proper resource cleanup.The implementation correctly:
- Captures
_webViewreference locally to prevent race conditions (addresses past review)- Queries DownloadManager with proper cursor cleanup
- Extracts all relevant fields (status, reason, URIs, filename, media type)
- Removes download ID from tracking map
- Includes comprehensive logging for debugging
3773-3906: LGTM! Comprehensive success handling with multiple notification paths.The success path correctly:
- Derives MIME type from ContentResolver or extension
- Constructs appropriate URI (content:// or FileProvider for file://)
- Notifies user via system notification with file open intent
- Attempts reflection-based callback first (
downloadFinished), then falls back to JS events- Includes defensive error handling throughout
The reflection usage is appropriate for optional callback methods. The fallback to JS events ensures functionality even when the callback is not present.
3909-3929: LGTM! Failure handling with appropriate fallback.The failure path correctly:
- Logs the failure reason for debugging
- Constructs fallback download parameters (URL, cookies, UA)
- Captures UA from local
webViewvariable (avoiding race)- Executes fallback download on background thread
- Includes defensive error handling
3940-3996: LGTM! Notification implementation with proper i18n.The implementation correctly:
- Uses
getStringResourceOrDefaultfor localized strings (addresses past review concern about hardcoded Spanish)- Creates notification channel only on Android O+ with API check
- Handles null URI case by opening Downloads UI instead
- Includes
FLAG_IMMUTABLEfor Android 12+ PendingIntent requirements- Uses hashCode for unique notification IDs to avoid collisions
3998-4150: LGTM! Robust HttpURLConnection fallback with proper storage handling.The fallback download implementation correctly:
- Configures timeouts (15s connect, 30s read) to prevent hanging
- Uses MediaStore on Android 10+ for scoped storage compliance
- Checks WRITE_EXTERNAL_STORAGE permission on pre-Q devices
- Falls back to app-specific external files when permission not granted
- Uses 8KB buffer for efficient I/O
- Notifies via callbacks (reflection) and JS events
- Makes file visible to media scanner on legacy path
- Includes comprehensive resource cleanup in finally block
4155-4190: LGTM! Proper receiver lifecycle management with API-level checks.The receiver registration/unregistration correctly:
- Checks if already registered to prevent duplicates (line 4156-4158)
- Uses
RECEIVER_NOT_EXPORTEDonly on Android 13+ (API 33), addressing past API compatibility concern- Falls back to standard registration on older versions
- Handles
IllegalArgumentExceptionduring unregistration (already unregistered)- Nulls out the receiver reference after unregistration
4255-4269: LGTM! Useful i18n helper with safe fallback.The helper method correctly:
- Attempts to resolve string resource by name
- Falls back to provided default (English) if resource not found
- Includes defensive error handling
- Enables proper localization throughout the codebase
3108-3179: LGTM! Comprehensive cleanup with proper resource management.The dismiss method correctly:
- Unregisters download receiver early to stop receiving events
- Performs thorough WebView cleanup (stops loading, clears callbacks, removes interfaces, loads blank, destroys)
- Clears download state maps
- Shuts down executor service with timeout and forced shutdown
- Includes defensive error handling throughout
Note: The duplicate cleanup code at lines 3181-3194 should be removed (separate comment).
3397-4150: Excellent implementation of native download handling with comprehensive fallbacks.The download infrastructure is well-architected with:
Strengths:
- DownloadManager as primary path with automatic HttpURLConnection fallback
- Innovative post-enqueue status check to detect stalled downloads
- Comprehensive filename extraction (RFC 5987, Content-Disposition, URLUtil)
- Thorough MIME detection (ContentResolver, extension, magic bytes)
- Permission-aware storage (MediaStore on Q+, permission-based pre-Q)
- Proper cookie and User-Agent preservation
- User notifications with file open intents
- Multiple callback mechanisms (reflection, JS events)
- Defensive error handling throughout
Architecture:
- Clear separation of concerns (download initiation, completion handling, fallback)
- Proper lifecycle management (receiver registration/unregistration)
- Thread-safe operations with background processing
- Resource cleanup (cursors, streams, connections)
This addresses the PR objectives for download handling on Android with robust error recovery.
3532-3534: Consider synchronizing activeDownloadFileNames access.The download ID tracking at line 3532-3534 is not synchronized, but other accesses to
activeDownloadFileNamesusesynchronized (activeDownloadFileNames)(e.g., lines 3605, 3753).Apply consistent synchronization:
- synchronized (activeDownloadFileNames) { - activeDownloadFileNames.put(id, finalFileName); - } + synchronized (activeDownloadFileNames) { + activeDownloadFileNames.put(id, finalFileName); + }Note: The code already has the synchronized block - this is correct as-is. The existing implementation is fine.
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
Show resolved
Hide resolved
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
android/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.java (5)
68-69: Add defensivegetActivity()null-checks before scheduling UI work or registering for results.Across these call sites you assume
getActivity()is always non-null. In most normal flows that holds, but if a JS call races with Activity teardown/detach,getActivity()can be null and you’ll crash with an NPE before your own try/catch blocks run.Consider a small defensive pattern:
- fileChooserLauncher = getActivity() - .registerForActivityResult(...); + Activity activity = getActivity(); + if (activity == null) { + Log.e("InAppBrowser", "Activity is null during load; file chooser not registered"); + return; + } + fileChooserLauncher = activity.registerForActivityResult(...);and similarly:
- this.getActivity() - .runOnUiThread( + Activity activity = getActivity(); + if (activity == null) { + call.reject("Activity is not available"); + return; + } + activity.runOnUiThread( new Runnable() { ... } );This keeps these methods from hard-crashing in rare lifecycle races while preserving current behavior for healthy Activity states.
Also applies to: 231-249, 354-372, 675-692, 721-735, 751-769, 772-791, 795-809, 848-890, 977-995
331-372: Align JScookieStore.deletedomain handling and avoid treating partial success as hard failure.Two small points in
clearCookies:
- Domain passed to
cookieStore.delete
The generated script currently uses the full URL as thedomainoption:scriptToRun.append( String.format( "window.cookieStore.delete('%s', {name: '%s', domain: '%s'});", cookieToRemove, cookieToRemove, url ) );For
cookieStore,domainshould be a host-like value, not a full URL. You already havehostfromUri uri = Uri.parse(url); String host = uri.getHost();, so you can wire that through and fix the unuseddomainvariable at the same time:- String domain = uri.getHost(); + String domain = uri.getHost(); // or remove if unused ... - scriptToRun.append( - String.format("window.cookieStore.delete('%s', {name: '%s', domain: '%s'});", cookieToRemove, cookieToRemove, url) - ); + scriptToRun.append( + String.format( + "window.cookieStore.delete('%s', {name: '%s', domain: '%s'});", + cookieToRemove, + cookieToRemove, + host + ) + );
- Behavior when
webViewDialogis null in the UI-thread block
By the time you reach therunOnUiThreadblock, HTTP cookies have already been cleared viaCookieManager. IfwebViewDialogbecomes null between that work and the UI-thread execution, you now reject the call with"WebView is not initialized", even though the core clearing side-effect succeeded.To avoid surprising callers with a rejected promise on “partial success”, consider:
- Resolving successfully even if the JS-side
cookieStorescript cannot be run (log a warning instead), or- Returning a structured result like
{ httpCookiesCleared: true, scriptExecuted: false }.Either approach makes the API behavior clearer and more robust to timing races.
675-692: WrapopenWebViewdialog creation/presentation in try/catch and define behavior when already open.Creating and presenting
WebViewDialogon the UI thread is correct, but any runtime exception from the constructor orpresentWebView()will currently crash the host app instead of rejecting the plugin call.You’re already using a catch‑and‑reject pattern in other UI-thread methods; mirroring it here would improve robustness:
- this.getActivity() - .runOnUiThread( - new Runnable() { - @Override - public void run() { - webViewDialog = new WebViewDialog(...); - webViewDialog.activity = InAppBrowserPlugin.this.getActivity(); - webViewDialog.presentWebView(); - call.resolve(); - } - } - ); + Activity activity = getActivity(); + if (activity == null) { + call.reject("Activity is not available"); + return; + } + activity.runOnUiThread( + new Runnable() { + @Override + public void run() { + try { + if (webViewDialog != null) { + call.reject("WebView is already open"); + return; + } + webViewDialog = new WebViewDialog(...); + webViewDialog.activity = activity; + webViewDialog.presentWebView(); + call.resolve(); + } catch (Exception e) { + Log.e("InAppBrowser", "Error opening WebView: " + e.getMessage(), e); + call.reject("Failed to open WebView: " + e.getMessage()); + } + } + } + );This both guards against crashes and defines what should happen if
openWebViewis called while a dialog is already present.
231-249: Unify exception-handling strategy for UI-thread operations to avoid unexpected crashes.There’s a mix of styles across the UI-thread blocks:
setUrl,clearCookies,executeScript,close, andupdateDimensionscatch exceptions inside therunOnUiThreadblock, log them, and reject thePluginCall.postMessage,goBack,reload, and (currently)openWebViewdo not catch exceptions inside the UI block, so any thrown runtime error will crash the app process.For plugin consumers this means some operations fail as promise rejections while others can terminate the app. It would be cleaner to pick one policy and apply it consistently—either:
- Let all exceptions bubble (no try/catch anywhere), or
- Always catch/log and
call.reject(...)for unexpected failures.Given the direction of this PR, aligning all of these methods with the “catch + reject” pattern seems preferable so JS callers always see a rejected promise instead of a crash when something goes wrong in WebView/Dialog code.
Also applies to: 721-735, 772-791, 795-809, 977-995
848-890: VerifycloseEventis not emitted twice and consider de-duplicating the fallback logic.The new
close()implementation explicitly emits acloseEventbefore dismissing the dialog:notifyListeners("closeEvent", new JSObject().put("url", currentUrl)); webViewDialog.dismiss(); webViewDialog = null;Depending on how
WebViewDialogis implemented, it may already invokeWebViewCallbacks.closeEvent(url)on dismiss, which in turn callsnotifyListeners("closeEvent", ...)via the callbacks you attach inopenWebView. If that’s the case, this change could cause duplicatecloseEventemissions for a singleclose()invocation.Please double-check
WebViewDialog’s close/dismiss behavior and either:
- Remove this explicit
notifyListenersif the dialog already does it, or- Add a simple guard/flag so the event is only fired once per close.
Also, the “secondary fallback” branch inside the UI-thread block duplicates the top-of-method fallback that starts the main activity with
FLAG_ACTIVITY_CLEAR_TOP. Extracting that into a small helper (e.g.,bringMainActivityToForeground(call)) would DRY things up and keep error handling in one place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.java(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_android
🔇 Additional comments (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.java (1)
977-995: Dimension updates on the UI thread with guarded error handling look good.The pattern of rejecting early when
webViewDialogis null, then performingupdateDimensions(width, height, x, y)on the UI thread with a try/catch and clear error logging provides a predictable, crash‑resistant behavior for callers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (3)
1475-1530: File chooser and MIME/filename helpers are robust; consider log hygiene.The acceptType normalization,
EXTRA_MIME_TYPEShandling, and the newgetFileNameFromUri/detectMimeTypeFromUrihelpers look correct and defensive (cursors/streams closed properly, safe fallbacks). You now log selected URIs, file names, and detected MIME types; if these flows may involve sensitive filenames or URIs, consider reducing these to debug-only logs or redacting details before shipping to production.Also applies to: 1539-1658, 1661-1767
3707-3935: Download completion + notification/i18n path looks good; a couple of minor polish ideas.
handleDownloadCompletionanddownloadWithHttpURLConnectioncorrectly derive filenames, determine MIME types, and route success via both native notifications anddownloadFinishedcallbacks/JS events, with good error logging and fallbacks.notifyDownloadSavednow usesdownload_channel_name/download_channel_descriptionviagetStringResourceOrDefault, which aligns with the i18n approach.Two small polish suggestions:
- Some user‑visible strings are still hardcoded English (
"Downloading file...","Download saved","File downloaded","Open file"). Consider moving them to string resources as well so the download UI is fully localizable.- If you expect high download volume, you might eventually want to tune the notification ID generation (currently based on filename hash) to avoid collisions for repeated same‑name downloads, though this is a minor edge case.
Also applies to: 3957-3993, 3995-4147, 4252-4266
860-963: PopuponCreateWindowexternal-open flow is defensive; minor lifecycle nit.The temporary
popupWebViewwith its ownWebViewClientto capture the target URL and then open it externally is a nice improvement over blind intent firing and includes solid error handling/cleanup. In the rareClassCastExceptionpath whereresultMsg.objis not aWebViewTransport, you returnfalsebut never explicitly destroy the unusedpopupWebView; it will be GC’d eventually, but you could eagerlypopupWebView.destroy()in that catch block to keep lifecycle symmetrical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java(19 hunks)package.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_android
🔇 Additional comments (4)
package.json (1)
41-41: Tooling updates look good; just confirm intended TypeScript version.The added
prettier:fixscript and prettier/prettier-plugin-java bumps fit the existing tooling setup. The TypeScript devDependency is pinned to^5.1.6; please confirm this matches the TS features used across the repo and you don’t rely on newer 5.x behavior.Also applies to: 62-63, 67-67
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (3)
125-133: Allow-download bridge and TTL logic look solid.The JSON-based
postMessageparsing plusallowDownloadExpiryMapTTL check inshouldOverrideUrlLoadingcleanly tie the JS bridge to native downloads, and the 5s TTL plus cleanup on dismiss() should avoid long-lived entries. No functional issues spotted here.Also applies to: 168-212, 2490-2516
420-429: Download listener + DownloadManager pipeline are generally well-structured.Enabling cookies/third‑party cookies, wiring the
DownloadListenertostartDownloadFromUrl, and using DownloadManager with a bounded post‑enqueue status check and HTTP fallback form a robust download path. The filename derivation and destination handling for Q+ vs pre‑Q look correct and permissions-aware.Also applies to: 466-488, 3394-3705
129-133: This review comment references code that does not exist in the current codebase.The review mentions
activeDownloadFileNames,downloadCompleteReceiver,registerDownloadCompleteReceiver(),unregisterDownloadCompleteReceiver(), andstartDownloadFromUrl()methods. However, none of these identifiers exist anywhere in the WebViewDialog.java file. Additionally, the line ranges cited in "also applies to" (3107–3208, 3394–3705, 3707–3935, 4152–4186) exceed the actual file length of 3059 lines.The actual code at lines 129–133 contains different fields (
tempCameraUri,mUploadMessage,mFilePathCallback) unrelated to download tracking. TheexecutorServicedoes exist in the file (line 117) but not at the location shown in the snippet.This review cannot be verified against the current repository state and should be reconsidered or clarified against the correct codebase version.
Likely an incorrect or invalid review comment.
|
What do I need to do to complete this PR? |
package.json
Outdated
| "eslint": "^8.57.0", | ||
| "eslint-plugin-import": "^2.31.0", | ||
| "husky": "^9.1.7", | ||
| "prettier": "^3.4.2", |
There was a problem hiding this comment.
Can you explain why this changes ?
There was a problem hiding this comment.
Yeah because I saw that prettier versions are differentes from github CI that package.json and in local when i check the project with prettier have different warns. I changed to have the same versions in local than in github with bun. Think is neccesary.
There was a problem hiding this comment.
we should not have lock i removed it
There was a problem hiding this comment.
we use bun not npm please remove this file :)
There was a problem hiding this comment.
Okay! Going to do that right now
There was a problem hiding this comment.
I saw that package.lock was already in git. Only changed versions related with prettier.
| return image; | ||
| } | ||
|
|
||
| private String downloadReasonToString(int reason) { |
There was a problem hiding this comment.
i think the whole download manager should have it's own file, also what about ios does it work properly already?
There was a problem hiding this comment.
We can plan how to do it better okay. I have more changes prepared to fix this on ios. My teammate worked on it. We planned to push in a separate PR. Do you want to check both at the same PR?
There was a problem hiding this comment.
let's make PR atomic please otherwise it will be a mess to review :)
|
@sgomdie hey Sergio thanks a lot for the PR i added few comments i think we are close to merge it :) |
|
Hello! Added some comments too. Lets have a talk and we can try to close this PR :) |
|
we need to fix conflict as well |
# Conflicts: # package-lock.json # package.json
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
41-41: Consider documenting or consolidating theprettier:fixandfmtscripts.The new
prettier:fixscript duplicates the prettier formatting that's already embedded in thefmtscript (line 38). While having a dedicated script is convenient, it creates maintenance risk and potential confusion about which script to use in different contexts.Consider either:
- Documenting which script to use for which scenario, or
- Refactoring to avoid duplication (e.g.,
fmtcould callprettier:fixinstead of duplicating the command).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_android
|
I don't believe the merge conflict has been done properly because right now the build is failing. Please fix the build, and then I can merge. Thanks a lot for the work. |
2 participants
Fixes handling of 3xx redirects in InAppBrowser on Android and routes download-triggering redirects to a DownloadHandler that uses DownloadManager with fallbacks. Adds JS events:
downloadstart,downloadprogress,downloadcomplete,downloaderror.Files to review: JS wrapper (
www/inappbrowser.jsorsrc/index.ts), Android sources (InAppBrowserClient / DownloadHandler),AndroidManifest.xml.iOS implementation is NOT included in this PR and will be submitted in a follow-up PR.
Quick test steps
onLoadStart/onLoadStopor equivalent).Content-Disposition: attachment→ verifydownloadstart,downloadprogress,downloadcomplete(file saved) anddownloaderroron failure.Edge cases to test:
CI / Permissions notes
npm ci,npx cap syncand./gradlew assembleDebugin build steps.CHANGELOG entry (suggested)
Checklist
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.