feat(android): SDKS-5018 introduce launchBridge in rn-core and migrate all scope.launch bridge sites#47
Open
pingidentity-gaurav wants to merge 1 commit into
Open
feat(android): SDKS-5018 introduce launchBridge in rn-core and migrate all scope.launch bridge sites#47pingidentity-gaurav wants to merge 1 commit into
pingidentity-gaurav wants to merge 1 commit into
Conversation
Introduces launchBridge coroutine extension in rn-core that re-throws CancellationException for correct structured-concurrency propagation, and settles the React Native promise via GenericError for all other throwables. Migrates all 48 scope.launch bridge call sites across 11 Android packages to use it. Packages migrated: rn-binding, rn-device-profile, rn-fido, rn-journey, rn-device-id, rn-oidc, rn-oath, rn-device-client, rn-external-idp, rn-browser, rn-push. Packages with no promise-settling launch sites (rn-logger) and non-promise event-emission sites (rn-push flush/token) are intentionally left as scope.launch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR centralizes React Native promise-based coroutine execution across 11 Android SDK modules by introducing a new ChangesAndroid Coroutine-Promise Bridge Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
pingidentity-gaurav
changed the title
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/browser/android/src/main/java/com/pingidentity/rnbrowser/RNPingBrowserCommon.kt (1)
206-235:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid resolving the promise on
CancellationException; rethrow instead.In
RNPingBrowserCommon.kt, the innercatch (e: Exception)turnsCancellationExceptionintoResult.failure, and the later branch treats it as a"cancel"outcome. This settles the JS promise instead of allowing coroutine cancellation to propagate.🛠️ Proposed adjustment
val result = try { val resolvedRedirectUri = redirectUri?.toUri() ?: browserLauncher.redirectUri browserLauncher.launch(launchUrl, resolvedRedirectUri) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Result.failure(e) }Drop
|| error is CancellationExceptionfrom the"cancel"handling so onlyBrowserCanceledExceptionmaps to{ type: "cancel" }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/browser/android/src/main/java/com/pingidentity/rnbrowser/RNPingBrowserCommon.kt` around lines 206 - 235, The current try/catch in the launch flow converts all Exceptions (including CancellationException) into Result.failure, which later leads to settling the JS promise as a "cancel" rather than propagating coroutine cancellation; update the catch in RNPingBrowserCommon.kt (the block that computes `val result = try { ... } catch (e: Exception) { ... }`) to rethrow CancellationException (e.g., if (e is CancellationException) throw e) and otherwise return Result.failure(e), and then remove `|| error is CancellationException` from the later cancel-handling branch so only BrowserCanceledException maps to `{ type: "cancel" }`.
🧹 Nitpick comments (2)
packages/core/android/src/test/java/com/pingidentity/rncore/utils/CoroutineBridgeTest.kt (1)
70-156: ⚡ Quick winExtract
TestPromise(and theArgumentsshadow) into a shared test fixture. This same ~90-linePromisestub and the@Implementsshadow are duplicated verbatim inCoroutineBridgeQaTest.kt. Both live in the same package/source set, so a single sharedTestPromise/shadow would remove the duplication and prevent the two copies from drifting as thePromiseinterface evolves.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/android/src/test/java/com/pingidentity/rncore/utils/CoroutineBridgeTest.kt` around lines 70 - 156, Duplicate TestPromise and the Arguments `@Implements` shadow exist in CoroutineBridgeTest and CoroutineBridgeQaTest; extract them into a single shared test fixture (e.g., a new file in the same test package) containing the TestPromise class and the Arguments shadow implementation, keep the same package and annotation details so Robolectric/shadowing still works, remove the duplicate definitions from CoroutineBridgeTest and CoroutineBridgeQaTest, and update their imports/usages to reference the shared TestPromise and shared Argument shadow to avoid drift as the Promise interface evolves.packages/oidc/android/src/main/java/com/pingidentity/rnoidc/RNPingOidcCommon.kt (1)
383-398: ⚡ Quick winOIDC authorize intentionally maps
CancellationExceptionto a"cancel"payload
- This mirrors the Android browser bridge (
RNPingBrowserCommon.kt), which also convertsCancellationExceptionto{ type: "cancel" }instead of lettinglaunchBridgerethrow.launchBridgeonly rethrowsCancellationExceptionwhen it escapes the block; if you want cleanup/scope cancellation to propagate without settling the JS promise (perlaunchBridgedocs), then don’t catchCancellationExceptionhere—otherwise this should be documented as “cleanup cancellation surfaces as cancel”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc/android/src/main/java/com/pingidentity/rnoidc/RNPingOidcCommon.kt` around lines 383 - 398, The current catch in RNPingOidcCommon.kt around the authorize call swallows CancellationException (and maps it to a "cancel" JS payload), preventing scope cancellation from propagating via launchBridge; update the error handling in the authorize block (the try/catch that wraps handle.web.authorize in launchBridge) so CancellationException is not caught and instead is rethrown (or remove CancellationException from the caught types) while still mapping BrowserCanceledException to the `{ type: "cancel" }` payload; ensure CancellationException is allowed to escape so launchBridge can handle scope cancellation as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/core/android/src/main/java/com/pingidentity/rncore/utils/CoroutineBridge.kt`:
- Line 28: The KDoc for launchBridge incorrectly states that the provided
promise is settled on success or failure; in reality launchBridge only rejects
the promise on non-cancellation failures and does not call resolve for
successful completions. Update the KDoc for the function launchBridge and its
param promise to clarify that callers are responsible for resolving the promise
on success and that launchBridge will only reject the promise on
non-cancellation errors (or leave resolution to the provided block). Refer to
the launchBridge function and the promise parameter in the comment to ensure the
wording matches the implemented behavior.
---
Outside diff comments:
In
`@packages/browser/android/src/main/java/com/pingidentity/rnbrowser/RNPingBrowserCommon.kt`:
- Around line 206-235: The current try/catch in the launch flow converts all
Exceptions (including CancellationException) into Result.failure, which later
leads to settling the JS promise as a "cancel" rather than propagating coroutine
cancellation; update the catch in RNPingBrowserCommon.kt (the block that
computes `val result = try { ... } catch (e: Exception) { ... }`) to rethrow
CancellationException (e.g., if (e is CancellationException) throw e) and
otherwise return Result.failure(e), and then remove `|| error is
CancellationException` from the later cancel-handling branch so only
BrowserCanceledException maps to `{ type: "cancel" }`.
---
Nitpick comments:
In
`@packages/core/android/src/test/java/com/pingidentity/rncore/utils/CoroutineBridgeTest.kt`:
- Around line 70-156: Duplicate TestPromise and the Arguments `@Implements` shadow
exist in CoroutineBridgeTest and CoroutineBridgeQaTest; extract them into a
single shared test fixture (e.g., a new file in the same test package)
containing the TestPromise class and the Arguments shadow implementation, keep
the same package and annotation details so Robolectric/shadowing still works,
remove the duplicate definitions from CoroutineBridgeTest and
CoroutineBridgeQaTest, and update their imports/usages to reference the shared
TestPromise and shared Argument shadow to avoid drift as the Promise interface
evolves.
In
`@packages/oidc/android/src/main/java/com/pingidentity/rnoidc/RNPingOidcCommon.kt`:
- Around line 383-398: The current catch in RNPingOidcCommon.kt around the
authorize call swallows CancellationException (and maps it to a "cancel" JS
payload), preventing scope cancellation from propagating via launchBridge;
update the error handling in the authorize block (the try/catch that wraps
handle.web.authorize in launchBridge) so CancellationException is not caught and
instead is rethrown (or remove CancellationException from the caught types)
while still mapping BrowserCanceledException to the `{ type: "cancel" }`
payload; ensure CancellationException is allowed to escape so launchBridge can
handle scope cancellation as intended.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0033052-1b2c-4265-ba91-2a9f7139ff77
📒 Files selected for processing (16)
packages/binding/android/src/main/java/com/pingidentity/rnbinding/RNPingBindingCommon.ktpackages/browser/android/src/main/java/com/pingidentity/rnbrowser/RNPingBrowserCommon.ktpackages/core/android/build.gradlepackages/core/android/src/main/java/com/pingidentity/rncore/utils/CoroutineBridge.ktpackages/core/android/src/test/java/com/pingidentity/rncore/utils/CoroutineBridgeQaTest.ktpackages/core/android/src/test/java/com/pingidentity/rncore/utils/CoroutineBridgeTest.ktpackages/device-client/android/src/main/java/com/pingidentity/rndeviceclient/RNPingDeviceClientCommon.ktpackages/device-id/android/src/main/java/com/pingidentity/rndeviceid/RNPingDeviceIdCommon.ktpackages/device-profile/android/src/main/java/com/pingidentity/rndeviceprofile/RNPingDeviceProfileCommon.ktpackages/external-idp/android/src/main/java/com/pingidentity/rnexternalidp/RNPingExternalIdpCommon.ktpackages/fido/android/src/main/java/com/pingidentity/rnfido/RNPingFidoCommon.ktpackages/fido/android/src/test/java/com/pingidentity/rnfido/RNPingFidoTest.ktpackages/journey/android/src/main/java/com/pingidentity/rnjourney/RNPingJourneyCommon.ktpackages/oath/android/src/main/java/com/pingidentity/rnoath/RNPingOathCommon.ktpackages/oidc/android/src/main/java/com/pingidentity/rnoidc/RNPingOidcCommon.ktpackages/push/android/src/main/java/com/pingidentity/rnpush/RNPingPushCommon.kt
| * mapped to a [com.pingidentity.rncore.error.GenericError] via | ||
| * [mapThrowableToGenericError] and the promise is rejected with the resulting error. | ||
| * | ||
| * @param promise The React Native promise to settle on success or failure. |
Contributor
There was a problem hiding this comment.
KDoc overstates what launchBridge settles. The function only rejects the promise on non-cancellation failure; success settlement (resolve) is the block's responsibility (as the tests confirm). The @param promise text "to settle on success or failure" can mislead callers into omitting their own resolve.
📝 Suggested wording
- * `@param` promise The React Native promise to settle on success or failure.
+ * `@param` promise The React Native promise rejected on non-cancellation failure.
+ * On success the promise is left untouched; the [block] is responsible for resolving it.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * @param promise The React Native promise to settle on success or failure. | |
| * `@param` promise The React Native promise rejected on non-cancellation failure. | |
| * On success the promise is left untouched; the [block] is responsible for resolving it. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/core/android/src/main/java/com/pingidentity/rncore/utils/CoroutineBridge.kt`
at line 28, The KDoc for launchBridge incorrectly states that the provided
promise is settled on success or failure; in reality launchBridge only rejects
the promise on non-cancellation failures and does not call resolve for
successful completions. Update the KDoc for the function launchBridge and its
param promise to clarify that callers are responsible for resolving the promise
on success and that launchBridge will only reject the promise on
non-cancellation errors (or leave resolution to the provided block). Refer to
the launchBridge function and the promise parameter in the comment to ensure the
wording matches the implemented behavior.
tsdamas
reviewed
Jun 3, 2026
| }, | ||
| onFailure = { err -> DeviceErrorClassifier.rejectThrowable(promise, err) }, | ||
| ) | ||
| } catch (e: CancellationException) { |
Contributor
There was a problem hiding this comment.
Why are we re-throwing the CancellationException when the launchBridge already does that?
tsdamas
reviewed
Jun 3, 2026
| ) | ||
| } | ||
| ) | ||
| } catch (e: CancellationException) { |
Contributor
There was a problem hiding this comment.
Same here and on line 299
tsdamas
reviewed
Jun 3, 2026
| private const val LOCATION_SERVICES_CLASS = | ||
| "com.google.android.gms.location.LocationServices" | ||
| private val scope = CoroutineScope(Dispatchers.IO) | ||
| private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO) |
Contributor
There was a problem hiding this comment.
This is a good improvement.
tsdamas
reviewed
Jun 3, 2026
| val credential = deserializeCredential(credentialMap) | ||
| val saved = client.saveCredential(credential).getOrThrow() | ||
| promise.resolve(serializeCredential(saved)) | ||
| } catch (e: CancellationException) { |
Contributor
|
Overall, looks good to me. Left minor comments. |
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
2 participants
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
launchBridgeKotlin extension onCoroutineScopeinrn-core/utils/CoroutineBridge.kt— re-throwsCancellationExceptionfor correct structured-concurrency propagation, maps all otherThrowables toGenericErrorviamapThrowableToGenericError, and settles the React Native promisescope.launchbridge call sites across 11 Android packages to uselaunchBridgern-device-profilemissingSupervisorJob()on itsCoroutineScopePackages migrated:
rn-binding,rn-browser,rn-device-client,rn-device-id,rn-device-profile,rn-external-idp,rn-fido,rn-journey,rn-oath,rn-oidc,rn-pushNot migrated (intentional):
rn-loggersync()(fire-and-forget, no promise) andrn-pushevent-emission sites (no promise)Problem
Every
scope.launchbridge site caughtThrowableorExceptionuniformly, which swallowedCancellationException. This violated Kotlin structured concurrency: scope cancellation produced spurious JS promise rejections instead of propagating cleanly.Approach
Three migration patterns depending on the package's error-handling needs:
launchBridgeouter catch handles all errors viamapThrowableToGenericError(rn-device-id,rn-oidcsimple sites)catch (e: CancellationException) { throw e }added before existing catch, preserving package-local error mappers (rn-oath,rn-device-client,rn-external-idp,rn-push)scope.launchwrapped (rn-browser,rn-oidcauthorize site)Test plan
./gradlew testDebugUnitTest— BUILD SUCCESSFUL)CoroutineBridgeTest.kt— 8 unit tests covering re-throw, error type mapping, context forwardingCoroutineBridgeQaTest.kt— 5 integration tests covering live scope cancellation, throwable identity, context passthroughSummary by CodeRabbit
Release Notes
Refactor
Tests