fix(android): return openWebView id exactly once#476
Conversation
📝 WalkthroughWalkthroughDeferred the Android Changes
Sequence Diagram(s)sequenceDiagram
participant JS as Client (JS)
participant Plugin as InAppBrowserPlugin
participant Dialog as WebViewDialog
participant WebView as NativeWebView
JS->>Plugin: openWebView()
Plugin->>Dialog: create & show (no immediate resolve)
Dialog->>WebView: create native webview
alt iOS
WebView-->>Dialog: created
Dialog->>Plugin: resolveOpenWebViewIfNeeded({id})
Plugin-->>JS: promise resolved (id)
else Android (hidden/immediate)
WebView-->>Dialog: ready for control
Dialog->>Plugin: resolveOpenWebViewIfNeeded({id})
Plugin-->>JS: promise resolved (id)
else Android (isPresentAfterPageLoad = true)
WebView->>WebView: load first page
WebView-->>Dialog: onPageFinished
Dialog->>Plugin: resolveOpenWebViewIfNeeded({id})
Plugin-->>JS: promise resolved (id)
end
alt Error / external URL handler missing
Dialog->>Plugin: rejectOpenWebViewIfNeeded(error)
Plugin-->>JS: promise rejected (error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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)
📝 Coding Plan
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85a54eaf64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| boolean usePreShowScript = _options.getPreShowScript() != null && !_options.getPreShowScript().isEmpty(); | ||
| if (!usePreShowScript) { | ||
| show(); | ||
| _options.getPluginCall().resolve(); | ||
| resolveOpenWebViewIfNeeded(); |
There was a problem hiding this comment.
With isPresentAfterPageLoad=true, this change makes openWebView() resolve only from onPageFinished, so any first-load failure path that does not reach that callback leaves the promise pending forever. In the same WebViewClient, failure branches like onReceivedError(...) and the custom-scheme ActivityNotFoundException path only emit pageLoadError and never resolve/reject the original plugin call, which means callers never get the { id } needed to recover (close/retry) when initial navigation fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in d26c6c7. The deferred-load path now rejects the original openWebView() call on initial main-frame failures before first initialization, specifically for custom-scheme ActivityNotFoundException handling and onReceivedError(...). That preserves the single-return contract while avoiding a hung promise when isPresentAfterPageLoad=true.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java`:
- Around line 170-177: In resolveOpenWebViewIfNeeded, don't silently return when
the plugin call is missing or resolve with an empty id: ensure _options != null
and obtain PluginCall call = _options.getPluginCall(); if call is null, set
openWebViewResolved = true and call.reject(...) (or otherwise send an explicit
error) so the promise won't hang; likewise validate instanceId is
non-null/non-empty before constructing the JSObject and if invalid reject the
call with a clear error instead of resolving; only mark openWebViewResolved
after a definitive resolve or reject to preserve the “exactly once” contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c4566d8-acce-4062-a8bb-996d8d8f7eff
📒 Files selected for processing (4)
README.mdandroid/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.javaandroid/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.javasrc/definitions.ts
💤 Files with no reviewable changes (1)
- android/src/main/java/ee/forgr/capacitor_inappbrowser/InAppBrowserPlugin.java
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
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)
2464-2468:⚠️ Potential issue | 🟠 MajorHandle
preventDeeplinkblocked initial navigation by rejecting once.When
preventDeeplink=trueand the initial URL is non-http(s), this path returns early without reject. IfisPresentAfterPageLoad=true,openWebView()can remain unresolved.💡 Proposed fix
if (_options.getPreventDeeplink()) { Log.d("InAppBrowser", "preventDeeplink is true"); if (isNotHttpOrHttps) { + if (request.isForMainFrame() && !isInitialized) { + if (_options.getCallbacks() != null) { + _options.getCallbacks().pageLoadError(); + } + rejectOpenWebViewIfNeeded("Blocked non-http(s) URL because preventDeeplink=true: " + url); + } return true; } }Also applies to: 2508-2508
🤖 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 2464 - 2468, When preventing deeplinks in WebViewDialog (check of _options.getPreventDeeplink() and isNotHttpOrHttps), the code currently returns early without rejecting the pending openWebView promise; update the branch to explicitly reject the promise returned by openWebView() (or call the stored reject handler for that promise) with a clear error message when blocking the initial navigation, and guard against double-rejection (only reject if the promise hasn’t been settled). Apply the same fix to the equivalent block referenced around the other occurrence (the second branch at ~2508).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java`:
- Around line 2464-2468: When preventing deeplinks in WebViewDialog (check of
_options.getPreventDeeplink() and isNotHttpOrHttps), the code currently returns
early without rejecting the pending openWebView promise; update the branch to
explicitly reject the promise returned by openWebView() (or call the stored
reject handler for that promise) with a clear error message when blocking the
initial navigation, and guard against double-rejection (only reject if the
promise hasn’t been settled). Apply the same fix to the equivalent block
referenced around the other occurrence (the second branch at ~2508).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6fa3af8-8346-4c8f-bb76-29bb02e5de3d
📒 Files selected for processing (1)
android/src/main/java/ee/forgr/capacitor_inappbrowser/WebViewDialog.java
1 participant
What
openWebView()promise resolution throughWebViewDialogso every success path returns{ id: instanceId }presentWebView()andonPageFinished()isPresentAfterPageLoadpromise timing in the generated API docsWhy
How
call.resolve({ id })fromInAppBrowserPlugin.openWebView()resolveOpenWebViewIfNeeded()inWebViewDialogto build the{ id: instanceId }payload once and reuse it from both immediate-present and page-finish flowsopenWebView/isPresentAfterPageLoad, then regeneratedREADME.mdwith docgenTesting
bunx eslint . --fixbunx prettier-pretty-check "**/*.{css,html,ts,js,java}" --plugin=prettier-plugin-java --writebunx node-swiftlint --fix --formatbun run buildbun run verify:iosbun run verify:androidNot Tested
isPresentAfterPageLoad=truewebview flows on AndroidSummary by CodeRabbit
Bug Fixes
Documentation