feat(all): SDKS-5019 code quality and pre-1.0 consolidation#49
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>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 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 Suggested reviewers
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
…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
|
Hey @george-bafaloukas-forgerock — thanks for the thorough review! Addressed all three points: 1. We removed the guard to keep all 13 2. Module-level caching not applied to Intentional — these three use 3. Good catch. Updated the |
- release.yml: fix VERSION interpolation using env var + split-based changelog extraction - device-client: update DeviceOf JSDoc — constraint is keyof DeviceByKind not DeviceKind - external-idp: add TODO(SDKS-separate-ticket) explaining __DEV__ guard removal - external-idp/README: remove IdPCallback (normalised to IdpCallback by journey bridge) - oidc: revert noopLogger to local const to fix Jest ESM issue; add dispose tests - oidc/android: add thread-safety comment on disposeClient/disposeWebClient - oidc/index: clarify loggerRegistry ownership in OidcWebClient.dispose() - storage/logger/device-profile NativeRNPing*.ts: add @remarks explaining intentional no-cache - types/index.ts: move callbackType import to top of file Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 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
New Features
Bug Fixes
Documentation
Refactor
Chores