Conversation
Contributor
📝 WalkthroughWalkthroughThe PR upgrades Android builds to Java 21, aligns Ping SDK dependencies to 2.1.0, adds modular iOS dependencies, changes OpenID overrides to patch discovered configuration, and adds cross-platform Push number-challenge error mappings. ChangesPlatform and SDK updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
Comment on lines
+54
to
+60
| // TODO-PARITY(SDKS-5245): PingOidc 2.1.0 made `openId` private(set); only | ||
| // `openIdOverride` (patches fields after discovery completes) is public now. | ||
| // Configs that previously relied on `openId` to skip discovery entirely | ||
| // (no `discoveryEndpoint`) can no longer do so — `openIdOverride` never | ||
| // fires without a prior successful `discover()` call. | ||
| if let openId = payload.openId { | ||
| config.openIdOverride = openIdOverride(for: openId) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reject or preserve OpenID-only configurations.
openIdOverride runs only after successful discovery, but these builders still accept openId when discoveryEndpoint is absent. Those configurations now initialize with an empty discovery endpoint and can never apply their supplied endpoints, breaking previously supported login flows. Preserve a standalone-configuration path or reject this combination at the JS/config validation boundary with a clear error.
packages/oidc/ios/OidcClientFactory.swift#L54-L60: validate or supportpayload.openIdwhenpayload.discoveryEndpointis absent for native OIDC clients.packages/oidc/ios/OidcClientFactory.swift#L108-L111: apply the same handling for web OIDC clients.packages/journey/ios/Factory/JourneyClientFactory.swift#L90-L95: do not convert the resolver’sopenId-only Journey configuration into a discovery-dependent override.
📍 Affects 2 files
packages/oidc/ios/OidcClientFactory.swift#L54-L60(this comment)packages/oidc/ios/OidcClientFactory.swift#L108-L111packages/journey/ios/Factory/JourneyClientFactory.swift#L90-L95
🤖 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/ios/OidcClientFactory.swift` around lines 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.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (21.05%) 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.41% 71.03% -0.38%
Complexity 181 181
============================================
Files 154 193 +39
Lines 18052 19970 +1918
Branches 539 866 +327
============================================
+ Hits 12891 14186 +1295
- Misses 5111 5659 +548
- Partials 50 125 +75
... and 39 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
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
1 participant
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