chore: sync upstream PR #8383 - fix(android): allow synchronous replies on the modern bridge - #93
Conversation
Store the JavaScript reply proxy before dispatching a modern bridge message so a plugin that resolves synchronously can reply through the modern bridge instead of falling back to WebView JavaScript execution. Add a regression test for the first modern-bridge call and a minimal TextUtils host-test stub so the real postMessage path can run in unit tests.
Beta npm buildMaintainers can publish one Capacitor Plus workspace package from this PR to npm for fast testing. Comment Examples: /publish-beta core
/publish-beta cli
/publish-beta @capacitor-plus/coreIf exactly one workspace package changed, Packages:
The workflow will:
Security note: beta publish is only enabled for branches inside this repository. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: low. Cursor Bugbot was not present on this PR, so that signal was skipped. The change is a narrow Android modern-bridge fix with unit test coverage and no automated findings requiring human review. No reviewers assigned.
Sent by Cursor Approval Agent: Pull Request Approver
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="android/capacitor/src/test/java/android/text/TextUtils.java">
<violation number="1" location="android/capacitor/src/test/java/android/text/TextUtils.java:3">
P3: This test source defines a package-level stub `android.text.TextUtils` that shadows the real framework class for the entire `capacitor` unit-test source set, reimplementing only the `join(CharSequence, Object[])` overload. It exists only to satisfy `Logger.tags(...)`'s `TextUtils.join` call without Robolectric. Prefer a scoped approach (mocking the dependency or using Robolectric with the real class) so unrelated tests aren't affected by a partial, framework-named stub.</violation>
<violation number="2" location="android/capacitor/src/test/java/android/text/TextUtils.java:5">
P2: This stub only recreates `join(CharSequence, Object[])`, but production code calls the other real Android overload `join(CharSequence, Iterable)` for collections: `JSExport.java:36` (and `:168`) pass a `List<String>`, and `HttpRequestHandler.java:295` also passes a `List<String>`. A unit test that reaches any of these paths resolves `TextUtils` to this stub and throws `NoSuchMethodError` at runtime, because the `Iterable` overload is missing here. Add an `Iterable` overload so the stub mirrors the API the code under test actually uses.</violation>
</file>
<file name="android/capacitor/src/test/java/com/getcapacitor/MessageHandlerTest.java">
<violation number="1" location="android/capacitor/src/test/java/com/getcapacitor/MessageHandlerTest.java:71">
P3: The test bypasses the constructor via the internal `sun.reflect.ReflectionFactory` API, which is not a stable JDK API and is subject to encapsulation/removal across JDK versions. Failure shows up as an opaque `AssertionError` and, being outside `ReflectiveOperationException`, an `InaccessibleObjectException` would escape uncaught. Prefer a supported mechanism (e.g. Mockito's constructor mocking, or testing via the real constructor with mocks) over deep reflection into `sun.reflect`.</violation>
</file>
<file name="android/capacitor/src/main/java/com/getcapacitor/MessageHandler.java">
<violation number="1" location="android/capacitor/src/main/java/com/getcapacitor/MessageHandler.java:45">
P2: Synchronous plugin replies now call `replyProxy.postMessage()` from the background 'CapacitorPlugins' handler thread, whereas before this change they went through `webView.post(...)`, which marshals to the WebView main thread. `JavaScriptReplyProxy` is `@UiThread` ("UI thread not currently enforced, but required"), so posting from a background thread is not guaranteed to deliver reliably. Marshal the reply to the main thread (e.g. via `bridge.executeOnMainThread(...)`) before calling `replyProxy.postMessage()`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| public class TextUtils { | ||
|
|
||
| public static String join(CharSequence delimiter, Object[] tokens) { |
There was a problem hiding this comment.
P2: This stub only recreates join(CharSequence, Object[]), but production code calls the other real Android overload join(CharSequence, Iterable) for collections: JSExport.java:36 (and :168) pass a List<String>, and HttpRequestHandler.java:295 also passes a List<String>. A unit test that reaches any of these paths resolves TextUtils to this stub and throws NoSuchMethodError at runtime, because the Iterable overload is missing here. Add an Iterable overload so the stub mirrors the API the code under test actually uses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At android/capacitor/src/test/java/android/text/TextUtils.java, line 5:
<comment>This stub only recreates `join(CharSequence, Object[])`, but production code calls the other real Android overload `join(CharSequence, Iterable)` for collections: `JSExport.java:36` (and `:168`) pass a `List<String>`, and `HttpRequestHandler.java:295` also passes a `List<String>`. A unit test that reaches any of these paths resolves `TextUtils` to this stub and throws `NoSuchMethodError` at runtime, because the `Iterable` overload is missing here. Add an `Iterable` overload so the stub mirrors the API the code under test actually uses.</comment>
<file context>
@@ -0,0 +1,17 @@
+
+public class TextUtils {
+
+ public static String join(CharSequence delimiter, Object[] tokens) {
+ StringBuilder builder = new StringBuilder();
+
</file context>
| } | ||
|
|
||
| void onModernBridgeMessage(String data, JavaScriptReplyProxy replyProxy) { | ||
| javaScriptReplyProxy = replyProxy; |
There was a problem hiding this comment.
P2: Synchronous plugin replies now call replyProxy.postMessage() from the background 'CapacitorPlugins' handler thread, whereas before this change they went through webView.post(...), which marshals to the WebView main thread. JavaScriptReplyProxy is @UiThread ("UI thread not currently enforced, but required"), so posting from a background thread is not guaranteed to deliver reliably. Marshal the reply to the main thread (e.g. via bridge.executeOnMainThread(...)) before calling replyProxy.postMessage().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At android/capacitor/src/main/java/com/getcapacitor/MessageHandler.java, line 45:
<comment>Synchronous plugin replies now call `replyProxy.postMessage()` from the background 'CapacitorPlugins' handler thread, whereas before this change they went through `webView.post(...)`, which marshals to the WebView main thread. `JavaScriptReplyProxy` is `@UiThread` ("UI thread not currently enforced, but required"), so posting from a background thread is not guaranteed to deliver reliably. Marshal the reply to the main thread (e.g. via `bridge.executeOnMainThread(...)`) before calling `replyProxy.postMessage()`.</comment>
<file context>
@@ -42,6 +41,11 @@ public MessageHandler(Bridge bridge, WebView webView, PluginManager cordovaPlugi
}
+ void onModernBridgeMessage(String data, JavaScriptReplyProxy replyProxy) {
+ javaScriptReplyProxy = replyProxy;
+ postMessage(data);
+ }
</file context>
|
|
||
| private static <T> T allocateWithoutConstructor(Class<T> type) { | ||
| try { | ||
| Class<?> reflectionFactoryClass = Class.forName("sun.reflect.ReflectionFactory"); |
There was a problem hiding this comment.
P3: The test bypasses the constructor via the internal sun.reflect.ReflectionFactory API, which is not a stable JDK API and is subject to encapsulation/removal across JDK versions. Failure shows up as an opaque AssertionError and, being outside ReflectiveOperationException, an InaccessibleObjectException would escape uncaught. Prefer a supported mechanism (e.g. Mockito's constructor mocking, or testing via the real constructor with mocks) over deep reflection into sun.reflect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At android/capacitor/src/test/java/com/getcapacitor/MessageHandlerTest.java, line 71:
<comment>The test bypasses the constructor via the internal `sun.reflect.ReflectionFactory` API, which is not a stable JDK API and is subject to encapsulation/removal across JDK versions. Failure shows up as an opaque `AssertionError` and, being outside `ReflectiveOperationException`, an `InaccessibleObjectException` would escape uncaught. Prefer a supported mechanism (e.g. Mockito's constructor mocking, or testing via the real constructor with mocks) over deep reflection into `sun.reflect`.</comment>
<file context>
@@ -0,0 +1,101 @@
+
+ private static <T> T allocateWithoutConstructor(Class<T> type) {
+ try {
+ Class<?> reflectionFactoryClass = Class.forName("sun.reflect.ReflectionFactory");
+ Method getReflectionFactory = reflectionFactoryClass.getDeclaredMethod("getReflectionFactory");
+ Object reflectionFactory = getReflectionFactory.invoke(null);
</file context>
| @@ -0,0 +1,17 @@ | |||
| package android.text; | |||
There was a problem hiding this comment.
P3: This test source defines a package-level stub android.text.TextUtils that shadows the real framework class for the entire capacitor unit-test source set, reimplementing only the join(CharSequence, Object[]) overload. It exists only to satisfy Logger.tags(...)'s TextUtils.join call without Robolectric. Prefer a scoped approach (mocking the dependency or using Robolectric with the real class) so unrelated tests aren't affected by a partial, framework-named stub.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At android/capacitor/src/test/java/android/text/TextUtils.java, line 3:
<comment>This test source defines a package-level stub `android.text.TextUtils` that shadows the real framework class for the entire `capacitor` unit-test source set, reimplementing only the `join(CharSequence, Object[])` overload. It exists only to satisfy `Logger.tags(...)`'s `TextUtils.join` call without Robolectric. Prefer a scoped approach (mocking the dependency or using Robolectric with the real class) so unrelated tests aren't affected by a partial, framework-named stub.</comment>
<file context>
@@ -0,0 +1,17 @@
+package android.text;
+
+public class TextUtils {
+
+ public static String join(CharSequence delimiter, Object[] tokens) {
</file context>
3 participants
Upstream PR Sync
This PR syncs changes from an external contributor's PR on the official Capacitor repository.
Original PR
Automation
Synced from upstream by Capacitor+ Bot