feat(android): SDKS-5018 introduce launchBridge in rn-core and migrate all scope.launch bridge sites#47
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>
|
Warning Review limit reached
More reviews will be available in 27 minutes and 38 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a coroutine-to-React-Native Promise bridge (launchBridge), unit/QA tests, and migrates many Android RN bridge methods across modules to use launchBridge with preserved cancellation semantics and consistent promise rejection mapping. ChangesAndroid Coroutine-Promise Bridge Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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
|
![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
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
|
Overall, looks good to me. Left minor comments. |
rodrigoareis
left a comment
rodrigoareis
left a comment
There was a problem hiding this comment.
Overall changes looks good. Left some comments
| ) | ||
| }, | ||
| onFailure = { error -> | ||
| promise.reject( |
There was a problem hiding this comment.
Should we consider rethrowing and relying on launchBridge with the correct errorCode? Otherwise, we can add a comment explaining the bypass is deliberate.
There was a problem hiding this comment.
Added a comment explaining this is deliberate. The Ping Device Profile SDK wraps all exceptions inside Result.Failure before the block returns, so launchBridge never sees them as thrown exceptions. Mapping errors explicitly in fold.onFailure is the correct pattern here.
| * @param id Logger handle identifier from JS. | ||
| * @return Native logger instance, or null when missing/invalid. | ||
| */ | ||
| private fun resolveLoggerFromCore(id: String?): Logger? { |
There was a problem hiding this comment.
This was a pre-existing issue but this PR. But now I noticed other packages have an identical private copy of this method. I suggest extract it to a top-level utility in rn-core/utils/ (e.g., LoggerResolver.kt) to keep a single source of truth.
There was a problem hiding this comment.
Pre-existing across all packages. com.pingidentity.logger is not a dependency of rn-core and we want to keep it that way since rn-core currently has no native SDK dependencies beyond react-android. Extracting the helper there would break that architectural boundary. The per-package copies are a one-liner each wrapping the logger registry resolve. Happy to revisit if there is appetite for a separate shared utility module outside of rn-core.
There was a problem hiding this comment.
This justification is architecturally valid, but accept duplication bugs me. I think there are better paths, as you suggested above. I am ok to keep it for now. We will probably need in a near future a new rn-android-utils / rn-bridge-core module that houses resolveNativeLogger, and potentially other helpers (DeviceType constants, etc.), then we can address it.
There was a problem hiding this comment.
Agreed. I m gonna add that in as an exploration task.
- Fix KDoc on launchBridge: promise param only handles rejection, not success settlement (caller's responsibility inside the block) - Remove unused android.util.Log import in rn-device-profile - Add explanatory comment on fold.onFailure bypass in rn-device-profile (SDK wraps exceptions in Result.Failure before launchBridge can see them) - Add import and de-qualify Arguments calls in rn-binding for consistency - Sort imports in rn-push alphabetically - Add @volatile to scope field in rn-push for cross-thread visibility - Fix push cleanup() to close PushClient instances before clearing the registry, using a dedicated short-lived scope since close() is suspend - Restructure device-client get/update/deleteDevice to move result.fold outside the try/catch, preventing potential double-rejection if onFailure's rejectThrowable were to throw - Add CancellationException rethrow comments in oath, device-client, external-idp, push explaining why the inner rethrow is necessary Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rodrigoareis
left a comment
rodrigoareis
left a comment
There was a problem hiding this comment.
Changes looks good to me
george-bafaloukas-forgerock
left a comment
george-bafaloukas-forgerock
left a comment
There was a problem hiding this comment.
Cross-platform consistency review of the launchBridge migration. The migration is clean; two sites conflate Kotlin coroutine CancellationException (structured-concurrency teardown) with user-initiated browser cancel, which contradicts both launchBridge's documented contract and the iOS RN bridge.
Verified against iOS (packages/browser/ios, packages/oidc/ios) and both native SDKs: iOS resolves {type:"cancel"} only for the dedicated user-cancel signals (BrowserError.externalUserAgentCancelled, ASWebAuthenticationSessionError.canceledLogin) and has no catch is CancellationError — Swift Task cancellation is rejected, never resolved as cancel. On both platforms the native user-cancel signal is a distinct type from the cooperative-cancellation primitive. See the 2 inline comments.
| payload.putString("type", "cancel") | ||
| promise.resolve(payload) | ||
| return@launch | ||
| return@launchBridge |
There was a problem hiding this comment.
CancellationException is swallowed and resolved as a {type:"cancel"} payload here (the if (error is BrowserCanceledException || error is CancellationException) at line 230), defeating the migration's purpose.
The inner catch (e: Exception) { Result.failure(e) } (~line 211) captures coroutine CancellationException into the Result, and this branch then resolves {type:"cancel"} for it. launchBridge exists precisely to re-throw CancellationException so scope cancellation propagates without settling the promise — this site subverts that.
Inconsistent with iOS: packages/browser/ios/RNPingBrowserCommon.swift resolves {type:"cancel"} only for BrowserError.externalUserAgentCancelled / ASWebAuthenticationSessionError.canceledLogin, and lets Swift CancellationError reject through the generic catch. BrowserCanceledException (Android's genuine user-cancel) is a plain RuntimeException, unrelated to kotlinx.coroutines.CancellationException, and is returned via normal suspend completion — not via coroutine cancellation.
Suggested fix: drop the || error is CancellationException disjunct at line 230, and stop catching CancellationException in the inner catch (e: Exception) (re-throw it). Keep BrowserCanceledException -> {type:"cancel"}. Genuine user cancels are unaffected; structured-concurrency cancellation then propagates per the launchBridge contract and matches iOS.
There was a problem hiding this comment.
Fixed. Added catch (e: CancellationException) { throw e } before catch (e: Exception) so CancellationException is never boxed into Result.failure, and dropped the || error is CancellationException disjunct from the result.exceptionOrNull() check. The kotlinx.coroutines.CancellationException import is still needed for the rethrow clause. BrowserCanceledException to {type:"cancel"} is unchanged.
| canceled.putString("type", "cancel") | ||
| promise.resolve(canceled) | ||
| return@launch | ||
| return@launchBridge |
There was a problem hiding this comment.
Same CancellationException-as-cancel conflation as the browser module — at both the inner catch (e: Exception) check (line 390) and this result.exceptionOrNull() check (line 408).
The catch (e: Exception) pre-empts launchBridge's CancellationException re-throw, and the || ... is CancellationException branches resolve {type:"cancel"} for structured-concurrency cancellation. iOS (packages/oidc/ios/RNPingOidcCommon.swift) resolves cancel only for BrowserError.externalUserAgentCancelled / ASWebAuthenticationSessionError.canceledLogin and rejects Swift CancellationError via the generic catch — it does not conflate the two.
Suggested fix: remove the || ... is CancellationException disjunct at both line 390 and line 408 (keep BrowserCanceledException), and let CancellationException propagate to launchBridge. This restores the documented contract and matches iOS; the real user-cancel path is carried by BrowserCanceledException via normal completion, so it is not regressed.
There was a problem hiding this comment.
Fixed. Same pattern as the browser module. Added a dedicated catch (e: CancellationException) { throw e } guard before the catch (e: Exception) block, removed || e is CancellationException from the inner if check, and dropped || error is CancellationException from the result.exceptionOrNull() check. Import retained for the rethrow clause.
…nch flows
CancellationException was being caught by the inner catch (e: Exception)
block and resolved as {type:"cancel"}, settling the JS promise instead of
letting launchBridge propagate structured-concurrency cancellation. Added
a dedicated catch (e: CancellationException) { throw e } guard before each
inner Exception handler and removed the spurious || error is
CancellationException disjuncts from the result.exceptionOrNull() checks.
BrowserCanceledException (the real user-cancel signal) is unaffected.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 participants
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
Refactor
Tests
Chore