feat(all): SDKS-5019 code quality and pre-1.0 consolidation#49
feat(all): SDKS-5019 code quality and pre-1.0 consolidation#49pingidentity-gaurav wants to merge 7 commits into
Conversation
- Gradle: align Kotlin/serialization/coroutines versions; bump all native SDK deps 2.0.0→2.0.1
- Union types: widen 12 error code / result types with | (string & {}) across 8 packages
- Push: make rn-storage fully optional — use PushStorageHandle from rn-types, no rn-storage dep
- noopLogger: extract to rn-types, remove local copies from 11 packages
- OidcClient/OidcWebClient: add dispose() — deregisters from CoreRuntime registries (iOS + Android)
- journeyCallbackType: add merged constant to rn-types (callbackType + nativeExtensionCallbackType)
- useJourneyForm: add JourneyFormOptions.handledCallbackTypes so canSubmit works for integration nodes
- getNativeModule: add __DEV__ guard + module-level caching across all 13 NativeRNPing*.ts files
- Browser iOS logger: wire JS logger through to BrowserLauncher.launch() — matches Android behaviour
- release.yml: auto-create GitHub release with changelog notes on tag push
- READMEs: add LICENSE files, standardise footers, document all configure* storage APIs
- Sample app: module-level logger constants, fix <></> → null, align DeviceOf types
- JourneyContinuePanel: derive blocking state from form.issues; submitDisabled = loading || !form.canSubmit
- TODO audit: tag blocked items, remove stale comments, update CONTRIBUTING.md scripts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 38 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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)
📝 WalkthroughWalkthroughThis PR consolidates SDK maintenance across 90+ files: exporting shared logger/callback type primitives from ChangesSDK-wide bridge hardening, typing alignment, and lifecycle updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…ncies Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…clude module list; add PingLogger import to browser iOS test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
pingidentity-gaurav
requested review from
george-bafaloukas-forgerock,
rodrigoareis,
tsdamas and
witrisna
Code reviewHi @pingidentity-gaurav — great consolidation work here, the pre-1.0 cleanup is much appreciated! I found 3 issues worth addressing before merge: 1. The 2. Module-level caching not applied consistently
ping-react-native-sdk/packages/logger/src/NativeRNPingLogger.ts Lines 56 to 72 in a05fa87 3.
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…t for tests - RNPingBrowserCommon: use async actor-based logger resolution instead of RegistrySync to avoid deadlock in test context - RNPingBrowser.podspec: add PingLogger as explicit dependency to main spec and test spec - NativeRNPingExternalIdp/DeviceId/Fido: export _resetNativeModuleForTesting() so tests can reset module-level cache between runs - external-idp native-module test: call _resetNativeModuleForTesting() in beforeEach Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/storage/src/NativeRNPingStorage.ts (1)
452-470:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd module-level caching to align with PR description.
The PR description states that module-level caching was added across all
NativeRNPing*.tsfiles, butgetNativeModule()in this file lacks the_nativeModulecaching pattern present in other modules (e.g.,packages/external-idp/src/NativeRNPingExternalIdp.ts). According to PR comment#2,NativeRNPingStorage.tsappears untouched and should either receive the caching pattern or the PR description should be updated to reflect the intentional exclusion.Without caching,
getNativeModule()probesTurboModuleRegistryandNativeModuleson every call, which is unnecessary since the native module reference does not change at runtime.⚡ Proposed fix to add module-level caching
+let _nativeModule: Spec | null = null; +/** `@internal` — resets the module cache for testing only. */ +export function _resetNativeModuleForTesting(): void { + _nativeModule = null; +} export function getNativeModule(): Spec { + if (_nativeModule) return _nativeModule; + const turbo = TurboModuleRegistry.get<Spec>('RNPingStorage'); if (turbo) { - return turbo; + _nativeModule = turbo; + return _nativeModule; } const classic = NativeModules.RNPingStorageClassic as Spec | undefined; if (classic) { - return classic; + _nativeModule = classic; + return _nativeModule; } const availableModules = __DEV__ ? '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)) : ''; throw new Error( '[`@ping-identity/rn-storage`] Native module RNPingStorage not found.\n' + 'Ensure the library is linked correctly and the app has been rebuilt.' + availableModules, ); }🤖 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/storage/src/NativeRNPingStorage.ts` around lines 452 - 470, getNativeModule currently probes TurboModuleRegistry/NativeModules on every call; add a module-level cache (e.g., a variable named _nativeModule: Spec | undefined | null) and update getNativeModule to return _nativeModule if set, otherwise resolve the native module from TurboModuleRegistry or NativeModules, assign it to _nativeModule, and then return it; ensure the thrown Error remains unchanged if resolution fails. This mirrors the caching pattern used in other files such as NativeRNPingExternalIdp.ts and prevents repeated lookups.packages/logger/src/NativeRNPingLogger.ts (1)
57-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winModule-level caching was not added.
The PR description states that "All 13
NativeRNPing*.tsmodules: added__DEV__guard on module list and module-level caching (per description)," but this file only received the error message reformatting. The_nativeModulecaching pattern implemented inNativeRNPingOath.tswas not applied here.Per the reviewer's feedback, this inconsistency should either be corrected or explained if intentional.
⚡ Proposed fix to add module-level caching
+let _nativeModule: Spec | null = null; export function getNativeModule(): Spec { + if (_nativeModule) return _nativeModule; + const turbo = TurboModuleRegistry.get<Spec>('Logger'); if (turbo) { - return turbo; + _nativeModule = turbo; + return _nativeModule; } const classic = NativeModules.RNPingLoggerClassic as Spec | undefined; if (classic) { - return classic; + _nativeModule = classic; + return _nativeModule; } const availableModules = '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)); throw new Error( '[`@ping-identity/rn-logger`] Native module Logger not found.\n' + 'Ensure the library is linked correctly and the app has been rebuilt.' + availableModules, ); }🤖 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/logger/src/NativeRNPingLogger.ts` around lines 57 - 75, getNativeModule currently always queries TurboModuleRegistry/NativeModules and throws if not found; add the same module-level caching used in NativeRNPingOath.ts by introducing a file-scoped _nativeModule variable and returning it when set to avoid repeated lookups, and add the __DEV__ guard around the list of available modules string construction as done in other NativeRNPing*.ts files; update getNativeModule to set _nativeModule to the found turbo or classic module before returning and reuse _nativeModule on subsequent calls.packages/device-profile/src/NativeRNPingDeviceProfile.ts (1)
52-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick winModule-level caching was not added.
The PR description states that "All 13
NativeRNPing*.tsmodules: added__DEV__guard on module list and module-level caching (per description)," but this file only received the error message reformatting. The_nativeModulecaching pattern implemented inNativeRNPingOath.tswas not applied here.Per the reviewer's feedback, this inconsistency should either be corrected or explained if intentional.
⚡ Proposed fix to add module-level caching
+let _nativeModule: Spec | null = null; export function getNativeModule(): Spec { + if (_nativeModule) return _nativeModule; + const turbo = TurboModuleRegistry.get<Spec>('RNPingDeviceProfile'); if (turbo) { - return turbo; + _nativeModule = turbo; + return _nativeModule; } const classic = NativeModules.RNPingDeviceProfileClassic as Spec | undefined; if (classic) { - return classic; + _nativeModule = classic; + return _nativeModule; } const availableModules = '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)); throw new Error( '[`@ping-identity/rn-device-profile`] Native module RNPingDeviceProfile not found.\n' + 'Ensure the library is linked correctly and the app has been rebuilt.' + availableModules, ); }🤖 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/device-profile/src/NativeRNPingDeviceProfile.ts` around lines 52 - 70, getNativeModule currently builds and throws an error each call but lacks the module-level cache used elsewhere; add a module-scoped cache (e.g., let _nativeModule: Spec | null = null) and have getNativeModule return _nativeModule when set, otherwise locate TurboModuleRegistry or NativeModules.RNPingDeviceProfileClassic, assign the found module to _nativeModule and return it; also mirror the other files' behavior by only including the available NativeModules list in the error when __DEV__ is true so production calls don't leak that info.packages/device-client/src/types/device.types.ts (1)
189-199:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift
DeviceOfconstraint and documentation are inconsistent withDeviceKindwidening.The constraint change from
K extends DeviceKindtoK extends keyof DeviceByKindcombined with wideningDeviceKindto include| (string & {})creates a breaking change:
keyof DeviceByKindis the closed union'oath' | 'push' | 'bound' | 'profile' | 'webAuthn'.DeviceKindnow includes(string & {}), which is NOT assignable tokeyof DeviceByKind.- Any external code using
DeviceOf<DeviceKind>(e.g., in generic helper functions) will now fail because the(string & {})branch cannot satisfy thekeyof DeviceByKindconstraint.The
@typeParamat line 192 referencesDeviceKindliterals but the constraint is now stricter. The@exampleshowsDeviceOf<'bound'>(which works) but does not demonstrate the correct generic patternDeviceOf<keyof DeviceByKind>that all sample app callsites now use.Impact:
- External consumers following the existing documentation will encounter type errors.
- The migration pattern (
DeviceOf<keyof DeviceByKind>) is not documented here.📝 Proposed documentation fix
Update the
@typeParamand add a generic usage example:/** * Resolves the concrete device type for a given {`@link` DeviceKind}. * - * `@typeParam` K - One of the supported {`@link` DeviceKind} literals. + * `@typeParam` K - A key from {`@link` DeviceByKind}. To accept all known kinds in a generic context, use `keyof DeviceByKind` rather than `DeviceKind` (which includes an extensible string fallback). * * `@example` + * // Specific device kind: * ```ts * type T = DeviceOf<'bound'>; // BoundDevice * ``` + * + * `@example` + * // Generic usage accepting any known kind: + * ```ts + * function processDevice<K extends keyof DeviceByKind>( + * kind: K, + * device: DeviceOf<K> + * ) { ... } + * ``` */ export type DeviceOf<K extends keyof DeviceByKind> = DeviceByKind[K];Additionally, note this as a breaking change in the CHANGELOG as flagged by reviewer george-bafaloukas-forgerock: code using
DeviceOf<DeviceKind>in generic contexts must migrate toDeviceOf<keyof DeviceByKind>.🤖 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/device-client/src/types/device.types.ts` around lines 189 - 199, The DeviceOf type's constraint was tightened to K extends keyof DeviceByKind while DeviceKind was widened to include (string & {}), causing code that used DeviceOf<DeviceKind> to break; update the DeviceOf documentation and examples to reflect the new constraint by changing the `@typeParam` text to reference keyof DeviceByKind, add a new generic example showing usage like function processDevice<K extends keyof DeviceByKind>(kind: K, device: DeviceOf<K>) { ... }, and add a note to the CHANGELOG calling out the breaking change for callers using DeviceOf<DeviceKind> so they can migrate to DeviceOf<keyof DeviceByKind>.
🧹 Nitpick comments (2)
packages/oidc/src/NativeRNPingOidc.ts (1)
121-129: 💤 Low valueConsider adding a test reset helper for consistency.
Other native modules in this PR added
_resetNativeModuleForTesting()exports to allow tests to clear the module-level cache. Adding a similar helper here would maintain consistency and improve testability.♻️ Suggested addition
let _nativeModule: Spec | null = null; export function getNativeModule(): Spec { if (_nativeModule) return _nativeModule; // ... rest of function } + +/** + * Reset cached native module for testing. + * `@internal` + */ +export function _resetNativeModuleForTesting(): void { + _nativeModule = null; +}🤖 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/src/NativeRNPingOidc.ts` around lines 121 - 129, Add a test helper that clears the module-level cache by exporting a function named _resetNativeModuleForTesting which sets the module variable _nativeModule to null; locate the module-level variable _nativeModule and the getNativeModule function and implement _resetNativeModuleForTesting() to assign null to _nativeModule so tests can reset state between runs.packages/oidc/android/src/main/java/com/pingidentity/rnoidc/RNPingOidcCommon.kt (1)
689-697: 💤 Low valueConsider documenting disposal order expectations.
These disposal methods remove registry entries without checking for dependent web clients. If a client is disposed while web clients derived from it still exist, those web clients may fail when attempting parent-client fallback (e.g., during token operations). While the fallback logic handles missing parents gracefully by returning "No authenticated user" errors, the expected disposal order (web clients first, then parent client) should be documented to prevent confusion.
🤖 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 689 - 697, Add documentation and/or a log comment near the disposeClient and disposeWebClient functions clarifying the expected disposal order: web clients (webRegistry entries) should be disposed before their parent native clients (clientRegistry entries) because web clients may attempt parent-client fallback for token operations and will surface "No authenticated user" errors if the parent is gone; update the doc comment for fun disposeClient(clientId: String, promise: Promise) and fun disposeWebClient(webClientId: String, promise: Promise) to state this ordering and the rationale so callers know to call disposeWebClient first or handle missing parents explicitly.
🤖 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 @.github/workflows/release.yml:
- Around line 93-97: The regex literal in the NOTES node script treats
`${VERSION}` as plain text so the changelog section never matches; update the
log.match call inside the NOTES assignment to build the regex via the RegExp
constructor (or a template string passed to RegExp) so the VERSION variable is
interpolated, and ensure the newline and character-class tokens (like \n and
[\s\S]) are properly escaped in the constructed pattern to preserve the original
intent and fallback behavior.
In `@packages/binding/src/NativeRNPingBinding.ts`:
- Around line 146-151: The error message in NativeRNPingBinding.ts
unconditionally appends JSON.stringify(Object.keys(NativeModules)) (via the
availableModules variable) which leaks module inventory; change the construction
of availableModules in the RNPingBinding error path so it only includes the
module list when __DEV__ is true (or a redacted placeholder in production).
Locate the throw in NativeRNPingBinding (where availableModules is built and
thrown when RNPingBinding is not found) and wrap the module-list generation in
an __DEV__ conditional (or replace with a fixed redaction string) so production
bundles never include the full NativeModules list.
In `@packages/external-idp/src/NativeRNPingExternalIdp.ts`:
- Around line 97-98: The error message currently builds availableModules by
unconditionally calling JSON.stringify(Object.keys(NativeModules)), leaking
native module names to production; wrap that construction in a __DEV__ guard so
availableModules is only populated when __DEV__ is true (otherwise set to an
empty string or a minimal non-sensitive placeholder) and use that guarded value
in the thrown error inside NativeRNPingExternalIdp (the variable
availableModules and the place where the error is thrown should be updated
accordingly).
In `@packages/oath/src/NativeRNPingOath.ts`:
- Around line 154-160: Restore the development-only guard around the
NativeModules diagnostic in NativeRNPingOath.ts: change the construction of
availableModules (used when throwing the Error that references RNPingOath) so
that Object.keys(NativeModules) is only serialized and appended when __DEV__ is
true; in non-dev builds append a generic, non-sensitive hint instead. Locate the
availableModules variable and the Error throw in NativeRNPingOath (the
RNPingOath not found error) and apply the same __DEV__-guarded pattern as in the
other two files.
In `@packages/oidc/src/NativeRNPingOidc.ts`:
- Around line 137-142: The error currently builds and includes availableModules
(JSON.stringify(Object.keys(NativeModules))) unconditionally when throwing the
'RNPingOidc not found' Error, which leaks native module names; change the logic
around the availableModules constant and the thrown Error in NativeRNPingOidc
(the throw new Error(...) block and the availableModules variable) so that the
detailed diagnostic is only generated and appended when __DEV__ is true,
otherwise append a generic hint (e.g. "rebuild/relink the app") without listing
NativeModules; ensure you reference NativeModules and availableModules only
inside the __DEV__ branch so production builds never include the module list.
In `@packages/storage/src/NativeRNPingStorage.ts`:
- Around line 463-464: The error message currently constructs availableModules
by unconditionally serializing Object.keys(NativeModules) (the availableModules
constant), which leaks native module names to production telemetry; wrap the
enumeration in a __DEV__ check so availableModules includes the
JSON.stringify(Object.keys(NativeModules)) only when __DEV__ is true (and use an
empty string or a minimal placeholder when false), updating the code that builds
availableModules and any thrown error that references it (look for the
availableModules constant and throw/Error creation in NativeRNPingStorage) to
prevent exposing module names in production.
---
Outside diff comments:
In `@packages/device-client/src/types/device.types.ts`:
- Around line 189-199: The DeviceOf type's constraint was tightened to K extends
keyof DeviceByKind while DeviceKind was widened to include (string & {}),
causing code that used DeviceOf<DeviceKind> to break; update the DeviceOf
documentation and examples to reflect the new constraint by changing the
`@typeParam` text to reference keyof DeviceByKind, add a new generic example
showing usage like function processDevice<K extends keyof DeviceByKind>(kind: K,
device: DeviceOf<K>) { ... }, and add a note to the CHANGELOG calling out the
breaking change for callers using DeviceOf<DeviceKind> so they can migrate to
DeviceOf<keyof DeviceByKind>.
In `@packages/device-profile/src/NativeRNPingDeviceProfile.ts`:
- Around line 52-70: getNativeModule currently builds and throws an error each
call but lacks the module-level cache used elsewhere; add a module-scoped cache
(e.g., let _nativeModule: Spec | null = null) and have getNativeModule return
_nativeModule when set, otherwise locate TurboModuleRegistry or
NativeModules.RNPingDeviceProfileClassic, assign the found module to
_nativeModule and return it; also mirror the other files' behavior by only
including the available NativeModules list in the error when __DEV__ is true so
production calls don't leak that info.
In `@packages/logger/src/NativeRNPingLogger.ts`:
- Around line 57-75: getNativeModule currently always queries
TurboModuleRegistry/NativeModules and throws if not found; add the same
module-level caching used in NativeRNPingOath.ts by introducing a file-scoped
_nativeModule variable and returning it when set to avoid repeated lookups, and
add the __DEV__ guard around the list of available modules string construction
as done in other NativeRNPing*.ts files; update getNativeModule to set
_nativeModule to the found turbo or classic module before returning and reuse
_nativeModule on subsequent calls.
In `@packages/storage/src/NativeRNPingStorage.ts`:
- Around line 452-470: getNativeModule currently probes
TurboModuleRegistry/NativeModules on every call; add a module-level cache (e.g.,
a variable named _nativeModule: Spec | undefined | null) and update
getNativeModule to return _nativeModule if set, otherwise resolve the native
module from TurboModuleRegistry or NativeModules, assign it to _nativeModule,
and then return it; ensure the thrown Error remains unchanged if resolution
fails. This mirrors the caching pattern used in other files such as
NativeRNPingExternalIdp.ts and prevents repeated lookups.
---
Nitpick comments:
In
`@packages/oidc/android/src/main/java/com/pingidentity/rnoidc/RNPingOidcCommon.kt`:
- Around line 689-697: Add documentation and/or a log comment near the
disposeClient and disposeWebClient functions clarifying the expected disposal
order: web clients (webRegistry entries) should be disposed before their parent
native clients (clientRegistry entries) because web clients may attempt
parent-client fallback for token operations and will surface "No authenticated
user" errors if the parent is gone; update the doc comment for fun
disposeClient(clientId: String, promise: Promise) and fun
disposeWebClient(webClientId: String, promise: Promise) to state this ordering
and the rationale so callers know to call disposeWebClient first or handle
missing parents explicitly.
In `@packages/oidc/src/NativeRNPingOidc.ts`:
- Around line 121-129: Add a test helper that clears the module-level cache by
exporting a function named _resetNativeModuleForTesting which sets the module
variable _nativeModule to null; locate the module-level variable _nativeModule
and the getNativeModule function and implement _resetNativeModuleForTesting() to
assign null to _nativeModule so tests can reset state between runs.
🪄 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: 7503375d-8195-4035-92e4-db3cfc28f012
⛔ Files ignored due to path filters (3)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzPingSampleApp/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (112)
.github/workflows/release.ymlCONTRIBUTING.mdPingSampleApp/android/settings.gradlePingSampleApp/ios/PingSampleApp.xcodeproj/project.pbxprojPingSampleApp/src/hooks/useDevices.tsPingSampleApp/ui/DevicesScreen.tsxPingSampleApp/ui/devices/components/molecules/DeviceRow.tsxPingSampleApp/ui/devices/components/organisms/DeviceListCard.tsxPingSampleApp/ui/devices/types.tsPingSampleApp/ui/journey/components/organisms/JourneyContinuePanel.tsxPingSampleApp/ui/journey/hooks/useJourneyClientPanelController.tsPingSampleApp/ui/oath/components/organisms/OathAccountDetailModal.tsxPingTestRunner/scenarios/UseOidcScenario.tsxREADME.mdpackages/binding/LICENSEpackages/binding/README.mdpackages/binding/TODOS.mdpackages/binding/android/build.gradlepackages/binding/ios/RNPingBindingImpl.swiftpackages/binding/src/NativeRNPingBinding.tspackages/binding/src/types/binding.types.tspackages/browser/LICENSEpackages/browser/README.mdpackages/browser/RNPingBrowser.podspecpackages/browser/android/build.gradlepackages/browser/ios/BrowserLaunching.swiftpackages/browser/ios/DefaultBrowserLauncherAdapter.swiftpackages/browser/ios/RNPingBrowserCommon.swiftpackages/browser/ios/Tests/RNPingBrowserCommonTests.swiftpackages/browser/src/NativeRNPingBrowser.tspackages/browser/src/index.tsxpackages/browser/src/types/browser.types.tspackages/core/LICENSEpackages/core/README.mdpackages/core/android/build.gradlepackages/core/android/src/main/java/com/pingidentity/rncore/CoreRuntime.ktpackages/core/ios/CoreRuntime.swiftpackages/device-client/LICENSEpackages/device-client/README.mdpackages/device-client/android/build.gradlepackages/device-client/src/NativeRNPingDeviceClient.tspackages/device-client/src/createDeviceClient.tspackages/device-client/src/types/device.types.tspackages/device-client/src/types/error.types.tspackages/device-id/LICENSEpackages/device-id/README.mdpackages/device-id/android/build.gradlepackages/device-id/src/NativeRNPingDeviceId.tspackages/device-profile/LICENSEpackages/device-profile/README.mdpackages/device-profile/android/build.gradlepackages/device-profile/src/NativeRNPingDeviceProfile.tspackages/device-profile/src/index.tsxpackages/device-profile/src/types/deviceProfile.types.tspackages/external-idp/LICENSEpackages/external-idp/README.mdpackages/external-idp/android/build.gradlepackages/external-idp/src/NativeRNPingExternalIdp.tspackages/external-idp/src/__tests__/native-module.test.tsxpackages/external-idp/src/externalIdp.tspackages/fido/LICENSEpackages/fido/README.mdpackages/fido/android/build.gradlepackages/fido/src/NativeRNPingFido.tspackages/fido/src/index.tsxpackages/fido/src/types/fido.types.tspackages/journey/LICENSEpackages/journey/README.mdpackages/journey/android/build.gradlepackages/journey/src/NativeRNPingJourney.tspackages/journey/src/callbackHelpers.tspackages/journey/src/journey.tspackages/journey/src/types/error.types.tspackages/journey/src/types/form.types.tspackages/journey/src/useJourneyForm.tspackages/logger/LICENSEpackages/logger/README.mdpackages/logger/android/build.gradlepackages/logger/src/NativeRNPingLogger.tspackages/oath/LICENSEpackages/oath/README.mdpackages/oath/android/build.gradlepackages/oath/src/NativeRNPingOath.tspackages/oath/src/oath.tspackages/oath/src/types/oath.types.tspackages/oidc/LICENSEpackages/oidc/README.mdpackages/oidc/android/build.gradlepackages/oidc/android/src/main/java/com/pingidentity/rnoidc/RNPingOidcCommon.ktpackages/oidc/android/src/newarch/java/com/pingidentity/rnoidc/RNPingOidcModule.ktpackages/oidc/android/src/oldarch/java/com/pingidentity/rnoidc/RNPingOidcClassicModule.ktpackages/oidc/ios/RNPingOidcCommon.swiftpackages/oidc/ios/RNPingOidcImpl.swiftpackages/oidc/src/NativeRNPingOidc.tspackages/oidc/src/index.tsxpackages/oidc/src/types/oidc.types.tspackages/oidc/src/useOidc.tsxpackages/push/LICENSEpackages/push/README.mdpackages/push/android/build.gradlepackages/push/package.jsonpackages/push/src/NativeRNPingPush.tspackages/push/src/push.tspackages/push/src/types/config.types.tspackages/storage/LICENSEpackages/storage/README.mdpackages/storage/android/build.gradlepackages/storage/android/src/main/java/com/pingidentity/rnstorage/RNPingStorageCommon.ktpackages/storage/ios/RNPingStorageCommon.swiftpackages/storage/src/NativeRNPingStorage.tspackages/storage/src/index.tsxpackages/types/src/index.ts
💤 Files with no reviewable changes (5)
- packages/push/package.json
- packages/binding/TODOS.md
- packages/storage/android/src/main/java/com/pingidentity/rnstorage/RNPingStorageCommon.kt
- PingSampleApp/android/settings.gradle
- packages/storage/ios/RNPingStorageCommon.swift
| NOTES=$(node -e " | ||
| const fs = require('fs'); | ||
| const log = fs.readFileSync('packages/core/CHANGELOG.md', 'utf8'); | ||
| const match = log.match(/## ${VERSION}\n([\s\S]*?)(?=\n## |\$)/); | ||
| process.stdout.write(match ? match[1].trim() : 'See CHANGELOG.md for details.'); |
There was a problem hiding this comment.
Fix changelog extraction: ${VERSION} is not interpolated in the regex literal.
The current script treats ${VERSION} as literal text, so the version section match will fail and release notes will always fall back to the default string.
Suggested fix
- NOTES=$(node -e "
+ NOTES=$(VERSION=\"$VERSION\" node -e "
const fs = require('fs');
const log = fs.readFileSync('packages/core/CHANGELOG.md', 'utf8');
- const match = log.match(/## ${VERSION}\n([\s\S]*?)(?=\n## |\$)/);
+ const version = process.env.VERSION;
+ const escaped = version.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+ const match = log.match(new RegExp(`## ${escaped}\\n([\\s\\S]*?)(?=\\n## |$)`));
process.stdout.write(match ? match[1].trim() : 'See CHANGELOG.md for details.');
")📝 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.
| NOTES=$(node -e " | |
| const fs = require('fs'); | |
| const log = fs.readFileSync('packages/core/CHANGELOG.md', 'utf8'); | |
| const match = log.match(/## ${VERSION}\n([\s\S]*?)(?=\n## |\$)/); | |
| process.stdout.write(match ? match[1].trim() : 'See CHANGELOG.md for details.'); | |
| NOTES=$(VERSION=\"$VERSION\" node -e " | |
| const fs = require('fs'); | |
| const log = fs.readFileSync('packages/core/CHANGELOG.md', 'utf8'); | |
| const version = process.env.VERSION; | |
| const escaped = version.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| const match = log.match(new RegExp(`## ${escaped}\\n([\\s\\S]*?)(?=\\n## |$)`)); | |
| process.stdout.write(match ? match[1].trim() : 'See CHANGELOG.md for details.'); | |
| ") |
🤖 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 @.github/workflows/release.yml around lines 93 - 97, The regex literal in the
NOTES node script treats `${VERSION}` as plain text so the changelog section
never matches; update the log.match call inside the NOTES assignment to build
the regex via the RegExp constructor (or a template string passed to RegExp) so
the VERSION variable is interpolated, and ensure the newline and character-class
tokens (like \n and [\s\S]) are properly escaped in the constructed pattern to
preserve the original intent and fallback behavior.
| const availableModules = | ||
| '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)); | ||
| throw new Error( | ||
| '[@ping-identity/rn-binding] Native module RNPingBinding not found.\n' + | ||
| 'Ensure the library is linked correctly and the app has been rebuilt.\n' + | ||
| 'Available NativeModules: ' + | ||
| JSON.stringify(Object.keys(NativeModules)), | ||
| 'Ensure the library is linked correctly and the app has been rebuilt.' + | ||
| availableModules, |
There was a problem hiding this comment.
Root cause: native-module diagnostic strings are no longer environment-gated.
Across all listed files, JSON.stringify(Object.keys(NativeModules)) is appended unconditionally to thrown errors. This creates a production telemetry disclosure path for native module inventory. Apply a shared fix pattern: include module lists only when __DEV__ is true (or redact entirely in production).
🤖 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/binding/src/NativeRNPingBinding.ts` around lines 146 - 151, The
error message in NativeRNPingBinding.ts unconditionally appends
JSON.stringify(Object.keys(NativeModules)) (via the availableModules variable)
which leaks module inventory; change the construction of availableModules in the
RNPingBinding error path so it only includes the module list when __DEV__ is
true (or a redacted placeholder in production). Locate the throw in
NativeRNPingBinding (where availableModules is built and thrown when
RNPingBinding is not found) and wrap the module-list generation in an __DEV__
conditional (or replace with a fixed redaction string) so production bundles
never include the full NativeModules list.
| const availableModules = | ||
| '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)); |
There was a problem hiding this comment.
Restore __DEV__ guard around native module list in error messages.
Lines 97-98 unconditionally embed JSON.stringify(Object.keys(NativeModules)) in the thrown error. According to PR comment #1, the __DEV__ guard introduced earlier was removed in this PR, causing the full list of registered native module names to be included in production telemetry (Crashlytics, Sentry, etc.). This exposes internal implementation details that should not leak to production error reporting.
Either restore the __DEV__ guard to limit module enumeration to development builds, or document why unconditional inclusion is acceptable for this module.
🔒 Proposed fix to restore __DEV__ guard
- const availableModules =
- '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules));
+ const availableModules = __DEV__
+ ? '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules))
+ : '';
throw new Error(
'[`@ping-identity/rn-external-idp`] Native module RNPingExternalIdp not found.\n' +
'Ensure the library is linked correctly and the app has been rebuilt.' +
availableModules,
);🤖 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/external-idp/src/NativeRNPingExternalIdp.ts` around lines 97 - 98,
The error message currently builds availableModules by unconditionally calling
JSON.stringify(Object.keys(NativeModules)), leaking native module names to
production; wrap that construction in a __DEV__ guard so availableModules is
only populated when __DEV__ is true (otherwise set to an empty string or a
minimal non-sensitive placeholder) and use that guarded value in the thrown
error inside NativeRNPingExternalIdp (the variable availableModules and the
place where the error is thrown should be updated accordingly).
| const availableModules = | ||
| '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)); | ||
| throw new Error( | ||
| '[@ping-identity/rn-oath] Native module RNPingOath not found.\n' + | ||
| 'Ensure the library is linked correctly and the app has been rebuilt.\n' + | ||
| 'Available NativeModules: ' + | ||
| JSON.stringify(Object.keys(NativeModules)), | ||
| 'Ensure the library is linked correctly and the app has been rebuilt.' + | ||
| availableModules, | ||
| ); |
There was a problem hiding this comment.
Unconditional module list in error messages spans three files.
All three files serialize Object.keys(NativeModules) into error messages without a __DEV__ guard. When these errors are thrown in production, crash-reporting and telemetry systems (Crashlytics, Sentry, etc.) will capture the full list of registered native module names, exposing internal app architecture.
Per the reviewer's feedback, the __DEV__ guard that previously protected these diagnostics was removed in this PR. The shared root cause is the architectural decision to unconditionally include module diagnostics in production error messages. Either restore the guard across all three files or document why unconditional inclusion is acceptable.
🤖 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/oath/src/NativeRNPingOath.ts` around lines 154 - 160, Restore the
development-only guard around the NativeModules diagnostic in
NativeRNPingOath.ts: change the construction of availableModules (used when
throwing the Error that references RNPingOath) so that
Object.keys(NativeModules) is only serialized and appended when __DEV__ is true;
in non-dev builds append a generic, non-sensitive hint instead. Locate the
availableModules variable and the Error throw in NativeRNPingOath (the
RNPingOath not found error) and apply the same __DEV__-guarded pattern as in the
other two files.
| const availableModules = | ||
| '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)); | ||
| throw new Error( | ||
| '[@ping-identity/rn-oidc] Native module RNPingOidc not found.\n' + | ||
| 'Ensure the library is linked correctly and the app has been rebuilt.\n' + | ||
| 'Available NativeModules: ' + | ||
| JSON.stringify(Object.keys(NativeModules)), | ||
| 'Ensure the library is linked correctly and the app has been rebuilt.' + | ||
| availableModules, |
There was a problem hiding this comment.
Production telemetry exposes internal module list.
The error message unconditionally includes JSON.stringify(Object.keys(NativeModules)), which exposes the full list of registered native module names in production crash reports (Crashlytics, Sentry, etc.). This can leak internal architecture details. Per reviewer feedback, either restore the __DEV__ guard around this diagnostic, or document why unconditional inclusion is acceptable.
🔒 Suggested fix: guard diagnostic output with __DEV__
- const availableModules =
- '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules));
+ const availableModules = __DEV__
+ ? '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules))
+ : '';
throw new Error(
'[`@ping-identity/rn-oidc`] Native module RNPingOidc not found.\n' +
'Ensure the library is linked correctly and the app has been rebuilt.' +
availableModules,
);📝 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.
| const availableModules = | |
| '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)); | |
| throw new Error( | |
| '[@ping-identity/rn-oidc] Native module RNPingOidc not found.\n' + | |
| 'Ensure the library is linked correctly and the app has been rebuilt.\n' + | |
| 'Available NativeModules: ' + | |
| JSON.stringify(Object.keys(NativeModules)), | |
| 'Ensure the library is linked correctly and the app has been rebuilt.' + | |
| availableModules, | |
| const availableModules = __DEV__ | |
| ? '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)) | |
| : ''; | |
| throw new Error( | |
| '[`@ping-identity/rn-oidc`] Native module RNPingOidc not found.\n' + | |
| 'Ensure the library is linked correctly and the app has been rebuilt.' + | |
| availableModules, | |
| ); |
🤖 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/src/NativeRNPingOidc.ts` around lines 137 - 142, The error
currently builds and includes availableModules
(JSON.stringify(Object.keys(NativeModules))) unconditionally when throwing the
'RNPingOidc not found' Error, which leaks native module names; change the logic
around the availableModules constant and the thrown Error in NativeRNPingOidc
(the throw new Error(...) block and the availableModules variable) so that the
detailed diagnostic is only generated and appended when __DEV__ is true,
otherwise append a generic hint (e.g. "rebuild/relink the app") without listing
NativeModules; ensure you reference NativeModules and availableModules only
inside the __DEV__ branch so production builds never include the module list.
| const availableModules = | ||
| '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)); |
There was a problem hiding this comment.
Restore __DEV__ guard around native module list in error messages.
Lines 463-464 unconditionally embed JSON.stringify(Object.keys(NativeModules)) in the thrown error, exposing the full list of registered native module names to production telemetry (Crashlytics, Sentry, etc.). This is the same issue flagged in PR comment #1 for other modules.
Either restore the __DEV__ guard to limit module enumeration to development builds, or document why unconditional inclusion is acceptable for this module.
🔒 Proposed fix to restore __DEV__ guard
- const availableModules =
- '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules));
+ const availableModules = __DEV__
+ ? '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules))
+ : '';
throw new Error(
'[`@ping-identity/rn-storage`] Native module RNPingStorage not found.\n' +
'Ensure the library is linked correctly and the app has been rebuilt.' +
availableModules,
);📝 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.
| const availableModules = | |
| '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)); | |
| const availableModules = __DEV__ | |
| ? '\nAvailable NativeModules: ' + JSON.stringify(Object.keys(NativeModules)) | |
| : ''; |
🤖 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/storage/src/NativeRNPingStorage.ts` around lines 463 - 464, The
error message currently constructs availableModules by unconditionally
serializing Object.keys(NativeModules) (the availableModules constant), which
leaks native module names to production telemetry; wrap the enumeration in a
__DEV__ check so availableModules includes the
JSON.stringify(Object.keys(NativeModules)) only when __DEV__ is true (and use an
empty string or a minimal placeholder when false), updating the code that builds
availableModules and any thrown error that references it (look for the
availableModules constant and throw/Error creation in NativeRNPingStorage) to
prevent exposing module names in production.
…e between tests 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.
Overall changes looks good. Left some comments
| ( | ||
| overrides: Partial<JourneyFormValues> = {}, | ||
| ): JourneyBuildNextInputResult => { | ||
| return buildNextInput(node, { |
There was a problem hiding this comment.
Not related to this PR. Should we be forwarding options.handledCallbackTypes as the third argument to buildNextInput?
| export type NativeExtensionCallbackType = | ||
| (typeof nativeExtensionCallbackType)[keyof typeof nativeExtensionCallbackType]; | ||
|
|
||
| import { callbackType } from '@forgerock/sdk-types'; |
There was a problem hiding this comment.
Move to the top of the file?
|
|
||
| // Intentionally no Activity sync here; Ping SDK manages its own context. | ||
|
|
||
| fun disposeClient(clientId: String, promise: Promise) { |
There was a problem hiding this comment.
It seems disposeClient/disposeWebClient bypass coroutine scope. All other operations dispatch via scope.launch(Dispatchers.IO), but here it runs synchronously on the bridge thread. Worth verifying remove() is thread-safe or adding a comment.
| loggerInstance.debug('OIDC hasUser false'); | ||
| return null; | ||
| }, | ||
| dispose: async () => { |
There was a problem hiding this comment.
Do we need to clean up loggerRegistry here? Disposing an OidcClient above removes its logger, but the OidcWebClient instances silently fall back to noopLogger.
| throw OidcError.from(error); | ||
| } | ||
| }, | ||
| dispose: async () => { |
There was a problem hiding this comment.
Should we add test coverage for the dispose createOidcClient().dispose() and createOidcWebClient().dispose()?
3 participants
Summary
Pre-1.0 code quality consolidation across all SDK packages. Resolves 25+ identified issues from a full codebase audit.
SDK changes:
2.0.0→2.0.1| (string & {})for semver safetynoopLoggerextracted to@ping-identity/rn-types, removed from 11 packagesOidcClient/OidcWebClient— adddispose()to deregister from CoreRuntime registries (iOS + Android)journeyCallbackTypemerged constant added to@ping-identity/rn-typesuseJourneyForm— addJourneyFormOptions.handledCallbackTypessocanSubmitworks correctly for integration-required nodesNativeRNPing*.tsmodules —__DEV__guard on module list + module-level cachingBrowserLauncher.launch()(matches Android)rn-storagefully removed as a dependency; usePushStorageHandlefromrn-typesRelease automation:
release.yml— auto-create GitHub Release with changelog notes on tag pushSample app:
useMemo(() => logger(...), []))JourneyContinuePanel—submitDisabled = loading || !form.canSubmit; blocking state derived fromform.issues<></>→nullfixDocumentation:
## Licensefooter, npm install steps confirmedconfigure*functionsuseJourneyForm+handledCallbackTypesexamplesyarn typecheckandyarn lintscriptsTest plan
yarn workspaces foreach --all --parallel run typecheck— 30/30 passyarn packages:build— 15/15 passSummary by CodeRabbit
Release Notes
New Features
useJourneyFormwith options to exclude pre-handled integrations from blocking form submission.Bug Fixes
Documentation
Refactor
Chores