feat(rn-protect,rn-davinci): add PingOne Protect React Native bridge (SDKS-5129) - #61
feat(rn-protect,rn-davinci): add PingOne Protect React Native bridge (SDKS-5129)#61pingidentity-gaurav wants to merge 3 commits into
Conversation
…(SDKS-5129) Introduces @ping-identity/rn-protect — a new React Native package that bridges the native PingOne Protect SDK on iOS and Android, plus wires a PROTECT collector into the DaVinci flow via daVinci.collectProtect(). Key changes: - New packages/protect package: startProtect(), pauseBehavioralData(), resumeBehavioralData() standalone functions with dual-arch bridge (TurboModule + classic) on both platforms - rn-davinci: collectProtect() on DaVinciClient, PROTECT collector mapping and serialization on both platforms, modules.protect logger support, ProtectLifecyclePayload.loggerId parsing - PingSampleApp: modules.protect wired into sampleDaVinciConfig; useDaVinciClientPanelController uses davinciClient.collectProtect() directly; DaVinci debug panel gated behind DAVINCI_SHOW_DEBUG_PANEL env flag (default false) - JS and native unit tests for all new code; integration tests added to PingTestRunner and wired into CI (js-unit-tests.yml) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (74.24%) 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 #61 +/- ##
============================================
- Coverage 71.70% 71.20% -0.51%
- Complexity 187 216 +29
============================================
Files 161 167 +6
Lines 18890 19706 +816
Branches 674 729 +55
============================================
+ Hits 13546 14032 +486
- Misses 5271 5590 +319
- Partials 73 84 +11
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
📝 WalkthroughWalkthroughThe pull request adds ChangesPingOne Protect integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SampleApp
participant DaVinciClient
participant NativeDaVinci
participant ProtectSDK
SampleApp->>DaVinciClient: collectProtect(index)
DaVinciClient->>NativeDaVinci: forward DaVinci ID and options
NativeDaVinci->>ProtectSDK: collect indexed PROTECT collector
ProtectSDK-->>NativeDaVinci: resolve or reject collection
NativeDaVinci-->>DaVinciClient: return collection result
DaVinciClient-->>SampleApp: continue submission or expose protectError
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts (1)
180-193: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize collection and submission.
loadingis checked beforeonProtectCollect, but this code sets no local in-flight guard whilecollectProtectis awaiting. Rapid taps can start multiple collection loops and then callnextmultiple times. Add a ref or state guard around the complete collection-plus-submit operation and clear it infinally.🤖 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 `@PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts` around lines 180 - 193, Update onSubmit to use a local in-flight ref or state guard covering both onProtectCollect and next, setting it before starting the asynchronous operation and returning early when already active. Clear the guard in a finally handler so it resets after success or failure, and include any new dependencies required by the hook.
🟡 Minor comments (7)
packages/protect/src/__tests__/index.test.tsx-113-120 (1)
113-120: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThree tests claim to verify
ProtectErrorwrapping but assert only the message text. Each test rejects the native mock withnew Error('PROTECT_INITIALIZE_ERROR')and then assertsrejects.toThrow('PROTECT_INITIALIZE_ERROR'). The unwrapped native error satisfies that assertion, so the tests pass even when no wrapping occurs. Assert thetype,error, andmessagefields required by the repository error contract at each site.
packages/protect/src/__tests__/index.test.tsx#L113-L120: assert the rejected value fromstartProtectexposestype,error, andmessage.packages/protect/src/__tests__/index.test.tsx#L194-L204: assert the rejected value frompauseBehavioralDataexposestype,error, andmessage.packages/protect/src/__tests__/index.test.tsx#L238-L248: assert the rejected value fromresumeBehavioralDataexposestype,error, andmessage.As per coding guidelines: "Use
GenericErrorwithtype,error, andmessage".🤖 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/protect/src/__tests__/index.test.tsx` around lines 113 - 120, Strengthen the error-wrapping assertions in the three tests for startProtect, pauseBehavioralData, and resumeBehavioralData at packages/protect/src/__tests__/index.test.tsx lines 113-120, 194-204, and 238-248. Capture each rejected value and assert its GenericError contract fields type, error, and message, rather than checking only the native error message; all three sites require direct test updates.Source: Coding guidelines
packages/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.kt-790-808 (1)
790-808: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe
resolvedFormFieldTypenil-result tests use indistinguishable fixtures on both platforms. Each platform declares two tests, "field missing" and "no form present", but both build the same node input. As a result the branch where a form exists and does not contain the collector key is untested on either platform.
packages/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.kt#L790-L808: the single-argumentmakeNodehelper at lines 45-48 injects{"form": {}}, so line 805 also has a form. ChangeresolvedFormFieldTypeReturnsNullWhenFieldMissingto build a form whosefieldsarray omits the collector key, and changeresolvedFormFieldTypeReturnsNullWhenNoFormPresentto pass an input object without theformkey.packages/davinci/ios/Tests/DaVinciNodeMapperTests.swift#L707-L719: both tests passinput: [:]. ChangetestResolvedFormFieldTypeReturnsNilWhenFieldMissingto supply a form with afieldsarray that omits the collector key, and keepinput: [:]only for the no-form test.🤖 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/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.kt` around lines 790 - 808, The nil-result tests currently use indistinguishable fixtures, leaving the existing-form/missing-field branch untested. In packages/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.kt:790-808, update resolvedFormFieldTypeReturnsNullWhenFieldMissing to provide a form whose fields omit the collector key, while resolvedFormFieldTypeReturnsNullWhenNoFormPresent must use input without form; make the equivalent changes in packages/davinci/ios/Tests/DaVinciNodeMapperTests.swift:707-719 for testResolvedFormFieldTypeReturnsNilWhenFieldMissing and keep input: [:] only in the no-form test.packages/protect/android/src/test/java/com/pingidentity/rnprotect/RNPingProtectTest.kt-85-108 (1)
85-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThese tests pass on a rejection.
Both tests assert
promise.await()andassertNull(promise.rejectCode).TestPromise.reject(throwable)at line 321 counts the latch down and sets onlyrejectThrowable, leavingrejectCodenull. A rejection through that overload therefore satisfies both assertions, and the test does not detect the failure.Assert that the promise resolved.
🐛 Proposed fix
assertTrue(promise.await()) assertNull(promise.rejectCode) + assertNull(promise.rejectThrowable) + assertNull(promise.rejectUserInfo)Apply the same assertions to
resumeBehavioralDataResolvesSuccessfully.🤖 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/protect/android/src/test/java/com/pingidentity/rnprotect/RNPingProtectTest.kt` around lines 85 - 108, Strengthen both pauseBehavioralDataResolvesSuccessfully and resumeBehavioralDataResolvesSuccessfully to assert that TestPromise was resolved, not merely completed without a rejectCode. Use the promise’s resolved-state assertion or equivalent existing TestPromise field, while retaining the await and rejection checks.PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts-169-176 (1)
169-176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear
protectErrorbefore each collection retry.A failed collection sets
protectError. A later successful collection does not clear it; onlyonStartclears the value. The controller can therefore expose a stale error after a successful retry. Clear the error before the firstcollectProtectcall.Suggested change
try { + setProtectError(null); for (let index = 0; index < protectFields.length; index++) {🤖 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 `@PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts` around lines 169 - 176, In the collection retry flow containing the protectFields loop, clear the existing protectError state immediately before the first collectProtect call. Keep the current catch behavior that records failures and rethrows, so a successful retry leaves no stale error visible.packages/protect/ios/RNPingProtectCommon.swift-203-215 (1)
203-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a dedicated behavioral-data error code for pause and resume failures. Both native platforms currently emit
PROTECT_INITIALIZE_ERROR. AddPROTECT_BEHAVIORAL_DATA_ERRORto the Swift enum, JSProtectErrorCode, and AndroidProtectErrorCodes, then use it for both methods.🤖 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/protect/ios/RNPingProtectCommon.swift` around lines 203 - 215, Replace the initialization error code used by the pause/resume behavioral-data failure paths with a new dedicated PROTECT_BEHAVIORAL_DATA_ERROR value. Add the corresponding value to the Swift error enum, JavaScript ProtectErrorCode, and Android ProtectErrorCodes, then update both pauseBehavioralData and resumeBehavioralData handlers to use it while preserving existing error handling.packages/protect/CHANGELOG.md-7-7 (1)
7-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the release-note API name.
The release note advertises
createProtectClient().collectForDaVinci(...). The new public DaVinci API isdaVinci.collectProtect(). List the exported Protect lifecycle functions separately if this entry must cover the full initial release.🤖 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/protect/CHANGELOG.md` at line 7, Update the initial-release entry in CHANGELOG.md to advertise the public DaVinci API as daVinci.collectProtect() instead of createProtectClient().collectForDaVinci(...). If documenting the complete initial release, list the exported Protect lifecycle functions separately.packages/davinci/src/types/node.types.ts-360-362 (1)
360-362: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the supported collection API.
createProtectClient().collectForDaVinci(...)does not match the API described by this PR. Documentawait daVinci.collectProtect()instead. This prevents consumers from implementing a nonexistent integration path.🤖 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/davinci/src/types/node.types.ts` around lines 360 - 362, Update the DaVinci collection documentation in the nearby node type comments to reference await daVinci.collectProtect() as the supported API, replacing createProtectClient().collectForDaVinci(daVinci). Keep the guidance to invoke it before daVinci.next({}) unchanged.
🧹 Nitpick comments (15)
packages/protect/ios/Tests/RNPingProtectImplTests.swift (2)
81-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport an unexpected resolve as a test failure.
When
collectForDaVinciresolves, the helper returns"UNEXPECTED_RESOLVE"in the code position. The calling test then fails on a code mismatch, which hides the real cause. CallXCTFail("Expected rejection, got resolve")in theresolveclosure before resuming the continuation.🤖 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/protect/ios/Tests/RNPingProtectImplTests.swift` around lines 81 - 101, Update invokeCollectForDaVinci’s resolve closure to call XCTFail("Expected rejection, got resolve") before resuming the continuation, while preserving the existing continuation return behavior.
70-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
ErrorTypeenum instead of the raw string"auth_error".Line 76 compares against a string literal. Lines 43, 49, and 55 use
ErrorType.argumentError.rawValuefor the same field. If the raw value of the error type changes, this test keeps passing against a stale literal. Use the matchingErrorTypecase.🤖 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/protect/ios/Tests/RNPingProtectImplTests.swift` around lines 70 - 77, Update testCollectForDaVinciRejectsWhenCollectFails to compare type using the matching ErrorType enum case’s rawValue instead of the hard-coded "auth_error" string, consistent with the other tests.packages/davinci/android/src/test/java/com/pingidentity/rndavinci/RNPingDavinciCommonTest.kt (1)
529-541: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the out-of-range test distinct from the empty-collector test.
The node is created with
actions = emptyList(). Index 5 and index 0 therefore follow the identical branch. This test duplicatescollectProtect_rejectsWithStateErrorWhenNoProtectCollectorand does not verify index bounds. Populate the node with at least one Protect collector, then request index 5.Note: this depends on the Protect SDK being available at test runtime. See the comment on lines 543-557.
🤖 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/davinci/android/src/test/java/com/pingidentity/rndavinci/RNPingDavinciCommonTest.kt` around lines 529 - 541, Update collectProtect_rejectsWithStateErrorWhenIndexOutOfRange to create a DummyContinueNode containing at least one Protect collector before requesting index 5, so the test exercises index bounds rather than the empty-collector path. Follow the Protect SDK runtime setup noted near the adjacent test while preserving the existing rejection assertions.packages/davinci/ios/Tests/DaVinciClientFactoryTests.swift (1)
50-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the duplicate test with a populated
protectpayload case.
testBuildWithNullProtectPayloadDoesNotThrowis identical totestBuildSucceedsWithRequiredFieldsOnlyat lines 24-48. Both build the same payload withprotect: niland assert the same result.The Android suite covers both cases. See
packages/davinci/android/src/test/java/com/pingidentity/rndavinci/factory/DaVinciClientFactoryTest.ktlines 126-155, which tests a null payload and a populatedProtectLifecyclePayload. Change this test to supply a populated Protect payload so the iOS suite matches.🤖 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/davinci/ios/Tests/DaVinciClientFactoryTests.swift` around lines 50 - 74, Replace the duplicate nil-protect case in testBuildWithNullProtectPayloadDoesNotThrow with a populated ProtectLifecyclePayload, while keeping the build-and-non-nil assertion. Update only the protect input so this test covers the populated payload scenario alongside testBuildSucceedsWithRequiredFieldsOnly.packages/davinci/android/src/test/java/com/pingidentity/rndavinci/config/DaVinciConfigParserTest.kt (1)
131-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNeither config parser suite asserts the parsed Protect logger id.
ProtectLifecyclePayloadcarries aloggerIdfield, and the PR objective states that Protect logging falls back to the DaVinci logger when no dedicated logger is configured. That fallback depends on the parsed value, which no test covers on either platform.
packages/davinci/android/src/test/java/com/pingidentity/rndavinci/config/DaVinciConfigParserTest.kt#L131-L182: assertprotect.loggerIdis null inparseProtectDefaultsAreMapped, and assert the mapped value inparseProtectAllFieldsMappedafter adding aloggerIdentry to theprotectmap.packages/davinci/ios/Tests/DaVinciConfigParserTests.swift#L92-L140: assertprotect.loggerIdis nil intestParseProtectDefaultsWhenEmpty, and assert the mapped value intestParseProtectAllFieldsMappedafter adding aloggerIdentry to theprotectdictionary.🤖 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/davinci/android/src/test/java/com/pingidentity/rndavinci/config/DaVinciConfigParserTest.kt` around lines 131 - 182, Update the Protect parser tests in packages/davinci/android/src/test/java/com/pingidentity/rndavinci/config/DaVinciConfigParserTest.kt:131-182 to assert protect.loggerId is null by default and mapped when a loggerId entry is provided. Apply the same assertions in packages/davinci/ios/Tests/DaVinciConfigParserTests.swift:92-140, using nil for the empty configuration and adding loggerId to the populated protect dictionary.PingSampleApp/ui/davinci/components/molecules/DaVinciTextField.tsx (1)
28-28: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep multiline mode stable during editing.
isMultilineuses the live text length. This changesTextInput.multilinewhen the value crosses 60 characters and changes it back when the value falls below 61. Base the mode on collector metadata, or latch it for the current field, instead of changing the native input mode on every edit.Also applies to: 42-42
🤖 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 `@PingSampleApp/ui/davinci/components/molecules/DaVinciTextField.tsx` at line 28, Update the isMultiline calculation in DaVinciTextField so TextInput.multiline is latched for the current field or derived from stable collector metadata rather than recalculated from live stringValue length on every edit. Preserve the selected mode while the field is being edited, including when the text crosses the 60-character threshold in either direction.packages/davinci/src/davinci.ts (1)
417-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the
collectProtectoutcome at info level.Every other client method logs its success outcome with
logInfo.collectProtectlogs success withlogDebug. The coding guidelines require outcomes at info.♻️ Proposed change
async collectProtect(options?: { index?: number }) { const id = await ensureConfigured(); logDebug('DaVinci collectProtect requested', { davinciId: id }); try { await collectProtectForDaVinci(id, options ?? {}); - logDebug('DaVinci collectProtect succeeded', { davinciId: id }); + logInfo('DaVinci collectProtect succeeded', { davinciId: id }); } catch (error) {As per coding guidelines: "log entry at debug, outcomes at info, and failures at error".
🤖 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/davinci/src/davinci.ts` around lines 417 - 427, Update the success log in the collectProtect method to use logInfo instead of logDebug, while keeping the request entry at debug and failure logging at error.Source: Coding guidelines
packages/protect/src/index.tsx (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
export type *for the type re-exports.The index re-exports types with a named
export type { ... }list. The repository guideline requiresexport type *for types in a package index.♻️ Proposed change
-export type { - ProtectCollectOptions, - ProtectConfig, - ProtectErrorCode, -} from './types'; +export type * from './types';Confirm that
./typesdoes not re-export internal helpers before you widen the export.As per coding guidelines: "The package index must contain public re-exports only; use
export type *for types, named exports for values".🤖 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/protect/src/index.tsx` around lines 14 - 18, Update the package index type re-export near ProtectCollectOptions, ProtectConfig, and ProtectErrorCode to use export type * from './types' instead of a named type list. First confirm ./types exposes only public types and no internal helpers, preserving the package index’s public API boundary.Source: Coding guidelines
packages/protect/src/NativeRNPingProtect.ts (2)
70-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the TSDoc block onto
getNativeModule.The doc block on Lines 70-76 describes
getNativeModule, but it is attached to the_nativeModulevariable declaration.getNativeModuleon Line 82 has no doc comment. Move the block directly abovegetNativeModule.♻️ Proposed change
+let _nativeModule: Spec | null = null; + +/** `@internal` — resets the module cache for testing only. */ +export function _resetNativeModuleForTesting(): void { + _nativeModule = null; +} + /** * Resolves the native module by probing TurboModule first, then falling back to the classic bridge module. * Result is cached — the native module does not change at runtime. * * `@returns` Native module implementation for the current architecture. * `@throws` Error when no native module is registered. */ -let _nativeModule: Spec | null = null; -/** `@internal` — resets the module cache for testing only. */ -export function _resetNativeModuleForTesting(): void { - _nativeModule = null; -} export function getNativeModule(): Spec {As per coding guidelines: "Use TSDoc on all exported declarations".
🤖 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/protect/src/NativeRNPingProtect.ts` around lines 70 - 83, Move the existing TSDoc block from the _nativeModule declaration to directly above the exported getNativeModule function, leaving the cache variable and _resetNativeModuleForTesting documentation unchanged.Source: Coding guidelines
109-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the double casts with typed mapping functions.
toNativeCollectOptionsandtoNativeConfiguseas unknown as Record<string, unknown>. The double cast removes all type checking between the public types and the native payload.toNativeProtectConfigbelow already builds an explicit payload. Use the same explicit approach, or type the parameters as objects with index-compatible shapes.This is a suggestion, not a blocker.
🤖 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/protect/src/NativeRNPingProtect.ts` around lines 109 - 122, Replace the double casts in toNativeCollectOptions and toNativeConfig with explicit typed mapping functions that construct Record<string, unknown> payloads from the respective public types. Follow the existing toNativeProtectConfig pattern and preserve all supported option and config fields without bypassing type checking.packages/protect/ios/RNPingProtect.mm (1)
33-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the main-thread hop into one helper.
The four exported methods repeat the same
isMainThreadcheck anddispatch_asyncblock.RNPingProtectClassic.mmalready uses awithSwiftImpl:helper for the identical pattern. Use the same helper here to remove the duplication.♻️ Proposed refactor
-- (RNPingProtectImpl *)swiftImpl -{ - return [RNPingProtectImpl shared]; -} +- (void)withSwiftImpl:(void (^)(RNPingProtectImpl *impl))block +{ + if ([NSThread isMainThread]) { + block([RNPingProtectImpl shared]); + return; + } + dispatch_async(dispatch_get_main_queue(), ^{ + block([RNPingProtectImpl shared]); + }); +} - (void)collectForDaVinci:(NSString *)davinciId options:(NSDictionary *)options config:(NSDictionary *)config resolve:(RCTPromiseResolveBlock)resolve reject:(RCTPromiseRejectBlock)rejecter { - if ([NSThread isMainThread]) { - [[self swiftImpl] collectForDaVinci:davinciId options:options config:config resolve:resolve rejecter:rejecter]; - return; - } - - dispatch_async(dispatch_get_main_queue(), ^{ - [[self swiftImpl] collectForDaVinci:davinciId options:options config:config resolve:resolve rejecter:rejecter]; - }); + [self withSwiftImpl:^(RNPingProtectImpl *impl) { + [impl collectForDaVinci:davinciId options:options config:config resolve:resolve rejecter:rejecter]; + }]; }Apply the same change to
initialize,pauseBehavioralData, andresumeBehavioralData.As per coding guidelines: "Follow existing package patterns, SOLID, DRY".
🤖 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/protect/ios/RNPingProtect.mm` around lines 33 - 99, Extract the repeated main-thread dispatch logic from collectForDaVinci, initialize, pauseBehavioralData, and resumeBehavioralData into a shared withSwiftImpl: helper, matching the existing pattern in RNPingProtectClassic.mm. Update all four methods to invoke the helper while preserving their current Swift implementation calls and arguments.Source: Coding guidelines
packages/davinci/src/types/client.types.ts (1)
116-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the public
collectProtectAPI documentation.
packages/davinci/src/types/client.types.ts#L116-L137: Add an@returnstag for successful completion.packages/davinci/ios/RNPingDavinciCommon.swift#L419-L484: Add Returns, Throws, and Note sections.packages/davinci/ios/RNPingDavinciImpl.swift#L178-L193: Add Returns, Throws, and Note sections.As per coding guidelines, “Use
///documentation on all public and internal Swift declarations with Parameters, Returns, Throws, and Note sections,” and TypeScript exports require TSDoc return tags.🤖 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/davinci/src/types/client.types.ts` around lines 116 - 137, Complete the public collectProtect documentation: in packages/davinci/src/types/client.types.ts lines 116-137, add a TSDoc `@returns` tag describing successful completion; in packages/davinci/ios/RNPingDavinciCommon.swift lines 419-484 and packages/davinci/ios/RNPingDavinciImpl.swift lines 178-193, add /// Returns, Throws, and Note sections for the corresponding declarations, preserving the documented behavior that collection succeeds with no return value and throws when Protect is unavailable or collection fails.Source: Coding guidelines
packages/protect/android/src/main/java/com/pingidentity/rnprotect/RNPingProtectCommon.kt (1)
128-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the required logger contract.
Use debug logging when an operation starts. Use info logging when an operation succeeds. Log a failure at error level before the bridge rejects it. Resolve the configured logger to a module-level no-op logger when no logger ID is available.
As per coding guidelines, logger entry events use debug, outcomes use info, failures use error, and optional loggers default to a module-level noop logger.
Also applies to: 163-176, 193-198, 214-219
🤖 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/protect/android/src/main/java/com/pingidentity/rnprotect/RNPingProtectCommon.kt` around lines 128 - 144, The collectForDaVinci flow and the additionally referenced operation blocks must use the configured logger, falling back to the module-level no-op logger when no logger ID exists. Change operation-start logs to debug, successful completion logs to info, and collector failures to error before bridge rejection; preserve the existing rejection behavior and messages.Source: Coding guidelines
packages/protect/android/src/main/java/com/pingidentity/rnprotect/ProtectErrorCodes.kt (1)
9-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd structured KDoc to the new Kotlin declarations.
The new Kotlin files use prose-only comments. Add the required
@param,@return, and@throwssections where applicable.
packages/protect/android/src/main/java/com/pingidentity/rnprotect/ProtectErrorCodes.kt#L9-L18: document the public error-code declarations with the required KDoc structure.packages/protect/android/src/main/java/com/pingidentity/rnprotect/RNPingProtectCommon.kt#L30-L67: add structured KDoc for the shared object and internal configuration declarations.packages/protect/android/src/newarch/java/com/pingidentity/rnprotect/RNPingProtectModule.kt#L14-L74: add structured KDoc for the module and bridge methods.packages/protect/android/src/newarch/java/com/pingidentity/rnprotect/RNPingProtectPackage.kt#L16-L50: add structured KDoc for the package and React Native registration methods.As per coding guidelines, all public and internal Kotlin declarations require KDoc with
@param,@return, and@throws.🤖 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/protect/android/src/main/java/com/pingidentity/rnprotect/ProtectErrorCodes.kt` around lines 9 - 18, Apply structured KDoc to all public and internal Kotlin declarations covered by this comment: ProtectErrorCodes.kt lines 9-18, RNPingProtectCommon.kt lines 30-67, RNPingProtectModule.kt lines 14-74, and RNPingProtectPackage.kt lines 16-50. Document each applicable parameter, return value, and thrown exception with `@param`, `@return`, and `@throws` tags, including the error-code declarations and bridge/package methods; preserve the existing APIs and behavior.Source: Coding guidelines
packages/davinci/android/src/main/java/com/pingidentity/rndavinci/factory/DaVinciClientFactory.kt (1)
81-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required logger levels for Protect lifecycle events.
- When
ProtectLifecycleis unavailable, log the caughtNoClassDefFoundErrorat error level before skipping the module.- Keep the
collectProtectrequest log at debug level and change the successful collection log to info level.🤖 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/davinci/android/src/main/java/com/pingidentity/rndavinci/factory/DaVinciClientFactory.kt` around lines 81 - 83, Update the Protect lifecycle logging in DaVinciClientFactory.kt at lines 81-83 to log the caught NoClassDefFoundError at error level before skipping the module. In RNPingDavinciCommon.kt at lines 524-526, keep the collectProtect request log at debug level and change the successful collection log to info level.Source: Coding guidelines
🤖 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/davinci/android/src/main/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapper.kt`:
- Around line 173-174: Remove the unconditional PROTECT exclusion in the
mapper’s field-processing logic. Let PROTECT fields proceed to the existing
registeredKeys.contains(key) check so fields without an instantiated
ProtectCollector are reported through unsupportedFields, while fields with a
native collector continue through the normal mapping path.
In
`@packages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciCommon.kt`:
- Around line 541-551: Update parseCollectorIndex to default to 0 only when
options or the index key is absent; otherwise validate the supplied value as a
non-negative integer. Reject fractional numbers, malformed strings, negative
values, and unsupported types by throwing GenericError with
ErrorType.ARGUMENT_ERROR, rather than coercing them to 0 before collector
selection.
In `@packages/davinci/ios/Models/DaVinciPayloads.swift`:
- Around line 74-75: Update the configuration dictionary construction in
RNPingDavinci.mm to forward modules.protect through the TurboModule bridge,
using the parser field names expected by ProtectLifecyclePayload. Preserve the
existing field-by-field mapping and ensure DaVinciPayloads.protect receives the
mapped configuration instead of remaining nil.
In `@packages/davinci/ios/RNPingDavinciCommon.swift`:
- Around line 492-499: Update parseCollectorIndex and its caller to distinguish
an absent index from invalid input: accept only non-negative integer values,
reject invalid strings and NSNumber values that are not exact integers, and
propagate a GenericError of type argumentError before collector.collect() is
invoked. Preserve the default index of 0 only when the index is absent.
In `@packages/davinci/src/NativeRNPingDavinci.ts`:
- Around line 147-157: Update the exported collectProtect declaration’s TSDoc to
add an `@returns` tag documenting successful void resolution and an `@throws` tag
documenting rejection with DAVINCI_PROTECT_COLLECT_ERROR when the Protect SDK is
unavailable or collection fails.
In `@packages/protect/android/build.gradle`:
- Line 29: Change the PingOne Protect SDK dependency declaration from
compileOnly to implementation so Protect classes used by RNPingProtectCommon.kt
are packaged transitively for consuming apps. Keep the existing SDK version and
Android Gradle plugin configuration unchanged.
- Line 100: Document the required Android Protect SDK dependency in the package
installation instructions, specifying
implementation("com.pingidentity.sdks:protect:2.0.1") for consuming apps. Keep
the compileOnly declaration in the Android build configuration unchanged unless
the installation guidance is intentionally replaced by changing it to
implementation.
In
`@packages/protect/android/src/main/java/com/pingidentity/rnprotect/RNPingProtectCommon.kt`:
- Around line 318-324: Update parseCollectorIndex to accept only finite,
nonnegative integer values from the options map, rejecting nonnumeric strings,
fractional numbers, negative values, and non-finite numbers instead of
defaulting or truncating. For invalid input, throw GenericError with type
ARGUMENT_ERROR before resolving the native collector; preserve the existing
default only when the index is absent or explicitly null.
In
`@packages/protect/android/src/test/java/com/pingidentity/rnprotect/RNPingProtectTest.kt`:
- Around line 238-256: Move the public top-level ShadowProtectArguments object
and its createMap/createArray implementations from RNPingProtectTest.kt into a
same-package file named ShadowProtectArguments.kt. Keep the object public so
Robolectric can resolve it reflectively, and remove the duplicate declaration
from RNPingProtectTest.kt.
In `@packages/protect/ios/RNPingProtectCommon.swift`:
- Around line 78-84: Update the Task isolation handling around the bridge
methods so synchronous SDK work, including Protect.initialize(), Protect.data(),
Protect.pauseBehavioralData(), and Protect.resumeBehavioralData(), does not
execute on `@MainActor`. Move the blocking collection and initialization logic
into a nonisolated async helper, and update the Task closures to invoke that
helper while preserving PromiseBridge resolution and rejection behavior.
In `@packages/protect/ios/Tests/RNPingProtectCommonTests.swift`:
- Around line 88-116: Update testPauseBehavioralDataRejectsWhenSDKNotInitialized
and testResumeBehavioralDataRejectsWhenSDKNotInitialized to be async tests, and
replace blocking wait(for:timeout:) calls with await fulfillment(of:timeout:).
Preserve the existing rejection assertions and expectation behavior.
In `@packages/protect/src/protect.ts`:
- Around line 39-52: Add `@returns` documentation stating that the Promise
resolves to void on each public lifecycle API: startProtect,
pauseBehavioralData, and resumeBehavioralData. Update the TSDoc for each
exported declaration while preserving its existing parameter, throws, example,
and visibility tags.
- Around line 23-35: Update withLogging so the operation entry log uses
logger.debug instead of logger.info, and the successful outcome log uses
logger.info instead of logger.debug. Keep the existing failure logging and error
propagation unchanged.
In `@packages/protect/src/types/protect.types.ts`:
- Around line 114-123: Document the exported ProtectError constructor and static
from method with TSDoc. Include parameter descriptions for message, code, type,
optional status, and error, plus the from method’s returned ProtectError using
the applicable required tags; keep the implementation unchanged.
In `@packages/protect/turbo.json`:
- Around line 11-12: Update packages/protect/turbo.json at lines 11-12 and 32-33
to replace the non-recursive src/*.ts and src/*.tsx inputs in both platform task
hashes with the same recursive source pattern, ensuring nested files such as
src/types/protect.types.ts invalidate build:android and build:ios caches.
In `@PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts`:
- Around line 188-192: Update the catch following onProtectCollect in the
submission flow to accept the caught error and log it at error level through the
configured logger, preserving the hook’s existing error state. Ensure failures
from both onProtectCollect and next are no longer silently consumed.
- Around line 188-192: Ensure every submission path in the controller, including
onFlowAction-triggered auto-submits, runs onProtectCollect before calling
next(plan.input). Centralize this behavior in a shared Protect-aware submission
helper and use it from both onSubmit and onFlowAction, preserving the hook’s
existing error update behavior.
- Around line 160-164: Update onProtectCollect in the missing davinciClient path
to fail closed: set protectError and reject with a GenericError containing type,
error, and message instead of returning successfully. Ensure onSubmit cannot
call next when Protect collection is unavailable, and preserve the requirement
that every caught error is logged or rethrown.
In `@PingTestRunner/jest.setup.js`:
- Around line 286-293: Add the missing toNativeProtectConfig mock export to the
NativeRNPingProtect shared mock, matching the existing pass-through behavior of
toNativeConfig, so startProtect() can call it before initialize without a
TypeError.
---
Outside diff comments:
In `@PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts`:
- Around line 180-193: Update onSubmit to use a local in-flight ref or state
guard covering both onProtectCollect and next, setting it before starting the
asynchronous operation and returning early when already active. Clear the guard
in a finally handler so it resets after success or failure, and include any new
dependencies required by the hook.
---
Minor comments:
In
`@packages/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.kt`:
- Around line 790-808: The nil-result tests currently use indistinguishable
fixtures, leaving the existing-form/missing-field branch untested. In
packages/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.kt:790-808,
update resolvedFormFieldTypeReturnsNullWhenFieldMissing to provide a form whose
fields omit the collector key, while
resolvedFormFieldTypeReturnsNullWhenNoFormPresent must use input without form;
make the equivalent changes in
packages/davinci/ios/Tests/DaVinciNodeMapperTests.swift:707-719 for
testResolvedFormFieldTypeReturnsNilWhenFieldMissing and keep input: [:] only in
the no-form test.
In `@packages/davinci/src/types/node.types.ts`:
- Around line 360-362: Update the DaVinci collection documentation in the nearby
node type comments to reference await daVinci.collectProtect() as the supported
API, replacing createProtectClient().collectForDaVinci(daVinci). Keep the
guidance to invoke it before daVinci.next({}) unchanged.
In
`@packages/protect/android/src/test/java/com/pingidentity/rnprotect/RNPingProtectTest.kt`:
- Around line 85-108: Strengthen both pauseBehavioralDataResolvesSuccessfully
and resumeBehavioralDataResolvesSuccessfully to assert that TestPromise was
resolved, not merely completed without a rejectCode. Use the promise’s
resolved-state assertion or equivalent existing TestPromise field, while
retaining the await and rejection checks.
In `@packages/protect/CHANGELOG.md`:
- Line 7: Update the initial-release entry in CHANGELOG.md to advertise the
public DaVinci API as daVinci.collectProtect() instead of
createProtectClient().collectForDaVinci(...). If documenting the complete
initial release, list the exported Protect lifecycle functions separately.
In `@packages/protect/ios/RNPingProtectCommon.swift`:
- Around line 203-215: Replace the initialization error code used by the
pause/resume behavioral-data failure paths with a new dedicated
PROTECT_BEHAVIORAL_DATA_ERROR value. Add the corresponding value to the Swift
error enum, JavaScript ProtectErrorCode, and Android ProtectErrorCodes, then
update both pauseBehavioralData and resumeBehavioralData handlers to use it
while preserving existing error handling.
In `@packages/protect/src/__tests__/index.test.tsx`:
- Around line 113-120: Strengthen the error-wrapping assertions in the three
tests for startProtect, pauseBehavioralData, and resumeBehavioralData at
packages/protect/src/__tests__/index.test.tsx lines 113-120, 194-204, and
238-248. Capture each rejected value and assert its GenericError contract fields
type, error, and message, rather than checking only the native error message;
all three sites require direct test updates.
In `@PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts`:
- Around line 169-176: In the collection retry flow containing the protectFields
loop, clear the existing protectError state immediately before the first
collectProtect call. Keep the current catch behavior that records failures and
rethrows, so a successful retry leaves no stale error visible.
---
Nitpick comments:
In
`@packages/davinci/android/src/main/java/com/pingidentity/rndavinci/factory/DaVinciClientFactory.kt`:
- Around line 81-83: Update the Protect lifecycle logging in
DaVinciClientFactory.kt at lines 81-83 to log the caught NoClassDefFoundError at
error level before skipping the module. In RNPingDavinciCommon.kt at lines
524-526, keep the collectProtect request log at debug level and change the
successful collection log to info level.
In
`@packages/davinci/android/src/test/java/com/pingidentity/rndavinci/config/DaVinciConfigParserTest.kt`:
- Around line 131-182: Update the Protect parser tests in
packages/davinci/android/src/test/java/com/pingidentity/rndavinci/config/DaVinciConfigParserTest.kt:131-182
to assert protect.loggerId is null by default and mapped when a loggerId entry
is provided. Apply the same assertions in
packages/davinci/ios/Tests/DaVinciConfigParserTests.swift:92-140, using nil for
the empty configuration and adding loggerId to the populated protect dictionary.
In
`@packages/davinci/android/src/test/java/com/pingidentity/rndavinci/RNPingDavinciCommonTest.kt`:
- Around line 529-541: Update
collectProtect_rejectsWithStateErrorWhenIndexOutOfRange to create a
DummyContinueNode containing at least one Protect collector before requesting
index 5, so the test exercises index bounds rather than the empty-collector
path. Follow the Protect SDK runtime setup noted near the adjacent test while
preserving the existing rejection assertions.
In `@packages/davinci/ios/Tests/DaVinciClientFactoryTests.swift`:
- Around line 50-74: Replace the duplicate nil-protect case in
testBuildWithNullProtectPayloadDoesNotThrow with a populated
ProtectLifecyclePayload, while keeping the build-and-non-nil assertion. Update
only the protect input so this test covers the populated payload scenario
alongside testBuildSucceedsWithRequiredFieldsOnly.
In `@packages/davinci/src/davinci.ts`:
- Around line 417-427: Update the success log in the collectProtect method to
use logInfo instead of logDebug, while keeping the request entry at debug and
failure logging at error.
In `@packages/davinci/src/types/client.types.ts`:
- Around line 116-137: Complete the public collectProtect documentation: in
packages/davinci/src/types/client.types.ts lines 116-137, add a TSDoc `@returns`
tag describing successful completion; in
packages/davinci/ios/RNPingDavinciCommon.swift lines 419-484 and
packages/davinci/ios/RNPingDavinciImpl.swift lines 178-193, add /// Returns,
Throws, and Note sections for the corresponding declarations, preserving the
documented behavior that collection succeeds with no return value and throws
when Protect is unavailable or collection fails.
In
`@packages/protect/android/src/main/java/com/pingidentity/rnprotect/ProtectErrorCodes.kt`:
- Around line 9-18: Apply structured KDoc to all public and internal Kotlin
declarations covered by this comment: ProtectErrorCodes.kt lines 9-18,
RNPingProtectCommon.kt lines 30-67, RNPingProtectModule.kt lines 14-74, and
RNPingProtectPackage.kt lines 16-50. Document each applicable parameter, return
value, and thrown exception with `@param`, `@return`, and `@throws` tags, including
the error-code declarations and bridge/package methods; preserve the existing
APIs and behavior.
In
`@packages/protect/android/src/main/java/com/pingidentity/rnprotect/RNPingProtectCommon.kt`:
- Around line 128-144: The collectForDaVinci flow and the additionally
referenced operation blocks must use the configured logger, falling back to the
module-level no-op logger when no logger ID exists. Change operation-start logs
to debug, successful completion logs to info, and collector failures to error
before bridge rejection; preserve the existing rejection behavior and messages.
In `@packages/protect/ios/RNPingProtect.mm`:
- Around line 33-99: Extract the repeated main-thread dispatch logic from
collectForDaVinci, initialize, pauseBehavioralData, and resumeBehavioralData
into a shared withSwiftImpl: helper, matching the existing pattern in
RNPingProtectClassic.mm. Update all four methods to invoke the helper while
preserving their current Swift implementation calls and arguments.
In `@packages/protect/ios/Tests/RNPingProtectImplTests.swift`:
- Around line 81-101: Update invokeCollectForDaVinci’s resolve closure to call
XCTFail("Expected rejection, got resolve") before resuming the continuation,
while preserving the existing continuation return behavior.
- Around line 70-77: Update testCollectForDaVinciRejectsWhenCollectFails to
compare type using the matching ErrorType enum case’s rawValue instead of the
hard-coded "auth_error" string, consistent with the other tests.
In `@packages/protect/src/index.tsx`:
- Around line 14-18: Update the package index type re-export near
ProtectCollectOptions, ProtectConfig, and ProtectErrorCode to use export type *
from './types' instead of a named type list. First confirm ./types exposes only
public types and no internal helpers, preserving the package index’s public API
boundary.
In `@packages/protect/src/NativeRNPingProtect.ts`:
- Around line 70-83: Move the existing TSDoc block from the _nativeModule
declaration to directly above the exported getNativeModule function, leaving the
cache variable and _resetNativeModuleForTesting documentation unchanged.
- Around line 109-122: Replace the double casts in toNativeCollectOptions and
toNativeConfig with explicit typed mapping functions that construct
Record<string, unknown> payloads from the respective public types. Follow the
existing toNativeProtectConfig pattern and preserve all supported option and
config fields without bypassing type checking.
In `@PingSampleApp/ui/davinci/components/molecules/DaVinciTextField.tsx`:
- Line 28: Update the isMultiline calculation in DaVinciTextField so
TextInput.multiline is latched for the current field or derived from stable
collector metadata rather than recalculated from live stringValue length on
every edit. Preserve the selected mode while the field is being edited,
including when the text crosses the 60-character threshold in either direction.
🪄 Autofix
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: 8276c540-5cb4-4fe8-86d0-41c16680a46c
⛔ Files ignored due to path filters (4)
.yarn/install-state.gzis excluded by!**/.yarn/**,!**/*.gzPingSampleApp/ios/Podfile.lockis excluded by!**/*.lockPingTestRunner/ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (91)
.github/workflows/js-unit-tests.ymlAGENTS.mdPingSampleApp/.env.examplePingSampleApp/android/app/build.gradlePingSampleApp/package.jsonPingSampleApp/src/clients.tsPingSampleApp/src/styles/componentStyles.tsPingSampleApp/ui/components/atoms/PingTextInput.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciFieldRenderer.tsxPingSampleApp/ui/davinci/components/molecules/DaVinciTextField.tsxPingSampleApp/ui/davinci/components/organisms/DaVinciContinueNodePanel.tsxPingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.tsPingTestRunner/__tests__/integration/davinci.test.tsPingTestRunner/__tests__/integration/protect.test.tsPingTestRunner/android/settings.gradlePingTestRunner/ios/PingTestRunner.xcodeproj/xcshareddata/xcschemes/RNPackagesTests.xcschemePingTestRunner/ios/PodfilePingTestRunner/jest.config.jsPingTestRunner/jest.setup.jsPingTestRunner/package.jsonPingTestRunner/scripts/test-native-android.shPingTestRunner/scripts/test-native-ios.shpackages/davinci/android/build.gradlepackages/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciCommon.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/config/DaVinciConfigParser.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/config/ProtectLifecyclePayload.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/error/DaVinciErrorCodes.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/factory/DaVinciClientFactory.ktpackages/davinci/android/src/main/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapper.ktpackages/davinci/android/src/newarch/java/com/pingidentity/rndavinci/RNPingDavinciModule.ktpackages/davinci/android/src/oldarch/java/com/pingidentity/rndavinci/RNPingDavinciClassicModule.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/RNPingDavinciCommonTest.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/config/DaVinciConfigParserTest.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/factory/DaVinciClientFactoryTest.ktpackages/davinci/android/src/test/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapperTest.ktpackages/davinci/ios/Config/DaVinciConfigParser.swiftpackages/davinci/ios/Error/DaVinciErrorCodes.swiftpackages/davinci/ios/Factory/DaVinciClientFactory.swiftpackages/davinci/ios/Mapper/DaVinciNodeMapper.swiftpackages/davinci/ios/Models/DaVinciPayloads.swiftpackages/davinci/ios/RNPingDavinci.mmpackages/davinci/ios/RNPingDavinciClassic.mmpackages/davinci/ios/RNPingDavinciCommon.swiftpackages/davinci/ios/RNPingDavinciImpl.swiftpackages/davinci/ios/Tests/DaVinciClientFactoryTests.swiftpackages/davinci/ios/Tests/DaVinciConfigParserTests.swiftpackages/davinci/ios/Tests/DaVinciNodeMapperTests.swiftpackages/davinci/src/NativeRNPingDavinci.tspackages/davinci/src/__tests__/createDaVinciClient.test.tspackages/davinci/src/collectorHelpers.tspackages/davinci/src/davinci.tspackages/davinci/src/davinciMethods.tspackages/davinci/src/types/client.types.tspackages/davinci/src/types/config.types.tspackages/davinci/src/types/node.types.tspackages/davinci/src/useDavinci.tsxpackages/protect/CHANGELOG.mdpackages/protect/LICENSEpackages/protect/README.mdpackages/protect/RNPingProtect.podspecpackages/protect/android/build.gradlepackages/protect/android/src/main/AndroidManifest.xmlpackages/protect/android/src/main/java/com/pingidentity/rnprotect/ProtectErrorCodes.ktpackages/protect/android/src/main/java/com/pingidentity/rnprotect/RNPingProtectCommon.ktpackages/protect/android/src/newarch/java/com/pingidentity/rnprotect/RNPingProtectModule.ktpackages/protect/android/src/newarch/java/com/pingidentity/rnprotect/RNPingProtectPackage.ktpackages/protect/android/src/oldarch/java/com/pingidentity/rnprotect/RNPingProtectClassicModule.ktpackages/protect/android/src/oldarch/java/com/pingidentity/rnprotect/RNPingProtectPackage.ktpackages/protect/android/src/test/java/com/pingidentity/rnprotect/RNPingProtectTest.ktpackages/protect/babel.config.jspackages/protect/eslint.config.mjspackages/protect/ios/RNPingProtect.hpackages/protect/ios/RNPingProtect.mmpackages/protect/ios/RNPingProtectClassic.mmpackages/protect/ios/RNPingProtectCommon.swiftpackages/protect/ios/RNPingProtectImpl.swiftpackages/protect/ios/Tests/RNPingProtectCommonTests.swiftpackages/protect/ios/Tests/RNPingProtectImplTests.swiftpackages/protect/jest.config.jspackages/protect/jest.setup.jspackages/protect/package.jsonpackages/protect/src/NativeRNPingProtect.tspackages/protect/src/__tests__/index.test.tsxpackages/protect/src/__tests__/native-module.test.tsxpackages/protect/src/index.tsxpackages/protect/src/protect.tspackages/protect/src/types/index.tspackages/protect/src/types/protect.types.tspackages/protect/tsconfig.build.jsonpackages/protect/tsconfig.jsonpackages/protect/turbo.json
| // PROTECT fields are handled in mapCollectorPayload via form-field type lookup. | ||
| if (resolvedType == PROTECT) continue |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not hide PROTECT fields without a native collector.
Line 173 excludes every PROTECT field before Line 176 checks registeredKeys. If rn-protect is unavailable, no ProtectCollector is instantiated. JavaScript then receives neither a Protect collector nor an unsupportedFields entry.
Remove this unconditional exclusion. The existing registeredKeys.contains(key) check already excludes fields that have an instantiated collector.
Proposed fix
- // PROTECT fields are handled in mapCollectorPayload via form-field type lookup.
- if (resolvedType == PROTECT) continue
-
// A field is supported when the SDK instantiated a collector for its key.
if (registeredKeys.contains(key)) continue📝 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.
| // PROTECT fields are handled in mapCollectorPayload via form-field type lookup. | |
| if (resolvedType == PROTECT) continue | |
| // A field is supported when the SDK instantiated a collector for its key. | |
| if (registeredKeys.contains(key)) continue |
🤖 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/davinci/android/src/main/java/com/pingidentity/rndavinci/mapper/DaVinciNodeMapper.kt`
around lines 173 - 174, Remove the unconditional PROTECT exclusion in the
mapper’s field-processing logic. Let PROTECT fields proceed to the existing
registeredKeys.contains(key) check so fields without an instantiated
ProtectCollector are reported through unsupportedFields, while fields with a
native collector continue through the normal mapping path.
| /** | ||
| * Parses the collector index from bridge options, defaulting to 0. | ||
| */ | ||
| private fun parseCollectorIndex(options: ReadableMap?): Int { | ||
| if (options == null || !options.hasKey("index") || options.isNull("index")) return 0 | ||
| return when (options.getType("index")) { | ||
| com.facebook.react.bridge.ReadableType.Number -> options.getDouble("index").toInt() | ||
| com.facebook.react.bridge.ReadableType.String -> options.getString("index")?.toIntOrNull() ?: 0 | ||
| else -> 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid supplied collector indexes.
Line 547 truncates fractional numbers. Line 548 and Line 549 convert malformed supplied values to index 0. A caller can therefore collect from the first Protect collector instead of receiving an error.
Default only when index is absent. Reject negative, fractional, malformed, and unsupported supplied values with GenericError(type = ErrorType.ARGUMENT_ERROR, ...) before collector selection.
🤖 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/davinci/android/src/main/java/com/pingidentity/rndavinci/RNPingDavinciCommon.kt`
around lines 541 - 551, Update parseCollectorIndex to default to 0 only when
options or the index key is absent; otherwise validate the supplied value as a
non-negative integer. Reject fractional numbers, malformed strings, negative
values, and unsupported types by throwing GenericError with
ErrorType.ARGUMENT_ERROR, rather than coercing them to 0 before collector
selection.
Source: Coding guidelines
| /// Optional protect lifecycle module configuration. Present only when modules.protect is provided. | ||
| let protect: ProtectLifecyclePayload? |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Forward Protect configuration through the TurboModule bridge.
packages/davinci/ios/RNPingDavinci.mm Lines 42-118 builds the configuration dictionary field by field but does not copy modules.protect. TurboModule clients therefore produce payload.protect == nil, and DaVinciClientFactory skips the lifecycle configuration at Lines 80-95. Map the Protect module payload into that dictionary with the parser field names. The classic bridge already forwards the raw configuration dictionary.
🤖 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/davinci/ios/Models/DaVinciPayloads.swift` around lines 74 - 75,
Update the configuration dictionary construction in RNPingDavinci.mm to forward
modules.protect through the TurboModule bridge, using the parser field names
expected by ProtectLifecyclePayload. Preserve the existing field-by-field
mapping and ensure DaVinciPayloads.protect receives the mapped configuration
instead of remaining nil.
| private static func parseCollectorIndex(_ options: NSDictionary) -> Int { | ||
| if let value = options["index"] as? NSNumber { | ||
| return value.intValue | ||
| } | ||
| if let value = options["index"] as? String, let parsed = Int(value) { | ||
| return parsed | ||
| } | ||
| return 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject invalid collector indexes.
An absent index can default to 0. An invalid string also defaults to 0, and non-integer NSNumber values are truncated. For example, "invalid" can collect from collector 0 instead of rejecting the caller input.
Validate that a supplied index is a non-negative integer. Reject invalid values with a GenericError of type argumentError before calling collector.collect().
As per coding guidelines, “validate inputs before native calls; throw argument_error for caller mistakes.”
🤖 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/davinci/ios/RNPingDavinciCommon.swift` around lines 492 - 499,
Update parseCollectorIndex and its caller to distinguish an absent index from
invalid input: accept only non-negative integer values, reject invalid strings
and NSNumber values that are not exact integers, and propagate a GenericError of
type argumentError before collector.collect() is invoked. Preserve the default
index of 0 only when the index is absent.
Source: Coding guidelines
| /** | ||
| * Run PingOne Protect data collection against the active PROTECT collector in the flow. | ||
| * | ||
| * @remarks | ||
| * Requires `@ping-identity/rn-protect` to be installed. Rejects with | ||
| * `DAVINCI_PROTECT_COLLECT_ERROR` when the Protect SDK is absent or collection fails. | ||
| * | ||
| * @param davinciId - Native DaVinci instance identifier. | ||
| * @param options - Per-call options (e.g. `index` for multi-collector nodes). | ||
| */ | ||
| collectProtect(davinciId: string, options: Object): Promise<void>; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add @returns and @throws tags to collectProtect.
The remarks describe failure behavior, but the exported declaration does not have the required TSDoc tags. Document the void resolution and DAVINCI_PROTECT_COLLECT_ERROR rejection explicitly.
As per coding guidelines, “Use TSDoc on all exported declarations with required parameter, return, throws, example, platform-difference, and visibility tags as applicable.”
🤖 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/davinci/src/NativeRNPingDavinci.ts` around lines 147 - 157, Update
the exported collectProtect declaration’s TSDoc to add an `@returns` tag
documenting successful void resolution and an `@throws` tag documenting rejection
with DAVINCI_PROTECT_COLLECT_ERROR when the Protect SDK is unavailable or
collection fails.
Source: Coding guidelines
| export class ProtectError extends PingError { | ||
| constructor(message: string, code: string, type: string, status?: number) { | ||
| super(message, code, type, status); | ||
| this.name = 'ProtectError'; | ||
| Object.setPrototypeOf(this, new.target.prototype); | ||
| } | ||
|
|
||
| static from(error: unknown): ProtectError { | ||
| return PingError.fromAs(error, ProtectError); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document the public ProtectError API.
Add TSDoc for the constructor and ProtectError.from(). Document constructor parameters and document the error parameter and returned ProtectError for from().
As per coding guidelines, “Use TSDoc on all exported declarations with required parameter, return, throws, example, platform-difference, and visibility tags as applicable.”
🤖 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/protect/src/types/protect.types.ts` around lines 114 - 123, Document
the exported ProtectError constructor and static from method with TSDoc. Include
parameter descriptions for message, code, type, optional status, and error, plus
the from method’s returned ProtectError using the applicable required tags; keep
the implementation unchanged.
Source: Coding guidelines
| "src/*.ts", | ||
| "src/*.tsx", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Track nested source files in both platform task hashes.
src/*.ts and src/*.tsx do not match files under src/types/. A change to packages/protect/src/types/protect.types.ts can reuse a stale build:android or build:ios cache result.
packages/protect/turbo.json#L11-L12: replace the direct-file patterns with a recursive source pattern.packages/protect/turbo.json#L32-L33: replace the direct-file patterns with the same recursive source pattern.
Proposed fix
- "src/*.ts",
- "src/*.tsx",
+ "src/**",📍 Affects 1 file
packages/protect/turbo.json#L11-L12(this comment)packages/protect/turbo.json#L32-L33
🤖 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/protect/turbo.json` around lines 11 - 12, Update
packages/protect/turbo.json at lines 11-12 and 32-33 to replace the
non-recursive src/*.ts and src/*.tsx inputs in both platform task hashes with
the same recursive source pattern, ensuring nested files such as
src/types/protect.types.ts invalidate build:android and build:ios caches.
| const onProtectCollect = useCallback(async (): Promise<void> => { | ||
| const davinciClient = davinciContext?.client; | ||
| if (!davinciClient) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when the Protect client is unavailable.
If Protect fields exist and davinciClient is missing, onProtectCollect resolves successfully. onSubmit then calls next, so a Protect node can advance without collection. Set protectError and reject the submission instead of returning.
As per coding guidelines, use GenericError with type, error, and message, and log or rethrow every caught error.
🤖 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 `@PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts` around
lines 160 - 164, Update onProtectCollect in the missing davinciClient path to
fail closed: set protectError and reject with a GenericError containing type,
error, and message instead of returning successfully. Ensure onSubmit cannot
call next when Protect collection is unavailable, and preserve the requirement
that every caught error is logged or rethrown.
Source: Coding guidelines
| onProtectCollect() | ||
| .then(() => next(plan.input)) | ||
| .catch(() => { | ||
| // `error` is already updated by the hook. | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not swallow submission failures.
The empty catch consumes both Protect collection failures and next failures without logging or rethrowing. Use the configured logger at error level and preserve the existing error state. Do not leave the catch empty.
As per coding guidelines, log or rethrow every caught error.
🤖 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 `@PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts` around
lines 188 - 192, Update the catch following onProtectCollect in the submission
flow to accept the caught error and log it at error level through the configured
logger, preserving the hook’s existing error state. Ensure failures from both
onProtectCollect and next are no longer silently consumed.
Source: Coding guidelines
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Run Protect collection before every submission path.
The new gate wraps only onSubmit. onFlowAction calls form.setValue, and the code documents that this auto-submits through next. A node containing both a flow collector and a Protect collector can therefore advance without collectProtect. Route all submission paths through one Protect-aware helper, or add a pre-submit callback to useDaVinciForm.
🤖 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 `@PingSampleApp/ui/davinci/hooks/useDaVinciClientPanelController.ts` around
lines 188 - 192, Ensure every submission path in the controller, including
onFlowAction-triggered auto-submits, runs onProtectCollect before calling
next(plan.input). Centralize this behavior in a shared Protect-aware submission
helper and use it from both onSubmit and onFlowAction, preserving the hook’s
existing error update behavior.
| jest.mock('../packages/protect/src/NativeRNPingProtect', () => ({ | ||
| __esModule: true, | ||
| getNativeModule: jest.fn(() => ({ | ||
| collectForDaVinci: jest.fn(async () => undefined), | ||
| })), | ||
| toNativeCollectOptions: jest.fn((options) => options), | ||
| toNativeConfig: jest.fn((config) => config), | ||
| })); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add toNativeProtectConfig to the shared mock.
startProtect() calls toNativeProtectConfig(config) before it calls initialize. This mock does not export that function. A test that uses this shared mock and invokes startProtect will fail with TypeError instead of testing the Protect bridge.
Proposed fix
toNativeCollectOptions: jest.fn((options) => options),
toNativeConfig: jest.fn((config) => config),
+ toNativeProtectConfig: jest.fn((config) => config),
}));📝 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.
| jest.mock('../packages/protect/src/NativeRNPingProtect', () => ({ | |
| __esModule: true, | |
| getNativeModule: jest.fn(() => ({ | |
| collectForDaVinci: jest.fn(async () => undefined), | |
| })), | |
| toNativeCollectOptions: jest.fn((options) => options), | |
| toNativeConfig: jest.fn((config) => config), | |
| })); | |
| jest.mock('../packages/protect/src/NativeRNPingProtect', () => ({ | |
| __esModule: true, | |
| getNativeModule: jest.fn(() => ({ | |
| collectForDaVinci: jest.fn(async () => undefined), | |
| })), | |
| toNativeCollectOptions: jest.fn((options) => options), | |
| toNativeConfig: jest.fn((config) => config), | |
| toNativeProtectConfig: jest.fn((config) => config), | |
| })); |
🤖 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 `@PingTestRunner/jest.setup.js` around lines 286 - 293, Add the missing
toNativeProtectConfig mock export to the NativeRNPingProtect shared mock,
matching the existing pass-through behavior of toNativeConfig, so startProtect()
can call it before initialize without a TypeError.
…tolinking in PingTestRunner - Skip testCollectForDaVinciRejectsWhenCollectFails: ProtectCollector(with:) accesses Bundle.main which crashes in the xctest agent (no host app bundle) - Add PingTestRunner/react-native.config.js excluding @ping-identity/rn-protect from autolinking on both platforms — it is only needed for Robolectric unit tests, not the BrowserStack E2E APK build; autolinking was failing the CMake step because codegen JNI output does not exist until the package is built Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…PingProtectSpec.h Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 `@PingTestRunner/react-native.config.js`:
- Line 10: Update the dependency comment in react-native.config.js to state that
`@ping-identity/rn-protect` is used by the integration test protect.test.ts and
that its Android project is manually included through settings.gradle, replacing
the incorrect Robolectric-only explanation.
🪄 Autofix
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: b8360be6-1c1f-4ba3-8e05-42247a1b9c9c
📒 Files selected for processing (1)
PingTestRunner/react-native.config.js
|
|
||
| module.exports = { | ||
| dependencies: { | ||
| // rn-protect is included in package.json for Robolectric unit tests only. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the dependency comment.
@ping-identity/rn-protect is also used by PingTestRunner/__tests__/integration/protect.test.ts. PingTestRunner/android/settings.gradle manually includes its Android project. The comment should describe the integration-test dependency and the manual Android linking rationale instead of limiting usage to Robolectric tests.
🤖 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 `@PingTestRunner/react-native.config.js` at line 10, Update the dependency
comment in react-native.config.js to state that `@ping-identity/rn-protect` is
used by the integration test protect.test.ts and that its Android project is
manually included through settings.gradle, replacing the incorrect
Robolectric-only explanation.
| c.isConsoleLogEnabled = initConfig.isConsoleLogEnabled | ||
| c.deviceAttributesToIgnore = initConfig.deviceAttributesToIgnore | ||
| } | ||
| try await Protect.initialize() |
There was a problem hiding this comment.
One thing to consider here, native Protect.initialize() no-ops once already initialized, so it seems that without a cleanup()/invalidate() hook (like RNPingJourneyImpl.swift has), a JS reload with new config would silently do nothing.
| * | ||
| * Handled entirely by `@ping-identity/rn-protect` — appears as `executionMode: | ||
| * 'integration_required'` and `kind: 'integration'` in normalized collectors. | ||
| * Call `createProtectClient().collectForDaVinci(daVinci)` before `daVinci.next({})`. |
There was a problem hiding this comment.
| * Call `createProtectClient().collectForDaVinci(daVinci)` before `daVinci.next({})`. | |
| * Call `createProtectClient().collectProtect()` before `daVinci.next({})`. |
| handlers.reject( | ||
| GenericError( | ||
| type: .authError, | ||
| error: ProtectErrorCode.initializeError.rawValue, |
There was a problem hiding this comment.
Just a nit pick. How about we use a dedicated error code here, or possibly reuse collect_error, for pause/resume failures? This error suggests an initialization issue, and separating them would make handling clearer for callers.
| resumeBehavioralDataOnStart = protect.resumeBehavioralDataOnStart | ||
| } | ||
| } catch (_: NoClassDefFoundError) { | ||
| // Protect SDK absent — rn-protect not installed; skip lifecycle module. |
There was a problem hiding this comment.
Worth adding a debug or warning log here?
|
Great work! Left minor comments. |
rodrigoareis
left a comment
rodrigoareis
left a comment
There was a problem hiding this comment.
Overall implementation looks good. Left some comments
| import PingOidc | ||
| import PingOrchestrate | ||
| import RNPingCore | ||
| #if canImport(PingOneProtect) |
There was a problem hiding this comment.
Regarding these #if canImport(PingOneProtect) / compileOnly (Android) items individually, I think we should settle the pattern — because rn-core already defines one, and this PR contains both the compliant and the non-compliant implementation.
CoreRuntime.kt:50-54 and CoreRuntime.swift:114, 135 already say exactly what we're trying to do:
"Plugin packages (external-idp, protect) cannot depend on rn-davinci directly — this indirection lets DaVinci inject its collector lookup at init time without creating a circular dependency."
The contract is: the host (rn-davinci) publishes an opaque List<Any> handle resolver into rn-core, and the plugin (rn-protect) owns the SDK dependency and does its own filterIsInstance. Nothing SDK-typed crosses the boundary — which means the host needs no compileOnly, no canImport, and no NoClassDefFoundError guard.
rn-protect already implements this correctly — RNPingProtectCommon.kt:310-313 and protect/ios/RNPingProtectCommon.swift:86 both go through CoreRuntime.resolveDaVinciCollectors. That code is currently dead. The path that actually ships (daVinci.collectProtect() → RNPingDavinciCommon.kt:492 / .swift:430) inverts the dependency, and that inversion is the direct cause of the canImport and compileOnly problems flagged above.
The JS-layer precedent is external-idp/src/externalIdp.ts:194 — authorizeForDaVinci lives on the plugin client and takes the DaVinci client as an argument (PingSampleApp/.../useDaVinciClientPanelController.ts:217).
Suggest mirroring that:
// instead of daVinci.collectProtect()
await collectProtect(davinciClient, { index }); // exported from @ping-identity/rn-protectThat deletes collectProtect from the davinci native module on both platforms, removes all 4 #if canImport blocks, keeps PingOneProtect out of RNPingDavinci.podspec, resolves the stale PingSampleApp/ios/Podfile.lock:2581 entry, and makes the already-written collectForDaVinci the single implementation. If we'd rather keep daVinci.collectProtect() for DX, it can be a thin JS re-export — but the native call should still land on the protect module.
Happy to go with the tactical fix for this PR (declare the deps properly, make the guard log) if a broader refactor is out of scope — but could we at least (a) route collectProtect through the plugin so no new canImport is introduced, and (b) open a follow-up ticket for the serializer/module registries covering rn-davinci, rn-external-idp, rn-journey and rn-fido?
| resumeBehavioralDataOnStart = protect.resumeBehavioralDataOnStart | ||
| } | ||
| } catch (_: NoClassDefFoundError) { | ||
| // Protect SDK absent — rn-protect not installed; skip lifecycle module. |
There was a problem hiding this comment.
| // Protect SDK absent — rn-protect not installed; skip lifecycle module. | |
| // Protect SDK absent — rn-protect not installed; skip lifecycle module. | |
| logger?.e("modules.protect was configured but the PingOne Protect SDK is not on the classpath; ProtectLifecycle was NOT registered. Add com.pingidentity.sdks:protect.", e) |
| * Call this after a successful authentication flow. Requires `startProtect()` first. | ||
| * | ||
| * @param options - Optional logger instance. | ||
| * @throws {@link ProtectError} when the SDK is not initialized. |
There was a problem hiding this comment.
So pauseBehavioralData() without a prior startProtect() rejects on iOS and silently succeeds on Android. That makes this doc only true on iOS.
The following tests document a platform divergence. Did we create a ticket to fix this on the native? For now I would recommend add a TODO to fix it
Android — RNPingProtectTest.kt:86 → pauseBehavioralDataResolvesSuccessfully (resolves with no prior initialize())
iOS — RNPingProtectCommonTests.swift:88 → testPauseBehavioralDataRejectsWhenSDKNotInitialized (rejects)
| * | ||
| * @defaultValue `false` | ||
| */ | ||
| pauseBehavioralDataOnSuccess?: boolean; |
There was a problem hiding this comment.
Do we really need this?
| let collectorIndex = parseCollectorIndex(options) | ||
|
|
||
| // `@objc` methods cannot be declared `async`, so a Task is required to | ||
| // enter the async context. The body runs off the main actor so that |
There was a problem hiding this comment.
This comment is not valid, the unstructured Task {} inherits the actor isolation of its enclosing context, and collectForDaVinci is annotated @MainActor (line 55). So the closure body is main-actor-isolated and collector.collect() runs on the main thread. Android correctly uses Dispatchers.IO.
3 participants
Summary
@ping-identity/rn-protect— a new React Native package bridging the native PingOne Protect SDK on iOS and AndroiddaVinci.collectProtect()toDaVinciClientfor running PROTECT collector data collection inside a DaVinci flowmodules.protect.loggersupport so protect operations get a dedicated scoped logger separate from the top-level DaVinci loggerDAVINCI_SHOW_DEBUG_PANELenv flag (defaultfalse)Changes
@ping-identity/rn-protect(new package)startProtect(config?),pauseBehavioralData(options?),resumeBehavioralData(options?)standalone async functions@ping-identity/rn-davincicollectProtect(options?)onDaVinciClientPROTECTcollector mapping and serialization on both platformsProtectLifecyclePayload.loggerId—modules.protect.loggerresolved to a native logger id at configure timeDaVinciHandle.protectLoggerId— protect logger falls back to the DaVinci-level logger when not setPingSampleApp
modules.protectwired intosampleDaVinciConfigwithappLoggeruseDaVinciClientPanelControllercallsdavinciClient.collectProtect()directly - no separate protect client neededPingTestRunner / CI
@ping-identity/rn-protectandcollectProtecton@ping-identity/rn-davincijs-unit-tests.ymlCI stepRNPingProtect-Unit-Testsadded to iOS test scheme andtest-native-ios.shrn-protectRobolectric tests added totest-native-android.shTest plan
./gradlew :ping-identity_rn-protect:testDebugUnitTest :ping-identity_rn-davinci:testDebugUnitTest- 62 tasks, all passRNPingProtect-Unit-TestsandRNPingDavinci-Unit-Testspass (pre-existing flaky timeout intestDisposeRemovesDaVinciFromRegistryis unrelated)yarn test:runner:integration)Summary by CodeRabbit
Summary by CodeRabbit