fix(android): resolve WebViewDialog with webViewId and return result#471
Conversation
📝 WalkthroughWalkthroughThe plugin call resolution was moved from immediate return in openWebView to deferred resolution inside WebViewDialog lifecycle events (shouldOverrideUrlLoading and onPageFinished), so the ID result is provided only after relevant WebView events occur. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin (InAppBrowserPlugin)
participant Dialog as WebViewDialog
participant WebView as WebViewClient / Page
Plugin->>Dialog: create & register dialog
Plugin->>Dialog: presentWebView() -- no immediate resolve
Note over Dialog,WebView: WebView lifecycle proceeds
WebView->>Dialog: shouldOverrideUrlLoading / onPageFinished
Dialog->>Plugin: resolve({ id: instanceId })
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 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 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: 2
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)
2784-2808:⚠️ Potential issue | 🔴 CriticalCritical: Same undefined
webViewIdissue inonPageFinished.The
webViewIdvariable is not defined in this scope either. UseinstanceIdinstead to match the pattern used elsewhere in this class (e.g., line 2649).🐛 Proposed fix
public void onPageFinished(WebView view, String url) { super.onPageFinished(view, url); if (view == null || _webView == null) { return; } - JSObject result = new JSObject(); - result.put("id", webViewId); + JSObject result = new JSObject(); + result.put("id", instanceId); if (!isInitialized) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java` around lines 2784 - 2808, In onPageFinished within WebViewDialog.java the undefined variable webViewId is used when building the JSObject result and resolving the PluginCall; replace all uses of webViewId in that block with the correctly scoped identifier instanceId (same pattern used around line 2649) so JSObject result.put("id", instanceId) and the call to _options.getPluginCall().resolve(result) references the result containing instanceId; ensure no other references to webViewId remain in this method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java`:
- Around line 975-986: The JSObject result is using an undefined variable
webViewId; replace that with the class field instanceId (the
WebViewDialog.instanceId set via setInstanceId()) so the object contains the
correct identifier. Update the code that builds the JSObject in the method shown
to call result.put("id", instanceId) and keep the surrounding logic (isHidden,
getInvisibilityMode, show, applyHiddenMode, getPluginCall().resolve) unchanged
so resolution uses the existing instanceId.
- Around line 975-986: Summarize and document the Android vs iOS
promise-resolution timing difference: in WebViewDialog (methods around
show()/openWebView and the use of _options.getPluginCall().resolve(result))
Android resolves the PluginCall only after the dialog is shown or after page
load when isPresentAfterPageLoad=true, while iOS resolves immediately at the end
of openWebView regardless of isPresentAfterPageLoad; add a concise note to the
plugin API docs (and method docs for openWebView / WebViewDialog and the
isPresentAfterPageLoad option) describing this platform-specific behavior and
advising consumers to account for the different timing.
---
Outside diff comments:
In `@android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java`:
- Around line 2784-2808: In onPageFinished within WebViewDialog.java the
undefined variable webViewId is used when building the JSObject result and
resolving the PluginCall; replace all uses of webViewId in that block with the
correctly scoped identifier instanceId (same pattern used around line 2649) so
JSObject result.put("id", instanceId) and the call to
_options.getPluginCall().resolve(result) references the result containing
instanceId; ensure no other references to webViewId remain in this method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 404fce4c-a150-454c-9787-ba35b02d3811
📒 Files selected for processing (2)
android/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.javaandroid/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java (1)
2784-2808: Consider moving result creation inside the conditional block.The
JSObject resultis created on everyonPageFinishedcall, but it's only used when!isInitialized && isPresentAfterPageLoad(). SinceonPageFinishedis called on each navigation, this creates unnecessary allocations after the first page load.Suggested optimization
`@Override` public void onPageFinished(WebView view, String url) { super.onPageFinished(view, url); if (view == null || _webView == null) { return; } - JSObject result = new JSObject(); - result.put("id", instanceId); if (!isInitialized) { isInitialized = true; _webView.clearHistory(); if (_options.isPresentAfterPageLoad()) { + JSObject result = new JSObject(); + result.put("id", instanceId); boolean usePreShowScript = _options.getPreShowScript() != null && !_options.getPreShowScript().isEmpty(); if (!usePreShowScript) { show(); _options.getPluginCall().resolve(result); } else { + final JSObject finalResult = result; executorService.execute( new Runnable() { `@Override` public void run() { // ... existing code ... activity.runOnUiThread( new Runnable() { `@Override` public void run() { show(); - _options.getPluginCall().resolve(result); + _options.getPluginCall().resolve(finalResult); } } ); } } ); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java` around lines 2784 - 2808, Move the creation of JSObject result into the branch where it is actually used: inside the if (!isInitialized && _options.isPresentAfterPageLoad()) block in onPageFinished so it's not allocated on every navigation; create it immediately before any calls to _options.getPluginCall().resolve(result) and ensure result is final/effectively-final so it can be referenced from the nested Runnables used by injectPreShowScript/show.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java`:
- Around line 975-986: presentWebView() currently resolves the same PluginCall
that onPageFinished() may also resolve, causing duplicate resolution attempts;
add a guard boolean (e.g., private boolean presentResolved) to WebViewDialog and
use it to ensure the PluginCall is resolved only once: before calling
_options.getPluginCall().resolve(result) in presentWebView() and before any
resolve call in onPageFinished(), check if presentResolved is false, then call
resolve and set presentResolved = true; reference the methods presentWebView(),
onPageFinished(), the _options.getPluginCall() call site, and the
isHidden()/isPresentAfterPageLoad() checks when adding this guard.
---
Nitpick comments:
In `@android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java`:
- Around line 2784-2808: Move the creation of JSObject result into the branch
where it is actually used: inside the if (!isInitialized &&
_options.isPresentAfterPageLoad()) block in onPageFinished so it's not allocated
on every navigation; create it immediately before any calls to
_options.getPluginCall().resolve(result) and ensure result is
final/effectively-final so it can be referenced from the nested Runnables used
by injectPreShowScript/show.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9091cbe6-0376-4965-b986-8522056e3bf5
📒 Files selected for processing (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
| JSObject result = new JSObject(); | ||
| result.put("id", instanceId); | ||
| if (this._options.isHidden()) { | ||
| if (_options.getInvisibilityMode() == Options.InvisibilityMode.FAKE_VISIBLE) { | ||
| show(); | ||
| applyHiddenMode(); | ||
| } | ||
| _options.getPluginCall().resolve(); | ||
| _options.getPluginCall().resolve(result); | ||
| } else if (!this._options.isPresentAfterPageLoad()) { | ||
| show(); | ||
| _options.getPluginCall().resolve(); | ||
| _options.getPluginCall().resolve(result); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any mutual exclusion logic between isHidden and isPresentAfterPageLoad
echo "Checking Options class for isHidden/isPresentAfterPageLoad relationship:"
rg -n -A 5 "isPresentAfterPageLoad|isHidden" android/src/main/java/ee/forgr/capacitor_inappbrowser/Options.java
echo ""
echo "Checking plugin for how these options are set together:"
rg -n -B 2 -A 5 "setHidden|setPresentAfterPageLoad" android/src/main/java/ee/forgr/capacitor_inappbrowser/Repository: Cap-go/capacitor-inappbrowser
Length of output: 10943
🏁 Script executed:
#!/bin/bash
# Check the onPageFinished method to see resolution logic
echo "Checking onPageFinished method (around lines 2784-2808):"
sed -n '2780,2820p' android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
echo ""
echo "Checking for any 'resolved' or 'completed' flags:"
rg -n "resolved|completed|isResolved" android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java | head -20Repository: Cap-go/capacitor-inappbrowser
Length of output: 2457
🏁 Script executed:
#!/bin/bash
# Search for validation logic that might prevent both flags being true
echo "Checking for validation in Options class:"
sed -n '1,600p' android/src/main/java/ee/forgr/capacitor_inappbrowser/Options.java | grep -A 10 -B 5 "setHidden\|setPresentAfterPageLoad" | head -40
echo ""
echo "Checking for validation in InAppBrowserPlugin:"
rg -B 5 -A 10 "setHidden|setPresentAfterPageLoad" android/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.java | grep -A 10 -B 5 "validate\|check\|if.*hidden\|if.*present"
echo ""
echo "Checking for any assertions or guards about these options together:"
rg -n "isHidden.*isPresentAfterPageLoad|isPresentAfterPageLoad.*isHidden" android/src/main/java/ee/forgr/capacitor_inappbrowser/Repository: Cap-go/capacitor-inappbrowser
Length of output: 1077
🏁 Script executed:
#!/bin/bash
# Check if PluginCall is defined in this repo or if it's from Capacitor
echo "Searching for PluginCall definition:"
find . -name "*.java" -type f -exec grep -l "interface PluginCall\|class PluginCall" {} \;
echo ""
echo "Checking imports to see where PluginCall comes from:"
rg "import.*PluginCall" android/src/main/java/ee/forgr/capacitor_inappbrowser/ | head -5
echo ""
echo "Checking Capacitor documentation or comments about resolve:"
rg -B 3 -A 3 "getPluginCall.*resolve" android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java | head -20Repository: Cap-go/capacitor-inappbrowser
Length of output: 1237
🌐 Web query:
Capacitor PluginCall resolve() called multiple times behavior
💡 Result:
By default in Capacitor, a PluginCall/CAPPluginCall is single-shot: once you call call.resolve() or call.reject() the bridge removes its bookkeeping for that callback, so later resolve() calls won’t be delivered back to JS (and may just log/behave like a no-op). [1]
If you intentionally need to send multiple results from one native call, you must mark the call as a callback/stream by setting keepAlive (call.setKeepAlive(true) on Android / call.keepAlive = true on iOS). With keepAlive = true, resolve() can be called repeatedly and each result is sent to JS. [1]
Also, even with keepAlive, you cannot receive multiple results via a normal JS Promise (await plugin.method() / .then(...)), because a promise resolves only once. You must use a callback-style method (or plugin events like notifyListeners) to receive multiple updates. [2]
Sources: [1] Saving/Persisting Plugin Calls (Capacitor docs) (capacitorjs.jp), [2] Ionic Forum: “Help with resolving call multiple times (Call keepAlive)” (forum.ionicframework.com)
Citations:
- 1: https://capacitorjs.jp/docs/v7/core-apis/saving-calls
- 2: https://forum.ionicframework.com/t/help-with-resolving-call-multiple-times-call-keepalive/214755
🏁 Script executed:
#!/bin/bash
# Check if setKeepAlive is used on PluginCall
echo "Searching for setKeepAlive usage:"
rg -n "setKeepAlive" android/src/main/java/ee/forgr/capacitor_inappbrowser/
echo ""
echo "Also check if the PluginCall returned by getPluginCall is modified:"
rg -n "getPluginCall()" android/src/main/java/ee/forgr/capacitor_inappbrowser/ | head -10Repository: Cap-go/capacitor-inappbrowser
Length of output: 876
Fix correctly includes instanceId in the resolved result.
The implementation properly creates a JSObject with the id field set to instanceId, addressing the issue where JavaScript received undefined for the webViewId.
However, there is a logical issue: if both isHidden() and isPresentAfterPageLoad() are true, presentWebView() will resolve at line 982, and onPageFinished() will attempt to resolve again at lines 2793/2808. While Capacitor's single-shot PluginCall prevents the second resolution from reaching JavaScript, this still represents flawed logic. Either:
- Ensure
isHidden()andisPresentAfterPageLoad()are mutually exclusive through validation, or - Add a guard to prevent duplicate resolution attempts (e.g., by checking if the call is already resolved or using a completion flag).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java`
around lines 975 - 986, presentWebView() currently resolves the same PluginCall
that onPageFinished() may also resolve, causing duplicate resolution attempts;
add a guard boolean (e.g., private boolean presentResolved) to WebViewDialog and
use it to ensure the PluginCall is resolved only once: before calling
_options.getPluginCall().resolve(result) in presentWebView() and before any
resolve call in onPageFinished(), check if presentResolved is false, then call
resolve and set presentResolved = true; reference the methods presentWebView(),
onPageFinished(), the _options.getPluginCall() call site, and the
isHidden()/isPresentAfterPageLoad() checks when adding this guard.
|
close in favor of #476 |
2 participants
The
webViewIdwas not returned when theWebViewDialogresolved. If that resolve happened before the one in the plugin then we receiveundefinedin javascript.Summary by CodeRabbit