Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR aligns Android builds with Java 21 and Kotlin 2.2.10, updates Ping SDK dependencies to 2.1.0, adds iOS modular dependencies, applies OpenID overrides after discovery, and adds cross-platform Push number-challenge error mappings. ChangesPlatform and SDK updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OidcClientFactory
participant OpenIdDiscovery
participant OpenIdConfiguration
OidcClientFactory->>OpenIdDiscovery: configure openIdOverride closure
OpenIdDiscovery->>OpenIdConfiguration: provide discovered endpoints
OidcClientFactory->>OpenIdConfiguration: apply endpoint overrides
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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
🤖 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/oidc/ios/OidcClientFactory.swift`:
- Around line 54-60: Handle OpenID-only configurations explicitly instead of
assigning discovery-dependent overrides: in
packages/oidc/ios/OidcClientFactory.swift lines 54-60 and 108-111, either
preserve standalone payload.openId initialization when discoveryEndpoint is
absent or reject that combination with a clear validation error; in
packages/journey/ios/Factory/JourneyClientFactory.swift lines 90-95, apply the
same rule and do not convert resolver openId-only configurations into
openIdOverride values that require discovery.
🪄 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 Plus
Run ID: 5544da85-fa6f-4c3d-95e4-2e057a7edcc2
⛔ Files ignored due to path filters (3)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzPingSampleApp/ios/Podfile.lockis excluded by!**/*.lockPingTestRunner/ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (48)
.github/workflows/browserstack-prep-android-artifacts.yml.github/workflows/build-and-test-android.yml.github/workflows/e2e-tests.ymlPingSampleApp/android/app/build.gradlePingSampleApp/android/build.gradlePingSampleApp/android/gradle.propertiesPingSampleApp/ios/PodfilePingTestRunner/android/app/build.gradlePingTestRunner/android/build.gradlePingTestRunner/android/gradle.propertiespackages/binding/RNPingBinding.podspecpackages/binding/android/build.gradlepackages/browser/RNPingBrowser.podspecpackages/browser/android/build.gradlepackages/core/android/build.gradlepackages/davinci/RNPingDavinci.podspecpackages/davinci/android/build.gradlepackages/device-client/RNPingDeviceClient.podspecpackages/device-client/android/build.gradlepackages/device-id/RNPingDeviceId.podspecpackages/device-id/android/build.gradlepackages/device-profile/RNPingDeviceProfile.podspecpackages/device-profile/android/build.gradlepackages/external-idp/RNPingExternalIdp.podspecpackages/external-idp/android/build.gradlepackages/fido/RNPingFido.podspecpackages/fido/android/build.gradlepackages/journey/RNPingJourney.podspecpackages/journey/android/build.gradlepackages/journey/ios/Factory/JourneyClientFactory.swiftpackages/logger/RNPingLogger.podspecpackages/logger/android/build.gradlepackages/oath/RNPingOath.podspecpackages/oath/android/build.gradlepackages/oidc/RNPingOidc.podspecpackages/oidc/android/build.gradlepackages/oidc/ios/OidcClientFactory.swiftpackages/oidc/ios/Tests/OidcClientFactoryTests.swiftpackages/push/RNPingPush.podspecpackages/push/android/build.gradlepackages/push/android/src/main/java/com/pingidentity/rnpush/PushErrorCode.ktpackages/push/android/src/main/java/com/pingidentity/rnpush/PushErrorMapper.ktpackages/push/android/src/test/java/com/pingidentity/rnpush/RNPingPushTest.ktpackages/push/ios/PushErrorCode.swiftpackages/push/ios/PushErrorMapper.swiftpackages/push/src/types/error.types.tspackages/storage/RNPingStorage.podspecpackages/storage/android/build.gradle
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (35.48%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
============================================
- Coverage 71.70% 70.81% -0.90%
Complexity 187 187
============================================
Files 161 193 +32
Lines 18890 20297 +1407
Branches 674 887 +213
============================================
+ Hits 13546 14374 +828
- Misses 5271 5796 +525
- Partials 73 127 +54
... and 34 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Contributor
|
tsdamas
changed the title
| targetSdkVersion = 36 | ||
| ndkVersion = "27.1.12297006" | ||
| kotlinVersion = "2.1.20" | ||
| kotlinVersion = "2.2.20" |
Contributor
There was a problem hiding this comment.
This should match with the sample app
pingidentity-gaurav
approved these changes
Aug 11, 2026
pingidentity-gaurav
left a comment
pingidentity-gaurav
left a comment
Contributor
There was a problem hiding this comment.
PR looks good, great work! Minor nitpick comment.
rodrigoareis
requested changes
Aug 12, 2026
rodrigoareis
left a comment
rodrigoareis
left a comment
Contributor
There was a problem hiding this comment.
Overall changes looks good. Left some minor comments
| let iosConfig = config["ios"] as? NSDictionary | ||
| if (discoveryEndpoint == nil || discoveryEndpoint?.isEmpty == true), openIdPayload == nil { | ||
| throw NSError(domain: "RNPingOidc", code: 400, userInfo: [NSLocalizedDescriptionKey: "Missing discoveryEndpoint or openId"]) | ||
| // PingOidc 2.1.0+ only applies `openId` as an override patched onto the result of a |
Contributor
There was a problem hiding this comment.
Lest add a TODO-PARITY(SDKS-5301)
|
|
||
| if let openId = payload.openId, let openIdConfig = buildOpenIdConfiguration(openId) { | ||
| config.openId = openIdConfig | ||
| // PingOidc 2.1.0 made `openId` private(set); only `openIdOverride` (patches fields |
Contributor
There was a problem hiding this comment.
Lest add a TODO-PARITY(SDKS-5301)
| implementation("com.pingidentity.sdks:davinci:2.0.1") | ||
| implementation("com.pingidentity.sdks:davinci:2.1.0") | ||
| // Required: IdpCollector is defined in external-idp. The JS authorizeForDaVinci API is optional but this native dep is not. | ||
| compileOnly("com.pingidentity.sdks:external-idp:2.0.1") |
Contributor
There was a problem hiding this comment.
Suggested change
| compileOnly("com.pingidentity.sdks:external-idp:2.0.1") | |
| compileOnly("com.pingidentity.sdks:external-idp:2.1.0") |
Contributor
There was a problem hiding this comment.
Suggested change
| testImplementation("com.pingidentity.sdks:external-idp:2.1.0") |
| implementation "com.facebook.react:react-android" | ||
| implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" | ||
| implementation("com.pingidentity.sdks:davinci:2.0.1") | ||
| implementation("com.pingidentity.sdks:davinci:2.1.0") |
Contributor
There was a problem hiding this comment.
Looks like external-idp lines were missed in the version bump
Contributor
Author
There was a problem hiding this comment.
Good catch. This was slipped from the merge conflict resolution when rebasing. Will be addressed.
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
3 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
Bumps the native Ping Android SDK (
com.pingidentity.sdks:*) and Ping iOS SDK (Ping*pods) from2.0.xto2.1.0across every package, and updates build tooling to match the new SDK's requirements.com.pingidentity.sdks:*to2.1.0in all 14 packages'build.gradle. The 2.1.0 artifacts are compiled for JVM 21, sosourceCompatibility/targetCompatibility/kotlinOptions.jvmTargetwere raised to 21 in every package, plusPingSampleAppandPingTestRunnerapp modules. Addedreact.internal.disableJavaVersionAlignment=trueto both apps'gradle.propertiesto stop React Native's Gradle plugin from force-resetting JVM target back to 17, andkotlin.jvm.target.validation.mode=warningto downgrade third-partynode_modulesJVM-mismatch errors (previously masked by RNGP's alignment) to warnings. Pinned the Kotlin Gradle plugin classpath explicitly to the declaredkotlinVersionin both apps to avoid an unrelated/older Kotlin compiler resolving and choking onkotlin-stdlibmetadata pulled in transitively by the SDK (e.g. viaktor-client).Ping*pod dependencies to2.1.0across every package's podspec, andPingExternalIdP*pods to2.1.0inPingSampleApp/ios/Podfile. AddedRecaptchaInteropandGoogleUtilitieswith:modular_headers => trueto the sample app'sPodfile, required becausePingExternalIdPGoogle -> GoogleSignIn -> AppCheckCorepulls them in and neither defines a module (required by Swift under static linkage).build-and-test-android.yml,browserstack-prep-android-artifacts.yml, ande2e-tests.ymlto match the new package JVM target.Native API adjustments driven by the bump
PingOidc2.1.0 madeOidcClientConfig.openIdprivate(set); the only public way to influence the discovered configuration is nowopenIdOverride, a closure applied afterdiscover()completes. Updatedpackages/oidc/ios/OidcClientFactory.swiftandpackages/journey/ios/Factory/JourneyClientFactory.swiftto build anopenIdOverrideclosure from the JS-suppliedopenIdpayload instead of constructing anOpenIdConfigurationdirectly. UpdatedOidcClientFactoryTestsaccordingly, since the previous test asserted onopenIdsynchronously before discovery runs.TODO-PARITY(SDKS-5245)in both files): Android's config parser still allows skipping discovery entirely by supplyingopenIdwithout adiscoveryEndpoint. On iOS,openIdOverridenever fires without a prior successfuldiscover()call, so that shortcut no longer works on iOS. Needs a native SDK fix or an Android-side follow-up to keep both platforms behaved the same way.PushNumberChallengeException→push_number_challenge_errormapping (AndroidPushErrorMapper.kt, iOSPushErrorMapper.swift, and the sharedPushErrorCodeTS type) to surface a push SDK error case that had no prior mapping. Addedcom.pingidentity.sdks:utils:2.1.0as an explicitimplementationdependency inpackages/push/android/build.gradle— the push SDK only declares it as aruntime-scope transitive dependency, soApiException(the actual superclass ofPushNumberChallengeException) was invisible to the Kotlin compiler, causing an "always false" instance-check warning on the new mapping and a hard compile error in the new unit test. AddedpushNumberChallengeExceptionMapsToPushNumberChallengeErrorandpushNumberChallengeExceptionMapsToArgumentErrorTypetoRNPingPushTest.ktto cover the new code path.Testing
yarn packages:build,lint,prettier,typecheck, andtestall pass.PushNumberChallengeExceptiontests);PingSampleAppandPingTestRunnerboth assemble successfully.modular_headerspods;RNPackagesTestspasses.Known follow-ups
push_number_challenge_errorerror code; runyarn changesetbefore merge.Summary by CodeRabbit